Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add delete task and list tasks manager api with request type and service type. #3378

Merged
merged 9 commits into from
Aug 13, 2024

Conversation

IRONICBo
Copy link
Contributor

@IRONICBo IRONICBo commented Jul 15, 2024

Description

Add a POST method to start an asynchronous task to clear the cache of the task id on the peer, and a query api to query the status of the asynchronous task and aggregate it into the existing asynchronous task creation logic.
The manager eventually collects the information needed to start the job and creates a GroupJob into the machine to send a message to the distributed task waiting for the scheduler to consume it.
There is already a Job definition that can reuse the job request to support a new Job, define a new JobRequest, and the response reuses the Job response.

Meanwhile, this PR add a delete task job and list tasks job implementation to the scheduler's async job to receive incoming jobs from the manager and call grpc's interface to complete tasks queries and deletions.

Related Issue

Motivation and Context

This change introduces api definitions for delete task and list task and request responsetype definitions, as well as interfaces under the services package, and introduced internaljob configuration parameters and job calls to asynchronously list or delete peers and task information for associated tasks in the scheduler.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation Update (if none of the other choices apply)

Checklist

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.

@IRONICBo IRONICBo requested a review from a team as a code owner July 15, 2024 11:54
@IRONICBo IRONICBo force-pushed the feat/add-delete-task-manager-api branch 2 times, most recently from 105e622 to a576067 Compare July 15, 2024 11:59
Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 12.50000% with 77 lines in your changes missing coverage. Please review.

Project coverage is 52.54%. Comparing base (83450d5) to head (915d880).
Report is 14 commits behind head on main.

Files Patch % Lines
manager/job/manager_tasks.go 0.00% 50 Missing ⚠️
manager/handlers/job.go 40.00% 8 Missing and 4 partials ⚠️
scheduler/resource/task.go 0.00% 10 Missing ⚠️
manager/job/job.go 0.00% 5 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3378      +/-   ##
==========================================
- Coverage   52.76%   52.54%   -0.22%     
==========================================
  Files         193      194       +1     
  Lines       20485    20578      +93     
==========================================
+ Hits        10808    10813       +5     
- Misses       8873     8955      +82     
- Partials      804      810       +6     
Flag Coverage Δ
unittests 52.54% <12.50%> (-0.22%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
manager/config/config.go 96.20% <100.00%> (+0.05%) ⬆️
manager/job/job.go 0.00% <0.00%> (ø)
scheduler/resource/task.go 78.47% <0.00%> (-3.69%) ⬇️
manager/handlers/job.go 63.85% <40.00%> (-7.58%) ⬇️
manager/job/manager_tasks.go 0.00% <0.00%> (ø)

... and 6 files with indirect coverage changes

@IRONICBo IRONICBo force-pushed the feat/add-delete-task-manager-api branch from 38d3b15 to 987e070 Compare July 24, 2024 01:54
@IRONICBo IRONICBo force-pushed the feat/add-delete-task-manager-api branch from 987e070 to df2ae9c Compare July 30, 2024 02:45

logger.Infof("create manager tasks group %s in queues %v, tasks: %#v", group.GroupUUID, queues, tasks)
if _, err := m.job.Server.SendGroupWithContext(ctx, group, 0); err != nil {
logger.Errorf("create manager tasks group %s failed", group.GroupUUID, err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove extra spaces in log.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, done.


type ListTasksArgs struct {
TaskID string `json:"task_id" binding:"required"`
Page int `json:"page" binding:"omitempty,gte=1"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Page and PerPage are required by many requests. We can define a common request argument to reuse, but that's not the focus of this PR. We can address it in a subsequent refactoring.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, there is a page and perpage parameter in the task itself, but this is mainly for handling page and perpage in the structure returned within the task result, and I'll consider defining a generic version in subsequent implementations as well.

@IRONICBo IRONICBo force-pushed the feat/add-delete-task-manager-api branch 2 times, most recently from 77ac45c to 7aee5fd Compare August 5, 2024 11:12
@IRONICBo IRONICBo requested a review from chlins August 5, 2024 11:50

// listTasks is a job to list tasks.
func (j *job) listTasks(ctx context.Context, data string) (string, error) {
ctx, cancel := context.WithTimeout(ctx, listTasksTimeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't see any reference or usage for the ctx in this method.

Copy link
Contributor Author

@IRONICBo IRONICBo Aug 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, this file should belong to another scheduler PR, which I have currently removed.

And I have fixed this ctx usage in another PR.


// deleteTask is a job to delete task.
func (j *job) deleteTask(ctx context.Context, data string) (string, error) {
ctx, cancel := context.WithTimeout(ctx, deleteTaskTimeout)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok.

@IRONICBo IRONICBo force-pushed the feat/add-delete-task-manager-api branch from f4dd25c to dcbb0f7 Compare August 5, 2024 12:22
@IRONICBo IRONICBo requested a review from chlins August 5, 2024 12:24
chlins
chlins previously approved these changes Aug 9, 2024
Copy link
Member

@chlins chlins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@IRONICBo IRONICBo force-pushed the feat/add-delete-task-manager-api branch from 572d87c to 16cb224 Compare August 9, 2024 02:49
@IRONICBo IRONICBo requested a review from chlins August 9, 2024 03:11
}

// getValidPeers try to get valid peers by task id
func (j *job) getValidPeers(taskID string) ([]*resource.Peer, error) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getFinishedPeers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I have updated this function's name.

}

// get peer info by task info
peers := make([]*resource.Peer, 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add LoadFinishedPeers to task.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, moved to scheduler/resource/task.go::LoadFinishedPeers.


// Create a wait group to limit delete rpc concurrency
// and avoid too many rpc requests to the host.
wg := sync.WaitGroup{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No concurrent deletion.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, removed.


// Get dfdaemon client from host
target := fmt.Sprintf("%s:%d", peer.Host.IP, peer.Host.Port)
conn, err := grpc.DialContext(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use pkg/rpc client.

Copy link
Contributor Author

@IRONICBo IRONICBo Aug 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I have added a GetV2ByAddr() function in pkg/rpc/dfdaemon/client/client_v2.go, refer to pkg/rpc/manager/client/client_v2.go::GetV2ByAddr()

@IRONICBo IRONICBo requested a review from gaius-qi August 10, 2024 13:22
@IRONICBo IRONICBo force-pushed the feat/add-delete-task-manager-api branch from 323c9fb to 2b2d3fd Compare August 10, 2024 13:24
gaius-qi
gaius-qi previously approved these changes Aug 12, 2024
Copy link
Member

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gaius-qi
Copy link
Member

@IRONICBo Fix Lint.

@IRONICBo IRONICBo force-pushed the feat/add-delete-task-manager-api branch from 2b2d3fd to 915d880 Compare August 12, 2024 03:40
Copy link
Member

@gaius-qi gaius-qi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@gaius-qi gaius-qi merged commit 5547307 into dragonflyoss:main Aug 13, 2024
29 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants