Mount OVS socket unconditionally #861
Conversation
kubevirt-bot
added
release-note-none
cluster/sync-operator.sh
Outdated
| @@ -6,12 +6,28 @@ kubectl=./cluster/kubectl.sh | |||
|
|
|||
| source ./cluster/sync-common.sh | |||
|
|
|||
| function __yq() { | |||
There was a problem hiding this comment.
Why don't we enable this by default?
There was a problem hiding this comment.
It requires OVS to be installed, otherwise handlers fail
There was a problem hiding this comment.
Can we make it to fail gracefully / detect OVS on runtime?
There was a problem hiding this comment.
Maybe detect whether the OVS socket is available while starting and based on it, set the variable. (I don't know how it is implemented now, so sorry if I'm totally off)
There was a problem hiding this comment.
We've discussed with @qinqon that it would probably make sense to expose this knob
to the user, to decide whether or not the handlers should attempt to mount the ovs socket.
There was a problem hiding this comment.
ENABLE_OVS causes us to try to mount the OVS socket from the host, and if nmstate finds the socket, it tries to use it, right?
I think we could make the mount unconditioned, possibly by mounting the whole directory with DirectoryOrCreate. That should be enough, no? It is not great that we would create a directory on host, but it would give us this flexibility.
There was a problem hiding this comment.
Interesting point. According to the docs, specifying no type at all performs no checks, which would also solve the empty directory (although that should be harmless).
There was a problem hiding this comment.
But what will do nmstate it find that we have a fake socket for ovs ?
There was a problem hiding this comment.
There would be no socket, Kubernetes will only mount an empty directory.
Mout ovs socket with unspecified type, which doesn't check it's actual existence on the host. Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>
rhrazdil
force-pushed
the
enable-ovs-cluster-sync
branch
from
6d905e0
to
7df5d5f
Compare
|
/hold |
kubevirt-bot
added
the
do-not-merge/hold
|
@rhrazdil: The following test failed, say
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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: qinqon 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 |
kubevirt-bot
added
the
approved
|
/hold cancel |
kubevirt-bot
removed
the
do-not-merge/hold
|
We'll want to extend our e2e test with a scenario that creates ovs-bridge with primary nic connected to it as a port as well as a veth pair connecting linux-bride to the ovs-bridge. |
|
You might want please to change PR title, it is outdated comparing to the commit that was pushed as part of this PR (just saw while trying to learn the flow) |
rhrazdil
changed the title
|
This PR looks to have broken non OVS providers (specifically calico) is there a way to revert it? |
|
@relyt0925 I believe that this PR is the one that fixed that usecase. With removal of "type: Socket", the mount does not fail when the socket does not exist. |
Signed-off-by: Radim Hrazdil rhrazdil@redhat.com
Is this a BUG FIX or a FEATURE ?:
What this PR does / why we need it:
Special notes for your reviewer:
Release note: