Add "oadm pod-network" tests to test/cmd, remove from test/extended #11247

danwinship · 2016-10-06T14:38:45Z

The connectivity testing in test/extended/networking/pod_network_cli.go is more-or-less redundant with other existing extended networking tests. So just remove it and test just the database-manipulating parts of it in test-cmd instead, to speed up the extended networking tests.

@openshift/networking, @stevekuznetsov PTAL
@danmcp FYI

stevekuznetsov

It doesn't seem like os::cmd has a nice way to test that one thing is not another thing, which is unfortunate. We can either improve os::cmd or make your tests work in a different way, but I am hesitant to test the output of test. We should also take advantage of the structure and metadata that jUnit suites give us whenever possible.

stevekuznetsov · 2016-10-06T15:19:16Z

test/cmd/sdn.sh

@@ -4,29 +4,82 @@ trap os::test::junit::reconcile_output EXIT

 os::test::junit::declare_suite_start "cmd/sdn"

+# ClusterNetworks


os::test::junit::declare_suite_start "cmd/sdn/cluster-networks"

stevekuznetsov · 2016-10-06T15:19:29Z

test/cmd/sdn.sh

 os::cmd::expect_success 'oc get clusternetworks'
 os::cmd::expect_success_and_text 'oc get clusternetwork default -o jsonpath="{.pluginName}"' 'redhat/openshift-ovs-multitenant'
 os::cmd::expect_failure_and_text 'oc patch clusternetwork default -p "{\"network\": \"1.0.0.0/8\"}"' 'Invalid value'
 os::cmd::expect_failure_and_text 'oc patch clusternetwork default -p "{\"hostsubnetlength\": 22}"' 'Invalid value'
 os::cmd::expect_failure_and_text 'oc patch clusternetwork default -p "{\"serviceNetwork\": \"1.0.0.0/8\"}"' 'Invalid value'

+# NetNamespaces


os::test::junit::declare_suite_start "cmd/sdn/net-namespaces"

stevekuznetsov · 2016-10-06T15:20:29Z

test/cmd/sdn.sh

-os::cmd::expect_success 'oc delete namespace sdn-test'
-os::cmd::try_until_failure 'oc get netnamespace sdn-test'
+
+os::cmd::expect_failure 'oc get netnamespace sdn-test-1'


We don't have a habit of checking assumptions like this. Given a good cleanup script for advanced use-cases for the tests, we don't need it. In a normal test-cmd run, we definitely don't need it.

stevekuznetsov · 2016-10-06T15:21:52Z

test/cmd/sdn.sh

+orig_vnid1="$(oc get netnamespace sdn-test-1 -o jsonpath='{.netid}')"
+orig_vnid2="$(oc get netnamespace sdn-test-2 -o jsonpath='{.netid}')"
+orig_vnid3="$(oc get netnamespace sdn-test-3 -o jsonpath='{.netid}')"
+os::cmd::expect_success "test '$orig_vnid1' != 0"


os::cmd::expect_success_and_text "oc get netnamespace sdn-test-1 -o jsonpath='{.netid}'" "[1-9][0-9]*"

if you want to express a test about the structure of the vnid or

os::cmd::expect_success_and_not_text "oc get netnamespace sdn-test-1 -o jsonpath='{.netid}'" "^0$"

if you want this exact test

I know you've done it this way since you do more tests with the same value below, but I really don't like:

not wrapping API interactions with os::cmd since that's where the logging is really useful

testing the result of a test

stevekuznetsov · 2016-10-06T15:25:02Z

test/cmd/sdn.sh

+os::cmd::expect_success "test '$orig_vnid1' != 0"
+os::cmd::expect_success "test '$orig_vnid2' != 0"
+os::cmd::expect_success "test '$orig_vnid3' != 0"
+os::cmd::expect_success "test '$orig_vnid1' != '$orig_vnid2'"


I know this would be a little more convoluted than what you've got, but can we:

make assumptions about the vnids?

do some sort of test on the aggregate list of vnids from all three namespaces?

stevekuznetsov · 2016-10-06T15:25:53Z

test/cmd/sdn.sh

+os::cmd::expect_success "test '$new_vnid2' = '$orig_vnid1'"
+os::cmd::expect_success "test '$new_vnid3' = '$orig_vnid3'"
+
+os::cmd::expect_success 'oc delete namespace sdn-test-1'


These should be in a cleanup block at the top along with any other cluster resources you create

stevekuznetsov · 2016-10-06T15:26:07Z

test/cmd/sdn.sh


 os::cmd::expect_success "oc project '${orig_project}'"

+# HostSubnets


