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

NetworkPolicy tests #12991

Merged
merged 3 commits into from
Aug 10, 2017
Merged

NetworkPolicy tests #12991

merged 3 commits into from
Aug 10, 2017

Conversation

danwinship
Copy link
Contributor

@danwinship danwinship commented Feb 16, 2017 •
edited
Loading

This pulls in the NetworkPolicy tests from kubernetes/kubernetes#35660, and then makes a few changes to get it to work with our extended networking test, and to get it to actually pass given the current state of our NetworkPolicy support; hopefully the last commit will be able to be reverted at some point.

@danwinship
Copy link
Contributor Author

[testextended][extended:networking]

@danwinship
Copy link
Contributor Author

node startup is failing for networkpolicy:

Feb 16 15:50:08 nettest-node-1 openshift[359]: I0216 15:50:08.281390     359 ovs.go:62] Executing: ovs-ofctl -O OpenFlow13 add-flow br0 table=21, priority=100, ip, actions=ct(commit,table=30)
Feb 16 15:50:08 nettest-node-1 openshift[359]: I0216 15:50:08.285473     359 ovs.go:66] Error executing ovs-ofctl: OFPT_ERROR (OF1.3) (xid=0x2): OFPBAC_BAD_TYPE

It works fine for me locally, and this is inside the dind image, so it ought to be exactly the same software...

@danwinship
Copy link
Contributor Author

Oh, right, the OVS userland is in dind and so is 2.5, but the kernel side of it is in the host and so is still 2.4 here.

@danwinship danwinship mentioned this pull request Feb 22, 2017
Closed
@danwinship danwinship mentioned this pull request Mar 8, 2017
Merged
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 25, 2017
@danwinship danwinship force-pushed the networkpolicy-tests branch from ae0344b to 10c0e0b Compare April 28, 2017 14:24
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 28, 2017
@danwinship danwinship force-pushed the networkpolicy-tests branch from 10c0e0b to 66aa597 Compare April 28, 2017 16:30
@danwinship danwinship force-pushed the networkpolicy-tests branch from 66aa597 to 8d6530c Compare June 6, 2017 17:57
@danwinship danwinship changed the title NetworkPolicy tests NetworkPolicy tests [DO NOT MERGE] Jun 6, 2017
@danwinship
Copy link
Contributor Author

@openshift/networking PTAL
The tests won't actually pass until both #14466 and #14498 have merged. Or, well, actually they will pass here, because jenkins is still using RHEL 7.2 and so the "is your kernel new enough" check will fail and the networkpolicy tests will be skipped. But they'd fail if you ran them locally.

@danwinship
Copy link
Contributor Author

the "is your kernel new enough" check

Oh, right. So, this adds a check to the extended networking test setup script that makes it skip the networkpolicy tests if your kernel is too old (which basically means "RHEL < 7.3" since any non-enterprise distro with a kernel too old to support OVS conntrack would presumably also have ancient golang and docker). This lets us commit the tests before jenkins gets updated (#13049).

@danwinship danwinship force-pushed the networkpolicy-tests branch from 8d6530c to 71fd775 Compare June 6, 2017 22:01
@knobunc
Copy link
Contributor

knobunc commented Jun 9, 2017

Since #12972 merged, can we revisit this?

@danwinship
Copy link
Contributor Author

as per the comment above, this is currently waiting on #14466 and #14498

@danwinship danwinship force-pushed the networkpolicy-tests branch from 71fd775 to c89e895 Compare June 15, 2017 11:14
@danwinship danwinship changed the title NetworkPolicy tests [DO NOT MERGE] NetworkPolicy tests Jun 15, 2017
@danwinship
Copy link
Contributor Author

rebased on top of #14466. we want to get this in, although 14466 should merge first. (Note that this only affects the full extended networking test, so even in the worst case it's not going to create more flakes for everyone else)

@danwinship danwinship mentioned this pull request Jun 15, 2017
Merged
@danwinship danwinship force-pushed the networkpolicy-tests branch 2 times, most recently from 4ee6208 to de4fb3e Compare June 19, 2017 14:02
@openshift-bot
Copy link
Contributor

Evaluated for origin testextended up to d2dfa9c

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/testextended SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_request_origin_extended/658/) (Base Commit: 795ba0a) (PR Branch Commit: d2dfa9c) (Extended Tests: networking)

dcbw
dcbw approved these changes Jun 24, 2017
View reviewed changes
Copy link
Contributor

@dcbw dcbw left a comment

Choose a reason for hiding this comment

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

lgtm

@openshift-merge-robot openshift-merge-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 24, 2017
@danwinship danwinship force-pushed the networkpolicy-tests branch from 0cf8481 to a536ccf Compare August 2, 2017 12:35
@danwinship
Copy link
Contributor Author

/test extended_networking

@danwinship
Copy link
Contributor Author

@stevekuznetsov how do I run the full extended networking test rather than just extended_networking_minimal?

@stevekuznetsov
Copy link
Contributor

/test extended_networking

@stevekuznetsov
Copy link
Contributor

Should be possible now

@rajatchopra
Copy link
Contributor

Looks great to me. How can we get this merged?

@stevekuznetsov
Copy link
Contributor

There is one last step:

Submit Queue — PR does not have lgtm label.

The reviewer ( @dcbw ) needs to add their /lgtm tag to this by commenting in the thread.

@dcbw
Copy link
Contributor

dcbw commented Aug 4, 2017

/approve
/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@0xmichalis
Copy link
Contributor

/test extended_templates

@openshift-merge-robot
Copy link
Contributor

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot
Copy link
Contributor

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

@openshift-bot
Copy link
Contributor

/retest

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

@0xmichalis
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot
Copy link
Contributor

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

@openshift-bot
Copy link
Contributor

/retest

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

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot
Copy link
Contributor

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

@openshift-bot
Copy link
Contributor

/retest

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

1 similar comment
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot
Copy link
Contributor

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

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue

@openshift-merge-robot openshift-merge-robot merged commit 783b189 into openshift:master Aug 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dcbw dcbw dcbw approved these changes

Assignees

@mfojtik mfojtik

@dcbw dcbw

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/networking lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.