Use Service Placement Constraints in Enforcer#2857
Merged
dperny merged 1 commit intomoby:masterfrom May 20, 2019
Merged
Conversation
This patch updates the ConstraintEnforcer to use the placement constraints from the current service spec rather than the placement constraints from the current task spec because they may be outdated in some cases (e.g., when the service was previously updated to modify placement constrainst but the node to which the task was scheduled still satisfies the constraints. If the node is updated in a way which causes it to no longer satisfy the new constraints then the task should be removed even if it still would satisfy the original task constraints). Docker-DCO-1.1-Signed-off-by: Josh Hawn <josh.hawn@docker.com> (github: jlhawn)
dperny
approved these changes
May 17, 2019
| // original and outdated constraints (that are still satisfied by | ||
| // the node). If we used those original constraints then the task | ||
| // would incorrectly not be removed. This is why the constraints | ||
| // from the service spec should be used instead. |
Collaborator
There was a problem hiding this comment.
i'm just... so proud. what a beautiful comment. truly brings a tear to my eye.
| // longer exists), so we use the placement constraints from the | ||
| // original task spec. | ||
| placement = t.Spec.Placement | ||
| } |
Collaborator
There was a problem hiding this comment.
perfect, this will also cover the network attachment tasks (if you don't know what that is, don't worry about it)
Contributor
Author
There was a problem hiding this comment.
Yeah, it looks like the constraint enforcer isn't concerned with the task runtime in general.
|
|
||
| // The task should be rejected immediately. | ||
| task = testutils.WatchTaskUpdate(t, watch) | ||
| assert.Equal(t, api.TaskStateRejected, task.Status.State) |
Collaborator
There was a problem hiding this comment.
good test to round it out.
Collaborator
|
rebuilding CI, tests are Known Flaky, whoever maintains swarmkit should really get onto fixing those tests |
Codecov Report
@@ Coverage Diff @@
## master #2857 +/- ##
==========================================
+ Coverage 62.13% 62.22% +0.08%
==========================================
Files 139 139
Lines 22341 22352 +11
==========================================
+ Hits 13882 13908 +26
+ Misses 6979 6959 -20
- Partials 1480 1485 +5 |
dperny
added a commit
to dperny/docker
that referenced
this pull request
May 20, 2019
Revendors docker/swarmkit to backport fixes made to the ConstraintEnforcer (see moby/swarmkit#2857) Signed-off-by: Drew Erny <drew.erny@docker.com>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
May 21, 2019
Revendors docker/swarmkit to backport fixes made to the ConstraintEnforcer (see moby/swarmkit#2857) Signed-off-by: Drew Erny <drew.erny@docker.com> Upstream-commit: 56e92239a6f470fa410d7e09f2fe137b9b634de0 Component: engine
dperny
added a commit
to dperny/docker
that referenced
this pull request
May 24, 2019
Includes the following changes since last vendoring: moby/swarmkit#2795 - Add capabilities list to container specification moby/swarmkit#2845 - Fix linting error moby/swarmkit#2848 - Bump fernet/fernet-go moby/swarmkit#2856 - Add ListServiceStatuses grpc method moby/swarmkit#2857 - Use Service Placement Constraints in Enforcer Signed-off-by: Drew Erny <drew.erny@docker.com>
docker-jenkins
pushed a commit
to docker-archive/docker-ce
that referenced
this pull request
May 26, 2019
Includes the following changes since last vendoring: moby/swarmkit#2795 - Add capabilities list to container specification moby/swarmkit#2845 - Fix linting error moby/swarmkit#2848 - Bump fernet/fernet-go moby/swarmkit#2856 - Add ListServiceStatuses grpc method moby/swarmkit#2857 - Use Service Placement Constraints in Enforcer Signed-off-by: Drew Erny <drew.erny@docker.com> Upstream-commit: 67e25ec5ac568a893e444891a6a583fd2f996f76 Component: engine
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
2 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.
Description
What I did and how I did it
This patch updates the ConstraintEnforcer to use the placement constraints from the current service spec rather than the placement constraints from the current task spec because they may be outdated in some cases (e.g., when the service was previously updated to modify placement constrainst but the node to which the task was scheduled still satisfies the constraints. If the node is updated in a way which causes it to no longer satisfy the new constraints then the task should be removed even if it still would satisfy the original task constraints).
For the Changelog
I'm not sure how to concisely describe it, but how about:
Testing
This patch includes a new unit test in the
manager/orchestrator/constraintenforcerpackage.How to test