create a jUnit suite like above

stevekuznetsov · 2016-10-06T15:26:11Z

test/cmd/sdn.sh

 # test-cmd environment has no nodes, hence no hostsubnets
 os::cmd::expect_success_and_not_text 'oc get hostsubnets' '.'

+# EgressNetworkPolicies


create a jUnit suite like above

danwinship · 2016-10-06T17:34:28Z

@stevekuznetsov hm... I guess the problem is that I'm really mostly testing the behavior of the master, not the behavior of the CLI. I should move that into an integration test and then simplify the cmd test...

stevekuznetsov · 2016-10-06T18:54:36Z

testing the behavior of the master, not the behavior of the CLI. I should move that into an integration test

I think that is appropriate.

marun · 2016-10-06T22:09:14Z

@danmcp fyi these tests are taking ~650 of 1800s of the multitenant test run.

danwinship · 2016-10-07T15:04:26Z

updated: made suggested general improvements to the cmd test, and moved the actual tests of the master behavior to a new integration test. The cmd test now only checks the output enough to confirm that the correct command executed.

danwinship · 2016-10-07T15:04:50Z

[test]

stevekuznetsov

Bash mostly LGTM

stevekuznetsov · 2016-10-07T15:24:07Z

test/cmd/sdn.sh

+os::cmd::expect_success 'oadm pod-network isolate-projects sdn-test-1'
+os::cmd::expect_success_and_not_text 'oc get netnamespace sdn-test-1 -o jsonpath="{.netid}"' '^0$'
+
+os::cmd::expect_success 'oc delete namespace sdn-test-1'


We've got these in the cluster cleanup block, I don't think we also need them here.

danwinship · 2016-10-07T19:42:22Z

flake is #11240, [test]

(also, @marun PTAL at the updated version?)

marun · 2016-10-07T19:57:52Z

lgtm

danwinship · 2016-10-08T14:53:14Z

flakes are #11274 and #11094. [test]

- add suite start/end markers - add cleanup code - remove code that was testing the master, not the CLI

danwinship · 2016-10-10T16:23:38Z

flake #9845, [test]

danwinship · 2016-10-10T20:48:27Z

flakes are #11094 and #11165. [test]

openshift-bot · 2016-10-10T20:52:23Z

Evaluated for origin test up to 7506909

openshift-bot · 2016-10-10T22:56:03Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9838/)

pravisankar · 2016-10-11T03:40:41Z

LGTM [merge]

knobunc · 2016-10-12T17:46:36Z

re[merge] not sure what happened there... the job just stopped.

danwinship · 2016-10-12T18:00:29Z

no, it hasn't stopped, it just keeps getting pushed back in the queue by other PRs. The "you are in the build queue at position N" comment has been updated numerous times over the last few days, but it never makes it to "1"

danwinship · 2016-10-12T23:35:30Z

@stevekuznetsov so, this has been in the merge queue for 2 days now, and it keeps bubbling up and down and apparently getting pre-empted by other PRs. But it will cut 11 minutes off every test run once it lands so it would be cool if there was some way to force to bot to give it higher priority...

openshift-bot · 2016-10-13T03:17:24Z

Evaluated for origin merge up to 7506909

openshift-bot · 2016-10-13T03:17:35Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/9992/) (Base Commit: 3687062) (Image: devenv-rhel7_5168)

stevekuznetsov · 2016-10-13T12:28:08Z

@danwinship we don't allow for line-cutting in the merge queue unless the PR in question has new pushed commits after the last merge tag, as we try to keep everyone on an equal footing. @danmcp could give more insight about why it seemed to be bubbling around. re: "fast merge" -- we specifically did not create a cut-the-line feature, the only tool we have in our shed for this is merge-pretest-success which can only be done globally and is fairly risky. I'm sorry about the frustrating merge process, any suggestions for improvement I will gladly consider.

danwinship · 2016-10-13T13:38:06Z

@stevekuznetsov Weird. I definitely saw it at position 2 at one point, and then at position 5 a few hours later. And I've seen the same thing before on other PRs. I assumed it was reordering the queue based on priority labels or something.

stevekuznetsov · 2016-10-13T14:05:30Z

No, unclear exactly what's happening, but I believe @danmcp had and still has a core design principle of "no cutting" in the merge queue.

danmcp · 2016-10-13T14:40:35Z

@stevekuznetsov @danwinship The order of the merge queue is based on commit order. So if someone is ahead of you, it means their commits were made before yours. The reason for picking commit order is it doesn't penalize someone getting their code reviewed that they are losing their potential spot in the merge queue. As far as why there is jumping around, there are a couple of reasons:

