Split microovn.central service.
#83
Conversation
mkalcok
force-pushed
the
split-services
branch
from
ac2a643 to
a87eb92
Compare
|
Thank you for extensive review @fnordahl @pmatulis . I believe I addressed all the comments.
The need for this documentation was created directly by code changes in this PR. We split the What I should've done though is to make a separate commit for the doc change for easier review. |
docs/reference/services.rst
Outdated
|
|
||
| * ``microovn.ovn-ovsdb-server-nb`` | ||
| * ``microovn.ovn-ovsdb-server-sb`` | ||
| * ``microovn.ovn-northd``. |
docs/reference/services.rst
Outdated
| MicroOVN services | ||
| ================= | ||
|
|
||
| This page presents a list of all MicroOVN services. The descriptions presented |
There was a problem hiding this comment.
apologies, my fault here:
s/The descriptions presented here is/Their descriptions are/
There was a problem hiding this comment.
I took this for a spin and tested both refresh, fresh install and post configuration reboots, looks great.
The commit messages may need updating to reflect changes during review.
I also have a couple of comments in-line that request splitting a couple of things into separate commits.
| # Disable some commands | ||
| mkdir -p "${OVN_RUNDIR}/bin/" | ||
| for i in install plymouth sudo systemctl; do | ||
| [ -e "${OVN_RUNDIR}/bin/${i}" ] && continue | ||
|
|
||
| ln -s "/bin/true" "${OVN_RUNDIR}/bin/${i}" |
There was a problem hiding this comment.
Good riddance!
However, I see these are also in snapcraft/commands/chassis.start and snapcraft/commands/switch.start.
Would it be more consistent to drop all of these simultaneously in a separate parent commit?
For the record, there are now some additional DENIED messages logged, but they do not affect operation.
[ 101.595042] audit: type=1400 audit(1697770466.057:241): apparmor="DENIED" operation="exec" profile="snap.microovn.ovn-ovsdb-server-nb" name="/usr/bin/systemctl" pid=2804 comm="ovn-ctl" requested_mask="x" denied_mask="x" fsuid=0 ouid=0
[ 101.595086] audit: type=1400 audit(1697770466.057:242): apparmor="DENIED" operation="exec" profile="snap.microovn.ovn-ovsdb-server-nb" name="/usr/bin/systemctl" pid=2804 comm="ovn-ctl" requested_mask="x" denied_mask="x" fsuid=0 ouid=0
[ 101.596966] audit: type=1400 audit(1697770466.061:243): apparmor="DENIED" operation="exec" profile="snap.microovn.ovn-ovsdb-server-nb" name="/usr/bin/plymouth" pid=2806 comm="ovn-ctl" requested_mask="x" denied_mask="x" fsuid=0 ouid=0
[ 101.597023] audit: type=1400 audit(1697770466.061:244): apparmor="DENIED" operation="exec" profile="snap.microovn.ovn-ovsdb-server-nb" name="/usr/bin/plymouth" pid=2806 comm="ovn-ctl" requested_mask="x" denied_mask="x" fsuid=0 ouid=0
Even without this change there are still other unhandled DENIED rules logged, such as:
[ 101.599507] audit: type=1400 audit(1697770466.065:245): apparmor="DENIED" operation="capable" profile="snap.microovn.switch" pid=2787 comm="ovsdb-server" capability=38 capname="perfmon"
So I still think we should still drop these hacks and figure out what indirect dependencies of the OVS/OVN ctl scripts end up calling the offending binaries and provide patches upstream at some point in the future.
There was a problem hiding this comment.
I agree. I'll drop those instances too. The commands do not work anyway, it's better to have the failures logged rather than silently failing.
These commands do not work within the snap context but to figure out how to properly fix this we need to stop masking them as `/bin/true` so the apparmor violations can be logged. Signed-off-by: Martin Kalcok <[email protected]>
Previously `microovn.central` started all three central services ( `microovn.ovn-ovsdb-server-nb`, `microovn.ovn-ovsdb-server-sb` and `microovn.ovn-northd`). Now these daemons are split into their own services for more fine-grained controll over them. The `microovn.central` will stay for now and act as an transitional service that can trigger start of all three services. However the usage is discouraged as it will be removed. It's also not capable of being used as shortcut for stopping these services. Additional benefits of splitting of the services is that we can now scale the cluster without restarting OVN NB/SB services as only the northd daemon needs restart. Signed-off-by: Martin Kalcok <[email protected]>
sha256sum doesn't work properly if string passed for hashing starts with "-". Since pubkeys start exactly with these characters, pipe them instead via STDIN Signed-off-by: Martin Kalcok <[email protected]>
The specific test suit in this general Make target was likely introduced unintentionally. Signed-off-by: Martin Kalcok <[email protected]>
Signed-off-by: Martin Kalcok <[email protected]>
Signed-off-by: Martin Kalcok <[email protected]>
mkalcok
force-pushed
the
split-services
branch
from
a87eb92 to
66c6d29
Compare
|
Thanks again for the review. I incorporated your comments and I separated the docs and the command masking removal into separate commits. |
fnordahl
dismissed
pmatulis’s stale review
I have confirmed that the latest update addresses the comments left in this review, and in the interest of time we'll proceed.
Previously
microovn.centralstarted all three central services (ovn-nb, ovn-sb and northd). Now these daemons are split into their own services for more fine-grained controll over them.The
microovn.centralwill stay for now and act as an umbrella service that can trigger start of all three services. However the usage is discouraged as it will be removed. It's also not capable of being used as shortcut for stopping these services.Additional consequence of splitting of the services is that we can now scale the cluster without restarting OVN NB/SB services as only the northd daemon needs restart.