-
Notifications
You must be signed in to change notification settings - Fork 235
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
[WIP] Start host openvswitch using systemctl #477
Conversation
Hi @mccv1r0. Thanks for your PR. I'm waiting for a openshift 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/test-infra repository. |
/ok-to-test |
/retest |
2 similar comments
/retest |
/retest |
/test e2e-gcp-ovn |
/retest |
/test e2e-gcp-ovn |
831d382
to
6c80243
Compare
/test e2e-gcp-ovn |
db927b0
to
f1ecb44
Compare
edc5050
to
519f685
Compare
@@ -42,23 +42,75 @@ spec: | |||
- | | |||
#!/bin/bash | |||
set -euo pipefail | |||
set -x | |||
if [[ -f /usr/bin/id ]]; then |
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.
-x
for executable?
I don't understand all these checks, they are not atomic and thus worse than redundant because they hid the real error message.
/usr/bin/id openvswitch || true
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.
or no || true
with the set -e
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.
or I guess it's not clear in which environment id
is not present?
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 don't understand all these checks, they are not atomic and thus worse than redundant because they hid the real error message.
Most (if not all) of these checks were added to debug and will not exist in the final PR.
echo "id command not found" | ||
fi | ||
if [[ -d /run/openvswitch ]]; then | ||
ls -al /run/openvswitch |
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.
ls -al /run/openvswitch/ || true
trailing /
forces treating it as a directory.
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 guess question here as well, when is /var/run
not a symlink to /run
?
if [[ -d /var/run/openvswitch ]]; then | ||
ls -al /var/run/openvswitch | ||
fi | ||
if [[ -f /var/run/openvswitch/ovs-vswitchd.pid ]] ; then |
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.
-r
for readable.
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: mccv1r0 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
can we get this PR rebased, wanted to test this with openshift/machine-config-operator#1636? |
@sinnykumari I think this is rebased now but not sure about all these failures in CI |
journalctl -xeu openvswitch --no-pager | ||
else | ||
echo "OVS started by ovs-node container, NOT RHCOS" | ||
# Need container up v0.0 just to see if host starts ovs and friends |
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.
Can you fix the indentation here?
if (systemctl is-active --quiet openvswitch) ; then | ||
echo "OVS started by RHCOS" | ||
systemctl is-active openvswitch | ||
journalctl -xeu openvswitch --no-pager |
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.
Probably just want to exec journalctl -f -u openvswitch ovsdb-server
and be done with the script.
Can it because we would need openshift/machine-config-operator#1636 merged first so that ovn related services are enabled on host to keep cluster install progressing further? |
fi | ||
if [[ -f /var/run/openvswitch/ovsdb-server.pid ]] ; then | ||
cat /var/run/openvswitch/ovsdb-server.pid | ||
fi | ||
function quit { |
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.
Yikes. This needs to be moved to the "non-systemd case"
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.
This is all debug code
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.
not the kill -9
...
Signed-off-by: Michael Cambria <mcambria@redhat.com>
Signed-off-by: Michael Cambria <mcambria@redhat.com>
journalctl -xeu openvswitch --no-pager | ||
else | ||
echo "OVS started by ovs-node container, NOT RHCOS" | ||
# Need container up v0.0 just to see if host starts ovs and friends |
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 don't really understand why we need to start OVS from this container. Shouldn't systemd OVS be configured as a service with restart=Always? Why does a pod need to start it?
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.
This might have been left-over from before the MCO was going to start OVS. Our first pass was starting system OVS from a pod via systemctl without MCO, but we moved to MCO because Clayton told us to.
@mccv1r0: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
@mccv1r0: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
Start host openvswitch using systemctl
Signed-off-by: Michael Cambria mcambria@redhat.com