Repository: dragonflyoss/Dragonfly2
Pull request #1754 (2022-10-13 04:18:26)

feat: peer will back-to-source when task switch state failed
Signed-off-by: Gaius <gaius.qi@gmail.com>

<!--- Provide a general summary of your changes in the Title above -->

## Description
- Peer will back-to-source when task switch state failed.
<!--- Describe your changes in detail -->

## Related Issue

<!--- This project only accepts pull requests related to open issues -->
<!--- If suggesting a new feature or change, please discuss it in an issue first -->
<!--- If fixing a bug, there should be an issue describing it with steps to reproduce -->
<!--- Please link to the issue here: -->

## Motivation and Context

<!--- Why is this change required? What problem does it solve? -->

## Screenshots (if appropriate)

## Types of changes

<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->

- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] 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

<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->

- [ ] My change requires a change to the documentation.
- [ ] I have updated the documentation accordingly.
- [x] I have read the **CONTRIBUTING** document.
- [ ] I have added tests to cover my changes.


codecov-commenter: # [Codecov](https://codecov.io/gh/dragonflyoss/Dragonfly2/pull/1754?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss) Report
> Merging [#1754](https://codecov.io/gh/dragonflyoss/Dragonfly2/pull/1754?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss) (0b48d5b) into [main](https://codecov.io/gh/dragonflyoss/Dragonfly2/commit/89a38e639bbdecd6817bcc9bb96348bf22b6a111?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss) (89a38e6) will **decrease** coverage by `0.03%`.
> The diff coverage is `71.42%`.

[![Impacted file tree graph](https://codecov.io/gh/dragonflyoss/Dragonfly2/pull/1754/graphs/tree.svg?width=650&height=150&src=pr&token=PUICJYSAT7&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss)](https://codecov.io/gh/dragonflyoss/Dragonfly2/pull/1754?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss)

```diff
@@            Coverage Diff             @@
##             main    #1754      +/-   ##
==========================================
- Coverage   43.68%   43.64%   -0.04%     
==========================================
  Files         128      128              
  Lines       15863    15863              
==========================================
- Hits         6930     6924       -6     
- Misses       8407     8413       +6     
  Partials      526      526              
```

| Flag | Coverage Δ | |
|---|---|---|
| Object-compatibility-e2etests | `?` | |
| e2etests | `?` | |
| unittests | `43.64% <71.42%> (-0.04%)` | :arrow_down: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/dragonflyoss/Dragonfly2/pull/1754?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss) | Coverage Δ | |
|---|---|---|
| [scheduler/service/service.go](https://codecov.io/gh/dragonflyoss/Dragonfly2/pull/1754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss#diff-c2NoZWR1bGVyL3NlcnZpY2Uvc2VydmljZS5nbw==) | `75.95% <71.42%> (+0.43%)` | :arrow_up: |
| [client/daemon/peer/peertask\_conductor.go](https://codecov.io/gh/dragonflyoss/Dragonfly2/pull/1754/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dragonflyoss#diff-Y2xpZW50L2RhZW1vbi9wZWVyL3BlZXJ0YXNrX2NvbmR1Y3Rvci5nbw==) | `56.35% <0.00%> (-0.66%)` | :arrow_down: |


Patch:
diff --git a/scheduler/service/service.go b/scheduler/service/service.go
index 31640c4e..337a6d62 100644
--- a/scheduler/service/service.go
+++ b/scheduler/service/service.go
@@ -80,12 +80,7 @@ func New(
 // RegisterPeerTask registers peer and triggers seed peer download task.
 func (s *Service) RegisterPeerTask(ctx context.Context, req *schedulerv1.PeerTaskRequest) (*schedulerv1.RegisterResult, error) {
 	// Register task and trigger seed peer download task.
-	task, needBackToSource, err := s.registerTask(ctx, req)
-	if err != nil {
-		msg := fmt.Sprintf("peer %s register is failed: %s", req.PeerId, err.Error())
-		logger.Error(msg)
-		return nil, dferrors.New(commonv1.Code_SchedTaskStatusError, msg)
-	}
+	task, needBackToSource := s.registerTask(ctx, req)
 	host := s.registerHost(ctx, req.PeerHost)
 	peer := s.registerPeer(ctx, req.PeerId, task, host, req.UrlMeta.Tag, req.UrlMeta.Application)
 	peer.Log.Infof("register peer task request: %#v %#v %#v", req, req.UrlMeta, req.HostLoad)
@@ -537,7 +532,7 @@ func (s *Service) LeaveTask(ctx context.Context, req *schedulerv1.PeerTarget) er
 }
 
 // registerTask creates a new task or reuses a previous task.
-func (s *Service) registerTask(ctx context.Context, req *schedulerv1.PeerTaskRequest) (*resource.Task, bool, error) {
+func (s *Service) registerTask(ctx context.Context, req *schedulerv1.PeerTaskRequest) (*resource.Task, bool) {
 	task, loaded := s.resource.TaskManager().Load(req.TaskId)
 	if loaded {
 		// Task is the pointer, if the task already exists, the next request will
@@ -547,8 +542,8 @@ func (s *Service) registerTask(ctx context.Context, req *schedulerv1.PeerTaskReq
 
 		if !task.FSM.Is(resource.TaskStateFailed) &&
 			!task.FSM.Is(resource.TaskStateLeave) && task.HasAvailablePeer() {
-			task.Log.Infof("task state is %s", task.FSM.Current())
-			return task, false, nil
+			task.Log.Infof("task dose not need to back-to-source, because of task has available peer and state is %s", task.FSM.Current())
+			return task, false
 		}
 	} else {
 		// Create a task for the first time.
@@ -561,29 +556,34 @@ func (s *Service) registerTask(ctx context.Context, req *schedulerv1.PeerTaskReq
 	// the concurrent download of the peer may trigger multiple seed peer downloads.
 	if !task.FSM.Is(resource.TaskStateRunning) {
 		if err := task.FSM.Event(resource.TaskEventDownload); err != nil {
-			return nil, false, err
+			task.Log.Errorf("task needs to back-to-source, because of %s", err.Error())
+			return task, true
 		}
 	}
 
 	// Seed peer registers the task, then it needs to back-to-source.
 	host, loaded := s.resource.HostManager().Load(req.PeerHost.Id)
 	if loaded && host.Type != resource.HostTypeNormal {
-		return task, true, nil
+		task.Log.Infof("task needs to back-to-source, because of host can be loaded and type is %d", host.Type)
+		return task, true
 	}
 
 	// FIXME Need to add the condition that the seed peer grpc client is
 	// available and can be triggered back-to-source.
 	if s.config.SeedPeer.Enable {
 		if task.IsSeedPeerFailed() {
-			return task, true, nil
+			task.Log.Info("task needs to back-to-source, because of seed peer is failed")
+			return task, true
 		}
 
 		go s.triggerSeedPeerTask(ctx, task)
-		return task, false, nil
+		task.Log.Info("task dose not need to back-to-source, because of seed peer has been triggered")
+		return task, false
 	}
 
 	// Task need to back-to-source.
-	return task, true, nil
+	task.Log.Info("task needs to back-to-source, because of seed peer disabled")
+	return task, true
 }
 
 // registerHost creates a new host or reuses a previous host.


Test patch:
diff --git a/scheduler/service/service_test.go b/scheduler/service/service_test.go
index 1acf50ac..4bd8d2a1 100644
--- a/scheduler/service/service_test.go
+++ b/scheduler/service/service_test.go
@@ -1749,9 +1749,8 @@ func TestService_registerTask(t *testing.T) {
 					mt.Load(gomock.Any()).Return(mockTask, true).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},
@@ -1777,9 +1776,8 @@ func TestService_registerTask(t *testing.T) {
 					mt.Load(gomock.Any()).Return(mockTask, true).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},
@@ -1805,9 +1803,8 @@ func TestService_registerTask(t *testing.T) {
 					mt.Load(gomock.Any()).Return(mockTask, true).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},
@@ -1844,9 +1841,8 @@ func TestService_registerTask(t *testing.T) {
 					mc.TriggerTask(gomock.Any(), gomock.Any()).Do(func(ctx context.Context, task *resource.Task) { wg.Done() }).Return(mockPeer, &schedulerv1.PeerResult{}, nil).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTaskURL, task.URL)
 				assert.EqualValues(mockTaskURLMeta, task.URLMeta)
@@ -1882,9 +1878,8 @@ func TestService_registerTask(t *testing.T) {
 					mc.TriggerTask(gomock.Any(), gomock.Any()).Do(func(ctx context.Context, task *resource.Task) { wg.Done() }).Return(mockPeer, &schedulerv1.PeerResult{}, nil).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},
@@ -1919,9 +1914,8 @@ func TestService_registerTask(t *testing.T) {
 					mc.TriggerTask(gomock.Any(), gomock.Any()).Do(func(ctx context.Context, task *resource.Task) { wg.Done() }).Return(mockPeer, &schedulerv1.PeerResult{}, nil).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},
@@ -1956,9 +1950,8 @@ func TestService_registerTask(t *testing.T) {
 					mc.TriggerTask(gomock.Any(), gomock.Any()).Do(func(ctx context.Context, task *resource.Task) { wg.Done() }).Return(mockPeer, &schedulerv1.PeerResult{}, nil).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},
@@ -1988,9 +1981,8 @@ func TestService_registerTask(t *testing.T) {
 					mh.Load(gomock.Any()).Return(mockHost, true).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.True(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},
@@ -2027,9 +2019,8 @@ func TestService_registerTask(t *testing.T) {
 					mc.TriggerTask(gomock.Any(), gomock.Any()).Do(func(ctx context.Context, task *resource.Task) { wg.Done() }).Return(mockPeer, &schedulerv1.PeerResult{}, errors.New("foo")).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTaskURL, task.URL)
 				assert.EqualValues(mockTaskURLMeta, task.URLMeta)
@@ -2065,9 +2056,8 @@ func TestService_registerTask(t *testing.T) {
 					mc.TriggerTask(gomock.Any(), gomock.Any()).Do(func(ctx context.Context, task *resource.Task) { wg.Done() }).Return(mockPeer, &schedulerv1.PeerResult{}, errors.New("foo")).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.False(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},
@@ -2098,9 +2088,8 @@ func TestService_registerTask(t *testing.T) {
 					mh.Load(gomock.Any()).Return(nil, false).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.True(needBackToSource)
 				assert.EqualValues(mockTaskURL, task.URL)
 				assert.EqualValues(mockTaskURLMeta, task.URLMeta)
@@ -2130,9 +2119,8 @@ func TestService_registerTask(t *testing.T) {
 					mh.Load(gomock.Any()).Return(nil, false).Times(1),
 				)
 
-				task, needBackToSource, err := svc.registerTask(context.Background(), req)
+				task, needBackToSource := svc.registerTask(context.Background(), req)
 				assert := assert.New(t)
-				assert.NoError(err)
 				assert.True(needBackToSource)
 				assert.EqualValues(mockTask, task)
 			},