Comments for Task Reaper. by anshulpundir · Pull Request #2421 · moby/swarmkit

anshulpundir · 2017-10-31T01:16:59Z

Based on recent conversations about 'orphaned' task state and the task reaper. Just wanted to capture these in the code for better readability.

Signed-off-by: Anshul Pundir anshul.pundir@docker.com

anshulpundir · 2017-10-31T01:22:19Z

@aaronlehmann please take a look.

cc @nishanttotla

aaronlehmann

Overall LGTM

aaronlehmann · 2017-10-31T03:28:23Z

api/types.proto

 	REJECTED = 768 [(gogoproto.enumvalue_customname)="TaskStateRejected"]; // task could not be executed here.
-	ORPHANED = 832 [(gogoproto.enumvalue_customname)="TaskStateOrphaned"]; // The node on which this task is scheduled is Down for too long
+	// The purpose of this state is to free up resources associated  with service tasks on unresponsive nodes 
+	// , without having to delete those tasks. This state is directly assigned as current task state by the


Weird comma at the beginning of the line here.

aaronlehmann · 2017-10-31T03:28:46Z

api/types.proto

-	ORPHANED = 832 [(gogoproto.enumvalue_customname)="TaskStateOrphaned"]; // The node on which this task is scheduled is Down for too long
+	// The purpose of this state is to free up resources associated  with service tasks on unresponsive nodes 
+	// , without having to delete those tasks. This state is directly assigned as current task state by the
+	// orchestrator (as opposed to setting it as the desired state which is not very intuitive).


I don't understand the last part.

aaronlehmann · 2017-10-31T03:29:25Z

api/types.proto

 	FAILED = 704 [(gogoproto.enumvalue_customname)="TaskStateFailed"]; // task execution failed with error
 	REJECTED = 768 [(gogoproto.enumvalue_customname)="TaskStateRejected"]; // task could not be executed here.
-	ORPHANED = 832 [(gogoproto.enumvalue_customname)="TaskStateOrphaned"]; // The node on which this task is scheduled is Down for too long
+	// The purpose of this state is to free up resources associated  with service tasks on unresponsive nodes 


Extra space after "associated"

aaronlehmann · 2017-10-31T03:30:19Z

manager/orchestrator/taskreaper/task_reaper.go

 	}
 }

+// Tick performs the actual cleanup.


Lowercase "tick"

codecov · 2017-10-31T07:15:45Z

Codecov Report

Merging #2421 into master will increase coverage by 0.11%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2421      +/-   ##
==========================================
+ Coverage   60.69%   60.81%   +0.11%     
==========================================
  Files         128      128              
  Lines       26401    26401              
==========================================
+ Hits        16025    16055      +30     
+ Misses       8974     8942      -32     
- Partials     1402     1404       +2

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

stevvooe · 2017-10-31T16:08:33Z

api/types.proto

 	FAILED = 704 [(gogoproto.enumvalue_customname)="TaskStateFailed"]; // task execution failed with error
 	REJECTED = 768 [(gogoproto.enumvalue_customname)="TaskStateRejected"]; // task could not be executed here.
-	ORPHANED = 832 [(gogoproto.enumvalue_customname)="TaskStateOrphaned"]; // The node on which this task is scheduled is Down for too long
+	// The main purpose of this state is to free up resources associated with service tasks on


Use Go style for these comments: start with the identifier.

// TaskStateOrphaned ...

stevvooe · 2017-10-31T16:08:58Z

manager/orchestrator/taskreaper/task_reaper.go

-	stopChan    chan struct{}
-	doneChan    chan struct{}
+
+	// List of slot tubles to be inspected for task history cleanup.


Again, make sure to follow godoc style for these: start with the identifier.

stevvooe · 2017-10-31T16:10:47Z

@anshulpundir It is considered poor form to self merge. I understand there are situations where it is okay, but this is an open source project and review standards must be met.

anshulpundir

It is considered poor form to self merge. I understand there are situations where it is okay, but this is an open source project and review standards must be met.

