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 validate-cluster.sh #47773

Merged
merged 1 commit into from
Jun 21, 2017
Merged

fix validate-cluster.sh #47773

merged 1 commit into from
Jun 21, 2017

Conversation

caesarxuchao
Copy link
Member

attempt to fix #47379.

Without this fix, the validate-cluster.sh never retries if kubectl-retry get cs fails.

cc @dchen1107

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 20, 2017
@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 Jun 20, 2017
@gmarek gmarek added this to the v1.7 milestone Jun 20, 2017
gmarek
gmarek reviewed Jun 20, 2017
View reviewed changes
cluster/validate-cluster.sh Outdated
@@ -170,7 +170,7 @@ while true; do
done

echo "Validate output:"
kubectl_retry get cs
return_value=$(kubectl_retry get cs)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is wrong. return_value is used in exit call, which expects integer, plus you shouldn't overwrite what's currently stored in return_value.

IIUC your main goal is to to prevent errexit to kill the script when get cs fails. If that's the case you should write:

echo $(kubectl_retry get cs || true)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. Thanks.

@caesarxuchao
Copy link
Member Author

/release-note-none

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Jun 20, 2017
@krousey
Copy link
Contributor

krousey commented Jun 20, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 20, 2017
@caesarxuchao
Copy link
Member Author

@gmarek @krzyzacy could you take another look? Thanks.

@krzyzacy
Copy link
Member

/lgtm

@krzyzacy
Copy link
Member

/assign @mikedanese

@mikedanese
Copy link
Member

/approve

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 20, 2017
@marun
Copy link
Contributor

marun commented Jun 20, 2017

/retest

@dchen1107
Copy link
Member

Can we run validation_cluster.sh with the new change against the existing k8s cluster to verify the change? upgrade.sh is flaky. Without any change, the new run of
both master and nodes are successfully upgraded to 1.7 in a new run: https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/ci-kubernetes-e2e-gce-1-6-1-7-upgrade-cluster/55?log#log

I0620 10:54:17.484] == Finished upgrading nodes to v1.7.0-beta.2.120+c8cd7f318aea33. ==
I0620 10:54:17.484] == Validating cluster post-upgrade ==
I0620 10:54:17.519] Validating gce cluster, MULTIZONE=
I0620 10:54:17.520] Project: gce-up-c1-3-g1-4-up-clu
I0620 10:54:17.520] Zone: us-central1-f
I0620 10:54:18.571] Found 4 node(s).
I0620 10:54:18.774] NAME                              STATUS                     AGE       VERSION
I0620 10:54:18.774] bootstrap-e2e-master              Ready,SchedulingDisabled   13m       v1.7.0-beta.2.120+c8cd7f318aea33
I0620 10:54:18.774] bootstrap-e2e-minion-group-4jm4   Ready                      8m        v1.7.0-beta.2.120+c8cd7f318aea33
I0620 10:54:18.774] bootstrap-e2e-minion-group-gw1d   Ready                      4m        v1.7.0-beta.2.120+c8cd7f318aea33
I0620 10:54:18.774] bootstrap-e2e-minion-group-km98   Ready                      23s       v1.7.0-beta.2.120+c8cd7f318aea33
I0620 10:54:19.486] Validate output:
I0620 10:54:20.191] NAME                 STATUS    MESSAGE              ERROR
I0620 10:54:20.191] controller-manager   Healthy   ok                   
I0620 10:54:20.191] scheduler            Healthy   ok                   
I0620 10:54:20.191] etcd-0               Healthy   {"health": "true"}   
I0620 10:54:20.191] etcd-1               Healthy   {"health": "true"}   
I0620 10:54:20.195] Cluster validation succeeded
I0620 10:54:20.195] == Post-Upgrade Node OS and Kubelet Versions ==
I0620 10:54:20.395] name: "bootstrap-e2e-master", osImage: "Container-Optimized OS from Google", kubeletVersion: "v1.7.0-beta.2.120+c8cd7f318aea33"
I0620 10:54:20.395] name: "bootstrap-e2e-minion-group-4jm4", osImage: "Debian GNU/Linux 7 (wheezy)", kubeletVersion: "v1.7.0-beta.2.120+c8cd7f318aea33"
I0620 10:54:20.395] name: "bootstrap-e2e-minion-group-gw1d", osImage: "Debian GNU/Linux 7 (wheezy)", kubeletVersion: "v1.7.0-beta.2.120+c8cd7f318aea33"
I0620 10:54:20.395] name: "bootstrap-e2e-minion-group-km98", osImage: "Debian GNU/Linux 7 (wheezy)", kubeletVersion: "v1.7.0-beta.2.120+c8cd7f318aea33"

@dchen1107
Copy link
Member

/lgtm

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, dchen1107, krousey, krzyzacy, mikedanese

Associated issue: 47379

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

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

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

@dchen1107
Copy link
Member

@caesarxuchao and I did validation this change against a non-exist cluster. It works as expected. Thanks!

@caesarxuchao
Copy link
Member Author

/test pull-kubernetes-federation-e2e-gce

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit 5ca33f5 into kubernetes:master Jun 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmarek gmarek gmarek left review comments

Assignees

@krousey krousey

@krzyzacy krzyzacy

@gmarek gmarek

@mikedanese mikedanese

@dchen1107 dchen1107

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-none Denotes a PR that doesn't merit a release note. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

test failure: cluster 1.6->1.7 upgrade should maintain a functioning cluster [Feature:ClusterUpgrade]
9 participants
@caesarxuchao @krousey @krzyzacy @mikedanese @marun @dchen1107 @k8s-github-robot @gmarek @k8s-ci-robot