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.

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

Fix NodePort test #15766

Merged
merged 3 commits into from
Sep 18, 2017
Merged

Conversation

danwinship
Copy link
Contributor

Pulls in the k8s test utils fix from kubernetes/kubernetes#49025 to make the NodePort test work on non-cloud installs again.

Fixes #15253

@openshift-merge-robot openshift-merge-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 14, 2017
@danwinship
Copy link
Contributor Author

/assign deads2k
/test extended_networking

@openshift-merge-robot openshift-merge-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 Aug 14, 2017
@danwinship danwinship force-pushed the fix-nodeport-test branch 2 times, most recently from 1ca6a15 to 2e54c68 Compare August 15, 2017 14:18
@danwinship
Copy link
Contributor Author

The initial test run failed, but on further investigation, it was a pre-existing flake, #13108, not anything new. After adding a commit with a bit of debugging, it has passed multiple times...

@0xmichalis 0xmichalis removed their assignment Sep 3, 2017
@openshift-ci-robot
Copy link

openshift-ci-robot commented Sep 12, 2017

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

Test name Commit Details Rerun command
ci/openshift-jenkins/extended_networking 0ad4094 link /test extended_networking

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.

@danwinship
Copy link
Contributor Author

@stevekuznetsov some bot bugginess here: after I pushed some new commits, causing the tests to be rerun, it's now telling me that extended_networking failed... but it didn't actually re-run extended_networking, and the link points to an old test run that has since been GCed. (Which is probably the logs from the last time it did get run for this PR, but I don't know.)

@danwinship
Copy link
Contributor Author

/retest

@stevekuznetsov
Copy link
Contributor

Weird -- you will see the old link in the table while it edits it with new info, so it may seem new, but that should not have stuck around as we can see the networking test is green.

@stevekuznetsov
Copy link
Contributor

@Kargakis maybe a github call failed?

@danwinship
Copy link
Contributor Author

@stevekuznetsov extended_networking_minimal is green. It's complaining that the full extended_networking test failed, but it didn't run it again.

@0xmichalis
Copy link
Contributor

Hrm, the extended_networking job seems to be always_run: false .... I guess we want it to run on every PR?

@0xmichalis
Copy link
Contributor

For context, /retest is not working currenty for jobs that are not running by default (always_runs: true). There is an issue upstream but I can't seem to find it now.

@danwinship
Copy link
Contributor Author

No, we don't want it to run on every PR; extended_networking_minimal should run on every test (which it does) and extended_networking should only run when requested. I did previously request that it be run here, and it did get run, but then when a retest happened after pushing a new commit, the bot seemed to get confused (claiming that the test had failed even though it hadn't even run again)

@stevekuznetsov
Copy link
Contributor

Hmm yeah what you saw may have just been the bot refreshing the table -- it happens in those situations.

@0xmichalis
Copy link
Contributor

Yeah, the stale comment is another known issue.

@danwinship
Copy link
Contributor Author

@deads2k: master's open, can this get a lgtm?

@deads2k
Copy link
Contributor

deads2k commented Sep 18, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2017
@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, deads2k

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

@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2017
@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16226, 16377, 15766, 16299, 16153)

@openshift-merge-robot openshift-merge-robot merged commit e7ae92c into openshift:master Sep 18, 2017
@danwinship danwinship deleted the fix-nodeport-test branch October 19, 2017 19:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fix e2e test: Services should be able to create a functioning NodePort service
7 participants