Given the logic just described, it's possible for people to add the merge tag after you but their commits were before yours
And the more likely cause in this case. People are hitting flakes and remerging. The alternative is that they move to the back of the queue because they hit a flake. Which seems less fair.

stevekuznetsov suggested changes Oct 6, 2016

View reviewed changes

danwinship force-pushed the move-pod-network-tests branch from 30b33e3 to 3cbc85c Compare October 7, 2016 15:02

stevekuznetsov suggested changes Oct 7, 2016

View reviewed changes

danwinship force-pushed the move-pod-network-tests branch 2 times, most recently from 4e6da66 to 669a2b0 Compare October 7, 2016 16:31

stevekuznetsov approved these changes Oct 7, 2016

View reviewed changes

danwinship added 2 commits October 10, 2016 09:30

test/cmd/sdn.sh improvements

7506909

- add suite start/end markers - add cleanup code - remove code that was testing the master, not the CLI

Move "oadm pod-network" tests from extended to integration and cmd

06f4fec

danwinship force-pushed the move-pod-network-tests branch from 669a2b0 to 06f4fec Compare October 10, 2016 13:30

knobunc self-assigned this Oct 12, 2016

openshift-bot merged commit c728fe9 into openshift:master Oct 13, 2016

danwinship deleted the move-pod-network-tests branch October 13, 2016 13:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add "oadm pod-network" tests to test/cmd, remove from test/extended #11247

Add "oadm pod-network" tests to test/cmd, remove from test/extended #11247

danwinship commented Oct 6, 2016

stevekuznetsov left a comment

stevekuznetsov Oct 6, 2016

stevekuznetsov Oct 6, 2016

stevekuznetsov Oct 6, 2016

stevekuznetsov Oct 6, 2016 •

edited

Loading

stevekuznetsov Oct 6, 2016

stevekuznetsov Oct 6, 2016

stevekuznetsov Oct 6, 2016

stevekuznetsov Oct 6, 2016

stevekuznetsov Oct 6, 2016

danwinship commented Oct 6, 2016

stevekuznetsov commented Oct 6, 2016

marun commented Oct 6, 2016

danwinship commented Oct 7, 2016

danwinship commented Oct 7, 2016

stevekuznetsov left a comment

stevekuznetsov Oct 7, 2016

danwinship commented Oct 7, 2016

marun commented Oct 7, 2016

danwinship commented Oct 8, 2016

danwinship commented Oct 10, 2016

danwinship commented Oct 10, 2016

openshift-bot commented Oct 10, 2016

openshift-bot commented Oct 10, 2016

pravisankar commented Oct 11, 2016

knobunc commented Oct 12, 2016

danwinship commented Oct 12, 2016

danwinship commented Oct 12, 2016

openshift-bot commented Oct 13, 2016

openshift-bot commented Oct 13, 2016 •

edited

Loading

stevekuznetsov commented Oct 13, 2016

danwinship commented Oct 13, 2016

stevekuznetsov commented Oct 13, 2016

danmcp commented Oct 13, 2016

		@@ -4,29 +4,82 @@ trap os::test::junit::reconcile_output EXIT

		os::test::junit::declare_suite_start "cmd/sdn"

		# ClusterNetworks


		os::cmd::expect_success "oc project '${orig_project}'"

		# HostSubnets

Add "oadm pod-network" tests to test/cmd, remove from test/extended #11247

Add "oadm pod-network" tests to test/cmd, remove from test/extended #11247

Conversation

danwinship commented Oct 6, 2016

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevekuznetsov Oct 6, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Oct 6, 2016

stevekuznetsov commented Oct 6, 2016

marun commented Oct 6, 2016

danwinship commented Oct 7, 2016

danwinship commented Oct 7, 2016

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship commented Oct 7, 2016

marun commented Oct 7, 2016

danwinship commented Oct 8, 2016

danwinship commented Oct 10, 2016

danwinship commented Oct 10, 2016

openshift-bot commented Oct 10, 2016

openshift-bot commented Oct 10, 2016

pravisankar commented Oct 11, 2016

knobunc commented Oct 12, 2016

danwinship commented Oct 12, 2016

danwinship commented Oct 12, 2016

openshift-bot commented Oct 13, 2016

openshift-bot commented Oct 13, 2016 • edited Loading

stevekuznetsov commented Oct 13, 2016

danwinship commented Oct 13, 2016

stevekuznetsov commented Oct 13, 2016

danmcp commented Oct 13, 2016

stevekuznetsov Oct 6, 2016 •

edited

Loading

openshift-bot commented Oct 13, 2016 •

edited

Loading