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

Guard the ready replica checking by server version #37303

Merged
merged 1 commit into from
Nov 23, 2016
Merged

Guard the ready replica checking by server version #37303

merged 1 commit into from
Nov 23, 2016

Conversation

krousey
Copy link
Contributor

@krousey krousey commented Nov 22, 2016 •
edited by k8s-oncall
Loading

I fixed replica readiness checking for 1.4->1.5 upgrades by using a field that only exists in versions >=1.4.0 in #36924

This fixed a lot of issues in 1.4->1.5 upgrade testing, but did not fix 1.3->1.5 upgrade tests. I've disabled replica checking for 1.3 masters as the old logic was broken anyway.

This will not affect the 1.3 CI tests. Just 1.3 -> {1.4, 1.5} upgrade tests.

https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/logs/kubernetes-e2e-gke-container_vm-1.3-container_vm-1.5-upgrade-cluster-new/330?log
is an example of this breakage. This is the tell-tale logs:

Nov 22 09:40:50.469: INFO: 11 / 11 pods in namespace 'kube-system' are running and ready (506 seconds elapsed)
Nov 22 09:40:50.469: INFO: expected 5 pod replicas in namespace 'kube-system', 0 are Running and Ready.
Nov 22 09:40:50.469: INFO: POD  NODE  PHASE  GRACE  CONDITIONS

This change is Reviewable

Guard the ready replica checking by server version
1614b97 
This disables ready replica checking for 1.3 masters, but only from 1.4
or 1.5 clients. The old logic was broken anyway due to overlapping
labels with replica sets.
@krousey krousey added area/test release-blocker release-note-none Denotes a PR that doesn't merit a release note. labels Nov 22, 2016
@krousey krousey added this to the v1.5 milestone Nov 22, 2016
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 22, 2016
@k8s-github-robot k8s-github-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Nov 22, 2016
@krousey
Copy link
Contributor Author

krousey commented Nov 22, 2016

cc @saad-ali

@saad-ali
Copy link
Member

cc @saad-ali

@spxtr can you take a look this will help fix upgrade tests for 1.5

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit 1614b97. Full PR test history.

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

spxtr
spxtr reviewed Nov 22, 2016
View reviewed changes
test/e2e/framework/util.go
@@ -493,6 +495,14 @@ func WaitForPodsSuccess(c clientset.Interface, ns string, successPodLabels map[s
// and some in Success. This is to allow the client to decide if "Success"
// means "Ready" or not.
func WaitForPodsRunningReady(c clientset.Interface, ns string, minPods int32, timeout time.Duration, ignoreLabels map[string]string) error {

// This can be removed when we no longer have 1.3 servers running with upgrade tests.
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO(krousey)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#37310

@spxtr
Copy link
Contributor

spxtr commented Nov 22, 2016

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 22, 2016
@krousey krousey mentioned this pull request Nov 22, 2016
Closed
@krousey
Copy link
Contributor Author

krousey commented Nov 22, 2016

@saad-ali: This should also be cherry-picked into 1.4 since the other one was.

@krousey
Copy link
Contributor Author

krousey commented Nov 22, 2016

@k8s-bot kubemark e2e test this

@krousey
Copy link
Contributor Author

krousey commented Nov 22, 2016

cc @roberthbailey

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@saad-ali
Copy link
Member

@saad-ali: This should also be cherry-picked into 1.4 since the other one was.

Ack. Once this is merged to 1.5, we can slap a 1.4 milestone on there.

CC @jessfraz

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-ci-robot
Copy link
Contributor

Jenkins Bazel Build failed for commit 1614b97. Full PR test history.

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

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit e69b497 into kubernetes:master Nov 23, 2016
@saad-ali saad-ali modified the milestones: v1.4, v1.5 Nov 23, 2016
@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 Nov 23, 2016
@jessfraz jessfraz self-assigned this Nov 23, 2016
@krousey
Copy link
Contributor Author

krousey commented Nov 28, 2016

@jessfraz did this ever make it into 1.4?

@jessfraz
Copy link
Contributor

looks like it has not been cherry-picked yet, but we can get it in for the next release

@jessfraz jessfraz mentioned this pull request Dec 1, 2016
Merged
@jessfraz
Copy link
Contributor

jessfraz commented Dec 1, 2016

cherry-picked in #37864

@jessfraz jessfraz 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 Dec 2, 2016
jessfraz added a commit that referenced this pull request Dec 2, 2016
@jessfraz
Merge pull request #37864 from jessfraz/automated-cherry-pick-of-#37248-
42e2ce2 
#37401-#37362-#37303-origin-release-1.4

Automated cherry pick of #37248 #37401 #37362 #37303 origin release 1.4
@chentao1596 chentao1596 mentioned this pull request Dec 5, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@spxtr spxtr spxtr left review comments

Assignees

@jessfraz jessfraz

@spxtr spxtr

Labels
area/test cherry-pick-approved Indicates a cherry-pick PR into a release branch has been approved by the release branch manager. 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-blocker release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
v1.4
Development

Successfully merging this pull request may close these issues.
6 participants
@krousey @saad-ali @k8s-ci-robot @spxtr @k8s-github-robot @jessfraz