Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update nvidia-gpu-device-plugin to apps/v1 and use RollingUpdate updateStrategy. #64296

Merged
merged 1 commit into from
May 25, 2018
Merged

Update nvidia-gpu-device-plugin to apps/v1 and use RollingUpdate updateStrategy. #64296

merged 1 commit into from
May 25, 2018

Conversation

rohitagarwal003
Copy link
Member

@rohitagarwal003 rohitagarwal003 commented May 25, 2018 •
edited
Loading

Even though RollingUpdate is the default updateStrategy, we need to
specify it explicitly here because otherwise updating from
extensions/v1beta1 to apps/v1 doesn't change the updateStrategy.

Related to #57125 and #63634

Update nvidia-gpu-device-plugin DaemonSet config to use RollingUpdate updateStrategy instead of OnDelete.

/assign @vishh @jiayingz
/cc @janetkuo

@rohitagarwal003
Update nvidia-gpu-device-plugin to apps/v1 and use RollingUpdate upda…
5139bb5 
…teStrategy.

Even though RollingUpdate is the default updateStrategy, we need to
specify it explicitly here because otherwise updating from
extensions/v1beta1 to apps/v1 doesn't change the updateStrategy.
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 25, 2018
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 25, 2018
@janetkuo
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2018
@jiayingz
Copy link
Contributor

/lgtm
/approve

@rohitagarwal003
Copy link
Member Author

/assign @roberthbailey

@rohitagarwal003
Copy link
Member Author

/test pull-kubernetes-e2e-gke-device-plugin-gpu

@rohitagarwal003
Copy link
Member Author

/retest

@vishh
Copy link
Contributor

vishh commented May 25, 2018

/lgtm

@roberthbailey
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: janetkuo, jiayingz, mindprince, roberthbailey, vishh

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2018
@janetkuo
Copy link
Member

We need to cherrypick this change to 1.9 and 1.10, right?

@rohitagarwal003
Copy link
Member Author

Only to 1.10

@jiayingz
Copy link
Contributor

/test pull-kubernetes-kubemark-e2e-gce-big

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 8306b0b into kubernetes:master May 25, 2018
@k8s-ci-robot
Copy link
Contributor

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

Test name Commit Details Rerun command
pull-kubernetes-kubemark-e2e-gce-big 5139bb5 link /test pull-kubernetes-kubemark-e2e-gce-big

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.

@yujuhong
Copy link
Contributor

I think this broke all the node e2e jobs:
This function can no longer de-serialize the manifest into DaemonSet, and returns an empty string.
Node e2e image prepulling apparently cannot handle puling an empty string: https://github.com/kubernetes/kubernetes/blob/v1.12.0-alpha.0/test/e2e_node/image_list.go#L55

@jiayingz
Copy link
Contributor

@mindprince is ooo. I will submit a fix PR to modify DsFromManifest() to use &apps.DeamonSet{}.

@jiayingz
Copy link
Contributor

Curious why it wasn't caught by pull-kubernetes-node-e2e.

@jiayingz jiayingz mentioned this pull request May 25, 2018
Merged
@yujuhong
Copy link
Contributor

Curious why it wasn't caught by pull-kubernetes-node-e2e.

Because https://raw.githubusercontent.com/kubernetes/kubernetes/master/cluster/addons/device-plugins/nvidia-gpu/daemonset.yaml wouldn't be updated until the PR was merged. Given that, wouldn't changes like this affect all branches?

@jiayingz
Copy link
Contributor

Ah right. Now I remember. The 1.8 and 1.10 branches are using the yaml config from their own branches, but we seem to forget to update 1.9. I will submit a PR to update 1.9.

@yujuhong
Copy link
Contributor

Ah right. Now I remember. The 1.8 and 1.10 branches are using the yaml config from their own branches, but we seem to forget to update 1.9. I will submit a PR to update 1.9.

This seems a bit fragile :-(

@jiayingz
Copy link
Contributor

I know :(. Would be better to just retrieve the yaml from current source, if we can figure out how to do this...

k8s-github-robot pushed a commit that referenced this pull request May 26, 2018
Merge pull request #64340 from jiayingz/ds_decode_fix
15c22b5 
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Fix DsFromManifest() after we switch from extensions/v1beta1 to apps/v1

in cluster/addons/device-plugins/nvidia-gpu/daemonset.yaml.



**What this PR does / why we need it**:
Fixes broken node e2e jobs caused by #64296.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #

**Special notes for your reviewer**:

**Release note**:

```release-note

```
@yujuhong yujuhong mentioned this pull request May 29, 2018
Closed
@jiayingz jiayingz mentioned this pull request May 29, 2018
Merged
k8s-github-robot pushed a commit that referenced this pull request May 30, 2018
Merge pull request #64478 from jiayingz/automated-cherry-pick-of-#64296-
4d9a60b 
#64340-#64404-upstream-release-1.10

Automatic merge from submit-queue.

Automated cherry pick of #64296: Update nvidia-gpu-device-plugin to apps/v1 and use #64340: Fix DsFromManifest() after we switch from extensions/v1beta1 #64404: DaemonSet internals are still in extensions

Cherry pick of #64296 #64340 #64404 on release-1.10.

#64296: Update nvidia-gpu-device-plugin to apps/v1 and use
#64340: Fix DsFromManifest() after we switch from extensions/v1beta1
#64404: DaemonSet internals are still in extensions

```release-note
Fix nvidia-gpu-device-plugin DaemonSet config to ensure correct device plugin upgrade
```
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetkuo janetkuo janetkuo approved these changes

Assignees

@janetkuo janetkuo

@vishh vishh

@roberthbailey roberthbailey

@jiayingz jiayingz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@rohitagarwal003 @janetkuo @jiayingz @vishh @roberthbailey @k8s-ci-robot @k8s-github-robot @yujuhong