-
Notifications
You must be signed in to change notification settings - Fork 32
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 gateway datapath disrupt on update #301
Fix gateway datapath disrupt on update #301
Conversation
Hi @elvgarrui. Thanks for your PR. I'm waiting for a openstack-k8s-operators member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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-sigs/prow repository. |
406c4f1
to
c1b5d62
Compare
This will need a kuttl test that would validate that flows injected by OVN are retained after ovs pod restart. |
/ok-to-test |
c1b5d62
to
8e183b3
Compare
New version includes the reviews from @ralonsoh. I have tested this in my env and it works better now. The flows are still not correctly written on the DB with the start script, though. |
Build failed (check pipeline). Post https://review.rdoproject.org/zuul/buildset/33635dc070e14fe3b0deb94ad63b556d ✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 06m 49s |
3ac4451
to
a677d49
Compare
|
||
# Start vswitchd by asking it to wait till flow restore is finished. | ||
ovs-vsctl --no-wait set open_vswitch . other_config:flow-restore-wait=true | ||
/usr/sbin/ovs-vswitchd --pidfile --mlockall --detach |
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.
I note that ovs-ctl does some additional things like setting MAXFDs and setting up logging. Do we need to do that here?
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.
@otherwiseguy maybe?.. Would be nice if you could check and report back. In the meantime, this PR does not attempt to change how the service is started. This command was used before to start the container, so there's no change here (except that --detach is added).
a677d49
to
580d6f3
Compare
I have removed the sleep on the ovsdb-server preStop. We still need the kuttl testing |
/hold |
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.
looking forward for kuttl scenario.
@@ -0,0 +1,36 @@ | |||
#!/bin/sh |
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.
(no action required) I know it's also sh
(and not bash
) in other scripts here, but I wonder if we should just use bash
here - I for one am not very knowledgeable to confirm that this script doesn't use any bash-isms.
7ae5f03
to
16ba507
Compare
cc6789b
to
948b152
Compare
948b152
to
9977e86
Compare
/retest-required |
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.
Overall LGTM, some nits and clarifications inline
Please also update the commit message:
|
9977e86
to
7b63923
Compare
7b63923
to
5358156
Compare
Hi folks, I think this patch is ready. I don't know (I think I can't) change the PR name (and remove the WIP tag) but is removed from the commit message now. Don't hesitate to ping me here, by mail or in IRC. Thanks! |
/lgtm |
/hold off |
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.
/lgtm
Just a cleanup inline if revision needed else can follow up
node=$(oc get nodes -o name|sort|head -1| sed "s|node/||g") | ||
controller_pod=$(oc get pod -n $NAMESPACE -l service=ovn-controller-ovs --field-selector spec.nodeName=$node -o name | head -1) | ||
expected_flows="table=100, priority=200 actions=drop" | ||
oc rsh -n $NAMESPACE --container ovs-vswitchd $controller_pod ovs-ofctl dump-flows br-test-flows --no-stats | grep -q "$expected_flows" || exit 1 |
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.
let's also delete the test bridge after test
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.
Let me push a new PS
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.
I've pushed a new PS. I was thinking about adding a trap in this script, but that means always deleting the bridge if the script exits. We only want to remove the bridge if the script first reads the flows correctly.
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.
We only want to remove the bridge if the script first reads the flows correctly.
Why?
Re: trap: you can set trap and then remove it with trap - EXIT
.
Reimplementing the vswitchd reload using separate start and stop scripts so that it can be executed partially between consecutives instances of the pod. To avoid disruptions, it runs ovs save-flows command on the stop-vswitchd script, saving the resulting backup on a mounted folder from the host and then loading that backup on the start script. For this to succeed, the script needs to set the flow-restore-wait flag from ovs to true while loading the flows. Note that ovs save-flows command needs to have ovsdb-server running, so a semaphore like mechanism was added to ensure the ovsdb-server container is never deleted before ovs-vswitchd. Closes-Issue: OSPRH-6326
5358156
to
303cbcd
Compare
/lgtm |
/approve There's only so much yak to shave. Some improvements to kuttl may still be useful, but I don't think we should wait for it addressed here. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: booxter, elvgarrui The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
9f91003
into
openstack-k8s-operators:main
Reimplementing the vswitchd reload using separate start and stop scripts so that it can be executed partially between consecutives instances of the pod.
TODO: