Fixing a small bug with GMSA support #74737
Merged
Conversation
This file contains 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
k8s-ci-robot
added
release-note-none
|
Hi @wk8. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
area/kubelet
sig/node
This was referenced Feb 28, 2019
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
A previous PR (kubernetes#73726) added GMSA support to the dockershim. Unfortunately, there was a bug in there: the registry keys used to pass the cred specs down to Docker were being cleaned up too early, right after the containers' creation - before Docker would ever try to read them, when trying to actually start the container. This patch fixes this. An e2e test is also provided in a separate PR. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
wk8
force-pushed
the
wk8/gmsa_bug_fix
branch
from
9bfd260 to
1908a0c
Compare
|
/sig windows |
k8s-ci-robot
added
the
sig/windows
|
/test pull-kubernetes-e2e-gce-100-performance |
michmike
approved these changes
Feb 28, 2019
|
/lgtm |
k8s-ci-robot
added
the
lgtm
k8s-ci-robot
removed
the
lgtm
|
@yujuhong @michmike @derekwaynecarr : thanks a lot for the reviews. Addressed your comments. Could you please have another look? |
michmike
approved these changes
Mar 2, 2019
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @derekwaynecarr |
|
/lgtm Leaving the approval for @derekwaynecarr |
|
thank you for the clarifications. /approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: derekwaynecarr, wk8 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
|
/milestone v1.14 |
wk8
added a commit
to wk8/kubernetes
that referenced
this pull request
Mar 7, 2019
This patch is adding an e2e on the Windows GMSA support added in kubernetes#73726 and kubernetes#74737. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
wk8
added a commit
to wk8/kubernetes
that referenced
this pull request
May 14, 2019
kubernetes#74737 introduced a new in-memory map for the dockershim, that could potentially (in pathological cases) cause memory leaks - for containers that use GMSA cred specs, get created successfully, but then never get started nor removed. This patch adds a new goroutine in the dockershim that periodically cleans up stale `containerCleanupInfos`. Resolves issue kubernetes#74843. Added unit tests. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
wk8
added a commit
to wk8/kubernetes
that referenced
this pull request
Aug 5, 2019
kubernetes#74737 introduced a new in-memory map for the dockershim, that could potentially (in pathological cases) cause memory leaks - for containers that use GMSA cred specs, get created successfully, but then never get started nor removed. This patch adds a new goroutine in the dockershim that periodically cleans up stale `containerCleanupInfos`. Resolves issue kubernetes#74843. Added unit tests. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
wk8
added a commit
to wk8/kubernetes
that referenced
this pull request
Aug 23, 2019
kubernetes#74737 introduced a new in-memory map for the dockershim, that could potentially (in pathological cases) cause memory leaks - for containers that use GMSA cred specs, get created successfully, but then never get started nor removed. This patch addresses this issue by making container removal fail altogether when platform-specific clean ups fail: this allows clean ups to be retried later, when the kubelet attempts to remove the container again. Resolves issue kubernetes#74843. Signed-off-by: Jean Rouge <rougej+github@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What type of PR is this?
/kind bug
What this PR does / why we need it:
A previous PR (#73726)
added GMSA support to the dockershim. Unfortunately, there was a
bug in there: the registry keys used to pass the cred specs down
to Docker were being cleaned up too early, right after the containers'
creation - before Docker would ever try to read them, when trying to
actually start the container.
This patch fixes this.
An e2e test is also provided in a separate PR (#74738)
Does this PR introduce a user-facing change?: