-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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 OVS test flake on release-1.5 #13200
Conversation
There's no way to tell when the nodes have fully processed the node/pod/service creation/deletion events, so if the flows look wrong, assume it's because they haven't been updated yet, and try again in a few seconds. Also, tweak the assertions and use of error objects so that when the flows don't match expectations, the expected/found flows end up logged in the test output.
(Which makes it consistent with the local multicast flow, and fixes a test flake.)
[testextended][extended:networking] |
Evaluated for origin testextended up to 876affc |
} | ||
} | ||
otx.AddFlow("table=111, priority=100, actions=move:NXM_NX_REG0[]->NXM_NX_TUN_ID[0..31]%s,goto_table:120", tun_dsts) | ||
sort.Strings(sort.StringSlice(tun_dsts)) | ||
otx.AddFlow("table=111, priority=100, actions=move:NXM_NX_REG0[]->NXM_NX_TUN_ID[0..31]%s,goto_table:120", strings.Join(tun_dsts, "")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you are going to keep each string in the slice why not handle the commas in the strings.Join()
and make the fmt a little less finiky? (Not that the fmt is wrong)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because we don't want "a comma between each element", we want "a comma before each element". If you look at how the tun_dsts get fit into the rest of the flow string, the string has to be "" when there are no tun_dsts, but it has to start with "," otherwise.
} | ||
} | ||
otx.AddFlow("table=111, priority=100, actions=move:NXM_NX_REG0[]->NXM_NX_TUN_ID[0..31]%s,goto_table:120", tun_dsts) | ||
sort.Strings(sort.StringSlice(tun_dsts)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you need the sort.StringSlice()
? Doesn't sort.Strings()
take a []string
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed. However, this is a cherry-pick from master, so we should keep it the same. (Or fix it there as well I guess.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, I say send a PR to fix in master and we'll allow the double casting since it apparently doesn't hurt much.
[merge] |
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/751/) (Image: devenv-rhel7_6027) |
Evaluated for origin merge up to 876affc |
[Test]ing while waiting on the merge queue |
Evaluated for origin test up to 876affc |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_future/751/) (Base Commit: d0d613e) |
continuous-integration/openshift-jenkins/testextended FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin_extended/1145/) (Base Commit: d0d613e) (Extended Tests: networking) |
Cherry-pick of #13061, fixing flake #12855. The flake only affects the extended networking tests, but it flakes a lot, so we'll want this.
@openshift/networking PTAL