Bug 2260492 - Review Request: incus - Powerful system container and virtual machine manager
Summary: Review Request: incus - Powerful system container and virtual machine manager
Keywords:
Status: POST
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Michel Lind
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2024-01-26 13:00 UTC by Neal Gompa
Modified: 2024-04-27 12:24 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---
Embargoed:
michel: fedora-review+


Attachments (Terms of Use)
The .spec file difference from Copr build 6963272 to 6967304 (10.42 KB, patch)
2024-01-27 19:22 UTC, Fedora Review Service
no flags Details | Diff

Description Neal Gompa 2024-01-26 13:00:04 UTC
Spec URL: https://ngompa.fedorapeople.org/for-review/incus.spec
SRPM URL: https://ngompa.fedorapeople.org/for-review/incus-0.5-1.fc39.src.rpm

Description:
Container hypervisor based on LXC
Incus offers a REST API to remotely manage containers over the network,
using an image based work-flow and with support for live migration.

Fedora Account System Username: ngompa

Comment 1 Fedora Review Service 2024-01-26 13:09:05 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6963272
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2260492-incus/fedora-rawhide-x86_64/06963272-incus/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 2 Neal Gompa 2024-01-27 13:36:34 UTC
[fedora-review-service-build]

Comment 3 Fedora Review Service 2024-01-27 19:22:46 UTC
Created attachment 2011019 [details]
The .spec file difference from Copr build 6963272 to 6967304

Comment 4 Fedora Review Service 2024-01-27 19:22:48 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/6967304
(failed)

Build log:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2260492-incus/fedora-rawhide-x86_64/06967304-incus/builder-live.log.gz

Please make sure the package builds successfully at least for Fedora Rawhide.

- If the build failed for unrelated reasons (e.g. temporary network
  unavailability), please ignore it.
- If the build failed because of missing BuildRequires, please make sure they
  are listed in the "Depends On" field


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 6 Fedora Review Service 2024-02-29 18:00:29 UTC
Copr build:
https://copr.fedorainfracloud.org/coprs/build/7092290
(succeeded)

Review template:
https://download.copr.fedorainfracloud.org/results/@fedora-review/fedora-review-2260492-incus/fedora-rawhide-x86_64/07092290-incus/fedora-review/review.txt

Please take a look if any issues were found.


---
This comment was created by the fedora-review-service
https://github.com/FrostyX/fedora-review-service

If you want to trigger a new Copr build, add a comment containing new
Spec and SRPM URLs or [fedora-review-service-build] string.

Comment 7 Neil Hanlon 2024-03-13 05:26:24 UTC
Skipping the formality of entire review template as there are no glaring errors or problems. That said, a few notes and comments for posterity, mostly/exclusively from rpmlint.

Expected: these are set purposefully.

```
incus.x86_64: E: non-standard-dir-perm /var/cache/incus 700
incus.x86_64: E: non-standard-dir-perm /var/lib/incus 711
incus.x86_64: E: non-standard-dir-perm /var/log/incus 700
```

These seem possibly problematic, but I admit I am not sure if the selinux one is expected, as above.

```
incus-selinux.noarch: E: non-readable /var/lib/selinux/targeted/active/modules/200/incus 0
incus.x86_64: W: log-files-without-logrotate ['/var/log/incus']
incus.x86_64: W: post-without-tmpfile-creation /usr/lib/tmpfiles.d/incus.conf
```

Expected -- comes from macros and are standard:

```
incus-selinux.noarch: W: dangerous-command-in-%pre cp
incus-selinux.noarch: W: dangerous-command-in-%postun rm
incus-selinux.noarch: W: dangerous-command-in-%posttrans rm
incus-selinux.noarch: W: dangerous-command-in-%post rm
```

The rest of the review looks great. Aside from these couple items/questions, this is good to go, from my perspective.

If you want to throw some answers back at me and/or address issues if you think they are in need of fixing, I'm happy to throw an Approved stamp on this.

Comment 8 Reto Gantenbein 2024-03-13 17:20:58 UTC
Regarding the non-default permissions:
That's enforced by the upstream source code anyway so I thought I make it transparent this way.
See: https://discuss.linuxcontainers.org/t/incus-log-file-and-directory-permissions/18520/4

Regarding incus-selinux error:
I think the few SELinux file context definitions should eventually be moved to container-selinux similar to how it's done for Podman or Docker. To have these as a sub-package was convenient on COPR as long as I wasn't sure if this could land in Fedora.  

Regarding log-files-without-logrotate:
In the current default configuration I actually disabled the creation of a daemon log and only depend on journald as a log target. The problem is that the /var/log/incus directory is still "misused" for all kind of per-container runtime configuration and logs that are fully controlled by the Incus daemon. That's why there is no log file that must be managed by logrotate. These runtime state files will be moved to /run in the next few releases so that it might be possible to drop /var/log/incus altogether in the future.
See: https://discuss.linuxcontainers.org/t/incus-0-5-has-been-released/18814#use-of-runincus-for-runtime-data-20

In case you're wondering why I reply instead of Neal Gompa is because he heavily based the submitted spec file on mine that I'm maintaining for the time being on COPR (https://github.com/ganto/copr-lxc4/tree/master/incus). Obviously I'm looking forward to co-maintain this package in Fedora too.

Comment 9 Neil Hanlon 2024-03-16 03:18:23 UTC
> Regarding the non-default permissions:
> That's enforced by the upstream source code anyway so I thought I make it transparent this way.
> See: https://discuss.linuxcontainers.org/t/incus-log-file-and-directory-permissions/18520/4

Ack.

> Regarding incus-selinux error:
> I think the few SELinux file context definitions should eventually be moved to container-selinux similar to how it's done for Podman or Docker. To have these as a sub-package was convenient on COPR as long as I wasn't sure if this could land in Fedora.

Right--definitely something that can be amended over time to be "correct".

> Regarding log-files-without-logrotate:
> In the current default configuration I actually disabled the creation of a daemon log and only depend on journald as a log target. The problem is that the /var/log/incus directory is still "misused" for all kind of per-container runtime configuration and logs that are fully controlled by the Incus daemon. That's why there is no log file that must be managed by logrotate. These runtime state files will be moved to /run in the next few releases so that it might be possible to drop /var/log/incus altogether in the future.
> See: https://discuss.linuxcontainers.org/t/incus-0-5-has-been-released/18814#use-of-runincus-for-runtime-data-20

Acknowledged, that makes sense. I think good to leave for now, then.

> In case you're wondering why I reply instead of Neal Gompa is because he heavily based the submitted spec file on mine that I'm maintaining for the time being on COPR (https://github.com/ganto/copr-lxc4/tree/master/incus). Obviously I'm looking forward to co-maintain this package in Fedora too.

Right, that makes sense :) I knew the spec was familiar! Very glad to see you here.

Comment 11 Michel Lind 2024-04-26 02:16:46 UTC
previous reviewer seems to have accepted there are no remaining concerns but never set this as approved, so I've approved to unblock

Comment 12 Neil Hanlon 2024-04-26 02:27:11 UTC
Thanks Michel. I had this in my queue several times and kept getting hit with login issues with FAS :|

This is indeed fine and ready to move forward. Apologies for the delay.

Comment 13 Fedora Admin user for bugzilla script actions 2024-04-27 12:24:57 UTC
The Pagure repository was created at https://src.fedoraproject.org/rpms/incus


Note You need to log in before you can comment on or make changes to this bug.