[libvirt] Patch replaces scriptlets with new systemd macros
Hi, I created patch for spec file to follow changes in guidelines with respect to new systemd macros (see https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd)
On Wed, Oct 24, 2012 at 12:37:51PM +0200, Václav Pavlín wrote:
Hi,
I created patch for spec file to follow changes in guidelines with respect to new systemd macros (see https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd)
What Fedora release are these macros available since ? The libvirt RPM spec needs to support systemd in Fedora >= 17. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/24/2012 08:29 AM, Daniel P. Berrange wrote:
On Wed, Oct 24, 2012 at 12:37:51PM +0200, Václav Pavlín wrote:
Hi,
I created patch for spec file to follow changes in guidelines with respect to new systemd macros (see https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd)
What Fedora release are these macros available since ? The libvirt RPM spec needs to support systemd in Fedora >= 17.
Reading the link, it looks like the scriptlets are only available in F18 and later; thus, we have to stick to the older longhand version for at least another year (that is, until F17 goes obsolete). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/24/2012 01:16 PM, Eric Blake wrote:
On 10/24/2012 08:29 AM, Daniel P. Berrange wrote:
On Wed, Oct 24, 2012 at 12:37:51PM +0200, Václav Pavlín wrote:
Hi,
I created patch for spec file to follow changes in guidelines with respect to new systemd macros (see https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd) What Fedora release are these macros available since ? The libvirt RPM spec needs to support systemd in Fedora >= 17. Reading the link, it looks like the scriptlets are only available in F18 and later; thus, we have to stick to the older longhand version for at least another year (that is, until F17 goes obsolete).
Possibly longer, if we want to continue to be able to do rpm builds on RHEL6 and CentOS6. (And in case you're considering asking, I think we'd all rather have the Fedora versions of the specfile be exact copies of the upstream specfile, as maintenance would otherwise be a major pain) or - is it possible to have these pieces only conditionally take effect based on %{?rhel} and %{?fedora}. I'm not familiar enough with specfile syntax to know if that's a possibility. (BTW, your patch appears to be made against the libvirt.spec that's in one of the branches of fedora-git for the libvirt package. Patches sent to libvir-list should be made against the files in the upstream git repo, which is at git://libvirt.org/libvirt.git)
On 10/24/2012 04:37 AM, Václav Pavlín wrote:
Hi,
I created patch for spec file to follow changes in guidelines with respect to new systemd macros (see https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd)
new-systemd-macros.patch
diff --git a/libvirt.spec b/libvirt.spec index b47ec0e..675290f 100644 --- a/libvirt.spec +++ b/libvirt.spec
Upstream, we would want to patch libvirt.spec.in (the parent of libvirt.spec).
@@ -316,7 +316,7 @@ Summary: Library providing a simple virtualization API Name: libvirt Version: 0.10.2 -Release: 3%{?dist}%{?extra_release} +Release: 4%{?dist}%{?extra_release}
This is a downstream-only change, since upstream libvirt.spec.in does not track downstream releases. Besides, downstream is more likely to use the upstream v0.10.2-maint branch and use 0.10.2.1-1 as the next build for Fedora 18.
%if %{with_systemd} -if [ $1 -eq 1 ] ; then - # Initial installation - /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : - /bin/systemctl enable cgconfig.service >/dev/null 2>&1 || : -fi +%systemd_post libvirtd.service cgconfig.service
Hmm - we may have a pre-existing issue: this is a case of enabling a service that we don't own, but where cgconfig.service is a prerequisite of libvirtd.service. Is this really the right order for enabling the two services, and is it really necessary for use to explicitly enable cgconfig.service, or should the contents of libvirtd.service be adequate for systemd to automatically figure out the dependency?
%changelog +* Fri Oct 19 2012 Václav Pavlín <vpavlin@redhat.com> - 0.10.2-4 +- Scriptlets replaced with new systemd macros (#850186)
Again, %changelog is something only needed for downstream builds. Also, your patch is missing a conversion for libvirt-guests.service, also in the spec file: $ grep -C2 libvirt-guests.service libvirt.spec.in # If the package is allowed to autostart: /bin/systemctl --no-reload enable libvirt-guests.service >/dev/null 2>&1 ||: # Run these because the SysV package being removed won't do them /sbin/chkconfig --del libvirt-guests >/dev/null 2>&1 || : /bin/systemctl try-restart libvirt-guests.service >/dev/null 2>&1 || : %endif -- %{_sysconfdir}/rc.d/init.d/libvirt-guests %if %{with_systemd} %{_unitdir}/libvirt-guests.service %endif %config(noreplace) %{_sysconfdir}/sysconfig/libvirt-guests -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Hi, sorry, I didn't realize I have to modify upstream version. I added Requires stanza to libvirtd service file for cgconfig.service, so you should not need to call enable in spec file for it. I added %with_systemd_macros so it should now work in F17 with old scriptlets and in F18+/RHEL7+ with systemd macros I missed libvirt-guests.service because there is no systemctl call for it. So I only added systemd macros calls. Release and Changelog are untouched so you can modify it as you need. Regards Vaclav Eric Blake píše v St 24. 10. 2012 v 11:30 -0600:
On 10/24/2012 04:37 AM, Václav Pavlín wrote:
Hi,
I created patch for spec file to follow changes in guidelines with respect to new systemd macros (see https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd)
new-systemd-macros.patch
diff --git a/libvirt.spec b/libvirt.spec index b47ec0e..675290f 100644 --- a/libvirt.spec +++ b/libvirt.spec
Upstream, we would want to patch libvirt.spec.in (the parent of libvirt.spec).
@@ -316,7 +316,7 @@ Summary: Library providing a simple virtualization API Name: libvirt Version: 0.10.2 -Release: 3%{?dist}%{?extra_release} +Release: 4%{?dist}%{?extra_release}
This is a downstream-only change, since upstream libvirt.spec.in does not track downstream releases. Besides, downstream is more likely to use the upstream v0.10.2-maint branch and use 0.10.2.1-1 as the next build for Fedora 18.
%if %{with_systemd} -if [ $1 -eq 1 ] ; then - # Initial installation - /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : - /bin/systemctl enable cgconfig.service >/dev/null 2>&1 || : -fi +%systemd_post libvirtd.service cgconfig.service
Hmm - we may have a pre-existing issue: this is a case of enabling a service that we don't own, but where cgconfig.service is a prerequisite of libvirtd.service. Is this really the right order for enabling the two services, and is it really necessary for use to explicitly enable cgconfig.service, or should the contents of libvirtd.service be adequate for systemd to automatically figure out the dependency?
%changelog +* Fri Oct 19 2012 Václav Pavlín <vpavlin@redhat.com> - 0.10.2-4 +- Scriptlets replaced with new systemd macros (#850186)
Again, %changelog is something only needed for downstream builds. Also, your patch is missing a conversion for libvirt-guests.service, also in the spec file:
$ grep -C2 libvirt-guests.service libvirt.spec.in # If the package is allowed to autostart: /bin/systemctl --no-reload enable libvirt-guests.service >/dev/null 2>&1 ||:
# Run these because the SysV package being removed won't do them /sbin/chkconfig --del libvirt-guests >/dev/null 2>&1 || : /bin/systemctl try-restart libvirt-guests.service >/dev/null 2>&1 || : %endif
-- %{_sysconfdir}/rc.d/init.d/libvirt-guests %if %{with_systemd} %{_unitdir}/libvirt-guests.service %endif %config(noreplace) %{_sysconfdir}/sysconfig/libvirt-guests
On 10/25/2012 04:10 AM, Václav Pavlín wrote:
Hi,
Hello, [please don't top-post on technical lists]
sorry, I didn't realize I have to modify upstream version.
No problem; we can probably figure out how to modify the upstream version based on your patch to downstream, if it comes to that, and if we decide that the modifications even make sense. Right now, our biggest concern is that using the new scriptlets will break the use of the same spec file when building for F17, so that concern needs to be addressed before we can accept your patch.
I added Requires stanza to libvirtd service file for cgconfig.service, so you should not need to call enable in spec file for it.
That sounds independently useful, and probably worth applying even while debating about the scriptlets.
I added %with_systemd_macros so it should now work in F17 with old scriptlets and in F18+/RHEL7+ with systemd macros
Ah, then maybe you did answer the big question. Except that I don't see the updated patch - did you forget to attach it?
I missed libvirt-guests.service because there is no systemctl call for it. So I only added systemd macros calls.
Release and Changelog are untouched so you can modify it as you need.
-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Fri, Oct 26, 2012 at 08:09:54AM +0200, Václav Pavlín wrote:
Eric Blake píše v Čt 25. 10. 2012 v 10:41 -0600:
Hello,
Ah, then maybe you did answer the big question. Except that I don't see the updated patch - did you forget to attach it?
Sure, I did forgot, sorry. Here it is...
diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in index b7afadf..b78c301 100644 --- a/daemon/libvirtd.service.in +++ b/daemon/libvirtd.service.in @@ -7,6 +7,7 @@ Description=Virtualization daemon Before=libvirt-guests.service After=network.target +Requires=cgconfig.service
I explicitly left that out when i created the libvirtd.service file. Since systemd itself is capable of setting up cgroups by default, the common need for cgconfig.service has gone away. A minority of people may still wish to use it, but they can customize the libvirtd.service file themselves if desired - we shouldn't force cgconfig.service onto everyone esle. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 10/29/2012 05:56 AM, Daniel P. Berrange wrote:
On Fri, Oct 26, 2012 at 08:09:54AM +0200, Václav Pavlín wrote:
Eric Blake píše v Čt 25. 10. 2012 v 10:41 -0600:
Hello,
Ah, then maybe you did answer the big question. Except that I don't see the updated patch - did you forget to attach it?
Sure, I did forgot, sorry. Here it is...
diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in index b7afadf..b78c301 100644 --- a/daemon/libvirtd.service.in +++ b/daemon/libvirtd.service.in @@ -7,6 +7,7 @@ Description=Virtualization daemon Before=libvirt-guests.service After=network.target +Requires=cgconfig.service
I explicitly left that out when i created the libvirtd.service file. Since systemd itself is capable of setting up cgroups by default, the common need for cgconfig.service has gone away. A minority of people may still wish to use it, but they can customize the libvirtd.service file themselves if desired - we shouldn't force cgconfig.service onto everyone esle.
Then why are we explicitly starting cgconfig.service in the spec file? Shouldn't the argument go that only those people customizing the service file to use cgconfig need cgconfig enabled in the first place? In other words, is there anything wrong with this one-liner? diff --git i/libvirt.spec.in w/libvirt.spec.in index ebebfab..9d11328 100644 --- i/libvirt.spec.in +++ w/libvirt.spec.in @@ -1465,13 +1465,12 @@ done %endif %if %{with_systemd} if [ $1 -eq 1 ] ; then # Initial installation /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : - /bin/systemctl enable cgconfig.service >/dev/null 2>&1 || : fi %else %if %{with_cgconfig} # Starting with Fedora 16/RHEL-7, systemd automounts all cgroups, # and cgconfig is no longer a necessary service. %if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?fedora} && 0%{?fedora} < 16) -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On Mon, Oct 29, 2012 at 02:53:31PM -0600, Eric Blake wrote:
On 10/29/2012 05:56 AM, Daniel P. Berrange wrote:
On Fri, Oct 26, 2012 at 08:09:54AM +0200, Václav Pavlín wrote:
Eric Blake píše v Čt 25. 10. 2012 v 10:41 -0600:
Hello,
Ah, then maybe you did answer the big question. Except that I don't see the updated patch - did you forget to attach it?
Sure, I did forgot, sorry. Here it is...
diff --git a/daemon/libvirtd.service.in b/daemon/libvirtd.service.in index b7afadf..b78c301 100644 --- a/daemon/libvirtd.service.in +++ b/daemon/libvirtd.service.in @@ -7,6 +7,7 @@ Description=Virtualization daemon Before=libvirt-guests.service After=network.target +Requires=cgconfig.service
I explicitly left that out when i created the libvirtd.service file. Since systemd itself is capable of setting up cgroups by default, the common need for cgconfig.service has gone away. A minority of people may still wish to use it, but they can customize the libvirtd.service file themselves if desired - we shouldn't force cgconfig.service onto everyone esle.
Then why are we explicitly starting cgconfig.service in the spec file? Shouldn't the argument go that only those people customizing the service file to use cgconfig need cgconfig enabled in the first place? In other words, is there anything wrong with this one-liner?
Oh that's probably a mistake then. We shouldn't be enabling it for systemd based installations.
diff --git i/libvirt.spec.in w/libvirt.spec.in index ebebfab..9d11328 100644 --- i/libvirt.spec.in +++ w/libvirt.spec.in @@ -1465,13 +1465,12 @@ done %endif
%if %{with_systemd} if [ $1 -eq 1 ] ; then # Initial installation /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : - /bin/systemctl enable cgconfig.service >/dev/null 2>&1 || : fi %else %if %{with_cgconfig} # Starting with Fedora 16/RHEL-7, systemd automounts all cgroups, # and cgconfig is no longer a necessary service. %if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?fedora} && 0%{?fedora} < 16)
ACK Daniel
On 10/30/2012 05:56 AM, Daniel P. Berrange wrote:
Then why are we explicitly starting cgconfig.service in the spec file? Shouldn't the argument go that only those people customizing the service file to use cgconfig need cgconfig enabled in the first place? In other words, is there anything wrong with this one-liner?
Oh that's probably a mistake then. We shouldn't be enabling it for systemd based installations.
diff --git i/libvirt.spec.in w/libvirt.spec.in index ebebfab..9d11328 100644 --- i/libvirt.spec.in +++ w/libvirt.spec.in @@ -1465,13 +1465,12 @@ done %endif
%if %{with_systemd} if [ $1 -eq 1 ] ; then # Initial installation /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : - /bin/systemctl enable cgconfig.service >/dev/null 2>&1 || : fi %else %if %{with_cgconfig} # Starting with Fedora 16/RHEL-7, systemd automounts all cgroups, # and cgconfig is no longer a necessary service. %if (0%{?rhel} && 0%{?rhel} < 7) || (0%{?fedora} && 0%{?fedora} < 16)
ACK
Thanks; I've (finally) pushed this to master, v0.10.2-maint (F18), and v0.9.11-main (F17) (too bad I forgot about this thread, and didn't get it applied before 1.0.0), under the following commit message. spec: don't enable cgconfig under systemd In Fedora 16, we quit enabling cgconfig because systemd set up default cgroups that were good enough for our use. But in F17, when we switched to systemd, we reverted and started up cgconfig again. See also the tail of this thread: https://www.redhat.com/archives/libvir-list/2012-October/msg01657.html * libvirt.spec.in (with_systemd): Rely on systemd for cgroups. I will now focus on reviewing the rest of Václav's patch. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 10/25/2012 04:10 AM, Václav Pavlín wrote:
Hi,
sorry, I didn't realize I have to modify upstream version.
No problem - we'll get it all straightened out.
I added Requires stanza to libvirtd service file for cgconfig.service, so you should not need to call enable in spec file for it.
We independently decided to drop that hunk as worthless (If F16 didn't need it, then why does F17?)
I added %with_systemd_macros so it should now work in F17 with old scriptlets and in F18+/RHEL7+ with systemd macros
That's the piece I was missing in the original submission. Looks like we're ready to take things now.
+++ b/daemon/libvirtd.service.in @@ -7,6 +7,7 @@ Description=Virtualization daemon Before=libvirt-guests.service After=network.target +Requires=cgconfig.service
This hunk is no longer needed.
+++ b/libvirt.spec.in @@ -319,6 +319,13 @@ %define with_rhel5 0 %endif
+%if 0%{?fedora} >= 18 || 0%{?rhel} >= 7 +%define with_systemd_macros 1 +%else +%define with_systemd_macros 0 +%endif
Looks reasonable.
%if %{with_systemd} -if [ $1 -eq 1 ] ; then - # Initial installation - /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : - /bin/systemctl enable cgconfig.service >/dev/null 2>&1 || : -fi +%if %{with_systemd_macros} + %systemd_post libvirtd.service +%else + if [ $1 -eq 1 ] ; then + # Initial installation + /bin/systemctl enable libvirtd.service >/dev/null 2>&1 || : + fi +%endif
I don't know if the extra indentation makes things more or less legible; it made the diff bigger, though, so I removed it. ACK and pushed. This will be present in libvirt 1.0.1 for rawhide, and will probably be picked up for libvirt 0.10.2.2 for F18. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (4)
-
Daniel P. Berrange -
Eric Blake -
Laine Stump -
Václav Pavlín