Skip to content

Fix networking-related test exclusions#382

Merged
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
danwinship:fix-network-test-exclusions
Oct 16, 2020
Merged

Fix networking-related test exclusions#382
openshift-merge-robot merged 2 commits intoopenshift:masterfrom
danwinship:fix-network-test-exclusions

Conversation

@danwinship
Copy link

The networking-related test skips got broken in the 1.19 rebase. We have logic set up to let us to distinguish "tests that don't pass when using openshift-sdn" and "tests that don't pass when using ovn-kubernetes", but a bunch of tests just got marked as "tests that OpenShift can't pass no matter what" in the 1.19 rebase (and so specifically, we are now skipping tests in the ovn-kubernetes jobs that should not be getting skipped, because ovn-kubernetes does implement those features, and we want to test that).

Also, for some reason, the network-plugin-specific annotations were left in origin even though they only apply to upstream k8s tests. I have moved them to openshift/kubernetes in this PR, assuming that the comments in openshift/origin are correct, and that we are supposed to have all k8s skips here and all origin skips in origin. Once this merges I'll submit another PR to remove the redundant skips in origin.

@danwinship
Copy link
Author

I guess this repo probably doesn't have the magic to revendor itself into origin and run CI with that? So maybe I should submit a WIP PR there...

@danwinship danwinship force-pushed the fix-network-test-exclusions branch 2 times, most recently from dac4ad0 to eb4c4e9 Compare October 2, 2020 14:37
@danwinship danwinship force-pushed the fix-network-test-exclusions branch from eb4c4e9 to 42ec323 Compare October 2, 2020 16:25
@dcbw
Copy link

dcbw commented Oct 8, 2020

/retest

@dcbw
Copy link

dcbw commented Oct 8, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2020
@dcbw
Copy link

dcbw commented Oct 9, 2020

/retest

@danwinship
Copy link
Author

/hold
(waiting for debugging on the origin PR)

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2020
@dcbw
Copy link

dcbw commented Oct 9, 2020

@danwinship hmm, are the regexes failing or something? k8s-e2e-gcp fails on the network tests that you're moving around here.

@danwinship
Copy link
Author

oh
the logic for, eg [Skipped:Network/OpenShiftSDN] is in openshift-tests, so it doesn't get used here

I guess we could just have the ci/prow/k8s-e2e-gcp job hardcode -ginkgo.skip "\[Skipped:Network/OpenShiftSDN\]" or something... @marun? (Explained more above, but basically openshift/kubernetes is forcing openshift-tests to skip any job that openshift-sdn can't pass, even in CI jobs that aren't using openshift-sdn.)

The NetworkPolicy tests work by trying to connect to a service by its
name, which means that for the tests that involved creating egress
policies, it had to always create an extra rule allowing egress for
DNS, but this assumed that DNS was running on UDP port 53. If it was
running somewhere else (eg if you changed the CoreDNS pods to use port
5353 to avoid needing to give them the NET_BIND_SERVICE capability)
then the NetworkPolicy tests would fail.

Fix this by making the tests connect to their services by IP rather
than by name, and removing all the DNS special-case rules. There are
other tests that ensure that Service DNS works.
@danwinship danwinship force-pushed the fix-network-test-exclusions branch from 42ec323 to bd3bb6f Compare October 9, 2020 20:36
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Oct 9, 2020
@marun
Copy link

marun commented Oct 9, 2020

@danwinship You'll need to run make update to update the generated annotations.

Tests that fail on openshift-sdn specifically should be tagged as
such, so that they don't also get skipped when running under
ovn-kubernetes or third-party network plugins.
@danwinship danwinship force-pushed the fix-network-test-exclusions branch from bd3bb6f to 6232c70 Compare October 10, 2020 13:05
@danwinship
Copy link
Author

@danwinship You'll need to run make update to update the generated annotations.

oh, no, I had done that (well, I did ./openshift-hack/update-test-annotations.sh). But it drops the [Suite:k8s] from every test when I do that because my working directory is .../openshift/kubernetes/... not .../k8s.io/kubernetes/.... Anyway, fixed now.

@danwinship
Copy link
Author

/retest

@openshift-ci-robot
Copy link

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

Test name Commit Details Rerun command
ci/prow/k8s-e2e-gcp 6232c70 link /test k8s-e2e-gcp
ci/prow/e2e-cmd 6232c70 link /test e2e-cmd
ci/prow/e2e-aws-serial 6232c70 link /test e2e-aws-serial
ci/prow/e2e-azure-upgrade 6232c70 link /test e2e-azure-upgrade

Full PR test history. Your PR dashboard.

Details

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
Author

/retest
the tests here seem to be pretty flaky, but I think this is ready to go

@marun
Copy link

marun commented Oct 15, 2020

/lgtm
/retest

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2020
@marun marun requested a review from mfojtik October 15, 2020 06:15
@mfojtik
Copy link

mfojtik commented Oct 15, 2020

/approve

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dcbw, marun, mfojtik

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 15, 2020
@danwinship
Copy link
Author

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 16, 2020
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 0c1205c into openshift:master Oct 16, 2020
@danwinship danwinship deleted the fix-network-test-exclusions branch December 4, 2020 16:37
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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants