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

adjust the expected output based kubectl verison #56508

Merged
merged 1 commit into from
Nov 30, 2017
Merged

adjust the expected output based kubectl verison #56508

merged 1 commit into from
Nov 30, 2017

Conversation

mengqiy
Copy link
Member

@mengqiy mengqiy commented Nov 28, 2017

Fixes #56414

NONE

/assign @spiffxp

@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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 28, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Nov 28, 2017

/assign @pwittrock

@k8s-github-robot k8s-github-robot added the do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. label Nov 28, 2017
@mengqiy mengqiy mentioned this pull request Nov 28, 2017
Closed
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 29, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Nov 29, 2017

@spiffxp @pwittrock PTAL

@mengqiy
Copy link
Member Author

mengqiy commented Nov 29, 2017

/assign @janetkuo
because of #42849

janetkuo
janetkuo reviewed Nov 29, 2017
View reviewed changes
test/e2e/kubectl/kubectl.go
{"Controlled By:", "ReplicationController/redis-master"},
{"Image:", redisImage},
{"State:", "Running"},
{"QoS Class:", "BestEffort"},
}
}...)
Copy link
Member

Choose a reason for hiding this comment

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

From checkOutput code, I don't think the order matters. Would you check again? If so, we can just append it at the end.

Copy link
Member Author

Choose a reason for hiding this comment

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

The order matters here.
The index of the actual output always increases after a match, so the order matters.
https://github.com/mengqiy/kubernetes/blob/3d5ff42cc5cc0a71668e9490ae8cb14b644d02e7/test/e2e/kubectl/kubectl.go#L1759

Copy link
Member

Choose a reason for hiding this comment

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

ok

test/e2e/kubectl/kubectl.go Outdated
@@ -847,6 +847,9 @@ metadata:

// Pod
forEachPod(func(pod v1.Pod) {
gte, err := framework.KubectlVersionGTE(utilversion.MustParseSemantic("v1.9.0-alpha.0"))
Copy link
Member

Choose a reason for hiding this comment

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

This is affected by efbfead#diff-fde2ce0a73639d19347a5ea6fe001ba6 which is in v1.9.0-alpha.2

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Server version should be checked instead

@janetkuo
Copy link
Member

janetkuo commented Nov 29, 2017 •
edited

cc @crimsonfaith91: kubectl skew test failed due to the removal of created-by annotation

janetkuo
janetkuo reviewed Nov 29, 2017
View reviewed changes
test/e2e/kubectl/kubectl.go Outdated
@@ -847,6 +847,9 @@ metadata:

// Pod
forEachPod(func(pod v1.Pod) {
gte, err := framework.KubectlVersionGTE(utilversion.MustParseSemantic("v1.9.0-alpha.2"))
Copy link
Member

Choose a reason for hiding this comment

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

Server version should be checked instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Why checking server version? The describe behavior is purely client-side, isn't?

Copy link
Member

Choose a reason for hiding this comment

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

Responded here: #56508 (comment)

Copy link
Member

@janetkuo janetkuo Nov 29, 2017 •
edited

Choose a reason for hiding this comment

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

Actually, check both server and client. Neither 1.9 server nor 1.9 client will print "Created By:" in kubectl describe against any server/clients.

@jpbetz jpbetz added approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. and removed do-not-merge/cherry-pick-not-approved Indicates that a PR is not yet approved to merge into a release branch. labels Nov 29, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Nov 29, 2017

/retest

@janetkuo janetkuo removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 29, 2017
@janetkuo
Copy link
Member

janetkuo commented Nov 29, 2017 •
edited

In failing tests, 1.8 clients are tested against 1.9 servers (in fact 1.10/master, but technically it's 1.9), see log here: https://storage.googleapis.com/kubernetes-jenkins/logs/ci-kubernetes-e2e-gce-master-new-gci-kubectl-skew/3839/build-log.txt

As I mentioned in #56414 (comment), this is affected by #54445 (not #42849), where created-by annotation generation was removed from server side: https://github.com/kubernetes/kubernetes/pull/54445/files#diff-44bba2c68a6ea2b4ca0dbb7f4465828eL498

The failure is because 1.9 server doesn't add those created-by annotations, therefore when 1.8 client parses annotations, it doesn't print "Created By: xxx".

@mengqiy mengqiy force-pushed the fix18test branch 2 times, most recently from e241f46 to 7a658dc Compare November 29, 2017 23:46
@mengqiy
Copy link
Member Author

mengqiy commented Nov 29, 2017

Actually, check both server and client. Neither 1.9 server nor 1.9 client will print "Created By:" in kubectl describe against any server/clients.

Done.

janetkuo
janetkuo reviewed Nov 30, 2017
View reviewed changes
test/e2e/kubectl/kubectl.go
@@ -847,6 +847,10 @@ metadata:

// Pod
forEachPod(func(pod v1.Pod) {
servergte, err := framework.ServerVersionGTE(utilversion.MustParseSemantic("v1.9.0-alpha.2"), c.Discovery())
Copy link
Member

Choose a reason for hiding this comment

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

this err needs to be checked too

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@janetkuo
Copy link
Member

One last comment and LGTM otherwise

@crimsonfaith91
Copy link
Contributor

Everything looks good to me!
Thanks @mengqiy for working on this!

@mengqiy
Copy link
Member Author

mengqiy commented Nov 30, 2017

Someone please apply the lgtm label.

@crimsonfaith91
Copy link
Contributor

/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 30, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: crimsonfaith91, mengqiy
We suggest the following additional approver: janetkuo

Assign the PR to them by writing /assign @janetkuo in a comment when ready.

Associated issue: 56414

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

@mengqiy
Copy link
Member Author

mengqiy commented Nov 30, 2017

@jpbetz Can you please put this in the 1.8 milestone?

@jpbetz jpbetz added this to the v1.8 milestone Nov 30, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Nov 30, 2017

/kind cleanup
/priority critical-urgent
/sig cli

@k8s-ci-robot k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/cli Categorizes an issue or PR as relevant to SIG CLI. labels Nov 30, 2017
@mengqiy
Copy link
Member Author

mengqiy commented Nov 30, 2017

/status approved-for-milestone

@k8s-ci-robot
Copy link
Contributor

You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to add status labels.

@mengqiy
Copy link
Member Author

mengqiy commented Nov 30, 2017

The bot is asking for so many labels :-/

This pull request must have the status/approved-for-milestone label applied by a SIG maintainer.

The bot is asking for status/approved-for-milestone label. @janetkuo Can you help me with that since you are one of the sig maintainers.

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request Current

@crimsonfaith91 @janetkuo @mengqiy @pwittrock @spiffxp

Pull Request Labels
  • sig/cli: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Help

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue.

@k8s-github-robot k8s-github-robot merged commit f774bb9 into kubernetes:release-1.8 Nov 30, 2017
@mengqiy mengqiy deleted the fix18test branch November 30, 2017 21:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@janetkuo janetkuo janetkuo left review comments

Assignees

@spiffxp spiffxp

@janetkuo janetkuo

@pwittrock pwittrock

@crimsonfaith91 crimsonfaith91

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-none Denotes a PR that doesn't merit a release note. sig/cli Categorizes an issue or PR as relevant to SIG CLI. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@mengqiy @janetkuo @crimsonfaith91 @k8s-github-robot @k8s-ci-robot @spiffxp @jpbetz @pwittrock