Allow microovn to install alongside OVS#319
Allow microovn to install alongside OVS#319crypticC0der wants to merge 1 commit intocanonical:mainfrom
Conversation
crypticC0der
force-pushed
the
handleaptovs
branch
from
9761ee3 to
29041c4
Compare
| fi | ||
| fi; | ||
|
|
||
| set -eu |
There was a problem hiding this comment.
We likely want to keep set -eu at the top, so that it can affect the additions above?
There was a problem hiding this comment.
with set -eu if one of the appctl command fails instead of continuing and explaining to the user, it will just fail
| Running multiple instances of Open vSwitch in the same namespace is not | ||
| supported. | ||
| supported. Starting microovn.switch will attempt to shut down the |
There was a problem hiding this comment.
Due to not returning with an error below, this message will never be printed or logged anywhere. We should likely move the check and message elsewhere.
snapcraft/commands/switch.start
Outdated
| if [ "$(ovs-vsctl show)" ]; then | ||
| ovs-appctl exit | ||
| rc=$? | ||
| ovs-appctl -t ovsdb-server exit |
There was a problem hiding this comment.
-
Using
ovs-vsctl showalone as a check for whether an instance of OVS already exists in the system appears brittle to me. The above command would fail for instances of OVS in unexpected state or locations (e.g. in a foreign snap, installed to/usr/localfrom upstream sources, partially running OVS e.g. only ovs-vswitchd and no ovsdb-server, and so on).Checking for the kernel datapath previously being initialized is a fairly bullet proof check, so I think we need that as one of the checks here, perhaps move it here from the install hook?
-
If someone already has an instance of OVS installed, it is likely that it is there for a reason, and with some configuration, possibly in use for connectivity to the system itself or services running on it.
Presumably this change is made in an attempt at helping with migration, but is it really safe and desirable to unsolicitedly shut down the running service without also ensuring the success of remaining migration steps?
-
It took me a moment to remember that we do not have global environment variables nor snap layouts that influence execution of the standard OVS binaries. So this works because by default these tools will reach into default system locations, for which access is granted by the
openvswitch-supportsnapd interface.I think it would be worth adding a comment about why this works, and/or possibly adding explicit path to the socket we expect to use in the system to make it even more clear what is going on.
There was a problem hiding this comment.
Presumably this change is made in an attempt at helping with migration, but is it really safe and desirable to unsolicitedly shut down the running service without also ensuring the success of remaining migration steps?
I could expand the check to ensure there's conf.db somewhere which means its presumably been moved over but I feel setting up microovn while OVS is present and starting microovn, switch without doing the one migration step is a user error
There was a problem hiding this comment.
but I feel setting up microovn while OVS is present and starting microovn, switch without doing the one migration step is a user error
Sure, and users do make those errors. I have recent memories of the team spending an extended period of time finding the root cause of issues caused by multiple instances of ovs-vswitchd running.
Unless you are a OVS/OVN developer, you may have no knowledge of the danger in doing this, and it is a really subtle and non-obvious condition to find.
Happy to see the check moved from the install hook if that helps migration, but we need to retain its function for the casual user about to fire a footgun.
I could expand the check to ensure there's conf.db somewhere which means its presumably been moved over
We definitively want stopping of system services to be a conscious choice by the human operator of the snap, presence of some file required to complete migration could indeed serve as such conscious choice.
There was a problem hiding this comment.
We definitively want stopping of system services to be a conscious choice by the human operator of the snap, presence of some file required to complete migration could indeed serve as such conscious choice.
I personally dont like enforcing conf.db to be there as it seems somewhat specific, so ive added a check for a file the user will need to create explicitly accepting this, inspired slightly by pips --break-system-packages flag
crypticC0der
force-pushed
the
handleaptovs
branch
from
29041c4 to
ffe40f8
Compare
If Open vSwitch is installed do not block microovn installation. When microovn.switch is started check if non-snap Open vSwitch is running, if it is shut it down and then prompt the user to disable the non-snap OVS services. Also expands the switchstart test to ensure this works properly. Signed-off-by: MJ Ponsonby <mj.ponsonby@canonical.com>
crypticC0der
force-pushed
the
handleaptovs
branch
from
ffe40f8 to
c115372
Compare
|
interesting thing about the openvswitch interface, along with not giving us access to conf.db it also doesnt give us access to |
2 participants
If Open vSwitch is installed do not block microovn installation. When microovn.switch is started check if non-snap Open vSwitch is running, if it is shut it down and then prompt the user to disable the non-snap OVS services.
Also expands the switchstart test to ensure this works properly.