[17.06] revert 0797241 and 5582db0#2556
Merged
Merged
Conversation
andrewhsu
changed the title
Contributor
|
we'll also need to revert #2476 |
Contributor
|
I've restarted the CI just to be double sure. |
| // this branch bounds the largest state achievable in the agent as SHUTDOWN, which | ||
| // is exactly the correct behavior for the agent. | ||
| if task.DesiredState >= api.TaskStateShutdown { | ||
| if task.DesiredState == api.TaskStateShutdown { |
Contributor
There was a problem hiding this comment.
I wonder if this should be reverted or not, because making it >= is a bug fix that should have been in SwarmKit anyway. WDYT @anshulpundir?
Contributor
There was a problem hiding this comment.
@andrewhsu can you make sure we're not reverting this line then?
Contributor
nishanttotla
left a comment
nishanttotla
left a comment
There was a problem hiding this comment.
Overall changes LGTM. I'm reviewing the task reaper code once again to make sure it is correctly reverted back to the old state.
Contributor
|
Ok test failure is a data race pointed out by the CI, but I don't see it as being related. @anshulpundir what is your opinion? |
| // Stop stops the TaskReaper and waits for the main loop to exit. | ||
| func (tr *TaskReaper) Stop() { | ||
| tr.cancelWatch() | ||
| // TODO(dperny) calling stop on the task reaper twice will cause a panic |
Contributor
There was a problem hiding this comment.
Not that it matters (this is just a comment), but the cherry pick commit doesn't modify this comment: 5582db0
Let's keep consistency so that things don't bother us later. (@andrewhsu)
Author
Contributor
There was a problem hiding this comment.
Ah okay, then ignore my comment.
Author
|
@anshulpundir looks like a backport of #2476 never made it to the |
nishanttotla
approved these changes
Mar 14, 2018
Contributor
nishanttotla
left a comment
nishanttotla
left a comment
There was a problem hiding this comment.
LGTM.
The test failure looks unrelated, but we can investigate that as testing is being done.
Contributor
|
Data race looks unrelated @nishanttotla |
andrewhsu
changed the title
Contributor
|
here's the commit likely leading to the data race detected in the CI run 700b319 @andrewhsu some of the code line numbers didn't make sense in the previous data race detection, so I'll wait to see if we hit it again before I analyze it further. |
anshulpundir
approved these changes
Mar 14, 2018
Codecov Report
@@ Coverage Diff @@
## bump_v17.06 #2556 +/- ##
===============================================
- Coverage 61.66% 61.11% -0.55%
===============================================
Files 42 120 +78
Lines 5692 20011 +14319
===============================================
+ Hits 3510 12230 +8720
- Misses 1841 6434 +4593
- Partials 341 1347 +1006 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Reverted 0797241 and 5582db0 from PR:
no conflicts