gpusInUse info error when kubelet restarts #46087

tianshapjq · 2017-05-19T09:17:28Z

What this PR does / why we need it:
In my test, I found 2 errors in the nvidia_gpu_manager.go.

the number of activePods in gpusInUse() equals to 0 when kubelet restarts. It seems the Start() method was called before pods recovery which caused this error. So I decide not to call gpusInUse() in the Start() function, just let it happen when new pod needs to be created.
the container.ContainerID in line 242 returns the id in format of "docker://<container_id>", this will make the client failed to inspect the container by id. We have to erase the prefix of "docker://".

Special notes for your reviewer:

Release note:

Avoid assigning the same GPU to multiple containers.

k8s-ci-robot · 2017-05-19T09:17:36Z

Hi @tianshapjq. 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 @k8s-bot ok to test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

tianshapjq · 2017-05-19T09:19:28Z

@vishh could you verify this? thx :)

cmluciano · 2017-05-19T18:51:36Z

pkg/kubelet/gpu/nvidia/nvidia_gpu_manager.go

@@ -101,8 +102,7 @@ func (ngm *nvidiaGPUManager) Start() error {
 	if err := ngm.discoverGPUs(); err != nil {
 		return err
 	}
-	// It's possible that the runtime isn't available now.
-	ngm.allocated = ngm.gpusInUse()
+


I'm -1 on this. If containers recover and continue using GPUs, then allocatable will be wrong for the node.

but as the PR described, the number of activePods always remains 0 in the Start() function. And as the ngm.allocated has been initialized, it takes no chance to get the right data anymore.
If we don't erase this line from code, we have to call the Start() function late after the pods have been recovered so the activePods will not be 0 in the gpusInUse() function.

I tried to figure out the right place to call Start() function, but seems pods' synchronization is done by the statusManager concurrently, which means there is no explicit checkpoint to tell if the pods have been recovered. So, if we can not get the right info of active pods in gpusInUse(), the initialization of ngm.allocated will never be right when kubelet restarts.

Ok. I suppose things should suss themselves out in the unit & e2e tests. I think this should be helped a bit once checkpoints are added with #43240.

cmluciano · 2017-05-19T18:52:41Z

pkg/kubelet/gpu/nvidia/nvidia_gpu_manager.go

@@ -239,7 +239,7 @@ func (ngm *nvidiaGPUManager) gpusInUse() *podGPUs {
 		var containersToInspect []containerIdentifier
 		for _, container := range pod.Status.ContainerStatuses {
 			if containers.Has(container.Name) {
-				containersToInspect = append(containersToInspect, containerIdentifier{container.ContainerID, container.Name})
+				containersToInspect = append(containersToInspect, containerIdentifier{strings.Replace(container.ContainerID, "docker://", "", 1), container.Name})


I'm very surprised that this would not happen by default. @yujuhong Is this a subtle bug from dockertools -> dockershim ?

I wonder the same thing.

No, the code has always been like that.

vishh · 2017-05-20T11:27:45Z

@tianshapjq Thanks for the PR. Can you extend test/e2e_node/gpus.go to reproduce the issues this PR is trying to fix? I'd appreciate if you can unit tests as well.

tianshapjq · 2017-05-20T14:53:14Z

@vishh ok, I will do it~

vishh · 2017-05-22T05:14:55Z

Your idea of synchronizing allocated GPUs lazily SGTM. Kubelet start up has to be reorganized, but that's beyond the scope of a bug fix

…

On May 22, 2017 12:52 PM, "Drinky Pool" ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/kubelet/gpu/nvidia/nvidia_gpu_manager.go <#46087 (comment)> : > @@ -101,8 +102,7 @@ func (ngm *nvidiaGPUManager) Start() error { if err := ngm.discoverGPUs(); err != nil { return err } - // It's possible that the runtime isn't available now. - ngm.allocated = ngm.gpusInUse() + I tried to figure out the right place to call Start() function, but seems pods' synchronization is done by the statusManager concurrently, which means there is no explicit checkpoint to tell if the pods have been recovered. So, if we can not get the right info of active pods in gpusInUse(), the initialization of ngm.allocated will never be right when kubelet restarts. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#46087 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AGvIKOi_I3NMAV1MEYMtp5sqITapaYqTks5r8Ph-gaJpZM4NgRq1> .

vishh · 2017-05-22T06:36:51Z

@k8s-bot ok to test

tianshapjq · 2017-05-24T09:04:34Z

@vishh anything wrong with my commit?

cmluciano · 2017-05-24T14:39:47Z

I think we need some e2e to prove out this logic @tianshapjq

tianshapjq · 2017-05-25T01:04:46Z

@cmluciano I am working on it.

vishh · 2017-05-30T21:15:27Z

cc @mindprince

tianshapjq · 2017-06-02T09:27:25Z

@cmluciano I considered to create a new e2e test in such scene: 1. node should contain at least 2 gpu cards; 2. create a pod requesting one gpu card; 3. restart kubelet; 4. create another pod to request one gpu card; 5. these two pods should not be assigned the same gpu card. How do you think about this?
And still something I did not figure out: how to restart kubelet? Does the function createConfigMap() and updateConfigMap() restart the kubelet in somewhere?

cmluciano · 2017-06-02T19:47:57Z

@tianshapjq I took a stab at this in #46644

tianshapjq · 2017-06-05T08:01:06Z

@cmluciano @vishh I am woking on the e2e test in gpus.go, is there any method to get the devices of a container in pod? cause I want to check if two pods assigned the same gpu card when Kubelet restarts, but seems no attribute to expose these info.

cmluciano · 2017-06-05T16:05:05Z

@tianshapjq I'm not sure if this exists or not. We only have available GPUs in the current suite.

grodrigues3 · 2017-06-06T18:03:36Z

Looks like this is a bugfix (intended to go in for code freeze). If there was an issue filed for the bug, @tianshapjq please reference it in the PR body.

Otherwise, @vishh the change looks small enough that you can approve this PR going in with no issue via (/approve no-issue)

tianshapjq · 2017-06-07T00:54:08Z

@grodrigues3 there is no issue for this pr, I just created this pr directly. Do I need to create one?

vishh · 2017-06-07T00:59:19Z

/approve no-issue

vishh · 2017-06-07T00:59:58Z

I'm waiting on @mindprince to validate this PR with #46644

rohitagarwal003 · 2017-06-07T15:55:41Z

/retest

rohitagarwal003 · 2017-06-07T18:32:26Z

/lgtm
/approve

k8s-ci-robot · 2017-06-07T18:32:34Z

@mindprince: changing LGTM is restricted to assignees, and only kubernetes org members may be assigned issues.

In response to this:

/lgtm
/approve

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.

vishh · 2017-06-07T18:33:41Z

/lgtm

k8s-github-robot · 2017-06-07T18:43:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mindprince, tianshapjq, vishh

Associated issue requirement bypassed by: vishh

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/kubelet/OWNERS~~ [vishh]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-ci-robot · 2017-06-07T19:56:11Z

@tianshapjq: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-federation-e2e-gce	`f3b9874`	link	`@k8s-bot pull-kubernetes-federation-e2e-gce test this`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

rohitagarwal003 · 2017-06-07T20:02:53Z

/retest

k8s-github-robot · 2017-06-08T00:55:49Z

Automatic merge from submit-queue (batch tested with PRs 45877, 46846, 46630, 46087, 47003)

gpusInUse info error when kubelet restarts

f3b9874

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 19, 2017

k8s-github-robot assigned Random-Liu and tmrts May 19, 2017

k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels May 19, 2017

cmluciano reviewed May 19, 2017

View reviewed changes

vishh mentioned this pull request May 22, 2017

schedule different containers on the same gpu device #46070

Closed

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 22, 2017

vishh added this to the v1.7 milestone May 22, 2017

vishh added the kind/bug Categorizes issue or PR as related to a bug. label May 22, 2017

k8s-github-robot removed the release-note-label-needed label May 22, 2017

vishh added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 22, 2017

k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 22, 2017

vishh assigned cmluciano and vishh and unassigned tmrts and Random-Liu May 22, 2017

cmluciano mentioned this pull request May 30, 2017

WIP: Add GPU e2e_node test for checking kubelet restarts #46644

Closed

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 7, 2017

vishh unassigned cmluciano and vishh Jun 7, 2017

k8s-github-robot assigned pmorie and timstclair Jun 7, 2017

vishh assigned vishh and unassigned pmorie and timstclair Jun 7, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 7, 2017

rohitagarwal003 approved these changes Jun 7, 2017

View reviewed changes

k8s-github-robot merged commit 56baaaa into kubernetes:master Jun 8, 2017

vishh mentioned this pull request Jun 9, 2017

GPU e2es are failing #47216

Closed

rohitagarwal003 mentioned this pull request Jun 15, 2017

poll GPU status on initial installation #44463

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gpusInUse info error when kubelet restarts #46087

gpusInUse info error when kubelet restarts #46087

tianshapjq commented May 19, 2017 •

edited by vishh

Loading

k8s-ci-robot commented May 19, 2017

tianshapjq commented May 19, 2017

cmluciano May 19, 2017

tianshapjq May 20, 2017

tianshapjq May 22, 2017

cmluciano May 22, 2017

cmluciano May 19, 2017

WIZARD-CXY May 22, 2017

yujuhong May 24, 2017

vishh commented May 20, 2017

tianshapjq commented May 20, 2017

vishh commented May 22, 2017 via email

vishh commented May 22, 2017

tianshapjq commented May 24, 2017

cmluciano commented May 24, 2017

tianshapjq commented May 25, 2017

vishh commented May 30, 2017

tianshapjq commented Jun 2, 2017

cmluciano commented Jun 2, 2017

tianshapjq commented Jun 5, 2017

cmluciano commented Jun 5, 2017

grodrigues3 commented Jun 6, 2017

tianshapjq commented Jun 7, 2017

vishh commented Jun 7, 2017

vishh commented Jun 7, 2017

rohitagarwal003 commented Jun 7, 2017

rohitagarwal003 commented Jun 7, 2017

k8s-ci-robot commented Jun 7, 2017

vishh commented Jun 7, 2017

k8s-github-robot commented Jun 7, 2017

k8s-ci-robot commented Jun 7, 2017 •

edited

Loading

rohitagarwal003 commented Jun 7, 2017

k8s-github-robot commented Jun 8, 2017

gpusInUse info error when kubelet restarts #46087

gpusInUse info error when kubelet restarts #46087

Conversation

tianshapjq commented May 19, 2017 • edited by vishh Loading

k8s-ci-robot commented May 19, 2017

tianshapjq commented May 19, 2017

cmluciano May 19, 2017

Choose a reason for hiding this comment

tianshapjq May 20, 2017

Choose a reason for hiding this comment

tianshapjq May 22, 2017

Choose a reason for hiding this comment

cmluciano May 22, 2017

Choose a reason for hiding this comment

cmluciano May 19, 2017

Choose a reason for hiding this comment

WIZARD-CXY May 22, 2017

Choose a reason for hiding this comment

yujuhong May 24, 2017

Choose a reason for hiding this comment

vishh commented May 20, 2017

tianshapjq commented May 20, 2017

vishh commented May 22, 2017 via email

vishh commented May 22, 2017

tianshapjq commented May 24, 2017

cmluciano commented May 24, 2017

tianshapjq commented May 25, 2017

vishh commented May 30, 2017

tianshapjq commented Jun 2, 2017

cmluciano commented Jun 2, 2017

tianshapjq commented Jun 5, 2017

cmluciano commented Jun 5, 2017

grodrigues3 commented Jun 6, 2017

tianshapjq commented Jun 7, 2017

vishh commented Jun 7, 2017

vishh commented Jun 7, 2017

rohitagarwal003 commented Jun 7, 2017

rohitagarwal003 commented Jun 7, 2017

k8s-ci-robot commented Jun 7, 2017

vishh commented Jun 7, 2017

k8s-github-robot commented Jun 7, 2017

k8s-ci-robot commented Jun 7, 2017 • edited Loading

rohitagarwal003 commented Jun 7, 2017

k8s-github-robot commented Jun 8, 2017

tianshapjq commented May 19, 2017 •

edited by vishh

Loading

k8s-ci-robot commented Jun 7, 2017 •

edited

Loading