Disable service on release network#35960
Merged
Merged
Conversation
Contributor
Author
abhi
force-pushed
the
service
branch
2 times, most recently
from
f62807c to
33daf5a
Compare
thaJeztah
added
status/needs-vendoring
status/2-code-review
rebuild/*
and removed
status/0-triage
labels
abhi
force-pushed
the
service
branch
2 times, most recently
from
4ecaaca to
0ee3ec2
Compare
abhi
force-pushed
the
service
branch
2 times, most recently
from
9168de7 to
227ed57
Compare
Member
|
ping @abhi can you rebase this PR? Also looks like you added a file that shouldn't be there (I noticed theres a stack dump in there);
|
abhi
force-pushed
the
service
branch
2 times, most recently
from
189a44f to
f61d536
Compare
thaJeztah
added
area/networking
Member
|
Looks like you updated the PR; note that GitHub doesn't send out notifications when you update, so its best to comment or "ping" someone after you did so 😄 |
Contributor
Author
|
@thaJeztah the guy I need to ping sits next to me :D Will do next time. |
cpuguy83
requested changes
Jan 17, 2018
Member
There was a problem hiding this comment.
Nit: logrus.WithField("container", container.ID).WithError(err).WithField("sandbox", sid).Error("Error removing service from sandbox")
Member
There was a problem hiding this comment.
Or .WithFields();
logrus.WithFields(logrus.Fields{"container": container.ID, "sandbox": sid}).WithError(err).Error("Error removing service from sandbox")
Contributor
|
LGTM after the log change 👍 |
This PR contains a fix for moby#30321. There was a moby#31142 PR intending to fix the issue by adding a delay between disabling the service in the cluster and the shutdown of the tasks. However disabling the service was not deleting the service info in the cluster. Added a fix to delete service info from cluster and verified using siege to ensure there is zero downtime on rolling update of a service.In order to support it and ensure consitency of enabling and disable service knob from the daemon, we need to ensure we disable service when we release the network from the container. This helps in making the enable and disable service less racy. The corresponding part of libnetwork fix is part of moby/libnetwork#1824 Signed-off-by: abhi <[email protected]>
Signed-off-by: abhi <[email protected]>
Contributor
Author
|
@thaJeztah @cpuguy83 done. |
cpuguy83
approved these changes
Jan 18, 2018
Member
cpuguy83
left a comment
cpuguy83
left a comment
There was a problem hiding this comment.
LGTM
I'm not sure I like the ambiguity of the "service" term, but it is what it is.
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
5 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.
This PR contains a fix for #30321. There was a #31142
PR intending to fix the issue by adding a delay between disabling the
service in the cluster and the shutdown of the tasks. However
disabling the service was not deleting the service info in the cluster.
Added a fix to delete service info from cluster and verified using siege
to ensure there is zero downtime on rolling update of a service.In order
to support it and ensure consitency of enabling and disable service knob
from the daemon, we need to ensure we disable service when we release
the network from the container. This helps in making the enable and
disable service less racy. The corresponding part of libnetwork fix is
part of moby/libnetwork#1824
Signed-off-by: abhi [email protected]
- What I did
In order to ensure enable and disable a service on an sandbox less racy , explictly disable service on the sandbox during a container exit event on the network side. This entry point is the releaseNetwork action which triggers the tearing of the network resource for a container.
- How I did it
Added DisableService action on releaseNetwork
- How to verify it
This is particularly important during rolling update of a service and when the tasks in a swarm service goes down. Verified using siege and performing zero downtime rolling update.
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)