Re: RFC: Switch to a date-based versioning scheme
On Thu, Oct 26, 2023 at 06:48:28AM -0700, Andrea Bolognani wrote: > Since we're just a few months away from the 10.0.0 release, I thought > it would be a good time to bring up this idea. > > Can we move to date-based version numbers? I suggest having > > libvirt 24.01.0 instead of 10.0.0 > 24.03.010.1.0 > 24.04.010.2.0 > ... > 24.11.010.9.0 > 24.12.010.10.0 > > The big advantage is that, once version numbers are obviously > date-based, any expectation of them being interpreted according to > semver[1] are immediately gone. I don't think that is the case. Any version number you ever see can plausibly be a semver version when seen without any context. The only way that confusion goes away is if we were to actually use semver, or if people stop blindly assuming every project uses semver. I don't think this is a reason to change what we're doing. > People are quite used to date-based version numbers thanks to Ubuntu > having used them for almost two decades, so I don't think anyone is > going to be confused by the move. And since our release schedule is > already date-based, having the versioning scheme match that just > makes perfect sense IMO. > > Up until now, one could have argued in favor of the current > versioning scheme because of the single-digit major version > component, but that's going away next year regardless, which makes > this the perfect time to raise the topic :) > > Thoughts? My thoughts on calver are unchanged since it was previously suggested this https://listman.redhat.com/archives/libvir-list/2016-June/131961.html https://listman.redhat.com/archives/libvir-list/2016-June/131958.html Our version numbers are explicitly just a counter that ticks over on a defined schedule and semantic meaning should not be attached to any single release number. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] util:hostcpu: Report physical address size based on Architecture
On Wed, Oct 25, 2023 at 09:12:32AM -0700, Andrea Bolognani wrote: > On Fri, Oct 20, 2023 at 12:48:26PM +0200, Michal Prívozník wrote: > > On 10/4/23 07:58, Narayana Murty N wrote: > > > This patch fixes this issue by returning the size=0 for architectures > > > other than x86 and SuperH. > > > > Whoa, I had no idea that SH is still alive (an well?). > > As of a few months ago, Debian builds libvirt on SuperH: > > https://buildd.debian.org/status/logs.php?pkg=libvirt&arch=sh4 > > The QEMU driver is disabled, since there is no matching qemu-system > binary, but the LXC driver should (at least theoretically) work :) Maybe I misinterpret what you're saying here, but the lack of a qemu-system-sh binary shouldn't be a reason to disable the QEMU driver, as the user can use non-native qemu-system binaries. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt RFCv11 00/33] multifd save restore prototype
On Mon, Oct 23, 2023 at 04:06:53PM +0200, Claudio Fontana wrote:
> On 10/11/23 17:29, Daniel P. Berrangé wrote:
> > On Wed, Oct 11, 2023 at 04:56:12PM +0200, Claudio Fontana wrote:
> >>
> >> On 10/11/23 16:05, Daniel P. Berrangé wrote:
> >>>
> >>> Instead of using 'getfd' though we have to use 'add-fd'.
> >>>
> >>> Anyway, this lets us do FD passing as normal, whle also
> >>> letting us specify the offset.
> >>>
> >>> {"execute": "add-fd", "arguments": {"fdset-id":"migrate"}}
> >>> {"execute": "migrate", "arguments":
> >>> {"detach":true,"blk":false,"inc":false,"uri":"file:/dev/fdset/migrate,offset=124456"}}'
>
>
> Hi Daniel,
>
> the "add-fd" is the part that I don't understand at all,
>
> should we actually pass an fd there like with fd-get, already open with the
> savevm file?
> Something in pseudocode like:
>
> virsh qemu-monitor-command --pass-fds 10 --cmd='{"execute": "add-fd",
> "arguments": {"fdset-id":10}} ?
>
> should we use "opaque" instead of "fdset-id" if you want to actually set it
> to "migrate"?
> And how to reference it later?
>
> virsh qemu-monitor-command --cmd='{"execute": "migrate", "arguments":
> {"detach":true,"blk":false,"inc":false,"uri":"file:/dev/fdset/migrate,offset=124456"}}
>
> ?
>
> "opaque" does not seem to get me a reachable /dev/fdset/migrate though.
>
> I can currently trigger the migration to the URI file:/mnt/nvme/savevm so
> that seems to work fine,
> it's the file:/dev/fdset part that I am still unable to glue together.
Sorry, I was mis-remembering the fdset usage. They have to be used via
a numeric ID value, not a named set. The numeric IDs don't match the FD
numbers either. So I think it would be :
virsh qemu-monitor-command --pass-fds 10 --cmd='{"execute": "add-fd",
"arguments": {"fdset-id":7}} ?
virsh qemu-monitor-command --cmd='{"execute": "migrate", "arguments":
{"detach":true,"blk":false,"inc":false,"uri":"file:/dev/fdset/7,offset=124456"}}
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH v7 0/4] Migration deprecated parts
On Wed, Oct 18, 2023 at 12:38:10PM +0200, Juan Quintela wrote:
> Juan Quintela wrote:
> > Based on: Message-ID: <20231018100651.32674-1-quint...@redhat.com>
> > [PULL 00/11] Migration 20231018 patches
> >
> > And here we are, at v7:
> > - drop black line at the end of deprecated.rst
> > - change qemu-iotest output due to warnings for deprecation.
> >
> > The only real change is the output of the qemu-iotest. That is the
> > reason why I maintained the reviewed-by. But will be happy if anyone
> > of the block people ack the changes.
>
> I forgot to include the link to the CI of the previous failure.
>
> https://gitlab.com/juan.quintela/qemu/-/jobs/5314070229
>
> tput mismatch (see
> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad)
> --- /builds/juan.quintela/qemu/tests/qemu-iotests/183.out
> +++
> /builds/juan.quintela/qemu/build/tests/qemu-iotests/scratch/raw-file-183/183.out.bad
> @@ -28,6 +28,8 @@
> { 'execute': 'migrate',
> 'arguments': { 'uri': 'unix:SOCK_DIR/migrate', 'blk': true } }
> +warning: parameter 'blk is deprecated; use blockdev-mirror with NBD instead
> +warning: block migration is deprecated; use blockdev-mirror with NBD instead
> {"return": {}}
> { 'execute': 'query-status' }
> {"return": {"status": "postmigrate", "singlestep": false, "running":
> false}}
IIUC, the JSON bits are being written on stdout, and the warnings
are being written on stderr. The interleaving of the data is
potentially going to be non-deterministic in the .out file.
Generally you'd want a filter in the iotests that culls the
warning: lines to avoid this mixing of stdout/err streams.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH] hypervisor: Move interface mgmt methods to hypervisor
On Sun, Oct 15, 2023 at 03:03:40PM -0400, Laine Stump wrote: > On 10/12/23 3:37 PM, Praveen K Paladugu wrote: > > Move guest interface management methods from qemu to hypervisor. These > > methods will be shared by networking support in ch driver. > > > > Signed-off-by: Praveen K Paladugu > > --- > > po/POTFILES | 1 + > > src/hypervisor/domain_interface.c | 279 ++ > > src/hypervisor/domain_interface.h | 38 > > src/hypervisor/meson.build| 1 + > > src/libvirt_private.syms | 6 + > > src/qemu/qemu_command.c | 7 +- > > src/qemu/qemu_hotplug.c | 3 +- > > src/qemu/qemu_interface.c | 246 +- > > src/qemu/qemu_interface.h | 6 - > > src/qemu/qemu_process.c | 3 +- > > 10 files changed, 341 insertions(+), 249 deletions(-) > > create mode 100644 src/hypervisor/domain_interface.c > > create mode 100644 src/hypervisor/domain_interface.h > > > > diff --git a/po/POTFILES b/po/POTFILES > > index 3a51aea5cb..023c041f61 100644 > > --- a/po/POTFILES > > +++ b/po/POTFILES > > @@ -92,6 +92,7 @@ src/hyperv/hyperv_util.c > > src/hyperv/hyperv_wmi.c > > src/hypervisor/domain_cgroup.c > > src/hypervisor/domain_driver.c > > +src/hypervisor/domain_interface.c > > src/hypervisor/virhostdev.c > > src/interface/interface_backend_netcf.c > > src/interface/interface_backend_udev.c > > diff --git a/src/hypervisor/domain_interface.c > > b/src/hypervisor/domain_interface.c > > new file mode 100644 > > index 00..b01b6ee767 > > --- /dev/null > > +++ b/src/hypervisor/domain_interface.c > > @@ -0,0 +1,279 @@ > > +/* > > + * Copyright Microsoft Corp. 2023 > > I haven't had time to look through the rest of this yet (although it seems > pretty straightforward, and I think it might have been me who suggested it > in the first place :-)), I did want to bring up this one topic now: > > > Best practices for the copyright notice in a new file have changed over the > years (for example, we no longer list the "Author" of new files and even > removed Author from existing files, since a more accurate and complete > version of that info is all available from git), and I haven't paid close > attention, but since this entire file is comprised of functions that were > just moved from an existing file and renamed (rather than actual new code), > I don't think it's appropriate for it to erase all trace of the original > copyright holder and simply assign the copyright entirely to Microsoft. If the original file that the code was copied from had any Copyright lines, those must be preserved in the new file. If the original file had no copyright lines, you're not required to do any archeology to figure out copyrighth holders, just leave the new file similarly devoid. Simply moving code is also not a copyrightable enhancement, so adding your own copyright on moved code is inappropriate. Once you add new functionality to the newfile though, you are free to add your own copyright statement at that point, if desired. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
ANNOUNCE: Forthcoming changes to libvirt mailing list hosting
Hello Libvirt Community, For the last 18(!) years, libvirt community mailing lists have been kindly hosted by Red Hat Corporate IT on the redhat.com Mailman installation. In retrospect this wasn't the ideal home for community mailing lists but that decision is ancient history. Unfortunately as a consequence of infrastructure changes, the redhat.com Mailman service is scheduled to be decomissioned, and in the very near future ($low weeks) will cease to be able to send/receive mail for libvirt lists. As a result, all the libvirt mailing lists will need to move to a new Mailman installation, managed on our behalf by the Red Hat Open Source Community Infrastructure team[1], who provide services for a variety of OSS projects. It is intended that this new service will be dedicated for our community and thus located somewhere under the libvirt.org domain. It should go without saying that all existing mail archives will be migrated over to the new server. None of the historical record should be lost. It is not considered appropriate netiquette, however, to bulk-resubscribe all current list members under a new domain. Thus anyone who wants to remain on the community lists will soon be requested to take action to enroll on the new mailman service. The new service is not quite ready for use yet. This mail serves as a heads up that the changes will be coming in the next couple of weeks that will require action. A further announcement will be sent when the new mailman is ready to accept subscriptions, as well as warnings before the old lists stop accepting messages. With regards, Daniel [1] https://www.osci.io/ -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt RFCv11 00/33] multifd save restore prototype
On Wed, Oct 11, 2023 at 04:56:12PM +0200, Claudio Fontana wrote:
>
> On 10/11/23 16:05, Daniel P. Berrangé wrote:
> >
> > Instead of using 'getfd' though we have to use 'add-fd'.
> >
> > Anyway, this lets us do FD passing as normal, whle also
> > letting us specify the offset.
> >
> > {"execute": "add-fd", "arguments": {"fdset-id":"migrate"}}
> > {"execute": "migrate", "arguments":
> > {"detach":true,"blk":false,"inc":false,"uri":"file:/dev/fdset/migrate,offset=124456"}}'
> >
> >> Internally, the QEMU multifd code just reads and writes using pread,
> >> pwrite, so there is in any case just one fd to worry about,
> >> but who should own it, libvirt or QEMU?
> >
> > How about both :-)
>
> I need to familiarize a bit with this, there are pieces I am missing. Can you
> correct here?
>
> OPTION 1)
>
> libvirt opens the file and has the FD, writes the header, marks the offset,
> then we dup the FD in libvirt for the benefit of QEMU, optionally set the
> flags of the dup to "O_DIRECT" (the usual case) depending on --bypass-cache,
> pass the duped FD to QEMU,
> QEMU does all the pread/pwrite on it with the correct offset (since it knows
> it from the file:// URI optional offset parameter),
> then libvirt closes the duped fd
> libvirt rewrites the header using the original fd (needed to update the
> metadata),
> libvirt closes the original fd
>
>
> OPTION 2)
>
> libvirt opens the file and has the FD, writes the header, marks the offset,
> then we pass the FD to QEMU,
> QEMU dups the FD and sets it as "O_DIRECT" depending on a passed parameter,
> QEMU does all the pread/pwrite on it with the correct offset (since it knows
> it from the file:// URI optional offset parameter),
> QEMU closes the duped FD,
> libvirt rewrites the header using the original fd (needed to update the
> metadata),
> libvirt closes the original fd
>
>
> I don't remember if QEMU changes for the file offsets optimization are
> already "block friendly" ie they operate correctly whatever the state of
> O_DIRECT or ~O_DIRECT,
> I think so. They have been thought with O_DIRECT in mind.
The 'file' protocol as it exists currently is not O_DIRECT
capable. It is not writing aligned buffers to aligned offsets
in the file. It is still running the regular old migration
stream format over the file, not taking advantage of it being
random access.
What's needed is the followup "fixed ram" format adaptation.
Use of that format should imply O_DIRECT, so in fact we
don't need an explicit 'bypass_cache' parameter in QAPI,
just a way to ask for the 'fixed ram' format.
> So I would tend to see OPTION 1) as more attractive as QEMU does not need to
> care about another parameter, whatever has been chosen in libvirt in terms of
> bypass cache is handled in libvirt.
The 'fixed ram' format will only take care of I/O for the
main RAM blocks which are nicely aligned and can be written
to aligned file offsets. The general device vmstate I/O
probably can't be assumed to be aligned. While we could
futz around with QEMUFile so that it bounce buffers vmstate
to an aligned region and flushes it in page sized chunks
that's probably too much of a pain.
IOW, actually I think what QEMU would likely want to
do is
1. qemu_open -> get a FD *without* O_DIRECT set
2. write some vmstate stuff
3. turn on O_DIRECT
4. write RAM in fixed locations
5. turn off O_DIRECT
6. write remaining vmstate
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt RFCv11 00/33] multifd save restore prototype
On Wed, Oct 11, 2023 at 03:46:59PM +0200, Claudio Fontana wrote:
> In terms of our use case, we would need to trigger these migrations from
> virsh save, restore, managedsave / start.
>
> 1) Can you confirm this is still a good target?
IIRC the 'dump' command also has a codepath that can exercise
the migrate-to-file logic too.
> It would seem right from my perspective to hook up save/restore first, and
> then reuse the same mechanism for managedsave / start.
All of save, restore, managedsave, start, dump end up calling
into the same internal helper methods. So once you update these
helpers, you essentially get all the commands converted in one
go.
> 2) Do we expect to pass filename or file descriptor from libvirt into QEMU?
>
>
> As is, libvirt today generally passes an already opened file descriptor to
> QEMU for migrations, roughly:
>
> {"execute": "getfd", "arguments": {"fdname":"migrate"}} (passing the already
> open fd from libvirt f.e. 10)
> {"execute": "migrate", "arguments":
> {"detach":true,"blk":false,"inc":false,"uri":"fd:migrate"}}'
>
> Do we want to change libvirt to migrate to a file: URI ? Does this have
> consequence for "labeling" / security sandboxing?
>
> Or would it be better to continue opening the fd in libvirt, writing the
> libvirt header, and then passing the existing open fd to QEMU, using QMP
> command "getfd",
> followed by "migrate"? In this second case we would need to inform QEMU of
> the offset into the already open fd.
How about both :-)
The current migration 'fd' protocol technically can cope with
any type of FD being passed on. QEMU doesn't try to interpret
the FD type right to any significant degree.
The 'file' protocol is explicitly providing a migration transport
supporting random access I/O to storage. As such we can specify
the offset too.
Now the neat trick is that 'file' protocol impl uses
qio_channel_file and this in turn uses qemu_open,
which supports FD passing.
Instead of using 'getfd' though we have to use 'add-fd'.
Anyway, this lets us do FD passing as normal, whle also
letting us specify the offset.
{"execute": "add-fd", "arguments": {"fdset-id":"migrate"}}
{"execute": "migrate", "arguments":
{"detach":true,"blk":false,"inc":false,"uri":"file:/dev/fdset/migrate,offset=124456"}}'
> Internally, the QEMU multifd code just reads and writes using pread, pwrite,
> so there is in any case just one fd to worry about,
> but who should own it, libvirt or QEMU?
How about both :-)
Libvirt will open the file, in order to write its header.
Then libvirt passes the open FD to QEMU, specifying the
offset, and QEMU does its thing with vmstate, etc and
closes the FD when its done. libvirt's copy of the FD
is still open, and libvirt can finalize its header and
close the FD.
> 3) How do we deal with O_DIRECT? In the prototype we were setting the
> O_DIRECT on the fd from libvirt in response to the user request for
> --bypass-cache,
> which is needed 99% of the time with large VMs. I think I remember that we
> plan to write from libvirt normally (without O_DIRECT) and then set the flag
> later,
> but should libvirt or QEMU set the O_DIRECT flag? This likely depends on who
> owns the fd?
For O_DIRECT, the 'file' protocol should gain a new parameter
'bypass_cache: bool'. If this is set to 'true' then QEMU can
set O_DIRECT on the FD it opens or receives from libvirt.
Libvirt probably just has to be careful to unset O_DIRECT
at the end before it finalizes the header.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH] logging: lockdown the systemd service configuration
Ping, for review feedback. On Tue, Sep 26, 2023 at 05:11:44PM +0100, Daniel P. Berrangé wrote: > The 'systemd-analyze security' command looks at the unit file > configuration and reports on any settings which increase the > attack surface for the daemon. Since most systemd units are > fairly minimalist, this is generally informing us about settings > that we never put any thought into using before. > > In its current configuration it reports > > # systemd-analyze security virtlogd.service > ...snip... > → Overall exposure level for virtlogd.service: 9.6 UNSAFE 😨 > > which is pretty terrible as a score. > > If we apply all of the recommendations that appear possible > without (knowingly) breaking functionality it reports: > > # systemd-analyze security virtlogd.service > ...snip... > → Overall exposure level for virtlogd.service: 2.2 OK 🙂 > > which is a pretty decent improvement. > > Some of the settings we would like to enable require a systemd > version that is newer than that available in our oldest distro > target - RHEL-8 at v239. > > NB, RestrictSUIDSGID is technically newer than 239, but RHEL-8 > backported it, and other distros we target have it by default. > > Remaining recommendations are > > ✗ CapabilityBoundingSet=~CAP_(DAC_*|FOWNER|IPC_OWNER) > > We block FOWNER/IPC_OWNER, but can't block the two DAC > capabilities. Historically apps/users might point QEMU > to log files in $HOME, pre-created with their own user > ID. > > ✗ IPAddressDeny= > > Not required since RestrictAddressFamilies blocks IP > usage. Ignoring this avoids the overhead of creating > a traffic filter than will never be used. > > ✗ NoNewPrivileges= > > Highly desirable, but cannot enable it yet, because it > will block the ability to transition to the virtlogd_t > SELinux domain during execve. The SELinux policy needs > fixing to permit this transition under NNP first. > > ✗ PrivateTmp= > > There is a decent chance people have VMs configured > with a serial port logfile pointing at /tmp. We would > cause a regression to use private /tmp for logging > > ✗ PrivateUsers= > > This would put virtlogd inside a user namespace where > its root is in fact unprivileged. Same problem as the > User= setting below > > ✗ ProcSubset= > > Libraries we link to might read certain non-PID related > files from /proc > > ✗ ProtectClock= > > Requires v245 > > ✗ ProtectHome= > > Same problem as PrivateTmp=. There's a decent chance > that someone has a VM configured to write a logfile > to /home > > ✗ ProtectHostname= > > Requires v241 > > ✗ ProtectKernelLogs > > Requires v244 > > ✗ ProtectProc > > Requires v247 > > ✗ ProtectSystem= > > We only set it to 'full', as 'strict' is not viable for > our required usage > > ✗ RootDirectory=/RootImage= > > We are not capable of running inside a custom chroot > given needs to write log files to arbitrary places > > ✗ RestrictAddressFamilies=~AF_UNIX > > We need AF_UNIX to communicate with other libvirt daemons > > ✗ SystemCallFilter=~@resources > > We link to libvirt.so which links to libnuma.so which has > a constructor that calls set_mempolicy. This is highly > undesirable todo during a constructor. > > ✗ User=/DynamicUser= > > This is highly desirable, but we currently read/write > logs as root, and directories we're told to write into > could be anywhere. So using a non-root user would have > a major risk of regressions for applications and also > have upgrade implications > > Signed-off-by: Daniel P. Berrangé > --- > src/logging/virtlogd.service.in | 94 + > 1 file changed, 94 insertions(+) > > diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in > index 8e245ddb43..9e3838ff34 100644 > --- a/src/logging/virtlogd.service.in > +++ b/src/logging/virtlogd.service.in > @@ -20,5 +20,99 @@ OOMScoreAdjust=-900 > # per systemd recommendations > LimitNOFILE=1024:524288 > > +CapabilityBoundingSet=~CAP_AUDIT_CONTROL > +CapabilityBoundingSet=~CAP_AUDIT_READ > +CapabilityBoundingSet=~CAP_AUDIT_WRITE > +CapabilityBoundingSet=~CAP_BLOCK_SUSPEND > +CapabilityBoundingSet=~CAP_CHOWN > +# Mgmt app/user might have pre-created log files that we're > +# told to open and write to, or be storing them in otherwise > +# inaccessible locations like $HOME. So we need to ignore > +# DAC permission checks. > +#CapabilityBoundingSet=~CAP_DAC_OVERRIDE > +#CapabilityBoundin
Re: [libvirt PATCH] rpm: Drop with_vz define
On Thu, Sep 28, 2023 at 03:32:15PM +0200, Andrea Bolognani wrote: > Commit 56edf2fefe30 removed the last use. > > Signed-off-by: Andrea Bolognani > --- > libvirt.spec.in | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 31/33] systemd: Add RemoveOnStop=yes to all sockets
On Wed, Sep 27, 2023 at 06:19:32PM +0200, Andrea Bolognani wrote: > Currently we only set this for the main sockets, which means > that > > $ systemctl stop virtqemud.socket > > will make the socket disappear from the filesystem while > > $ systemctl stop virtqemud-ro.socket > > won't. Get rid of this inconsistency. systemd recommands against using RemoveOnStop, on the basis that it is valid to keep the service running but stop the socket. We've used deps to ensure thats not possible though, so adding RemoveOnStop isn't creating problems we don't already have. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd-admin.socket.in | 1 + > src/locking/virtlockd.socket.in | 1 + > src/logging/virtlogd-admin.socket.in | 1 + > src/logging/virtlogd.socket.in| 1 + > src/remote/libvirtd-admin.socket.in | 1 + > src/remote/libvirtd-ro.socket.in | 1 + > src/virtd-admin.socket.in | 1 + > src/virtd-ro.socket.in | 1 + > 8 files changed, 8 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 06/33] systemd: Introduce common templates
On Thu, Sep 28, 2023 at 06:52:35AM -0500, Andrea Bolognani wrote: > On Thu, Sep 28, 2023 at 04:30:03AM -0500, Andrea Bolognani wrote: > > On Thu, Sep 28, 2023 at 09:24:11AM +0100, Daniel P. Berrangé wrote: > > > On Wed, Sep 27, 2023 at 06:19:07PM +0200, Andrea Bolognani wrote: > > > > +++ b/scripts/merge-systemd-units.py > > > > @@ -0,0 +1,91 @@ > > > > +#!/usr/bin/env python3 > > > > > > Stick a license header of SPDX tag on this. > > > > Done (patch below). > > > > > Also if you didn't already do it, run the file through 'black' > > > and let it do whatever it wants todo to formatting. > > > > It just changed all single quotes into double quotes :) > > > > > > - 8< - 8< - 8< - 8< - 8< - 8< - 8< - 8< > > - > > diff --git a/scripts/merge-systemd-units.py b/scripts/merge-systemd-units.py > > index f54c9556c9..bc3321230d 100755 > > --- a/scripts/merge-systemd-units.py > > +++ b/scripts/merge-systemd-units.py > > @@ -1,5 +1,8 @@ > > #!/usr/bin/env python3 > > > > +# Copyright (C) 2023 Red Hat, Inc. > > +# SPDX-License-Identifier: LGPL-2.1-or-later > > + > > import sys > > > > SECTIONS = [ > > - >8 - >8 - >8 - >8 - >8 - >8 - >8 - >8 > > - > > Can I consider the patch Reviewed-by: you with the above (and the > trivial changes to quotess applied by black) squashed in, or do you > want me to send a v3 for that? Everything else is ACKed at this > point, but I'm not going to push until 9.9.0 is open for business > anyway. Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 10/33] systemd: Switch virtnwfilterd to common templates
On Wed, Sep 27, 2023 at 06:19:11PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/nwfilter/meson.build | 4 > src/nwfilter/virtnwfilterd.service.in | 25 - > 2 files changed, 29 deletions(-) > delete mode 100644 src/nwfilter/virtnwfilterd.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 07/33] systemd: Use common templates by default
On Wed, Sep 27, 2023 at 06:19:08PM +0200, Andrea Bolognani wrote: > All services are still listing their input files explicitly, so > no changes to the output files will occur yet. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 10 +- > 1 file changed, 5 insertions(+), 5 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 26/33] systemd: Downgrade read-only/admin sockets to Wants
On Wed, Sep 27, 2023 at 06:19:27PM +0200, Andrea Bolognani wrote: > Only the main socket is actually necessary for the service to be > usable. > > In the past, we've had security issues that could be exploited via > access to the read-only socket, so a security-minded administrator > might consider disabling all optional sockets. This change makes > such a setup possible. > > Note that the services will still try to activate all their > sockets on startup, even if they have been disabled. To make sure > that the optional sockets are never started, they will have to be > masked. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd.service.in | 2 +- > src/logging/virtlogd.service.in | 2 +- > src/virtd.service.in | 4 ++-- > 3 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 30/33] systemd: Add Also between sockets
On Wed, Sep 27, 2023 at 06:19:31PM +0200, Andrea Bolognani wrote: > This results in all sockets for a service being enabled when a > single one of them is. > > The -tcp and -tls sockets are intentionally excluded, because > enabling them should require explicit action on the > administrator's part; moreover, disabling them should not result > in the local sockets being disabled too. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd-admin.socket.in | 1 + > src/locking/virtlockd.socket.in | 1 + > src/logging/virtlogd-admin.socket.in | 1 + > src/logging/virtlogd.socket.in| 1 + > src/remote/libvirtd-admin.socket.in | 2 ++ > src/remote/libvirtd-ro.socket.in | 2 ++ > src/remote/libvirtd.socket.in | 2 ++ > src/virtd-admin.socket.in | 2 ++ > src/virtd-ro.socket.in| 2 ++ > src/virtd.socket.in | 2 ++ > 10 files changed, 16 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 32/33] systemd: Improve and unify unit descriptions
On Wed, Sep 27, 2023 at 06:19:33PM +0200, Andrea Bolognani wrote: > Hypervisors are referred to by their user-facing name rather > than the name of their libvirt driver, the monolithic daemon is > explicitly referred to as legacy, and a consistent format is > used throughout. > > Signed-off-by: Andrea Bolognani > --- > src/ch/meson.build| 2 +- > src/interface/meson.build | 2 +- > src/libxl/meson.build | 2 +- > src/locking/meson.build | 2 +- > src/locking/virtlockd-admin.socket.in | 2 +- > src/locking/virtlockd.service.in | 2 +- > src/locking/virtlockd.socket.in | 2 +- > src/logging/meson.build | 2 +- > src/logging/virtlogd-admin.socket.in | 2 +- > src/logging/virtlogd.service.in | 2 +- > src/logging/virtlogd.socket.in| 2 +- > src/lxc/meson.build | 2 +- > src/network/meson.build | 2 +- > src/node_device/meson.build | 2 +- > src/nwfilter/meson.build | 2 +- > src/qemu/meson.build | 2 +- > src/remote/libvirtd-admin.socket.in | 2 +- > src/remote/libvirtd-ro.socket.in | 2 +- > src/remote/libvirtd-tcp.socket.in | 2 +- > src/remote/libvirtd-tls.socket.in | 2 +- > src/remote/libvirtd.service.in| 2 +- > src/remote/libvirtd.socket.in | 2 +- > src/remote/meson.build| 4 ++-- > src/secret/meson.build| 2 +- > src/storage/meson.build | 2 +- > src/vbox/meson.build | 2 +- > src/virtd-admin.socket.in | 2 +- > src/virtd-ro.socket.in| 2 +- > src/virtd-tcp.socket.in | 2 +- > src/virtd-tls.socket.in | 2 +- > src/virtd.service.in | 2 +- > src/virtd.socket.in | 2 +- > src/vz/meson.build| 2 +- > 33 files changed, 34 insertions(+), 34 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 14/33] systemd: Switch virtvboxd to common templates
On Wed, Sep 27, 2023 at 06:19:15PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/vbox/meson.build| 5 + > src/vbox/virtvboxd.service.extra.in | 2 ++ > src/vbox/virtvboxd.service.in | 26 -- > 3 files changed, 3 insertions(+), 30 deletions(-) > create mode 100644 src/vbox/virtvboxd.service.extra.in > delete mode 100644 src/vbox/virtvboxd.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 08/33] systemd: Switch virtnodedevd to common templates
On Thu, Sep 28, 2023 at 05:38:45AM -0500, Andrea Bolognani wrote: > On Thu, Sep 28, 2023 at 11:16:53AM +0100, Daniel P. Berrangé wrote: > > On Wed, Sep 27, 2023 at 06:19:09PM +0200, Andrea Bolognani wrote: > > > Signed-off-by: Andrea Bolognani > > > --- > > > src/node_device/meson.build | 4 > > > src/node_device/virtnodedevd.service.in | 25 - > > > > > 2 files changed, 29 deletions(-) > > > delete mode 100644 src/node_device/virtnodedevd.service.in > > > > Reviewed-by: Daniel P. Berrangé > > > > Though I wonder if its worth just keeping an empty stub here, with > > the section headings. It'd be a little confusing to see the stubs > > present for some daemons but not others. > > We'd have to do the same for sockets then, on account of virtxend > using an override for them. > > Maybe we could change the merge script so that contents before the > start of the first section are simply ignored, and then have > > $ cat src/node_device/virtnodedevd.service.in > # Merged into src/virtd.service.in > $ cat src/node_device/virtnodedevd.socket.in > # Merged into src/virtd*.socket.in > > for services that don't need any overrides, and > > $ cat src/libxl/virtxend.service.extra.in > # Merged into src/virtd.service.in > > [Unit] > Wants=virtlockd.socket > After=virtlockd.socket > ... > $ cat src/libxl/virtxend.socket.extra.in > # Merged into src/virtd*.socket.in > > [Unit] > ConditionPathExists=/proc/xen/capabilities > > for services that do. It would mean introducing quite a number of > additional files, but maybe the advantages in terms of > discoverability make up for that downside? Yeah, I think that's a nice idea. > If we allow empty overrides, we might be even able to simplify the > way the various services are defined in their meson.build files, by > somehow deriving the path of the file instead of requiring it to be > provided explicitly. That part could be tricky though. > > Overall I'm not opposed to the idea, but let's consider it for a > follow-up instead of stalling this further, okay? Sure With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 16/33] systemd: Switch virtchd to common templates
On Wed, Sep 27, 2023 at 06:19:17PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/ch/meson.build | 5 +--- > src/ch/virtchd.service.extra.in | 22 + > src/ch/virtchd.service.in | 44 - > 3 files changed, 23 insertions(+), 48 deletions(-) > create mode 100644 src/ch/virtchd.service.extra.in > delete mode 100644 src/ch/virtchd.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 13/33] systemd: Switch virtstoraged to common templates
On Wed, Sep 27, 2023 at 06:19:14PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/storage/meson.build | 5 + > src/storage/virtstoraged.service.extra.in | 3 +++ > src/storage/virtstoraged.service.in | 27 --- > 3 files changed, 4 insertions(+), 31 deletions(-) > create mode 100644 src/storage/virtstoraged.service.extra.in > delete mode 100644 src/storage/virtstoraged.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 09/33] systemd: Switch virtinterfaced to common templates
On Wed, Sep 27, 2023 at 06:19:10PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/interface/meson.build | 4 > src/interface/virtinterfaced.service.in | 25 - > 2 files changed, 29 deletions(-) > delete mode 100644 src/interface/virtinterfaced.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 15/33] systemd: Switch virtvzd to common templates
On Wed, Sep 27, 2023 at 06:19:16PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/vz/meson.build | 5 + > src/vz/virtvzd.service.extra.in | 2 ++ > src/vz/virtvzd.service.in | 26 -- > 3 files changed, 3 insertions(+), 30 deletions(-) > create mode 100644 src/vz/virtvzd.service.extra.in > delete mode 100644 src/vz/virtvzd.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 19/33] systemd: Switch virtqemud to common templates
On Wed, Sep 27, 2023 at 06:19:20PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/qemu/meson.build| 5 +-- > src/qemu/virtqemud.service.extra.in | 28 + > src/qemu/virtqemud.service.in | 48 - > 3 files changed, 29 insertions(+), 52 deletions(-) > create mode 100644 src/qemu/virtqemud.service.extra.in > delete mode 100644 src/qemu/virtqemud.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 21/33] systemd: Drop libvirtd_socket*_in values
On Wed, Sep 27, 2023 at 06:19:22PM +0200, Andrea Bolognani wrote: > Now that the migration to common templates has been completed, > we no longer need these. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 4 > 1 file changed, 4 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 11/33] systemd: Switch virtsecretd to common templates
On Wed, Sep 27, 2023 at 06:19:12PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/secret/meson.build| 4 > src/secret/virtsecretd.service.in | 25 - > 2 files changed, 29 deletions(-) > delete mode 100644 src/secret/virtsecretd.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 08/33] systemd: Switch virtnodedevd to common templates
On Wed, Sep 27, 2023 at 06:19:09PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/node_device/meson.build | 4 > src/node_device/virtnodedevd.service.in | 25 - > 2 files changed, 29 deletions(-) > delete mode 100644 src/node_device/virtnodedevd.service.in Reviewed-by: Daniel P. Berrangé Though I wonder if its worth just keeping an empty stub here, with the section headings. It'd be a little confusing to see the stubs present for some daemons but not others. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 02/33] systemd: Introduce service_in/service_out variables
On Wed, Sep 27, 2023 at 06:19:03PM +0200, Andrea Bolognani wrote: > They're similar to the existing socket_in/socket_out variables > and will make future changes nicer. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 18/33] systemd: Switch virtlxcd to common templates
On Wed, Sep 27, 2023 at 06:19:19PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/lxc/meson.build | 5 +--- > src/lxc/virtlxcd.service.extra.in | 22 > src/lxc/virtlxcd.service.in | 44 --- > 3 files changed, 23 insertions(+), 48 deletions(-) > create mode 100644 src/lxc/virtlxcd.service.extra.in > delete mode 100644 src/lxc/virtlxcd.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 20/33] systemd: Switch virtproxyd to common templates
On Wed, Sep 27, 2023 at 06:19:21PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/remote/meson.build | 6 -- > src/remote/virtproxyd.service.in | 25 - > 2 files changed, 31 deletions(-) > delete mode 100644 src/remote/virtproxyd.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 12/33] systemd: Switch virtnetworkd to common templates
On Wed, Sep 27, 2023 at 06:19:13PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/network/meson.build | 5 + > src/network/virtnetworkd.service.extra.in | 2 ++ > src/network/virtnetworkd.service.in | 26 --- > 3 files changed, 3 insertions(+), 30 deletions(-) > create mode 100644 src/network/virtnetworkd.service.extra.in > delete mode 100644 src/network/virtnetworkd.service.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 17/33] systemd: Switch virtxend to common templates
On Wed, Sep 27, 2023 at 06:19:18PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/libxl/meson.build | 7 ++- > src/libxl/virtxend.service.extra.in | 12 +++ > src/libxl/virtxend.service.in | 32 - > src/libxl/virtxend.socket.extra.in | 2 ++ > 4 files changed, 16 insertions(+), 37 deletions(-) > create mode 100644 src/libxl/virtxend.service.extra.in > delete mode 100644 src/libxl/virtxend.service.in > create mode 100644 src/libxl/virtxend.socket.extra.in Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 04/33] systemd: Introduce temporary libvirtd_socket*_in values
On Wed, Sep 27, 2023 at 06:19:05PM +0200, Andrea Bolognani wrote: > These will be useful during the upcoming migration to common > templates for systemd units and will be dropped as soon as all > services have been converted. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 4 > 1 file changed, 4 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 03/33] systemd: Make @service_in@ optional
On Wed, Sep 27, 2023 at 06:19:04PM +0200, Andrea Bolognani wrote: > It is currently considered required, but we're soon going to > provide a default that will be suitable for most services. > > Since all services currently provide a value explicitly, we > can implement a default without breaking anything. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 05/33] systemd: Provide all input files explicitly
On Wed, Sep 27, 2023 at 06:19:06PM +0200, Andrea Bolognani wrote: > We're about to change the defaults and start migrating to common > templates: in order to be able to switch units over one at a > time, make the input files that are currently used explicit > rather than implicit. > > Signed-off-by: Andrea Bolognani > --- > src/ch/meson.build | 3 +++ > src/interface/meson.build | 3 +++ > src/libxl/meson.build | 3 +++ > src/lxc/meson.build | 3 +++ > src/network/meson.build | 3 +++ > src/node_device/meson.build | 3 +++ > src/nwfilter/meson.build| 3 +++ > src/qemu/meson.build| 3 +++ > src/remote/meson.build | 10 ++ > src/secret/meson.build | 3 +++ > src/storage/meson.build | 3 +++ > src/vbox/meson.build| 3 +++ > src/vz/meson.build | 3 +++ > 13 files changed, 46 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 06/33] systemd: Introduce common templates
On Wed, Sep 27, 2023 at 06:19:07PM +0200, Andrea Bolognani wrote:
> We already use templating to generate sockets, which are all
> based off libvirtd's. Push the idea further, and extend it to
> cover services as well.
>
> This is more challenging, as the various modular daemons each have
> their own needs in terms of what system services needs to be
> available before they can be started, which other components of
> libvirt they depend on, and so on.
>
> In order to make this sort of per-service tweaks possible, we
> introduce a Python script that can merge two systemd units
> together. The script is aware of the semantics of systemd's unit
> definition format, so it can intelligently merge sections
> together.
>
> This generic systemd unit merging mechanism will also supersede
> the extremely ad-hoc @deps@ variable, which is currently used in
> a single scenario.
>
> Signed-off-by: Andrea Bolognani
> ---
> scripts/merge-systemd-units.py | 91 ++
> scripts/meson.build| 1 +
> src/meson.build| 22
> src/virtd-admin.socket.in | 13 +
> src/virtd-ro.socket.in | 13 +
> src/virtd-tcp.socket.in| 12 +
> src/virtd-tls.socket.in| 12 +
> src/virtd.service.in | 25 ++
> src/virtd.socket.in| 12 +
> 9 files changed, 201 insertions(+)
> create mode 100755 scripts/merge-systemd-units.py
> create mode 100644 src/virtd-admin.socket.in
> create mode 100644 src/virtd-ro.socket.in
> create mode 100644 src/virtd-tcp.socket.in
> create mode 100644 src/virtd-tls.socket.in
> create mode 100644 src/virtd.service.in
> create mode 100644 src/virtd.socket.in
>
> diff --git a/scripts/merge-systemd-units.py b/scripts/merge-systemd-units.py
> new file mode 100755
> index 00..136bc8d416
> --- /dev/null
> +++ b/scripts/merge-systemd-units.py
> @@ -0,0 +1,91 @@
> +#!/usr/bin/env python3
Stick a license header of SPDX tag on this.
Also if you didn't already do it, run the file through 'black'
and let it do whatever it wants todo to formatting.
...reminds me we really ought to get around to running 'black'
on the rest of our existing python.
> +
> +import sys
> +
> +SECTIONS = [
> +'[Unit]',
> +'[Service]',
> +'[Socket]',
> +'[Install]',
> +]
> +
> +
> +def parse_unit(unit_path):
> +unit = {}
> +current_section = '[Invalid]'
> +
> +with open(unit_path) as f:
> +for line in f:
> +line = line.strip()
> +
> +if line == '':
> +continue
> +
> +if line[0] == '[' and line[-1] == ']':
> +if line not in SECTIONS:
> +print('Unknown section {}'.format(line))
> +sys.exit(1)
> +
> +current_section = line
> +continue
> +
> +if current_section not in unit:
> +unit[current_section] = []
> +
> +unit[current_section].append(line)
> +
> +if '[Invalid]' in unit:
> +print('Contents found outside of any section')
> +sys.exit(1)
> +
> +return unit
> +
> +
> +def format_unit(unit):
> +lines = []
> +
> +for section in SECTIONS:
> +if section not in unit:
> +continue
> +
> +lines.append(section)
> +
> +for line in unit[section]:
> +lines.append(line)
> +
> +lines.append('')
> +
> +return '\n'.join(lines)
> +
> +
> +def merge_units(base, extra):
> +merged = {}
> +
> +for section in SECTIONS:
> +if section in extra and section not in base:
> +print('Section {} in extra but not in base'.format(section))
> +sys.exit(1)
> +
> +if section not in base:
> +continue
> +
> +merged[section] = base[section]
> +
> +if section not in extra:
> +continue
> +
> +merged[section].extend(extra[section])
> +
> +return merged
> +
> +
> +if len(sys.argv) < 2:
> +print('usage: {} BASE EXTRA'.format(sys.argv[0]))
> +sys.exit(1)
> +
> +base = parse_unit(sys.argv[1])
> +extra = parse_unit(sys.argv[2])
> +
> +merged = merge_units(base, extra)
> +
> +sys.stdout.write(format_unit(merged))
> diff --git a/scripts/meson.build b/scripts/meson.build
> index 05b71184f1..65fd1e21c5 100644
> --- a/scripts/meson.build
> +++ b/scripts/meson.build
> @@ -19,6 +19,7 @@ scripts = [
>'header-ifdef.py',
>'hvsupport.py',
>'hyperv_wmi_generator.py',
> + 'merge-systemd-units.py',
>'meson-dist.py',
>'meson-gen-authors.py',
>'meson-gen-def.py',
> diff --git a/src/meson.build b/src/meson.build
> index 2fbf98b9fe..02c92621ba 100644
> --- a/src/meson.build
> +++ b/src/meson.build
> @@ -203,6 +203,8 @@ libvirtd_socket_admin_in = files('remote' /
> 'libvirtd-admin.socket.in')
> # * sockets - array of additional sockets (optional, default [ 'main',
> 'ro', 'admin' ])
> # * service_in - service source file
Re: [libvirt PATCH v2 22/33] systemd: Drop @deps@
On Wed, Sep 27, 2023 at 06:19:23PM +0200, Andrea Bolognani wrote: > It's no longer used anywhere. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 2 -- > src/remote/libvirtd-admin.socket.in | 1 - > src/remote/libvirtd-ro.socket.in| 1 - > src/remote/libvirtd-tcp.socket.in | 1 - > src/remote/libvirtd-tls.socket.in | 1 - > src/remote/libvirtd.socket.in | 1 - > 6 files changed, 7 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 25/33] systemd: Replace Requires with BindTo+After for main socket
On Wed, Sep 27, 2023 at 06:19:26PM +0200, Andrea Bolognani wrote: > This is the strongest relationship that can be declared between > two units, and causes the service to be terminated immediately > if its main socket disappears. This is the behavior we want. > > Note that we don't do the same for the read-only/admin sockets, > because those are not as critical for the core functionality of > services as the main socket it. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd.service.in | 3 ++- > src/logging/virtlogd.service.in | 3 ++- > src/virtd.service.in | 3 ++- > 3 files changed, 6 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 23/33] systemd: Drop parametrization from libvirtd sockets
On Wed, Sep 27, 2023 at 06:19:24PM +0200, Andrea Bolognani wrote: > Up until now the files have been used as template for most > services, but now that those have been converted to common > templates we can drop parametrization and make it clear that > these files are for libvirtd only. > > Signed-off-by: Andrea Bolognani > --- > src/remote/libvirtd-admin.socket.in | 10 +- > src/remote/libvirtd-ro.socket.in| 10 +- > src/remote/libvirtd-tcp.socket.in | 8 > src/remote/libvirtd-tls.socket.in | 8 > src/remote/libvirtd.socket.in | 6 +++--- > 5 files changed, 21 insertions(+), 21 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 01/33] systemd: Drop Conflicts from virtproxyd sockets
On Wed, Sep 27, 2023 at 06:19:02PM +0200, Andrea Bolognani wrote: > The idea behind these is to prevent running both modular daemons > and monolithic daemon at the same time. We will implement a more > effective solution for that shortly. > > Signed-off-by: Andrea Bolognani > --- > src/remote/meson.build | 3 --- > 1 file changed, 3 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH] docs: Go bindings release at the same time as the C library
On Wed, Sep 27, 2023 at 07:46:22PM +0200, Andrea Bolognani wrote: > The actual versioning policy[1] is a bit more nuanced, and in > particular there are scenarios in which the monthly release > is intentionally skipped, but overall it's not inaccurate to > claim that the release cadence of the Go bindings follows the > one of the C library. > > [1] https://gitlab.com/libvirt/libvirt-go-module/-/blob/master/VERSIONING.rst > > Signed-off-by: Andrea Bolognani > --- The caveats around skipping releases for Go also apply to the Perl and Python too. Reviewed-by: Daniel P. Berrangé > docs/downloads.rst | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/docs/downloads.rst b/docs/downloads.rst > index c7d4237f66..d3deec554d 100644 > --- a/docs/downloads.rst > +++ b/docs/downloads.rst > @@ -261,7 +261,7 @@ The core libvirt module follows a time based plan, with > releases made once a > month on the 1st of each month give or take a few days. The only exception > is at > the start of the year where there are two 6 weeks gaps (first release in the > middle of Jan, then skip the Feb release), giving a total of 11 releases a > year. > -The Python and Perl modules will aim to release at the same time as the core > +The Python, Perl and Go modules will aim to release at the same time as the > core > libvirt module. Other modules have independent ad-hoc releases with no fixed > time schedule. > > -- > 2.41.0 > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 41/42] systemd: Improve and unify unit descriptions
On Mon, Sep 25, 2023 at 08:58:39PM +0200, Andrea Bolognani wrote: > Hypervisors are referred to by their user-facing name rather > than the name of their libvirt driver, the monolithic daemon is > explicitly referred to as legacy, and a consistent format is > used throughout. > > Signed-off-by: Andrea Bolognani > --- > src/ch/meson.build| 2 +- > src/interface/meson.build | 2 +- > src/libxl/meson.build | 2 +- > src/locking/meson.build | 2 +- > src/locking/virtlockd-admin.socket.in | 2 +- > src/locking/virtlockd.service.in | 2 +- > src/locking/virtlockd.socket.in | 2 +- > src/logging/meson.build | 2 +- > src/logging/virtlogd-admin.socket.in | 2 +- > src/logging/virtlogd.service.in | 2 +- > src/logging/virtlogd.socket.in| 2 +- > src/lxc/meson.build | 2 +- > src/network/meson.build | 2 +- > src/node_device/meson.build | 2 +- > src/nwfilter/meson.build | 2 +- > src/qemu/meson.build | 2 +- > src/remote/libvirtd-admin.socket.in | 2 +- > src/remote/libvirtd-ro.socket.in | 2 +- > src/remote/libvirtd-tcp.socket.in | 2 +- > src/remote/libvirtd-tls.socket.in | 2 +- > src/remote/libvirtd.service.in| 2 +- > src/remote/libvirtd.socket.in | 2 +- > src/remote/meson.build| 4 ++-- > src/secret/meson.build| 2 +- > src/storage/meson.build | 2 +- > src/vbox/meson.build | 2 +- > src/virtd-admin.socket.in | 2 +- > src/virtd-ro.socket.in| 2 +- > src/virtd-tcp.socket.in | 2 +- > src/virtd-tls.socket.in | 2 +- > src/virtd.service.in | 2 +- > src/virtd.socket.in | 2 +- > src/vz/meson.build| 2 +- > 33 files changed, 34 insertions(+), 34 deletions(-) > > diff --git a/src/locking/virtlockd-admin.socket.in > b/src/locking/virtlockd-admin.socket.in > index a773b511bd..90077b4915 100644 > --- a/src/locking/virtlockd-admin.socket.in > +++ b/src/locking/virtlockd-admin.socket.in > @@ -1,5 +1,5 @@ > [Unit] > -Description=Virtual machine lock manager admin socket > +Description=libvirt @name@ daemon admin socket Using a subsitution here does not add any value IMHO, it just obscures the final text. Likewise for the similar changes that follow. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 42/42] systemd: Move Documentation lines
On Mon, Sep 25, 2023 at 08:58:40PM +0200, Andrea Bolognani wrote: > Like the Description, these are intended to be displayed to the > user, so it makes sense to have them towards the top of the file > before all the information that systemd will parse to calculate > dependencies. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd.service.in | 4 ++-- > src/logging/virtlogd.service.in | 4 ++-- > src/remote/libvirtd.service.in | 4 ++-- > src/virtd.service.in | 4 ++-- > 4 files changed, 8 insertions(+), 8 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 40/42] systemd: Drop BindTo/After between sockets
On Mon, Sep 25, 2023 at 08:58:38PM +0200, Andrea Bolognani wrote: > They are unnecessary, since all sockets for a service are now > enabled as soon as one of them is and each service has a very > strong dependency on all of its sockets. You earlier modified the .service units to have BindsTo= for each of the sockets it depends to. Thus if any one of the .sockets is stopped, this means the .service is stopped too. The logic removed here though was doing a different job. That said that that if $FOO.socket is stopped, it would force stop the $FOO-admin.socket and $FOO-ro.socket too. IOW, it prevented having only the RO/admin sockets running, without the primary socket. I believe that's still needed Also, you didn't add BindsTo on the libvirtd.service, because that has to be able to run without socket activation for upgrade scenarios. So we shouldn't be modifying the libvirtd sockets anyway. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd-admin.socket.in | 2 -- > src/logging/virtlogd-admin.socket.in | 2 -- > src/remote/libvirtd-admin.socket.in | 2 -- > src/remote/libvirtd-ro.socket.in | 2 -- > src/remote/libvirtd-tcp.socket.in | 2 -- > src/remote/libvirtd-tls.socket.in | 2 -- > src/virtd-admin.socket.in | 2 -- > src/virtd-ro.socket.in| 2 -- > src/virtd-tcp.socket.in | 2 -- > src/virtd-tls.socket.in | 2 -- > 10 files changed, 20 deletions(-) > > diff --git a/src/locking/virtlockd-admin.socket.in > b/src/locking/virtlockd-admin.socket.in > index 63f78a02da..a773b511bd 100644 > --- a/src/locking/virtlockd-admin.socket.in > +++ b/src/locking/virtlockd-admin.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=Virtual machine lock manager admin socket > -BindsTo=virtlockd.socket > -After=virtlockd.socket > > [Socket] > ListenStream=@runstatedir@/libvirt/virtlockd-admin-sock > diff --git a/src/logging/virtlogd-admin.socket.in > b/src/logging/virtlogd-admin.socket.in > index 1d18fe6f56..e0d35cbcf3 100644 > --- a/src/logging/virtlogd-admin.socket.in > +++ b/src/logging/virtlogd-admin.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=Virtual machine log manager socket > -BindsTo=virtlogd.socket > -After=virtlogd.socket > > [Socket] > ListenStream=@runstatedir@/libvirt/virtlogd-admin-sock > diff --git a/src/remote/libvirtd-admin.socket.in > b/src/remote/libvirtd-admin.socket.in > index 6df038d95a..ba060eaea4 100644 > --- a/src/remote/libvirtd-admin.socket.in > +++ b/src/remote/libvirtd-admin.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=@name@ admin socket > -BindsTo=libvirtd.socket > -After=libvirtd.socket > > [Socket] > ListenStream=@runstatedir@/libvirt/libvirt-admin-sock > diff --git a/src/remote/libvirtd-ro.socket.in > b/src/remote/libvirtd-ro.socket.in > index 6797517c50..d2ab7ba4f2 100644 > --- a/src/remote/libvirtd-ro.socket.in > +++ b/src/remote/libvirtd-ro.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=@name@ local read-only socket > -BindsTo=libvirtd.socket > -After=libvirtd.socket > > [Socket] > ListenStream=@runstatedir@/libvirt/libvirt-sock-ro > diff --git a/src/remote/libvirtd-tcp.socket.in > b/src/remote/libvirtd-tcp.socket.in > index 8b8fbcd01a..e32daddf25 100644 > --- a/src/remote/libvirtd-tcp.socket.in > +++ b/src/remote/libvirtd-tcp.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=@name@ non-TLS IP socket > -BindsTo=libvirtd.socket > -After=libvirtd.socket > > [Socket] > ListenStream=16509 > diff --git a/src/remote/libvirtd-tls.socket.in > b/src/remote/libvirtd-tls.socket.in > index fefda22c6b..2f34e8e0cd 100644 > --- a/src/remote/libvirtd-tls.socket.in > +++ b/src/remote/libvirtd-tls.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=@name@ TLS IP socket > -BindsTo=libvirtd.socket > -After=libvirtd.socket > > [Socket] > ListenStream=16514 > diff --git a/src/virtd-admin.socket.in b/src/virtd-admin.socket.in > index a4faeb7da8..dc2cb737ce 100644 > --- a/src/virtd-admin.socket.in > +++ b/src/virtd-admin.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=@name@ admin socket > -BindsTo=@service@.socket > -After=@service@.socket > Conflicts=libvirtd-admin.socket > After=libvirtd-admin.socket > @socket_unit_extra@ > diff --git a/src/virtd-ro.socket.in b/src/virtd-ro.socket.in > index 829c2e8b1f..ef1716e3f3 100644 > --- a/src/virtd-ro.socket.in > +++ b/src/virtd-ro.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=@name@ local read-only socket > -BindsTo=@service@.socket > -After=@service@.socket > Conflicts=libvirtd-ro.socket > After=libvirtd-ro.socket > @socket_unit_extra@ > diff --git a/src/virtd-tcp.socket.in b/src/virtd-tcp.socket.in > index 2873c35135..26ead32789 100644 > --- a/src/virtd-tcp.socket.in > +++ b/src/virtd-tcp.socket.in > @@ -1,7 +1,5 @@ > [Unit] > Description=@name@ non-TLS IP socket > -BindsTo=@service@.socket > -After=@service@.socket > Conflicts=libvirtd-tcp.socket > After=libvirtd-tcp.socket > @socket_unit
Re: [libvirt PATCH 39/42] systemd: Add Also between sockets
On Mon, Sep 25, 2023 at 08:58:37PM +0200, Andrea Bolognani wrote: > This results in all sockets for a service being enabled when a > single one of them is. > > The -tcp and -tls sockets are intentionally excluded, because > enabling them should require explicit action on the > administrator's part; moreover, disabling them should not result > in the local sockets being disabled too. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd-admin.socket.in | 1 + > src/locking/virtlockd.socket.in | 1 + > src/logging/virtlogd-admin.socket.in | 1 + > src/logging/virtlogd.socket.in| 1 + > src/remote/libvirtd-admin.socket.in | 2 ++ > src/remote/libvirtd-ro.socket.in | 2 ++ > src/remote/libvirtd.socket.in | 2 ++ > src/virtd-admin.socket.in | 2 ++ > src/virtd-ro.socket.in| 2 ++ > src/virtd.socket.in | 2 ++ > 10 files changed, 16 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 34/42] systemd: Make modular daemons conflict with libvirtd
On Mon, Sep 25, 2023 at 08:58:32PM +0200, Andrea Bolognani wrote: > We want to make sure that, at any given time, we have either the > modular daemons or the monolithic one running, never both. In > order to achieve that, make every single modular unit conflict > with the corresponding libvirtd unit. > > We set both Conflicts=libvirtd.unit and After=libvirtd.unit: this > tells systemd that, whenever virtfood.unit and libvirtd.unit are > part of the same transaction, the former should win out. > > Thanks to this, if both the modular daemons and the monolithic > one have been enabled because of outdated automation or a simple > mistake of the administrator, the request to start libvirtd at > boot will be ignored and the result will be a regular modular > deployment. > > If the request to start libvirtd is made when the modular daemons > are already running, we have no way to prevent systemd from > complying with that request; however, thanks to the way the > conflict relationship has been declared, they will be shut down > cleanly before libvirtd is started. From the user's point of > view, the transition from modular to monolithic will be > completely transparent: it's basically the same scenario as a > regular package upgrade, just with an extra twist. > > Note that, while switching from modular to monolithic at runtime > happens automatically, going back requires manual intervention, > i.e. starting all the necessary sockets one by one. That's okay: > the goal here is to prevent misconfiguration and force of habit > to accidentally disrupt a working setup, not to encourage the > scenario. In a correctly configured and managed host, it should > never occur. > > Signed-off-by: Andrea Bolognani > --- > src/virtd-admin.socket.in | 2 ++ > src/virtd-ro.socket.in| 2 ++ > src/virtd-tcp.socket.in | 2 ++ > src/virtd-tls.socket.in | 2 ++ > src/virtd.service.in | 3 ++- > src/virtd.socket.in | 2 ++ > 6 files changed, 12 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 37/42] systemd: Drop Before=libvirtd from virtlogd/virtlockd
On Mon, Sep 25, 2023 at 08:58:35PM +0200, Andrea Bolognani wrote: > We have already declared the mirror relationship, so this one > is now redundant. > > Moreover, this version was incomplete: it only ever worked for > the monolithic daemon, but the modular daemons for QEMU and Xen > also want the sockets to be active. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd-admin.socket.in | 1 - > src/locking/virtlockd.service.in | 1 - > src/locking/virtlockd.socket.in | 1 - > src/logging/virtlogd-admin.socket.in | 1 - > src/logging/virtlogd.service.in | 1 - > src/logging/virtlogd.socket.in| 1 - > 6 files changed, 6 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 38/42] systemd: Drop Before=foo.service from sockets
On Mon, Sep 25, 2023 at 08:58:36PM +0200, Andrea Bolognani wrote: > systemd will automatically infer this dependency based on the > socket's Service=foo.service setting. > > Signed-off-by: Andrea Bolognani > --- > src/remote/libvirtd-admin.socket.in | 1 - > src/remote/libvirtd-ro.socket.in| 1 - > src/remote/libvirtd-tcp.socket.in | 1 - > src/remote/libvirtd-tls.socket.in | 1 - > src/remote/libvirtd.socket.in | 1 - > src/virtd-admin.socket.in | 1 - > src/virtd-ro.socket.in | 1 - > src/virtd-tcp.socket.in | 1 - > src/virtd-tls.socket.in | 1 - > src/virtd.socket.in | 1 - > 10 files changed, 10 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 36/42] systemd: Augment Requires/Wants with After
On Mon, Sep 25, 2023 at 08:58:34PM +0200, Andrea Bolognani wrote: > Requires/Wants only tells systemd that the corresponding unit > should be started when the current one is, but that could very > well happen in parallel. For virtlogd/virtlockd, we want the > socket to be already active when the hypervisor driver is > started. > > Signed-off-by: Andrea Bolognani > --- > src/libxl/meson.build | 1 + > src/qemu/meson.build | 2 ++ > src/remote/libvirtd.service.in | 7 ++- > 3 files changed, 9 insertions(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
[libvirt PATCH] logging: lockdown the systemd service configuration
The 'systemd-analyze security' command looks at the unit file configuration and reports on any settings which increase the attack surface for the daemon. Since most systemd units are fairly minimalist, this is generally informing us about settings that we never put any thought into using before. In its current configuration it reports # systemd-analyze security virtlogd.service ...snip... → Overall exposure level for virtlogd.service: 9.6 UNSAFE 😨 which is pretty terrible as a score. If we apply all of the recommendations that appear possible without (knowingly) breaking functionality it reports: # systemd-analyze security virtlogd.service ...snip... → Overall exposure level for virtlogd.service: 2.2 OK 🙂 which is a pretty decent improvement. Some of the settings we would like to enable require a systemd version that is newer than that available in our oldest distro target - RHEL-8 at v239. NB, RestrictSUIDSGID is technically newer than 239, but RHEL-8 backported it, and other distros we target have it by default. Remaining recommendations are ✗ CapabilityBoundingSet=~CAP_(DAC_*|FOWNER|IPC_OWNER) We block FOWNER/IPC_OWNER, but can't block the two DAC capabilities. Historically apps/users might point QEMU to log files in $HOME, pre-created with their own user ID. ✗ IPAddressDeny= Not required since RestrictAddressFamilies blocks IP usage. Ignoring this avoids the overhead of creating a traffic filter than will never be used. ✗ NoNewPrivileges= Highly desirable, but cannot enable it yet, because it will block the ability to transition to the virtlogd_t SELinux domain during execve. The SELinux policy needs fixing to permit this transition under NNP first. ✗ PrivateTmp= There is a decent chance people have VMs configured with a serial port logfile pointing at /tmp. We would cause a regression to use private /tmp for logging ✗ PrivateUsers= This would put virtlogd inside a user namespace where its root is in fact unprivileged. Same problem as the User= setting below ✗ ProcSubset= Libraries we link to might read certain non-PID related files from /proc ✗ ProtectClock= Requires v245 ✗ ProtectHome= Same problem as PrivateTmp=. There's a decent chance that someone has a VM configured to write a logfile to /home ✗ ProtectHostname= Requires v241 ✗ ProtectKernelLogs Requires v244 ✗ ProtectProc Requires v247 ✗ ProtectSystem= We only set it to 'full', as 'strict' is not viable for our required usage ✗ RootDirectory=/RootImage= We are not capable of running inside a custom chroot given needs to write log files to arbitrary places ✗ RestrictAddressFamilies=~AF_UNIX We need AF_UNIX to communicate with other libvirt daemons ✗ SystemCallFilter=~@resources We link to libvirt.so which links to libnuma.so which has a constructor that calls set_mempolicy. This is highly undesirable todo during a constructor. ✗ User=/DynamicUser= This is highly desirable, but we currently read/write logs as root, and directories we're told to write into could be anywhere. So using a non-root user would have a major risk of regressions for applications and also have upgrade implications Signed-off-by: Daniel P. Berrangé --- src/logging/virtlogd.service.in | 94 + 1 file changed, 94 insertions(+) diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in index 8e245ddb43..9e3838ff34 100644 --- a/src/logging/virtlogd.service.in +++ b/src/logging/virtlogd.service.in @@ -20,5 +20,99 @@ OOMScoreAdjust=-900 # per systemd recommendations LimitNOFILE=1024:524288 +CapabilityBoundingSet=~CAP_AUDIT_CONTROL +CapabilityBoundingSet=~CAP_AUDIT_READ +CapabilityBoundingSet=~CAP_AUDIT_WRITE +CapabilityBoundingSet=~CAP_BLOCK_SUSPEND +CapabilityBoundingSet=~CAP_CHOWN +# Mgmt app/user might have pre-created log files that we're +# told to open and write to, or be storing them in otherwise +# inaccessible locations like $HOME. So we need to ignore +# DAC permission checks. +#CapabilityBoundingSet=~CAP_DAC_OVERRIDE +#CapabilityBoundingSet=~CAP_DAC_READ_SEARCH +CapabilityBoundingSet=~CAP_FOWNER +CapabilityBoundingSet=~CAP_FSETID +CapabilityBoundingSet=~CAP_IPC_LOCK +CapabilityBoundingSet=~CAP_IPC_OWNER +CapabilityBoundingSet=~CAP_KILL +CapabilityBoundingSet=~CAP_LEASE +CapabilityBoundingSet=~CAP_LINUX_IMMUTABLE +CapabilityBoundingSet=~CAP_MAC_ADMIN +CapabilityBoundingSet=~CAP_MAC_OVERRIDE +CapabilityBoundingSet=~CAP_MKNOD +CapabilityBoundingSet=~CAP_NET_ADMIN +CapabilityBoundingSet=~CAP_NET_BIND_SERVICE +CapabilityBoundingSet=~CAP_NET_BROADCAST +CapabilityBoundingSet=~CAP_NET_RAW +CapabilityBoundingSet=~CAP_SETFCAP +CapabilityBoundingSet=~CAP_SETPCAP +CapabilityBoundingSet=~CAP_SETGID +CapabilityBoundingSet=~CAP_SETUID +CapabilityBoundingSet=~CAP_SYSLOG +CapabilityBoundingSet=~CAP_SYS_ADMIN +CapabilityBoundingSet=~C
Re: [libvirt PATCH 26/42] systemd: Switch virtchd to common templates
On Tue, Sep 26, 2023 at 08:12:43AM -0500, Andrea Bolognani wrote:
> On Tue, Sep 26, 2023 at 01:14:33PM +0100, Daniel P. Berrangé wrote:
> > On Tue, Sep 26, 2023 at 07:02:19AM -0500, Andrea Bolognani wrote:
> > > I don't think it helps much with not storing additional data inside
> > > the build system, unless we want to store the contents of the various
> > > common snippets in separate files? Something like
> > >
> > > common_service = fs.read('common_service.inc')
> > > unit_conf = configuration_data({
> > > 'common_service' = common_service,
> > > })
> > >
> > > We'd have to fake fs.read() because it was introduced in 0.57 though.
> > > And we'd have to run the contents of the common parts through
> > > variable substitution anyway, because they will contain a bunch of
> > > lines like
> > >
> > > Also=@service@.socket
> > > Also=@service@-ro.socket
> > > Also=@service@-admin.socket
> > >
> > > I'm not sure the result would look much better, but I can give it a
> > > try.
> >
> > Don't try to do any of this in meson. We should just have a standalone
> > python script that can combine the daemon specific unit file contents
> > with the common unit file contents. eg
> >
> > scripts/merge-unit-file.py \
> > src/qemu/virtqemud.service.in \
> > src/rpc/virtd.service.in \
> > build/src/virtqemud.service
>
> It feels a bit silly to shell out to Python to perform what is
> ultimately a bunch of variable substitutions, as if that wasn't part
> of Meson's core feature set... But I'll give it a try and see how it
> turns out.
IMHO Meson's job is to control the build process, rather than to
actually be the build process. I think of this as "compiling" the
unit files and the python sript is our compiler, which meson is
to control.
> Can you please take a look at the remaining patches in the meantime,
> and provide feedback on the changes that are made to the various
> services and sockets as part of them? Thanks in advance :)
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 35/42] systemd: Replace Requires with BindTo+After for sockets
On Tue, Sep 26, 2023 at 04:09:17AM -0500, Andrea Bolognani wrote:
> On Tue, Sep 26, 2023 at 09:44:52AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 25, 2023 at 08:58:33PM +0200, Andrea Bolognani wrote:
> > > This is the strongest relationship that can be declared between
> > > two units, and causes the service to be terminated immediately
> > > if any of its sockets disappear. This is the behavior we want.
> >
> > IIUC, this prevents running the service with /only/ the main
> > socket, and ro/admin sockets disabled. Running without the
> > ro socket in particular was something we wanted to allow to
> > reduce exposure to unprivileged services (there have been
> > a number of CVEs where the read-only socket was the way in)
>
> This doesn't work today either AFAICT, since the ro/admin sockets are
> marked as Required by the various services.
Doh, yes, I've confirmed. I'm sure it used to work, but we must have
broken it at some point as we tweaked the deps countless times over
to finese the setup.
> If we want to support this configuration, then we need
>
> # foo.service
> [Unit]
> BindsTo=foo.socket
> Wants=foo-ro.socket
> Wants=foo-admin.socket
> After=foo.socket
>
> In the default scenario, things will work just the same as they do
> here, but it will also be possible to mask foo{-ro,-admin}.socket to
> obtain the hardened setup you describe.
Or we just decide to keep life simple, and if people want to harden
things they can change permissions on the socket via a system unit
override locally.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 26/42] systemd: Switch virtchd to common templates
On Tue, Sep 26, 2023 at 07:02:19AM -0500, Andrea Bolognani wrote: > On Tue, Sep 26, 2023 at 11:23:51AM +0100, Daniel P. Berrangé wrote: > > On Tue, Sep 26, 2023 at 11:09:44AM +0200, Pavel Hrdina wrote: > > > On Mon, Sep 25, 2023 at 08:58:24PM +0200, Andrea Bolognani wrote: > > > > +'service_unit_extra': [ > > > > + 'Wants=systemd-machined.service', > > > > + 'After=systemd-machined.service', > > > > + 'After=remote-fs.target', > > > > +], > > > > +'service_service_extra': [ > > > > + 'KillMode=process', > > > > + '# Raise hard limits to match behaviour of systemd >= 240.', > > > > + '# During startup, daemon will set soft limit to match hard > > > > limit', > > > > + '# per systemd recommendations', > > > > + 'LimitNOFILE=1024:524288', > > > > + '# The cgroups pids controller can limit the number of tasks > > > > started by', > > > > + '# the daemon, which can limit the number of domains for some > > > > hypervisors.', > > > > + '# A conservative default of 8 tasks per guest results in a > > > > TasksMax of', > > > > + '# 32k to support 4096 guests.', > > > > + 'TasksMax=32768', > > > > + '# With cgroups v2 there is no devices controller anymore, we > > > > have to use', > > > > + '# eBPF to control access to devices. In order to do that we > > > > create a eBPF', > > > > + '# hash MAP which locks memory. The default map size for 64 > > > > devices together', > > > > + '# with program takes 12k per guest. After rounding up we will > > > > get 64M to', > > > > + '# support 4096 guests.', > > > > + 'LimitMEMLOCK=64M', > > > > +], > > > > > > This feels wrong to have it in meson.build file. In addition it is the > > > same as for virtlxcd and virtqemud so we are basically duplicating the > > > data and which makes it easy to make inconsistent changes not affecting > > > all places. > > > > > > IMHO it would be better to have additional file that will be included > > > into the template for services where we need it. > > > > > > I'm not sure about the `service_unit_extra` as well if we want to have > > > it in meson.build files as it is not strictly related to the build > > > process and there is more data compared to the old `deps`. > > > > If anything I'd reverse the model. The 'virtchd.service.in' file > > should be the primary template, the common bits the injected data. > > > > ie > > > > cat virtchd.service.in > > [Unit] > > Description=Virtualization Cloud-Hypervisor daemon > > ::common-unit:: > > Wants=systemd-machined.service > > After=remote-fs.target > > After=systemd-machined.service > > Documentation=man:virtchd(8) > > > > > > [Service] > > ::common-service:: > > KillMode=process > > # Raise hard limits to match behaviour of systemd >= 240. > > # During startup, daemon will set soft limit to match hard limit > > # per systemd recommendations > > LimitNOFILE=1024:524288 > > # The cgroups pids controller can limit the number of tasks started by > > # the daemon, which can limit the number of domains for some hypervisors. > > # A conservative default of 8 tasks per guest results in a TasksMax of > > # 32k to support 4096 guests. > > TasksMax=32768 > > # With cgroups v2 there is no devices controller anymore, we have to use > > # eBPF to control access to devices. In order to do that we create a eBPF > > # hash MAP which locks memory. The default map size for 64 devices > > together > > # with program takes 12k per guest. After rounding up we will get 64M to > > # support 4096 guests. > > LimitMEMLOCK=64M > > > > [Install] > > ::common-install:: > > This doesn't address the problem with duplication that Pavel pointed > out. > > I don't think it helps much with not storing additional data inside > the build system, unless we want to store the contents of the various > common snippets in separate files? Something like > > common_service
Re: [RFC PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
On Tue, Sep 26, 2023 at 02:11:37PM +0200, Marc Hartmayer wrote:
> On Mon, Sep 25, 2023 at 04:15 PM +0100, Daniel P. Berrangé
> wrote:
> > On Mon, Sep 25, 2023 at 03:39:09PM +0200, Marc Hartmayer wrote:
> >> When starting a guest via libvirt (`virsh start --console`), early
> >> console output was missed because the guest was started first and then
> >> the console was attached. This patch changes this to the following
> >> sequence:
> >>
> >> 1. create a paused guest
> >> 2. attach the console
> >> 3. resume the guest
> >>
> >> Reviewed-by: Boris Fiuczynski
> >> Signed-off-by: Marc Hartmayer
> >> ---
> >> tools/virsh-domain.c | 11 +--
> >> 1 file changed, 9 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> >> index 5c3c6d18aebf..3581161c6f53 100644
> >> --- a/tools/virsh-domain.c
> >> +++ b/tools/virsh-domain.c
> >> @@ -4065,6 +4065,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> >> g_autoptr(virshDomain) dom = NULL;
> >> #ifndef WIN32
> >> bool console = vshCommandOptBool(cmd, "console");
> >> +bool resume_domain = false;
> >> #endif
> >> unsigned int flags = VIR_DOMAIN_NONE;
> >> int rc;
> >> @@ -4083,8 +4084,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> >> if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0)
> >> return false;
> >>
> >> -if (vshCommandOptBool(cmd, "paused"))
> >> +if (vshCommandOptBool(cmd, "paused")) {
> >> flags |= VIR_DOMAIN_START_PAUSED;
> >> +#ifndef WIN32
> >> +} else if (console) {
> >> +flags |= VIR_DOMAIN_START_PAUSED;
> >> +resume_domain = true;
> >> +#endif
> >
> > Hypervisor drivers are not required to support VIR_DOMAIN_START_PAUSED.
> >
> > So we need to detect the error code, and retry without that flag
> > set as a fallback. Same in next patch.
>
> Yep, makes sense - will change. Thanks for the feedback.
It'll be VIR_ERR_INVALID_ARG btw for an unsupported flag.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 26/42] systemd: Switch virtchd to common templates
On Tue, Sep 26, 2023 at 11:09:44AM +0200, Pavel Hrdina wrote:
> On Mon, Sep 25, 2023 at 08:58:24PM +0200, Andrea Bolognani wrote:
> > Signed-off-by: Andrea Bolognani
> > ---
> > src/ch/meson.build| 27
> > src/ch/virtchd.service.in | 44 ---
> > 2 files changed, 23 insertions(+), 48 deletions(-)
> > delete mode 100644 src/ch/virtchd.service.in
> >
> > diff --git a/src/ch/meson.build b/src/ch/meson.build
> > index dc08069dcd..f6c443f3c6 100644
> > --- a/src/ch/meson.build
> > +++ b/src/ch/meson.build
> > @@ -57,11 +57,30 @@ if conf.has('WITH_CH')
> >
> >virt_daemon_units += {
> > 'service': 'virtchd',
> > -'service_in': files('virtchd.service.in'),
> > 'name': 'Libvirt ch',
> > -'socket_in': libvirtd_socket_in,
> > -'socket_ro_in': libvirtd_socket_ro_in,
> > -'socket_admin_in': libvirtd_socket_admin_in,
> > +'service_unit_extra': [
> > + 'Wants=systemd-machined.service',
> > + 'After=systemd-machined.service',
> > + 'After=remote-fs.target',
> > +],
> > +'service_service_extra': [
> > + 'KillMode=process',
> > + '# Raise hard limits to match behaviour of systemd >= 240.',
> > + '# During startup, daemon will set soft limit to match hard limit',
> > + '# per systemd recommendations',
> > + 'LimitNOFILE=1024:524288',
> > + '# The cgroups pids controller can limit the number of tasks started
> > by',
> > + '# the daemon, which can limit the number of domains for some
> > hypervisors.',
> > + '# A conservative default of 8 tasks per guest results in a TasksMax
> > of',
> > + '# 32k to support 4096 guests.',
> > + 'TasksMax=32768',
> > + '# With cgroups v2 there is no devices controller anymore, we have
> > to use',
> > + '# eBPF to control access to devices. In order to do that we create
> > a eBPF',
> > + '# hash MAP which locks memory. The default map size for 64 devices
> > together',
> > + '# with program takes 12k per guest. After rounding up we will get
> > 64M to',
> > + '# support 4096 guests.',
> > + 'LimitMEMLOCK=64M',
> > +],
>
> This feels wrong to have it in meson.build file. In addition it is the
> same as for virtlxcd and virtqemud so we are basically duplicating the
> data and which makes it easy to make inconsistent changes not affecting
> all places.
>
> IMHO it would be better to have additional file that will be included
> into the template for services where we need it.
>
> I'm not sure about the `service_unit_extra` as well if we want to have
> it in meson.build files as it is not strictly related to the build
> process and there is more data compared to the old `deps`.
If anything I'd reverse the model. The 'virtchd.service.in' file
should be the primary template, the common bits the injected data.
ie
cat virtchd.service.in
[Unit]
Description=Virtualization Cloud-Hypervisor daemon
::common-unit::
Wants=systemd-machined.service
After=remote-fs.target
After=systemd-machined.service
Documentation=man:virtchd(8)
[Service]
::common-service::
KillMode=process
# Raise hard limits to match behaviour of systemd >= 240.
# During startup, daemon will set soft limit to match hard limit
# per systemd recommendations
LimitNOFILE=1024:524288
# The cgroups pids controller can limit the number of tasks started by
# the daemon, which can limit the number of domains for some hypervisors.
# A conservative default of 8 tasks per guest results in a TasksMax of
# 32k to support 4096 guests.
TasksMax=32768
# With cgroups v2 there is no devices controller anymore, we have to use
# eBPF to control access to devices. In order to do that we create a eBPF
# hash MAP which locks memory. The default map size for 64 devices together
# with program takes 12k per guest. After rounding up we will get 64M to
# support 4096 guests.
LimitMEMLOCK=64M
[Install]
::common-install::
arguably we don't even need the '::common-XXX::' lines in there. We can
simply see the headers [Unit], [Service], etc and inject the common
bits under each header.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 12/42] systemd: Make @service_in@ optional
On Mon, Sep 25, 2023 at 08:58:10PM +0200, Andrea Bolognani wrote: > It is currently considered required, but we're soon going to > provide a default that will be suitable for most services. > > Since all services currently provide a value explicitly, we > can implement a default without breaking anything. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 6 -- > 1 file changed, 4 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 35/42] systemd: Replace Requires with BindTo+After for sockets
On Mon, Sep 25, 2023 at 08:58:33PM +0200, Andrea Bolognani wrote: > This is the strongest relationship that can be declared between > two units, and causes the service to be terminated immediately > if any of its sockets disappear. This is the behavior we want. IIUC, this prevents running the service with /only/ the main socket, and ro/admin sockets disabled. Running without the ro socket in particular was something we wanted to allow to reduce exposure to unprivileged services (there have been a number of CVEs where the read-only socket was the way in) > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd.service.in | 6 -- > src/logging/virtlogd.service.in | 6 -- > src/virtd.service.in | 9 ++--- > 3 files changed, 14 insertions(+), 7 deletions(-) > > diff --git a/src/locking/virtlockd.service.in > b/src/locking/virtlockd.service.in > index 9e91fa3261..a21a2c2c19 100644 > --- a/src/locking/virtlockd.service.in > +++ b/src/locking/virtlockd.service.in > @@ -1,7 +1,9 @@ > [Unit] > Description=Virtual machine lock manager > -Requires=virtlockd.socket > -Requires=virtlockd-admin.socket > +BindsTo=virtlockd.socket > +BindsTo=virtlockd-admin.socket > +After=virtlockd.socket > +After=virtlockd-admin.socket > Before=libvirtd.service > Documentation=man:virtlockd(8) > Documentation=https://libvirt.org > diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in > index 97c942ffb0..f3bd576301 100644 > --- a/src/logging/virtlogd.service.in > +++ b/src/logging/virtlogd.service.in > @@ -1,7 +1,9 @@ > [Unit] > Description=Virtual machine log manager > -Requires=virtlogd.socket > -Requires=virtlogd-admin.socket > +BindsTo=virtlogd.socket > +BindsTo=virtlogd-admin.socket > +After=virtlogd.socket > +After=virtlogd-admin.socket > Before=libvirtd.service > Documentation=man:virtlogd(8) > Documentation=https://libvirt.org > diff --git a/src/virtd.service.in b/src/virtd.service.in > index 21391a65b0..b9e6345e8c 100644 > --- a/src/virtd.service.in > +++ b/src/virtd.service.in > @@ -1,8 +1,11 @@ > [Unit] > Description=@name@ daemon > -Requires=@service@.socket > -Requires=@service@-ro.socket > -Requires=@service@-admin.socket > +BindsTo=@service@.socket > +BindsTo=@service@-ro.socket > +BindsTo=@service@-admin.socket > +After=@service@.socket > +After=@service@-ro.socket > +After=@service@-admin.socket > Conflicts=libvirtd.service > After=libvirtd.service > After=network.target > -- > 2.41.0 > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 09/42] systemd: Drop unnecessary uses of @sockets@
On Mon, Sep 25, 2023 at 08:58:07PM +0200, Andrea Bolognani wrote: > For most services, the value provided explicitly matches the > documented default. > > Signed-off-by: Andrea Bolognani > --- > src/ch/meson.build | 1 - > src/interface/meson.build | 1 - > src/libxl/meson.build | 1 - > src/lxc/meson.build | 1 - > src/network/meson.build | 1 - > src/node_device/meson.build | 1 - > src/nwfilter/meson.build| 1 - > src/qemu/meson.build| 1 - > src/secret/meson.build | 1 - > src/storage/meson.build | 1 - > src/vbox/meson.build| 1 - > src/vz/meson.build | 1 - > 12 files changed, 12 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 10/42] systemd: Make @sockprefix@ optional
On Mon, Sep 25, 2023 at 08:58:08PM +0200, Andrea Bolognani wrote: > For most services, the socket paths can be derived trivially from > the name of the daemon: for virtqemud, for example, they will be > > /run/libvirt/virtqemud-sock > /run/libvirt/virtqemud-sock-ro > /run/libvirt/virtqemud-admin-sock > > libvirtd and virtproxyd are the exceptions, since their socket > paths will be > > /run/libvirt/libvirt-sock > /run/libvirt/libvirt-sock-ro > /run/libvirt/libvirt-admin-sock > > So we still need to be able to provide a custom @sockprefix@ in > those cases, but in the most common scenario we can do away with > the requirement by introducing a sensible default. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 11/42] systemd: Drop unnecessary uses of @sockprefix@
On Mon, Sep 25, 2023 at 08:58:09PM +0200, Andrea Bolognani wrote: > Now that providing the value is optional, we can remove almost > all uses. > > Signed-off-by: Andrea Bolognani > --- > src/ch/meson.build | 1 - > src/interface/meson.build | 1 - > src/libxl/meson.build | 1 - > src/locking/meson.build | 1 - > src/logging/meson.build | 1 - > src/lxc/meson.build | 1 - > src/network/meson.build | 1 - > src/node_device/meson.build | 1 - > src/nwfilter/meson.build| 1 - > src/qemu/meson.build| 1 - > src/secret/meson.build | 1 - > src/storage/meson.build | 1 - > src/vbox/meson.build| 1 - > src/vz/meson.build | 1 - > 14 files changed, 14 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 02/42] systemd: Add missing WantedBy for virtlogd/virtlockd
On Mon, Sep 25, 2023 at 08:58:00PM +0200, Andrea Bolognani wrote: > This annotation being missing resulted in virtlogd and virtlockd > being marked as "indirect" services, i.e. services that cannot > be started directly but have to be socket activated instead. > > While this is our preferred configuration, we shouldn't prevent > the admin to start them at boot if they want to. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd.service.in | 1 + > src/logging/virtlogd.service.in | 1 + > 2 files changed, 2 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 04/42] systemd: Set Type=notify for virtlogd/virtlockd
On Mon, Sep 25, 2023 at 08:58:02PM +0200, Andrea Bolognani wrote: > This tells systemd that the services in question support the > native socket activation protocol. > > virtlogd and virtlockd, just like all the other daemons, implement > the necessary handshake. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd.service.in | 1 + > src/logging/virtlogd.service.in | 1 + > 2 files changed, 2 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 07/42] systemd: Rename @mode@ -> @sockmode@
On Mon, Sep 25, 2023 at 08:58:05PM +0200, Andrea Bolognani wrote: > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 6 +++--- > src/remote/libvirtd.socket.in | 2 +- > 2 files changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 08/42] systemd: Only set @sockmode@ once
On Mon, Sep 25, 2023 at 08:58:06PM +0200, Andrea Bolognani wrote: > The decision is based only on whether Polkit support is enabled, > so there's no need to go through it again for every single > service. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 12 ++-- > 1 file changed, 6 insertions(+), 6 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 06/42] systemd: Rename socket_in_def -> socket_in_default
On Mon, Sep 25, 2023 at 08:58:04PM +0200, Andrea Bolognani wrote: > The meaning of the _def suffix might not be immediately obvious, > especially since it's also used to refer to the output of the > meson-gen-def.py script elsewhere in the same file. > > Signed-off-by: Andrea Bolognani > --- > src/meson.build | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 05/42] systemd: Set @name@ for virtlogd/virtlockd
On Mon, Sep 25, 2023 at 08:58:03PM +0200, Andrea Bolognani wrote: > The information is not used anywhere right now, but the > documentation for virt_daemon_units claims it's mandatory. > > More importantly, we're going to start actually using it later > on. > > Signed-off-by: Andrea Bolognani > --- > src/locking/meson.build | 2 +- > src/logging/meson.build | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 01/42] systemd: Add missing Also for admin socket
On Mon, Sep 25, 2023 at 08:57:59PM +0200, Andrea Bolognani wrote: > When libvirtd, virtlog and virtlockd are enabled, we want their > admin sockets to be enabled as well. s/enabled/enabled for socket activation/ because these admin sockets were enabled automatically when the service eventually starts already. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd.service.in | 1 + > src/logging/virtlogd.service.in | 1 + > src/remote/libvirtd.service.in | 1 + > 3 files changed, 3 insertions(+) Reviewed-by: Daniel P. Berrangé > > diff --git a/src/locking/virtlockd.service.in > b/src/locking/virtlockd.service.in > index dd0bbab083..18873f86a6 100644 > --- a/src/locking/virtlockd.service.in > +++ b/src/locking/virtlockd.service.in > @@ -22,3 +22,4 @@ LimitNOFILE=1024:524288 > > [Install] > Also=virtlockd.socket > +Also=virtlockd-admin.socket > diff --git a/src/logging/virtlogd.service.in b/src/logging/virtlogd.service.in > index 8e245ddb43..14a991f348 100644 > --- a/src/logging/virtlogd.service.in > +++ b/src/logging/virtlogd.service.in > @@ -22,3 +22,4 @@ LimitNOFILE=1024:524288 > > [Install] > Also=virtlogd.socket > +Also=virtlogd-admin.socket > diff --git a/src/remote/libvirtd.service.in b/src/remote/libvirtd.service.in > index 84f1613adc..8839c00a15 100644 > --- a/src/remote/libvirtd.service.in > +++ b/src/remote/libvirtd.service.in > @@ -50,3 +50,4 @@ Also=virtlockd.socket > Also=virtlogd.socket > Also=libvirtd.socket > Also=libvirtd-ro.socket > +Also=libvirtd-admin.socket > -- > 2.41.0 > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 03/42] systemd: Add missing Service for virtlogd/virtlockd
On Mon, Sep 25, 2023 at 08:58:01PM +0200, Andrea Bolognani wrote: > While systemd will automatically match foo.socket with foo.service > based on their names, it's nicer to connect the two explicitly. > > This is what we do for all services, with virtlogd and virtlockd > being the only exceptions. > > Signed-off-by: Andrea Bolognani > --- > src/locking/virtlockd.socket.in | 1 + > src/logging/virtlogd.socket.in | 1 + > 2 files changed, 2 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 04/31] docs: mark CRIS support as deprecated
On Mon, Sep 25, 2023 at 03:48:27PM +0100, Alex Bennée wrote: > This might be premature but while streamling the avocado tests I > realised the only tests we have are "check-tcg" ones. The aging > fedora-criss-cross image works well enough for developers but can't be > used in CI as we need supported build platforms to build QEMU. > > Does this mean the writing is on the wall for this architecture? > > Signed-off-by: Alex Bennée > Cc: Rabin Vincent > Cc: Edgar E. Iglesias > --- > docs/about/deprecated.rst | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index dc4da95329..7cfe313aa6 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -399,6 +399,17 @@ Specifying the iSCSI password in plain text on the > command line using the > used instead, to refer to a ``--object secret...`` instance that provides > a password via a file, or encrypted. > > +TCG CPUs > + > + > +CRIS CPU architecture (since 8.1) > +' > + > +The CRIS architecture was pulled from Linux in 4.17 and the compiler > +is no longer packaged in any distro making it harder to run the > +``check-tcg`` tests. Unless we can improve the testing situation there > +is a chance the code will bitrot without anyone noticing. Deprecated is generally a warning that we intend to delete the feature. If we're just going to relegate it to untested status (what I'd call "tier 3" quality), then we should document that elsewhere. I don't mind which way we go. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 04/31] docs: mark CRIS support as deprecated
On Mon, Sep 25, 2023 at 03:48:27PM +0100, Alex Bennée wrote: > This might be premature but while streamling the avocado tests I > realised the only tests we have are "check-tcg" ones. The aging > fedora-criss-cross image works well enough for developers but can't be > used in CI as we need supported build platforms to build QEMU. > > Does this mean the writing is on the wall for this architecture? It was deleted in Linux, and GCC dropped the cris-*linux target, but GCC keeps other cris targets. IOW, at very least, it has become a niche use case target. We don't need Linux/GCC support as a pre-requisite for having a target in QEMU, but it sure makes it increasingly challenging to test. > > Signed-off-by: Alex Bennée > Cc: Rabin Vincent > Cc: Edgar E. Iglesias > --- > docs/about/deprecated.rst | 11 +++ > 1 file changed, 11 insertions(+) > > diff --git a/docs/about/deprecated.rst b/docs/about/deprecated.rst > index dc4da95329..7cfe313aa6 100644 > --- a/docs/about/deprecated.rst > +++ b/docs/about/deprecated.rst > @@ -399,6 +399,17 @@ Specifying the iSCSI password in plain text on the > command line using the > used instead, to refer to a ``--object secret...`` instance that provides > a password via a file, or encrypted. > > +TCG CPUs > + > + > +CRIS CPU architecture (since 8.1) > +' > + > +The CRIS architecture was pulled from Linux in 4.17 and the compiler > +is no longer packaged in any distro making it harder to run the > +``check-tcg`` tests. Unless we can improve the testing situation there > +is a chance the code will bitrot without anyone noticing. > + > Backwards compatibility > --- > > -- > 2.39.2 > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH 02/31] tests/lcitool: add swtpm to the package list
On Mon, Sep 25, 2023 at 03:48:25PM +0100, Alex Bennée wrote: > We need this to test some TPM stuff. > > Signed-off-by: Alex Bennée > --- > .gitlab-ci.d/cirrus/macos-12.vars| 2 +- > tests/docker/dockerfiles/alpine.docker | 1 + > tests/docker/dockerfiles/centos8.docker | 1 + > tests/docker/dockerfiles/debian-amd64-cross.docker | 1 + > tests/docker/dockerfiles/debian-amd64.docker | 1 + > tests/docker/dockerfiles/debian-arm64-cross.docker | 1 + > tests/docker/dockerfiles/debian-armhf-cross.docker | 1 + > tests/docker/dockerfiles/debian-ppc64el-cross.docker | 1 + > tests/docker/dockerfiles/debian-s390x-cross.docker | 1 + > tests/docker/dockerfiles/fedora-win32-cross.docker | 1 + > tests/docker/dockerfiles/fedora-win64-cross.docker | 1 + > tests/docker/dockerfiles/fedora.docker | 1 + > tests/docker/dockerfiles/opensuse-leap.docker| 1 + > tests/docker/dockerfiles/ubuntu2204.docker | 1 + > tests/lcitool/libvirt-ci | 2 +- > tests/lcitool/projects/qemu.yml | 1 + > 16 files changed, 16 insertions(+), 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [RFC PATCH libvirt v1 2/3] Improve `virsh start --console` behavior
On Mon, Sep 25, 2023 at 03:39:09PM +0200, Marc Hartmayer wrote:
> When starting a guest via libvirt (`virsh start --console`), early
> console output was missed because the guest was started first and then
> the console was attached. This patch changes this to the following
> sequence:
>
> 1. create a paused guest
> 2. attach the console
> 3. resume the guest
>
> Reviewed-by: Boris Fiuczynski
> Signed-off-by: Marc Hartmayer
> ---
> tools/virsh-domain.c | 11 +--
> 1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/tools/virsh-domain.c b/tools/virsh-domain.c
> index 5c3c6d18aebf..3581161c6f53 100644
> --- a/tools/virsh-domain.c
> +++ b/tools/virsh-domain.c
> @@ -4065,6 +4065,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> g_autoptr(virshDomain) dom = NULL;
> #ifndef WIN32
> bool console = vshCommandOptBool(cmd, "console");
> +bool resume_domain = false;
> #endif
> unsigned int flags = VIR_DOMAIN_NONE;
> int rc;
> @@ -4083,8 +4084,14 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> if (virshFetchPassFdsList(ctl, cmd, &nfds, &fds) < 0)
> return false;
>
> -if (vshCommandOptBool(cmd, "paused"))
> +if (vshCommandOptBool(cmd, "paused")) {
> flags |= VIR_DOMAIN_START_PAUSED;
> +#ifndef WIN32
> +} else if (console) {
> +flags |= VIR_DOMAIN_START_PAUSED;
> +resume_domain = true;
> +#endif
Hypervisor drivers are not required to support VIR_DOMAIN_START_PAUSED.
So we need to detect the error code, and retry without that flag
set as a fallback. Same in next patch.
> +}
> if (vshCommandOptBool(cmd, "autodestroy"))
> flags |= VIR_DOMAIN_START_AUTODESTROY;
> if (vshCommandOptBool(cmd, "bypass-cache"))
> @@ -4142,7 +4149,7 @@ cmdStart(vshControl *ctl, const vshCmd *cmd)
> vshPrintExtra(ctl, _("Domain '%1$s' started\n"),
>virDomainGetName(dom));
> #ifndef WIN32
> -if (console && !cmdRunConsole(ctl, dom, NULL, false, 0))
> +if (console && !cmdRunConsole(ctl, dom, NULL, resume_domain, 0))
> return false;
> #endif
>
> --
> 2.34.1
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [PATCH] libxl: Fix connection to modular network daemon
On Fri, Sep 22, 2023 at 02:20:10PM -0600, Jim Fehlig wrote:
> In a modular daemon configuration, virtxend does not support the
> virNetwork* APIs. It should open a connection to virtnetworkd when
> using those APIs, but currently always opens a connection to
> "xen:///system". Switch to using virGetConnectNetwork to obtain a
> valid connection instead of using the hardcoded URI.
>
> Signed-off-by: Jim Fehlig
> ---
> src/libxl/libxl_conf.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index 4582126d19..62e1be6672 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -1372,7 +1372,7 @@ libxlMakeNic(virDomainDef *def,
> break;
> case VIR_DOMAIN_NET_TYPE_NETWORK:
> {
> -if (!(conn = virConnectOpen("xen:///system")))
> +if (!(conn = virGetConnectNetwork()))
> goto cleanup;
>
> if (!(network =
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v3 10/12] ci: jobs.sh: integration: Execute commands via 'run_cmd[_quiet]' helpers
On Tue, Sep 19, 2023 at 11:15:05AM +0200, Erik Skultety wrote: > Unfortunately, once we go down the line of running our own scripts as > part of GitLab CI jobs rather than open coding Shell in YAML, we lose > the benefit of seeing each line the script executes. The downside of > the default YAML however is that we have to maintain the same piece of > code on 2 places in that case. Let's adopt what we use with other > container jobs and prefix each shell command with 'run_cmd' or > 'run_cmd_quiet' which will dump it in the logs before executing. > Flow control expressions and structures are a problem though in this > regard, so let's just print some important values for debugging > purposes. > > Signed-off-by: Erik Skultety > --- > ci/jobs.sh | 26 +- > 1 file changed, 13 insertions(+), 13 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v3 09/12] ci: jobs.sh: Introduce a quiet version of run_cmd
On Tue, Sep 19, 2023 at 11:15:04AM +0200, Erik Skultety wrote: > We've started using the run_cmd helper function to log what kind of > command is being executed as well as actually executing the command. > The problem however is doing I/O redirections for commands which we > don't wish to see any output for whatever reason. Now, if the > redirection is applied at parameter passing to run_cmd it's going to be > applied to the debug print as well. Let's introduce another helper, > run_cmd_quiet which takes care of the I/O redirection and executes the > command completely silently. > > Signed-off-by: Erik Skultety > --- > ci/jobs.sh | 5 + > 1 file changed, 5 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 12/12] ci: jobs.sh: Define and create SCRATCH_DIR for local executions
On Mon, Sep 18, 2023 at 12:22:48PM +0200, Erik Skultety wrote: > Running outside of GitLab will likely not have the variable set and > hence the execution would fail. To make sure we always start with a > clean scratch dir (which may or may not be the best thing), create it > with 'mktemp'. The main reason for a temporary directory is to ensure a > clean environment for the job every time run_integration function is > run. For repeated interactive use case, it is imperative that the > developer takes care of their environment. > > Signed-off-by: Erik Skultety > --- > ci/jobs.sh | 7 +++ > 1 file changed, 7 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 11/12] ci: jobs: run_integration: Print DAEMONS variable for debugging
On Mon, Sep 18, 2023 at 12:22:47PM +0200, Erik Skultety wrote: > One advantage that GitLab's YAML has with Shell commands is that every > single line is printed out as is, including control structures. In > order to see whether the logic did the same thing and the tests are > going to operate on the right set of daemons (monolithic vs modular), > lets print the DAEMONS variable that we set depending on the distro we > execute the tests on. > > Signed-off-by: Erik Skultety > --- > ci/jobs.sh | 2 ++ > 1 file changed, 2 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 10/12] ci: jobs: integration: Execute raw commands via 'run_cmd' helper
On Mon, Sep 18, 2023 at 12:22:46PM +0200, Erik Skultety wrote:
> Unfortunately, once we go down the line of running our own scripts as
> part of GitLab CI jobs rather than open coding Shell in YAML, we lose
> the benefit of seeing each line the script executes. The downside of
> the default YAML however is that we have to maintain the same piece of
> code on 2 places in that case. Let's adopt what we use with other
> container jobs and prefix each shell command with 'run_cmd' which will
> dump it in the logs before executing.
> Flow control expressions and structures are a problem though in this
> regard, so let's just print some important values for debugging
> purposes.
>
> Signed-off-by: Erik Skultety
> ---
> ci/jobs.sh | 26 +-
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/ci/jobs.sh b/ci/jobs.sh
> index 3a89cb1a69..27add3d105 100644
> --- a/ci/jobs.sh
> +++ b/ci/jobs.sh
> @@ -82,16 +82,16 @@ run_website_build() {
> }
>
> run_integration() {
> -sudo pip3 install --prefix=/usr avocado-framework
> +run_cmd sudo pip3 install --prefix=/usr avocado-framework
>
> # Explicitly allow storing cores globally
> -sudo sh -c "echo DefaultLimitCORE=infinity >> /etc/systemd/system.conf"
> +run_cmd sudo sh -c "echo DefaultLimitCORE=infinity >>
> /etc/systemd/system.conf" # Explicitly allow storing cores globally
>
> # Need to reexec systemd after changing config
> -sudo systemctl daemon-reexec
> +run_cmd sudo systemctl daemon-reexec # need to reexec systemd after
> changing config
Accidentally re-adding the comments at the end of line that you
removed in an earlier patch.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 08/12] ci: jobs: run_integration: Make POSIX-compliant
On Mon, Sep 18, 2023 at 12:22:44PM +0200, Erik Skultety wrote:
> Neither '&>' nor 'source' are defined in POSIX.
>
> Signed-off-by: Erik Skultety
> ---
> ci/jobs.sh | 8
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/ci/jobs.sh b/ci/jobs.sh
> index 37bca452fa..f4e83dda2e 100644
> --- a/ci/jobs.sh
> +++ b/ci/jobs.sh
> @@ -91,7 +91,7 @@ run_integration() {
> sudo systemctl daemon-reexec
>
> # Source the os-release file to query the vendor-provided variables
> -source /etc/os-release
> +. /etc/os-release
> if test "$ID" = "centos" && test "$VERSION_ID" -eq 8
> then
> DAEMONS="libvirtd virtlockd virtlogd"
> @@ -102,8 +102,8 @@ run_integration() {
> do
> LOG_OUTPUTS="1:file:/var/log/libvirt/${daemon}.log"
> LOG_FILTERS="3:remote 4:event 3:util.json 3:util.object 3:util.dbus
> 3:util.netlink 3:node_device 3:rpc 3:access 1:*"
> -sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters
> "'$LOG_FILTERS'" &>/dev/null
> -sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs
> "'$LOG_OUTPUTS'" &>/dev/null
> +sudo augtool set /files/etc/libvirt/${daemon}.conf/log_filters
> "'$LOG_FILTERS'" 2>/dev/null 1>&2
> +sudo augtool set /files/etc/libvirt/${daemon}.conf/log_outputs
> "'$LOG_OUTPUTS'" 2>/dev/null 1>&2
Nit-picking here, but "1>/dev/null 2>&1" always felt like the more common way
around.
> sudo systemctl --quiet stop ${daemon}.service
> sudo systemctl restart ${daemon}.socket
> done
> @@ -113,7 +113,7 @@ run_integration() {
> # Shell scripts with -e by default and virsh returns an error if one
> tries
> # to start a machine/network that is already active which is both fine
> and
> # should also be a non-fatal error
> -sudo virsh --quiet net-start default &>/dev/null || true
> +sudo virsh --quiet net-start default 2>/dev/null 1>&2 || true
>
> cd "$SCRATCH_DIR"
> git clone --depth 1 https://gitlab.com/libvirt/libvirt-tck.git
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 07/12] ci: jobs.sh: run_integration: Add/Rewrite/Reformat commentaries
On Mon, Sep 18, 2023 at 12:22:43PM +0200, Erik Skultety wrote: > Because of the nature of writing inline shell commands to YAML, most of > the commentaries where inlined with the command not to hinder YAML > readability any further. Since we moved the logic to a standalone > script, we can now do whatever formatting & readability adjustments we > want. > > Signed-off-by: Erik Skultety > --- > ci/jobs.sh | 15 --- > 1 file changed, 12 insertions(+), 3 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 06/12] ci: jobs.sh: integration: Use --quiet with virsh
On Mon, Sep 18, 2023 at 12:22:42PM +0200, Erik Skultety wrote: > We've not been interested in any extra output from the command at all > since we always redirected both stdout and stderr to /dev/null. Future > patch will change that slightly, so --quiet will start making sense. > > Signed-off-by: Erik Skultety > --- > ci/jobs.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 05/12] ci: jobs.sh: Drop comment about the need for Avocado 98.0
On Mon, Sep 18, 2023 at 12:22:41PM +0200, Erik Skultety wrote: > We needed v98.0 due to a bug in the past and have been installing the > latest Avocado for a while, yet we kept the comment by a mistake. > Besides, looks like v98.0 ignores the avocado.config file in the TCK > repo instructing it to run the test suite sequentially leading to test > stability issues, so abandoning the v98.0 was a good thing in the end. Heh, ok ignore my comment in an earlier patch then :-) > Signed-off-by: Erik Skultety > --- > ci/jobs.sh | 1 - > 1 file changed, 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 04/12] ci: integration: Drop the 'install-deps' hidden job and reference
On Mon, Sep 18, 2023 at 12:22:40PM +0200, Erik Skultety wrote: > Since the section now only consists of a single command, we can happily > move the command to the main integration template job body. > > Signed-off-by: Erik Skultety > --- > ci/integration-template.yml | 6 +- > 1 file changed, 1 insertion(+), 5 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 03/12] ci: integration: Adjust the check for CentOS Stream version
On Mon, Sep 18, 2023 at 12:22:39PM +0200, Erik Skultety wrote: > All supported versions of Fedora and CentOS Stream 9 default to modular > setup, it's probably better if we cosmetically adjust the CentOS Stream > version check to make it explicit that monolithic daemon services ought > to be started only on Stream 8. > > Signed-off-by: Erik Skultety > --- > ci/jobs.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 02/12] ci: integration: Extract the integration CI main recipe to jobs.sh
On Mon, Sep 18, 2023 at 12:22:38PM +0200, Erik Skultety wrote:
> Follow what's been done to other jobs in .gitlab-ci.yml and extract the
> shell logic from YAML to a function in ci/jobs.sh
>
> Signed-off-by: Erik Skultety
> ---
> ci/integration-template.yml | 36 ++--
> ci/jobs.sh | 32
> 2 files changed, 34 insertions(+), 34 deletions(-)
Reviewed-by: Daniel P. Berrangé
> +run_integration() {
> +# Avocado >98.0 fails with the nwfilter TCK tests, so stick with 98.0
> for now
> +sudo pip3 install --prefix=/usr avocado-framework
I don't recall that mentioned problem with nwfilter, but could we have
a bug filed againts TCK and put the bug link in the comment for tracking.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 01/12] syntax-check: Drop the shell's 'check for minus' rule
On Mon, Sep 18, 2023 at 12:22:37PM +0200, Erik Skultety wrote: > Apparently we've only had it because the -[ao] options weren't portable > at the time, but according to > https://pubs.opengroup.org/onlinepubs/9699919799/utilities/test.html > > both are defined in POSIX.1-2017 revision which is old enough for all > our supported platforms to have adopted it already. Therefore, drop the > rule. The link above has an annotation against -a and -o which says "[OB] Obsolescent The functionality described may be removed in a future version of this volume of POSIX.1-2017. Strictly Conforming POSIX Applications and Strictly Conforming XSI Applications shall not use obsolescent features." In practice I'm skeptical any shell is going to remove this, even if POSIX removed it, because it would break about $bazillion user scripts in the wild. IOW, I don't mind dropping this blocking rule, but please update the commit message to say we don't care that it is obsolescent in POSIX. Reviewed-by: Daniel P. Berrangé > > Signed-off-by: Erik Skultety > --- > build-aux/syntax-check.mk | 9 - > 1 file changed, 9 deletions(-) > > diff --git a/build-aux/syntax-check.mk b/build-aux/syntax-check.mk > index d7cf109fbd..5718768193 100644 > --- a/build-aux/syntax-check.mk > +++ b/build-aux/syntax-check.mk > @@ -1201,15 +1201,6 @@ sc_prohibit_double_semicolon: > halt="Double semicolon detected" \ > $(_sc_search_regexp) > > -_ptm1 = use "test C1 && test C2", not "test C1 -''a C2" > -_ptm2 = use "test C1 || test C2", not "test C1 -''o C2" > -# Using test's -a and -o operators is not portable. > -# We prefer test over [, since the latter is spelled [[ in configure.ac. > -sc_prohibit_test_minus_ao: > - @prohibit='(\ - halt='$(_ptm1); $(_ptm2)' \ > - $(_sc_search_regexp) > - > # Avoid a test bashism. > sc_prohibit_test_double_equal: > @prohibit='(\ -- > 2.41.0 > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 09/12] ci: jobs.sh: run_cmd: Use eval to run commands
On Mon, Sep 18, 2023 at 01:47:03PM +0200, Erik Skultety wrote:
> On Mon, Sep 18, 2023 at 11:31:53AM +0100, Daniel P. Berrangé wrote:
> > On Mon, Sep 18, 2023 at 12:22:45PM +0200, Erik Skultety wrote:
> > > We tried to evade usage of eval in commit 6214ae55f6a, but trying to
> > > use I/O redirections with a command doesn't have the desired effect,
> > > because when Shell eats the redirection it is applied to anything
> > > inside the run_cmd function, even the print command we use for
> > > debugging purposes. In order to print all commands and use the
> > > redirection only on the actual execution of a given command, let's
> > > adopt eval on "$@" and allow passing redirections as strings later on.
> > > Future patches will demonstrate this.
> > >
> > > Signed-off-by: Erik Skultety
> > > ---
> > > ci/jobs.sh | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/ci/jobs.sh b/ci/jobs.sh
> > > index f4e83dda2e..3a89cb1a69 100644
> > > --- a/ci/jobs.sh
> > > +++ b/ci/jobs.sh
> > > @@ -15,7 +15,7 @@ fi
> > > GIT_ROOT="$(git rev-parse --show-toplevel)"
> > > run_cmd() {
> > > printf "\e[32m[RUN COMMAND]: '%s'\e[0m\n" "$*"
> > > -"$@"
> > > +eval "$@"
> > > }
> >
> > IMHO, we'd be better just creating a "run_cmd_quiet" variant which does
> >
> >"$@" 1>/dev/null 2>&1
>
> I don't have a problem with ^this in principle, but eval is more flexible (and
> hence more dangerous) in what we can pass as parameters in the future.
I really dislike the use of eval because it forces the callers to worry
about nested quoting of parameters, which is one of the worst aspects
of shell.
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH 09/12] ci: jobs.sh: run_cmd: Use eval to run commands
On Mon, Sep 18, 2023 at 12:22:45PM +0200, Erik Skultety wrote:
> We tried to evade usage of eval in commit 6214ae55f6a, but trying to
> use I/O redirections with a command doesn't have the desired effect,
> because when Shell eats the redirection it is applied to anything
> inside the run_cmd function, even the print command we use for
> debugging purposes. In order to print all commands and use the
> redirection only on the actual execution of a given command, let's
> adopt eval on "$@" and allow passing redirections as strings later on.
> Future patches will demonstrate this.
>
> Signed-off-by: Erik Skultety
> ---
> ci/jobs.sh | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ci/jobs.sh b/ci/jobs.sh
> index f4e83dda2e..3a89cb1a69 100644
> --- a/ci/jobs.sh
> +++ b/ci/jobs.sh
> @@ -15,7 +15,7 @@ fi
> GIT_ROOT="$(git rev-parse --show-toplevel)"
> run_cmd() {
> printf "\e[32m[RUN COMMAND]: '%s'\e[0m\n" "$*"
> -"$@"
> +eval "$@"
> }
IMHO, we'd be better just creating a "run_cmd_quiet" variant which does
"$@" 1>/dev/null 2>&1
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [sdl-qemu] [PATCH 1/1] No checks, dereferencing possible
On Thu, Sep 14, 2023 at 12:22:26PM +0200, Peter Krempa wrote:
> On Thu, Sep 14, 2023 at 09:44:16 +, Миронов Сергей Владимирович wrote:
> > No checks, dereferencing possible.
> >
> >
> > Return value of a function 'virDomainChrSourceDefNew'
> > is dereferenced at qemu_command.c without checking
> > for NULL, but it is usually checked for this function.
>
> This description here doesn't make sense. You are checking the presence
> of 'privateData' in 'virDomainVideoDef'.
>
> >
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> >
> > Fixes: 1f85f0967b ("ci: jobs.sh: Add back '--no-suite syntax-check
> > --print-errorlogs'")
> >
> > Signed-off-by: Sergey Mironov
> >
> > ---
> > src/qemu/qemu_command.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index e84374b4cf..8d11972c88 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -4698,6 +4698,8 @@ qemuBuildVideoCommandLine(virCommand *cmd,
> > g_autofree char *name = g_strdup_printf("%s-vhost-user",
> > video->info.alias);
> > qemuDomainChrSourcePrivate *chrsrcpriv =
> > QEMU_DOMAIN_CHR_SOURCE_PRIVATE(chrsrc);
> >
> > + if (chrsrc == NULL)
> > + return -1;
>
> This addition doesn't make sense as it's dead code. The private data is
> always allocated and checked that it's non-NULL in the qemu driver via
> the callback in virDomainVideoDefNew.
This is checking the result of virDomainChrSourceDefNew(), which can
fail as virDomainChrSourceDefInitialize() can fail if virClassNew()
fails its sanity checks.
The check is too late though, as we de-referenced chrsrc in the line
above when we accessed the private data.
>
> Do you have a call trace that would prove me otherwise?
>
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On Wed, Sep 13, 2023 at 04:14:55PM +0100, Daniel P. Berrangé wrote: > On Wed, Sep 13, 2023 at 05:07:27PM +0200, Ján Tomko wrote: > > On a Tuesday in 2023, Daniel P. Berrangé wrote: > > > On Tue, Sep 12, 2023 at 04:05:04PM +0200, Ján Tomko wrote: > > > > On a Monday in 2023, Daniel P. Berrangé wrote: > > > > > I would expect libvirt to "do the right thing" and automatically load > > > > > the /etc/subuid data for the current user and NOT require any extra > > > > > XML mapping to be set for unprivileged usage. > > > > > > > > > > > > > So, by default libvirt would assume that unprivileged > > > > accessmode='passthrough' means "use the whole range for my user > > > > from /etc/subuid"? > > > > > > > > Podman treats /etc/subuid as a pool and chooses a 64K range that is > > > > (to its knowledge) unused. I'm undecided whether that would also be > > > > a reasonable option for a default. > > > > > > I thought podman simply used the entry that is in /etc/subuid > > > as is: > > > > D'oh. Right. By default it uses --userns=host, which behaves as you > > describe. > > > > What I described is --userns=auto behavior, suggested in the bug > > discussion: > > https://bugzilla.redhat.com/show_bug.cgi?id=2034630#c8 > > What I'm also missing is understanding what component enforces that > you have grabbed a range that is actually present for your user > in /etc/subuid, as opposed to grabbing a range belonging to a > different user. > > Something must enforce that otherwise it is a total free for all > and /etc/subuid is largely pointless. Ah, virtiofsd is invoking newuidmap, which is a program with the 'setuid' capability flag set on its binary. This lets us do the privileged /proc/uidmap setup on behalf of virtiofsd and validates the /etc/subuid ranges. I think libvirt could, by default, read /etc/subuid and pick a 64k range from it, if it had more than 64k available. In future we could track the ranges to keep them unique per instance, but for now even the simple picking is better than requiring a manua user config every time. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On Wed, Sep 13, 2023 at 05:07:27PM +0200, Ján Tomko wrote: > On a Tuesday in 2023, Daniel P. Berrangé wrote: > > On Tue, Sep 12, 2023 at 04:05:04PM +0200, Ján Tomko wrote: > > > On a Monday in 2023, Daniel P. Berrangé wrote: > > > > I would expect libvirt to "do the right thing" and automatically load > > > > the /etc/subuid data for the current user and NOT require any extra > > > > XML mapping to be set for unprivileged usage. > > > > > > > > > > So, by default libvirt would assume that unprivileged > > > accessmode='passthrough' means "use the whole range for my user > > > from /etc/subuid"? > > > > > > Podman treats /etc/subuid as a pool and chooses a 64K range that is > > > (to its knowledge) unused. I'm undecided whether that would also be > > > a reasonable option for a default. > > > > I thought podman simply used the entry that is in /etc/subuid > > as is: > > D'oh. Right. By default it uses --userns=host, which behaves as you > describe. > > What I described is --userns=auto behavior, suggested in the bug > discussion: > https://bugzilla.redhat.com/show_bug.cgi?id=2034630#c8 What I'm also missing is understanding what component enforces that you have grabbed a range that is actually present for your user in /etc/subuid, as opposed to grabbing a range belonging to a different user. Something must enforce that otherwise it is a total free for all and /etc/subuid is largely pointless. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH] ci: jobs.sh: Add back '--no-suite syntax-check --print-errorlogs'
On Wed, Sep 13, 2023 at 01:55:54PM +0200, Erik Skultety wrote:
> Commit f688a53a converted .gitlab-ci.yml to the usage of ci/jobs.sh
> functions, but in doing that our test options
> '--no-suite syntax-check --print-errorlogs'
> got lost in the process and since commit 8e660c52 didn't introduce them
> in the first place, it caused a behavioral regression. This patch adds
> them back.
>
> Fixes: 8e660c5286d7e2d07dd61681074bf155592d
>
> Signed-off-by: Erik Skultety
> ---
>
> Technically this is a build breaker fix, but sending for review anyway to see
> if there's an agreement on this approach.
These defaults for run_test() are consistent with the override
done in run_codestyle(), so:
Reviewed-by: Daniel P. Berrangé
>
> ci/jobs.sh | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/ci/jobs.sh b/ci/jobs.sh
> index abd695e231..eb4a4e29cf 100644
> --- a/ci/jobs.sh
> +++ b/ci/jobs.sh
> @@ -39,7 +39,10 @@ run_dist() {
> }
>
> run_test() {
> +TEST_ARGS="${TEST_ARGS:=--no-suite syntax-check --print-errorlogs}"
> +
> test -f $GIT_ROOT/build/build.ninja || run_meson_setup
> +
> run_cmd meson test -C build $TEST_ARGS
> }
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 18/35] .gitlab-ci.yml: Convert the native build job to the build.sh usage
On Mon, Sep 11, 2023 at 03:43:19PM +0200, Erik Skultety wrote: > Individual shell command executions are replaced by respective > functions in the ci/build.sh base script. This will make sure we use > the same recipes in GitLab jobs as well as in local executions. > > Signed-off-by: Erik Skultety > Reviewed-by: Daniel P. Berrangé > Erik Skultety : > --- > .gitlab-ci.yml | 10 -- > 1 file changed, 4 insertions(+), 6 deletions(-) > > diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml > index 1c6af8f8b3..c837812091 100644 > --- a/.gitlab-ci.yml > +++ b/.gitlab-ci.yml > @@ -25,15 +25,13 @@ include: >- ccache/ > key: "$CI_JOB_NAME" >script: > -- *script_variables > -- meson setup build --werror $MESON_ARGS || (cat > build/meson-logs/meson-log.txt && exit 1) > -- meson dist -C build --no-tests > +- source ci/jobs.sh > - if test -x /usr/bin/rpmbuild && test "$RPM" != "skip"; >then > -rpmbuild --clean --nodeps --define "_without_mingw 1" -ta > build/meson-dist/libvirt-*.tar.xz; > +run_rpmbuild; >else > -meson compile -C build; > -meson test -C build --no-suite syntax-check --print-errorlogs; > +run_build; > +run_test; I missed a regression here - we're loosing the --no-suite and --print-errorlogs args when running tests, so we can no longer diagnose the failures. >fi >after_script: > - test "$CI_JOB_STATUS" != "success" && exit 1; > -- > 2.41.0 > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 0/5] Add support for vDPA block devices
On Tue, Sep 12, 2023 at 04:11:01PM -0500, Jonathon Jongsma wrote: > On 9/12/23 7:00 AM, Peter Krempa wrote: > > On Mon, Sep 11, 2023 at 16:53:42 -0500, Jonathon Jongsma wrote: > > > see https://bugzilla.redhat.com/show_bug.cgi?id=1900770. > > > > > > Changes in v2: > > > - Don't use virStorageSource->path for vdpa device path to avoid > > > clashing with > > > existing path functionality > > > - Move vdpa device opening to the qemuProcessPrepareHostStorageSource() > > > function rather than the qemuDomainPrepareStorageSource() function. > > > This > > > also required some additional support in the tests for setting up the > > > objects properly for testing. > > > - rebased to latest master branch > > > > Reviewed-by: Peter Krempa > > > > I pushed this series, but for some reason only the ubuntu images are failing > CI with no useful output: > https://gitlab.com/libvirt/libvirt/-/pipelines/1001459836 This is a behavioural regression from the recent CI refactoring of Eriks'. We have lost the "--no-suite syntax-check --print-errorlogs" arguments when running tests. With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On Tue, Sep 12, 2023 at 04:05:04PM +0200, Ján Tomko wrote: > On a Monday in 2023, Daniel P. Berrangé wrote: > > On Mon, Sep 11, 2023 at 03:51:28PM +0200, Ján Tomko wrote: > > > Signed-off-by: Ján Tomko > > > --- > > > docs/kbase/virtiofs.rst | 29 + > > > 1 file changed, 29 insertions(+) > > > > > > diff --git a/docs/kbase/virtiofs.rst b/docs/kbase/virtiofs.rst > > > index 5940092db5..ecfb8e4236 100644 > > > --- a/docs/kbase/virtiofs.rst > > > +++ b/docs/kbase/virtiofs.rst > > > @@ -59,6 +59,35 @@ Sharing a host directory with a guest > > > > > > Note: this requires virtiofs support in the guest kernel (Linux v5.4 > > > or later) > > > > > > +ID mapping > > > +== > > > + > > > +In unprivileged mode (``qemu:///session``), mapping user/group IDs is > > > available > > > +(since libvirt version TBD). After reserving an ID range from the host > > > for your > > > +regular user > > > > Is the GUID/GID mapping available as in optional, or available as > > in mandatory ? > > > > In this series, optional. > > My reasoning was that someone might want to not do it and would prefer > to run virtiofsd as their own user. > > On second thought, that is not what accessmode='passthrough' means, > so for non-mapping non-privileged use, a different/new accessmode > attribute will be needed. > > > I would expect libvirt to "do the right thing" and automatically load > > the /etc/subuid data for the current user and NOT require any extra > > XML mapping to be set for unprivileged usage. > > > > So, by default libvirt would assume that unprivileged > accessmode='passthrough' means "use the whole range for my user > from /etc/subuid"? > > Podman treats /etc/subuid as a pool and chooses a 64K range that is > (to its knowledge) unused. I'm undecided whether that would also be > a reasonable option for a default. I thought podman simply used the entry that is in /etc/subuid as is: $ grep $LOGNAME /etc/subuid berrange:165536:65536 $ podman run -it centos:stream9 cat /proc/self/uid_map 0 1001 1 1 165536 65536 Maps "root" to my original unpriv login UID, and maps everything else to the 64k IDs reserved in /etc/subuid With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCHv1 8/8] docs: virtiofs: add section about ID remapping
On Mon, Sep 11, 2023 at 03:51:28PM +0200, Ján Tomko wrote: > Signed-off-by: Ján Tomko > --- > docs/kbase/virtiofs.rst | 29 + > 1 file changed, 29 insertions(+) > > diff --git a/docs/kbase/virtiofs.rst b/docs/kbase/virtiofs.rst > index 5940092db5..ecfb8e4236 100644 > --- a/docs/kbase/virtiofs.rst > +++ b/docs/kbase/virtiofs.rst > @@ -59,6 +59,35 @@ Sharing a host directory with a guest > > Note: this requires virtiofs support in the guest kernel (Linux v5.4 or > later) > > +ID mapping > +== > + > +In unprivileged mode (``qemu:///session``), mapping user/group IDs is > available > +(since libvirt version TBD). After reserving an ID range from the host for > your > +regular user Is the GUID/GID mapping available as in optional, or available as in mandatory ? I would expect libvirt to "do the right thing" and automatically load the /etc/subuid data for the current user and NOT require any extra XML mapping to be set for unprivileged usage. By all means have an XML config for it, but it should be optional and something that is essentially never used except for niche scenarios > + > +:: > + > + $ cat /etc/subuid > + jtomko:10:65536 > + $ cat /etc/subgid > + jtomko:10:65536 > + > +you can let virtiofsd map guest UIDs from 0 to 65535 > +to host IDs 10 to 165535 for example: > + > +:: > + > + > + > +... > + > + > + > + > + > + > + > Optional parameters > === > > -- > 2.41.0 > With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 10/35] ci: build.sh: Add a wrapper function over the 'potfile' job
On Mon, Sep 11, 2023 at 03:43:11PM +0200, Erik Skultety wrote: > This helper is a shell function transcript of its original GitLab CI > counterpart. There's one notable difference such that we pass '-j1' to > the meson compile command otherwise we'd have to execute the 'run_build' > function twice, passing 'libvirt-pot-dep' and 'libvirt-pot' targets > in a serial manner. > > Signed-off-by: Erik Skultety > Erik Skultety : > --- > ci/build.sh | 11 +++ > 1 file changed, 11 insertions(+) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 13/35] ci: build.sh: Drop changing working directory to CI_CONT_DIR
On Mon, Sep 11, 2023 at 03:43:14PM +0200, Erik Skultety wrote: > Firstly, this would mangle with "sourcing" this file in either > execution environment later down the road. Secondly, we won't need this > as future ci/helper patches will generate a throwaway script that will > take care of a correct execution of a build job in a similar fashion as > if the job ran in a GitLab environment. > > Signed-off-by: Erik Skultety > Erik Skultety : > --- > ci/build.sh | 2 -- > 1 file changed, 2 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 16/35] ci: Rename build.sh -> jobs.sh
On Mon, Sep 11, 2023 at 03:43:17PM +0200, Erik Skultety wrote:
> After the recent changes, this script no longer executes any logic
> anymore, it merely defines the jobs run in the GitLab environment. In
> order to use it, one has to source the file in the environment and then
> run one of the job "functions". For that, the 'build.sh' name is no
> longer descriptive enough and 'jobs.sh' feels more suitable and less
> misleading.
>
> Signed-off-by: Erik Skultety
> Erik Skultety :
> ---
> ci/{build.sh => jobs.sh} | 0
> 1 file changed, 0 insertions(+), 0 deletions(-)
> rename ci/{build.sh => jobs.sh} (100%)
Reviewed-by: Daniel P. Berrangé
With regards,
Daniel
--
|: https://berrange.com -o-https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o-https://fstop138.berrange.com :|
|: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
Re: [libvirt PATCH v2 14/35] ci: build.sh: Drop direct invocation of meson/ninja commands
On Mon, Sep 11, 2023 at 03:43:15PM +0200, Erik Skultety wrote: > We've moved all invocations to the respective helper function which > we'll execute both from gitlab CI jobs and local environments so we > don't need to have them on the global level as it would also not work > with "sourcing" this file to populate the environment with function > definitions. > > Signed-off-by: Erik Skultety > Erik Skultety : > --- > ci/build.sh | 5 + > 1 file changed, 1 insertion(+), 4 deletions(-) Reviewed-by: Daniel P. Berrangé With regards, Daniel -- |: https://berrange.com -o-https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o-https://fstop138.berrange.com :| |: https://entangle-photo.org-o-https://www.instagram.com/dberrange :|