I was not aware of that. Thanks for letting me know!
edit: Can you please elaborate on why this is considered poor form ? I'd also suggest adding that to https://github.com/docker/swarmkit/blob/master/CONTRIBUTING.md @stevvooe

Upgrade swarmkit dependency. Changes: moby/swarmkit@ce5f7b8a (HEAD -> master, origin/master, origin/HEAD) Merge pull request moby/swarmkit#2411 from crunchywelch/2401-arm64_support moby/swarmkit@b0856099 Merge pull request moby/swarmkit#2423 from thaJeztah/new-misty-handle moby/swarmkit@2bd294fc Update Misty's GitHub handle moby/swarmkit@0769c605 Comments for orphaned state/task reaper. (moby/swarmkit#2421) moby/swarmkit@de950a7e Generic resource cli (moby/swarmkit#2347) moby/swarmkit@312be598 Provide custom gRPC dialer to override default proxy dialer (moby/swarmkit#2419) moby/swarmkit@4f12bf79 Merge pull request moby/swarmkit#2415 from cheyang/master moby/swarmkit@8f9f7dc1 add pid limits moby/swarmkit@da5ee2a6 Merge pull request moby/swarmkit#2409 from dperny/workaround-attachments moby/swarmkit@0c7b2fc2 Delete node attachments when node is removed moby/swarmkit@9d702763 normalize "aarch64" architectures to "arm64" moby/swarmkit@28f91d8...ce5f7b8 Signed-off-by: Marcus Martins <marcus@docker.com>

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

anshulpundir force-pushed the orphaned branch 2 times, most recently from 1703dfb to 221f945 Compare October 31, 2017 01:21

anshulpundir requested a review from aaronlehmann October 31, 2017 01:22

aaronlehmann approved these changes Oct 31, 2017

View reviewed changes

anshulpundir force-pushed the orphaned branch from 221f945 to 64e6304 Compare October 31, 2017 07:15

Comments for orphaned state/task reaper.

e08257f

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

anshulpundir force-pushed the orphaned branch from 64e6304 to e08257f Compare October 31, 2017 08:12

anshulpundir merged commit 0769c60 into moby:master Oct 31, 2017

stevvooe reviewed Oct 31, 2017

View reviewed changes

anshulpundir commented Oct 31, 2017

View reviewed changes

marcusmartins mentioned this pull request Nov 3, 2017

Bump docker/swarmkit to ce5f7b8a6f5a1b1e4a29cafdf887304e400e08a9 moby/moby#35405

Closed

denverdino pushed a commit to AliyunContainerService/swarmkit that referenced this pull request Nov 5, 2017

Comments for orphaned state/task reaper. (moby#2421)

df8cdd2

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments for Task Reaper.#2421

Comments for Task Reaper.#2421
anshulpundir merged 1 commit intomoby:masterfrom
anshulpundir:orphaned

anshulpundir commented Oct 31, 2017

Uh oh!

anshulpundir commented Oct 31, 2017

Uh oh!

aaronlehmann left a comment

Uh oh!

aaronlehmann Oct 31, 2017

Uh oh!

aaronlehmann Oct 31, 2017

Uh oh!

aaronlehmann Oct 31, 2017

Uh oh!

aaronlehmann Oct 31, 2017

Uh oh!

codecov bot commented Oct 31, 2017 •

edited

Loading

Uh oh!

stevvooe Oct 31, 2017

Uh oh!

stevvooe Oct 31, 2017

Uh oh!

stevvooe commented Oct 31, 2017

Uh oh!

anshulpundir left a comment •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

anshulpundir commented Oct 31, 2017

Uh oh!

anshulpundir commented Oct 31, 2017

Uh oh!

aaronlehmann left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

aaronlehmann Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stevvooe Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

stevvooe Oct 31, 2017

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Oct 31, 2017

Uh oh!

anshulpundir left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Oct 31, 2017 •

edited

Loading

anshulpundir left a comment •

edited

Loading