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

Fix upgrade.sh image setup #34468

Merged
merged 1 commit into from
Oct 11, 2016
Merged

Fix upgrade.sh image setup #34468

merged 1 commit into from
Oct 11, 2016

Conversation

mtaufen
Copy link
Contributor

@mtaufen mtaufen commented Oct 10, 2016 •
edited by jessfraz
Loading

The original fix (#33147) sourced the correct node-helper.sh but set
node_os_distribution instead of NODE_OS_DISTRIBUTION. The
set-node-image function is imported indirectly via source "${KUBE_ROOT}/cluster/kube-util.sh", which in turn (in the GCE case)
sources cluster/gce/util.sh. Since the set-node-image function
relies on the NODE_OS_DISTRIBUTION variable, the original fix
did not have the entire intended effect.

I have confirmed that cherry-picking #33147 into the release-1.4
branch and layering this commit on top of it make for a successful
upgrade from a GCI based K8s 1.3 cluster to a GCI based K8s 1.4 cluster.

Fix upgrade.sh image setup

NOTE: This, along with #33147, should be cherry-picked into the release-1.4 branch.

This change is Reviewable

@mtaufen
Fix upgrade.sh image setup
263c54c 
The original fix (kubernetes#33147) sourced the correct `node-helper.sh` but set
`node_os_distribution` instead of `NODE_OS_DISTRIBUTION`. The
`set-node-image` function is imported indirectly via `source
"${KUBE_ROOT}/cluster/kube-util.sh"`, which in turn (in the GCE case)
sources `cluster/gce/util.sh`. Since the `set-node-image` function
relies on the `NODE_OS_DISTRIBUTION` variable, the original fix
did not have the entire intended effect.

I have confirmed that cherry-picking kubernetes#33147 into the `release-1.4`
branch and layering this commit on top of it make for a successful
upgrade from a GCI based K8s 1.3 cluster to a GCI based K8s 1.4 cluster.
@mtaufen
Copy link
Contributor Author

mtaufen commented Oct 10, 2016 •
edited
Loading

@vishh Just to be thorough, we should probably additionally check that KUBE_NODE_OS_DISTRIBUTION and KUBE_OS_DISTRIBUTION being different from NODE_OS_DISTRIBUTION won't affect anything, as those variables are also floating around in the env and won't be affected by this PR. We should probably just set all three in setup-base-image, but I still need to check that nothing relies on any of the three relevant vars prior to setup-base-image being called. But at least upgrades seem to succeed with this PR.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels Oct 10, 2016
@mtaufen mtaufen added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 10, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit 263c54c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

vishh
vishh reviewed Oct 10, 2016
View reviewed changes
cluster/gce/upgrade.sh
@@ -179,8 +179,8 @@ function upgrade-nodes() {
function setup-base-image() {
if [[ "${env_os_distro}" == "false" ]]; then
echo "== Ensuring that new Node base OS image matched the existing Node base OS image"
node_os_distribution=$(get-node-os "${NODE_NAMES[0]}")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was meant to be a local variable. I did not realize that it is actually necessary. Err......

@vishh
Copy link
Contributor

vishh commented Oct 10, 2016

@k8s-bot gke e2e test this github issue #IGNORE

@vishh
Copy link
Contributor

vishh commented Oct 10, 2016

@k8s-bot gci gke e2e test this github issue #IGNORE

@vishh vishh added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 10, 2016
@vishh
Copy link
Contributor

vishh commented Oct 10, 2016

@mtaufen get the tests to pass.

@mtaufen
Copy link
Contributor Author

mtaufen commented Oct 10, 2016

@vishh I'll keep an eye on them and figure it out.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit 263c54c. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@mtaufen mtaufen mentioned this pull request Oct 10, 2016
Closed
@mtaufen
Copy link
Contributor Author

mtaufen commented Oct 10, 2016

Seems failure is related to #34483, re-running tests

@mtaufen
Copy link
Contributor Author

mtaufen commented Oct 10, 2016

@k8s-bot gci gke test this

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 788787a into kubernetes:master Oct 11, 2016
@mtaufen mtaufen added this to the v1.4 milestone Oct 11, 2016
@mtaufen
Copy link
Contributor Author

mtaufen commented Oct 11, 2016

@jessfraz This should be cherrypicked into 1.4, pending cherry-pick of #33147

@jessfraz jessfraz added the cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. label Oct 11, 2016
@jessfraz
Copy link
Contributor

@mtaufen thanks will do

@jessfraz jessfraz added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Oct 11, 2016
@jessfraz jessfraz removed the release-note-none Denotes a PR that doesn't merit a release note. label Oct 11, 2016
@mtaufen
Copy link
Contributor Author

mtaufen commented Oct 11, 2016

Thanks :)

@jessfraz jessfraz mentioned this pull request Oct 12, 2016
Merged
k8s-github-robot pushed a commit that referenced this pull request Oct 13, 2016
Merge pull request #34628 from jessfraz/automated-cherry-pick-of-#33147-
616243c 
#34468-#34416-#34010-origin-release-1.4

Automatic merge from submit-queue

Automated cherry pick of #33147 #34468 #34416 #34010 origin release 1.4

Cherry pick of #33147 #34468 #34416 #34010 on release-1.4.

#33147: fix base image pinning during upgrades via
#34468: Fix upgrade.sh image setup
#34416: hyperkube image: add cifs-utils
#34010: Match GroupVersionKind against specific version
@jessfraz
Copy link
Contributor

cherry-picked from #34628

shyamjvs pushed a commit to shyamjvs/kubernetes that referenced this pull request Dec 1, 2016
Merge pull request kubernetes#34628 from jessfraz/automated-cherry-pi…
ddd27a2 
…ck-of-#33147-kubernetes#34468-kubernetes#34416-kubernetes#34010-origin-release-1.4

Automatic merge from submit-queue

Automated cherry pick of kubernetes#33147 kubernetes#34468 kubernetes#34416 kubernetes#34010 origin release 1.4

Cherry pick of kubernetes#33147 kubernetes#34468 kubernetes#34416 kubernetes#34010 on release-1.4.

kubernetes#33147: fix base image pinning during upgrades via
kubernetes#34468: Fix upgrade.sh image setup
kubernetes#34416: hyperkube image: add cifs-utils
kubernetes#34010: Match GroupVersionKind against specific version
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vishh vishh vishh approved these changes

Assignees

@vishh vishh

Labels
area/upgrade cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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
v1.4
Development

Successfully merging this pull request may close these issues.
7 participants
@mtaufen @k8s-ci-robot @vishh @k8s-github-robot @jessfraz @googlebot @wojtek-t