Skip to content

[manager/orchestrator/task_reaper] Fix task reaper test to also set the desired state on tasks to prevent reconciliation races#2526

Merged
nishanttotla merged 1 commit into
moby:masterfrom
anshulpundir:reaper
Feb 26, 2018
Merged

[manager/orchestrator/task_reaper] Fix task reaper test to also set the desired state on tasks to prevent reconciliation races#2526
nishanttotla merged 1 commit into
moby:masterfrom
anshulpundir:reaper

Conversation

@anshulpundir
Copy link
Copy Markdown
Contributor

We force set the task state in the unit-tests to verify task reaper behavior, but don't set the desired state, which triggers reconciliation which can race with further task related changes triggered from the unit-test.

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

…he desired state on tasks to revent reconciliation races.

Signed-off-by: Anshul Pundir <anshul.pundir@docker.com>
@anshulpundir
Copy link
Copy Markdown
Contributor Author

#2510 addresses this, but that might not go in for a while.

@codecov
Copy link
Copy Markdown

codecov Bot commented Feb 23, 2018

Codecov Report

Merging #2526 into master will increase coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #2526      +/-   ##
==========================================
+ Coverage   61.35%   61.36%   +<.01%     
==========================================
  Files         133      133              
  Lines       21726    21726              
==========================================
+ Hits        13330    13332       +2     
- Misses       6952     6962      +10     
+ Partials     1444     1432      -12

@dperny
Copy link
Copy Markdown
Collaborator

dperny commented Feb 26, 2018

LGTM


// Set both tasks to COMPLETED.
updatedTask3 := observedTask1.Copy()
updatedTask3.DesiredState = api.TaskStateCompleted
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm in favor of setting this to api.TaskStateShutdown because this is what happens in actual behavior (even though the test would work fine with this).

Copy link
Copy Markdown
Contributor

@nishanttotla nishanttotla left a comment

Choose a reason for hiding this comment

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

LGTM (since my comment doesn't really affect this test as such)

@nishanttotla nishanttotla merged commit 322181a into moby:master Feb 26, 2018
@anshulpundir anshulpundir deleted the reaper branch February 26, 2018 19:08
@anshulpundir anshulpundir restored the reaper branch June 19, 2018 18:24
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