Skip to content

Fix manager/state/store.timedMutex#2603

Merged
cyli merged 1 commit into
moby:masterfrom
Adirio:patch-1
Apr 12, 2018
Merged

Fix manager/state/store.timedMutex#2603
cyli merged 1 commit into
moby:masterfrom
Adirio:patch-1

Conversation

@Adirio
Copy link
Copy Markdown
Contributor

@Adirio Adirio commented Apr 11, 2018

- What I did
Fix for #2602

- How I did it
Move the conflicting operation inside the critical section (before the Unlock() call).

- How to test it

- Description for the changelog
Fix #2602: timedMutex critical operation outside of critical section

@GordonTheTurtle
Copy link
Copy Markdown

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "patch-1" git@github.com:Adirio/swarmkit.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

Signed-off-by: Adrián Orive <aorive@ikerlan.es>

#2602
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 11, 2018

Codecov Report

Merging #2603 into master will increase coverage by 0.58%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2603      +/-   ##
==========================================
+ Coverage   61.58%   62.16%   +0.58%     
==========================================
  Files         134      134              
  Lines       21763    21763              
==========================================
+ Hits        13403    13530     +127     
+ Misses       6918     6778     -140     
- Partials     1442     1455      +13

@Adirio
Copy link
Copy Markdown
Contributor Author

Adirio commented Apr 11, 2018

ci/circleci reporting:

time="2018-04-11T12:33:11Z" level=error msg="task allocation failure" error="failed to retrieve network testID3 while allocating task testTaskID3"
time="2018-04-11T12:33:11Z" level=error msg="Failed allocation for network testID5" error="failed while allocating driver state for network testID5: could not obtain vxlan id for pool 10.0.4.0/24: requested bit is already allocated"
time="2018-04-11T12:33:11Z" level=error msg="task allocation failure" error="network testID5 attached to task testTaskID6 not allocated yet"
time="2018-04-11T12:33:12Z" level=error msg="Failed allocation for service testServiceID4" error="requested bit is already allocated"
time="2018-04-11T12:33:12Z" level=error msg="task allocation failure" error="service testServiceID4 to which this task testTaskID7 belongs has pending allocations"
--- FAIL: TestNodeAllocator (0.03s)
	allocator_test.go:1427: timed out before watchNode found expected node state
FAIL
coverage: 71.8% of statements
FAIL	github.com/docker/swarmkit/manager/allocator	4.328s
make: *** [coverage] Error 1

Is this change really making the difference?

@cyli cyli requested a review from aaronlehmann April 11, 2018 19:54
Copy link
Copy Markdown
Contributor

@cyli cyli left a comment

Choose a reason for hiding this comment

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

@Adirio Thanks for finding and submitting this fix. The TestNodeAllocator is a flakey test, and the failure is probably unrelated. I've kicked off a rebuild.

LGTM pending CI - this seems like if there is a context switch at an inopportune time, it could potentially cause swarm to occasionally fail to detect a wedge, since LockedAt().IsZero() would return true and hence Wedged() would return false. I can't think of a good way to unit test this change, though.

@Adirio
Copy link
Copy Markdown
Contributor Author

Adirio commented Apr 12, 2018

Thank you two for the revission. I changed the PR description to incorporate the just merged template.

@cyli
Copy link
Copy Markdown
Contributor

cyli commented Apr 12, 2018

Thanks @aaronlehmann and @Adirio!

@cyli cyli merged commit 9c2aa15 into moby:master Apr 12, 2018
@Adirio Adirio deleted the patch-1 branch April 13, 2018 05:33
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Apr 17, 2018
Relevant changes:

- moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership
- moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown
- moby/swarmkit#2561 Avoid predefined error log
- moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned
- moby/swarmkit#2587 [fips] Agent reports FIPS status
- moby/swarmkit#2603 Fix manager/state/store.timedMutex

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request Apr 18, 2018
Relevant changes:

- moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership
- moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown
- moby/swarmkit#2561 Avoid predefined error log
- moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned
- moby/swarmkit#2587 [fips] Agent reports FIPS status
- moby/swarmkit#2603 Fix manager/state/store.timedMutex

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 333b2f28fef4ba857905e7263e7b9bbbf7c522fc
Component: engine
thaJeztah added a commit to thaJeztah/docker-ce that referenced this pull request Apr 19, 2018
Relevant changes:

- moby/swarmkit#2551 RoleManager will remove deleted nodes from the cluster membership
- moby/swarmkit#2574 Scheduler/TaskReaper: handle unassigned tasks marked for shutdown
- moby/swarmkit#2561 Avoid predefined error log
- moby/swarmkit#2557 Task reaper should delete tasks with removed slots that were not yet assigned
- moby/swarmkit#2587 [fips] Agent reports FIPS status
- moby/swarmkit#2603 Fix manager/state/store.timedMutex

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 333b2f28fef4ba857905e7263e7b9bbbf7c522fc)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
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.

4 participants