Hello Fabian Deutsch,
I'd like you to do a code review. Please visit
https://gerrit.ovirt.org/64904
to review the following change.
Change subject: service: Add vdsm-tool-update service ......................................................................
service: Add vdsm-tool-update service
oVirt-Node Next provides updates via squashfs images and between major/minor updates based on EL7 might occur changes of selinux policy/booleans affecting previous vdsm settings. This patch creates a new service 'vdsm-tool-update' that handle such image updates scenarios.
Change-Id: Ib9c4fffb09d2a5c49b0462693fb2beca47fb58ea Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1373389 Signed-off-by: Fabian Deutsch fabiand@fedoraproject.org Signed-off-by: Douglas Schilling Landgraf dougsland@redhat.com --- M init/systemd/85-vdsmd.preset M static/Makefile.am A static/usr/lib/systemd/system/vdsm-tool-update.service M vdsm.spec.in 4 files changed, 19 insertions(+), 0 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/04/64904/1
diff --git a/init/systemd/85-vdsmd.preset b/init/systemd/85-vdsmd.preset index 6da4332c..632235f 100644 --- a/init/systemd/85-vdsmd.preset +++ b/init/systemd/85-vdsmd.preset @@ -5,6 +5,7 @@ enable supervdsmd.service enable vdsm-network.service enable vdsm-network-init.service +enable vdsm-tool-update.service enable mom-vdsm.service
disable ksmtuned.service diff --git a/static/Makefile.am b/static/Makefile.am index 040d6ec..68a98c0 100644 --- a/static/Makefile.am +++ b/static/Makefile.am @@ -86,6 +86,7 @@ ./usr/lib/systemd/system/supervdsmd.service \ ./usr/lib/systemd/system/vdsm-network.service \ ./usr/lib/systemd/system/vdsm-network-init.service \ + ./usr/lib/systemd/system/vdsm-tool-update.service \ ./usr/lib/systemd/system/vdsmd.service \ $(NULL)
@@ -111,5 +112,6 @@ ./usr/lib/systemd/system/supervdsmd.service.in \ ./usr/lib/systemd/system/vdsm-network.service.in \ ./usr/lib/systemd/system/vdsm-network-init.service.in \ + ./usr/lib/systemd/system/vdsm-tool-update.service \ ./usr/lib/systemd/system/vdsmd.service.in \ $(NULL) diff --git a/static/usr/lib/systemd/system/vdsm-tool-update.service b/static/usr/lib/systemd/system/vdsm-tool-update.service new file mode 100644 index 0000000..ad39290 --- /dev/null +++ b/static/usr/lib/systemd/system/vdsm-tool-update.service @@ -0,0 +1,13 @@ +[Unit] +Description=Reconfigure vdsmd +Conflicts=shutdown.target +Before=supervdsmd.service vdsmd.service shutdown.target systemd-update-done.service +ConditionNeedsUpdate=/usr + +[Service] +Type=oneshot +RemainAfterExit=yes +ExecStart=/bin/vdsm-tool configure --force + +[Install] +WantedBy=multi-user.target diff --git a/vdsm.spec.in b/vdsm.spec.in index 80360d0..23e079e 100644 --- a/vdsm.spec.in +++ b/vdsm.spec.in @@ -849,6 +849,7 @@ %systemd_post supervdsmd.service %systemd_post vdsm-network.service %systemd_post vdsm-network-init.service +%systemd_post vdsm-tool-update.service %systemd_post mom-vdsm.service %systemd_post ksmtuned.service
@@ -863,6 +864,7 @@ %systemd_preun vdsmd.service %systemd_preun vdsm-network.service %systemd_preun vdsm-network-init.service +%systemd_preun vdsm-tool-update.service %systemd_preun supervdsmd.service %systemd_preun mom-vdsm.service %systemd_preun ksmtuned.service @@ -907,6 +909,7 @@ %{_unitdir}/vdsmd.service %{_unitdir}/vdsm-network.service %{_unitdir}/vdsm-network-init.service +%{_unitdir}/vdsm-tool-update.service %{_unitdir}/supervdsmd.service %{_unitdir}/mom-vdsm.service %{_sysconfdir}/systemd/system/libvirtd.service.d/unlimited-core.conf
gerrit-hooks has posted comments on this change.
Change subject: service: Add vdsm-tool-update service ......................................................................
Patch Set 1:
* update_tracker: OK * Check Bug-Url::OK * Check Public Bug::#1373389::OK, public bug * Check Product::#1373389::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Douglas Schilling Landgraf has posted comments on this change.
Change subject: service: Add vdsm-tool-update service ......................................................................
Patch Set 1: Verified+1
Hi,
Additional info from our tests: I have backported this patch to downstream vdsm and executed the yum update in the host and everything worked. For more info, please see the bug.
Dan Kenigsberg has posted comments on this change.
Change subject: service: Add vdsm-tool-update service ......................................................................
Patch Set 1: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/64904/1/static/usr/lib/systemd/system/vdsm-tool... File static/usr/lib/systemd/system/vdsm-tool-update.service:
Line 6: Line 7: [Service] Line 8: Type=oneshot Line 9: RemainAfterExit=yes Line 10: ExecStart=/bin/vdsm-tool configure --force this is considered rude in sysadmin circles, as it auto-configures the host on each boot. remember that vdsm's configure is quite disruptive (e.g. restarts libvirtd).
we used to reconfigure on each vdsmd restart, and intentionally moved to on-request-only `vdsm-tool configure`. we should not go back. Line 11: Line 12: [Install]
Fabian Deutsch has posted comments on this change.
Change subject: service: Add vdsm-tool-update service ......................................................................
Patch Set 1:
(2 comments)
https://gerrit.ovirt.org/#/c/64904/1/static/usr/lib/systemd/system/vdsm-tool... File static/usr/lib/systemd/system/vdsm-tool-update.service:
To match what it does I'd suggest to rename the service to vdsm-tool-configure.service Line 1: [Unit] Line 2: Description=Reconfigure vdsmd Line 3: Conflicts=shutdown.target Line 4: Before=supervdsmd.service vdsmd.service shutdown.target systemd-update-done.service
Line 6: Line 7: [Service] Line 8: Type=oneshot Line 9: RemainAfterExit=yes Line 10: ExecStart=/bin/vdsm-tool configure --force
this is considered rude in sysadmin circles, as it auto-configures the host
The ConditionNeedsUpdate in line 5 will ensure that this service is only run when i.e. on Node an image update is detected.
This will also be run if in general the timestamp of /usr will differ from the timestamp which was saved in /etc/.$something
See ConditionNeedsUpdate in https://www.freedesktop.org/software/systemd/man/systemd.unit.html Line 11: Line 12: [Install]
Dan Kenigsberg has posted comments on this change.
Change subject: service: Add vdsm-tool-update service ......................................................................
Patch Set 1:
(1 comment)
https://gerrit.ovirt.org/#/c/64904/1/static/usr/lib/systemd/system/vdsm-tool... File static/usr/lib/systemd/system/vdsm-tool-update.service:
PS1, Line 2: vdsmd actually, this reconfigures the host FOR Vdsm.
gerrit-hooks has posted comments on this change.
Change subject: service: Add vdsm-tool-update service ......................................................................
Patch Set 2:
* #1373389::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1373389::OK, public bug * Check Product::#1373389::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
gerrit-hooks has posted comments on this change.
Change subject: service: Add vdsm-tool-configure service ......................................................................
Patch Set 3:
* #1373389::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1373389::OK, public bug * Check Product::#1373389::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6 ovirt-4.0) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6', 'ovirt-4.0'])
Douglas Schilling Landgraf has posted comments on this change.
Change subject: service: Add vdsm-tool-configure service ......................................................................
Patch Set 3: Verified+1
Dan Kenigsberg has posted comments on this change.
Change subject: service: Add vdsm-tool-configure service ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/64904/3/static/usr/lib/systemd/system/vdsm-tool... File static/usr/lib/systemd/system/vdsm-tool-configure.service:
Line 1: [Unit] Line 2: Description=configure vdsm again, it's more of a configuration of the system FOR vdsm. and only after and update of /usr
P.S., what --- except for ovirt-node update --- can trigger such and update?
Please explain in the commit message when a user is expected to see a "surprise" configuration. Line 3: Conflicts=shutdown.target Line 4: Before=supervdsmd.service vdsmd.service shutdown.target systemd-update-done.service Line 5: ConditionNeedsUpdate=/usr Line 6:
Fabian Deutsch has posted comments on this change.
Change subject: service: Add vdsm-tool-configure service ......................................................................
Patch Set 3:
(1 comment)
https://gerrit.ovirt.org/#/c/64904/3/static/usr/lib/systemd/system/vdsm-tool... File static/usr/lib/systemd/system/vdsm-tool-configure.service:
Line 1: [Unit] Line 2: Description=configure vdsm
again, it's more of a configuration of the system FOR vdsm.
The only case when this condition will kick in, is when the timestamp of /usr changes.
And this is only the case on major updates - or on image based systems like Node and Atomic.
Bottom line: No, with this condition, the service will usually not be triggered on regular hosts - because the timestamp of /usr does not change (AFAIU) Line 3: Conflicts=shutdown.target Line 4: Before=supervdsmd.service vdsmd.service shutdown.target systemd-update-done.service Line 5: ConditionNeedsUpdate=/usr Line 6:
Nir Soffer has posted comments on this change.
Change subject: service: Add vdsm-tool-configure service ......................................................................
Patch Set 3: Code-Review-1
(1 comment)
https://gerrit.ovirt.org/#/c/64904/3//COMMIT_MSG Commit Message:
Line 9: oVirt-Node Next provides updates via squashfs images and Line 10: between major/minor updates based on EL7 might occur changes Line 11: of selinux policy/booleans affecting previous vdsm settings. Line 12: This patch creates a new service 'vdsm-tool-update' that handle Line 13: such image updates scenarios. If this is a node specific issue, it should be solved in node. The service should be part of node and it should use the standard vdsm-tool to reconfigure things. Line 14: Line 15: Change-Id: Ib9c4fffb09d2a5c49b0462693fb2beca47fb58ea Line 16: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1373389 Line 17: Signed-off-by: Fabian Deutsch fabiand@fedoraproject.org
Douglas Schilling Landgraf has abandoned this change.
Change subject: service: Add vdsm-tool-configure service ......................................................................
Abandoned
I wish we could drop it in vdsm, dropping the package for now.
gerrit-hooks has posted comments on this change.
Change subject: service: Add vdsm-tool-configure service ......................................................................
Patch Set 3:
* #1373389::Update tracker: OK
vdsm-patches@lists.fedorahosted.org
-
automation＠ovirt.org -
Dan Kenigsberg -
Douglas Schilling Landgraf -
fabiand＠redhat.com -
Nir Soffer