New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

orchestrator: Check that service's SpecVersion exists before dereferencing #2233

Merged
merged 1 commit into from Jun 9, 2017

Conversation

Projects
None yet
4 participants
@aaronlehmann
Collaborator

aaronlehmann commented Jun 9, 2017

IsTaskDirty assumed that if a task had a SpecVersion, the corresponding service must also have a SpecVersion, since that is where the task's field is copied from. However, there are mixed-version scenarios where a service could get updated by a manager running an older version of swarmkit than the manager that created the task. Add a check to avoid a crash in this case.

orchestrator: Check that service's SpecVersion exists before derefere…
…ncing

IsTaskDirty assumed that if a task had a SpecVersion, the corresponding
service must also have a SpecVersion, since that is where the task's
field is copied from. However, there are mixed-version scenarios where a
service could get updated by a manager running an older version of
swarmkit than the manager that created the task. Add a check to avoid a
crash in this case.

Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@cyli

cyli approved these changes Jun 9, 2017

LGTM

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Jun 9, 2017

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2233      +/-   ##
==========================================
+ Coverage   60.23%   60.72%   +0.49%     
==========================================
  Files         124      124              
  Lines       20156    20156              
==========================================
+ Hits        12141    12240      +99     
+ Misses       6658     6543     -115     
- Partials     1357     1373      +16

codecov bot commented Jun 9, 2017

Codecov Report

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

@@            Coverage Diff             @@
##           master    #2233      +/-   ##
==========================================
+ Coverage   60.23%   60.72%   +0.49%     
==========================================
  Files         124      124              
  Lines       20156    20156              
==========================================
+ Hits        12141    12240      +99     
+ Misses       6658     6543     -115     
- Partials     1357     1373      +16
@cpuguy83

LGTM

Cool, I didn't want to just submit a PR to do this since I wasn't sure if the bug is that it sent a nil version or not

👍

@nishanttotla

LGTM

@nishanttotla nishanttotla merged commit 42cab91 into docker:master Jun 9, 2017

3 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/project 60.72% (target 0%)
Details
dco-signed All commits are signed

@aaronlehmann aaronlehmann deleted the aaronlehmann:spec-version-sanity branch Jun 9, 2017

@cyli cyli added this to the 17.06 milestone Jun 9, 2017

@cyli cyli referenced this pull request Jun 9, 2017

Merged

[17.06] Re-vendor swarmkit #43

3 of 3 tasks complete

cyli added a commit to cyli/docker-ce that referenced this pull request Jun 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment