Bug 866495

I already sponsored Glauber so I'm going to do it again.

Lifting FE-NEEDSPONSOR - I've just sponsored Glauber Costa.

Hi, a few remark :
- no need for BuildRoot 

- no need for 
rm -rf $RPM_BUILD_ROOT
in %install ( done by default )

- this is also done by default
%defattr(-,root,root)

- adding %attr(755,root,root) to all directory is useless, since that's the default setting

- this is likely incorrect, 777 is quite dangerous in fact
%attr(777, root, root) /etc/vz/conf

- why is there such requires :
Requires: /sbin/chkconfig

- Requires: libxml2
this is likly autodetected

- License: GPL
this is incorrect, you need to explicit the version of the GPL

- the initrd script should be replaced by a systemd file

- why does it create a directory in / ? I think this would be blocker since we have tried to reduce cruft there, and not increase it.

- %description is quite misleading, this doesn't permit to manage linux container, but openvz container.

- I think there is also some grammar/typo fixes to add :
This utility allows system administator to control Linux containers,

Hello Michael, thank you very much.

Some questions:
1 ) Why would libxml2 be autodetected?

2 ) Note that I am not installing any initrd script. OpenVZ installs it because it is needed when running it fully-featured ontop of non-upstream kernels. It will dump everything in its "make install", but it is only to be seen in my package in the %exclude directive. Isn't this alright ? Or do you mean something else?

3) About the directory in /, I believe you mean /vz, right? This is unfortunately hard-coded into a lot of places in OpenVZ, but I fully understand the objection. I'll work to patch it and replace it. Meanwhile, it would be good if we could keep the review going, aside from that point.


I'll post a new .spec shortly once I understand better the questions I posted in 1) and 2).

BTW, I believe I can get rid of the libxml dependency as well. Strictly speaking, it is only used when a subpackage that I am not packaging here is used.

I never tested, though. I will, and if everything goes right, I can just remove that dependency.

1) because rpm detect the dynamic library used by various binary, using ldd. So no need to explicitly add this one.

2) I think it may be better to remove in %install than adding lots of thing in %files ( in fact, this make it easier to review ), but that's not blocking

3) yes, i speak of /vz. But I can continue looking for others issues.

I guess one way to have this pushed upstream would be to have it non hardcoded ( ie, using a prefix ) and then you will only have 1 path to change.

Thanks Again, Michael,

I am already looking at using a prefix for that in upstream OpenVZ.

I'll publish a new version with your other comments merged shortly

Hi,

I've pushed new versions of the files to their old location:

Spec URL: http://glommer.net/vzctl-fedora/vzctl.spec
SRPM URL: http://glommer.net/vzctl-fedora/vzctl-4.0-1.fc17.src.rpm

I left the changelog and versions alone, because this is not released yet. Please tell me if it was supposed to have been bumped.

(In reply to comment #8)
> I left the changelog and versions alone, because this is not released yet.
> Please tell me if it was supposed to have been bumped.

You are supposed to add changelog entries for each change you make and to bump the release tag each time you update your package.

I believed this would only apply to packages already released, not in review stage. Sorry.

I've updated the changelog now:

Spec URL: http://glommer.net/vzctl-fedora/vzctl.spec
SRPM URL: http://glommer.net/vzctl-fedora/vzctl-4.0-2.fc17.src.rpm

(In reply to comment #10)
> I believed this would only apply to packages already released, not in review
> stage. Sorry.

No, this also applies to packages under review, because this tremendiously helps reviewers, as it e.g. helps reviewers to destinguish the versions.

Hi.

OpenVZ just released a new version that makes the /vz path configurable, as requested by Fedora. I changed it to /var/lib/vz, following what other packages like libvirt does for their images.

The updated spec and srpm can be both found at:

 http://glommer.net/vzctl-fedora/v3/

Thanks.

Hello, any further comments here ?

I'll review it.

REVIEW:

Legend: + = PASSED, - = FAILED, 0 = Not Applicable

+ rpmlint is NOT silent. Apart from incorrect-fsf-address messages which are not that critical (however I advise you to update licensing headers) here are few others:

work ~/Desktop: rpmlint vzctl-* | grep -v incorrect-fsf-address
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-vswap-1g.conf-sample
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-vswap-1024m.conf-sample
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-vswap-4g.conf-sample
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-basic.conf-sample
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-vswap-512m.conf-sample
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-light.conf-sample
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-unlimited.conf-sample
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/0.conf
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-vswap-256m.conf-sample
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz
vzctl-core.x86_64: W: conffile-without-noreplace-flag /etc/sysconfig/vz-scripts/ve-vswap-2g.conf-sample
vzctl-core.x86_64: W: non-conffile-in-etc /etc/vz/dists/distribution.conf-template

^^^ see below.

vzctl-core.x86_64: E: non-standard-dir-perm /var/lib/vz/private 0700L
vzctl-core.x86_64: E: non-standard-dir-perm /var/lib/vz/root 0700L

^^^ I suspect that's a requrement for VZ.

vzctl-core.x86_64: W: non-conffile-in-etc /etc/bash_completion.d/vzctl.sh

^^^ see below.

vzctl-core.x86_64: E: incoherent-logrotate-file /etc/logrotate.d/vzctl

^^^ That's ok. It just warns you that package name doesn't match logrotate's file name.

vzctl-core.x86_64: W: non-conffile-in-etc /etc/logrotate.d/vzctl
vzctl-core.x86_64: W: non-conffile-in-etc /etc/vz/dists/default

^^^ see below.

3 packages and 0 specfiles checked; 161 errors, 15 warnings.
work ~/Desktop: 


Regarding non-conffile-in-etc messages. There are two possible scenarios of installing files which are intended to be edited by user. We either can install them as is and mark them as conffiles (in this case RPM won't override them if they already exists) or we can install them as *.template / *.example / *.sample / etc, and require user to create them explicitly using provided ones as an examples.

You apparently choose the second option. And that's why rpmlint complaints a lot.

+ The package is named according to the  Package Naming Guidelines.
+ The spec file name matches the base package %{name}, in the format %{name}.spec.
+ The package meets the Packaging Guidelines.
+ The package is licensed with a Fedora approved license and meets the Licensing Guidelines.

- The License field in the package spec file MUST match the actual license. Correct tag is GPLv2+.

+ The file, containing the text of the license(s) for the package, is included in %doc.
+ The spec file is written in American English.
+ The spec file for the package is legible.
+ The sources used to build the package, match the upstream source, as provided in the spec URL.

sulaco ~/rpmbuild/SOURCES: sha256sum vzctl-4.1.tar.bz2*
a77a9b4db34259ca9ded9d0e869dc8d0a65b2534530a57c79fb284b9745a2f7d  vzctl-4.1.tar.bz2
a77a9b4db34259ca9ded9d0e869dc8d0a65b2534530a57c79fb284b9745a2f7d  vzctl-4.1.tar.bz2.1
sulaco ~/rpmbuild/SOURCES: 


+ The package successfully compiles and builds into binary rpms on at least one primary architecture.

* http://koji.fedoraproject.org/koji/taskinfo?taskID=4690929

+ All build dependencies are listed in BuildRequires.
0 No need to handle locales.
+ The package stores shared library files in some of the dynamic linker's default paths, and it calls ldconfig in %post and %postun.
+ The package does NOT bundle copies of system libraries.
0 The package is not designed to be relocatable.

- The package MUST own all directories that it creates. You must claim ownership on the following directories:

* %{_vzdir}/template/
* /var/lib/vzctl/

+ The package does not list a file more than once in the spec file's %files listings.
+ Permissions on files are set properly.
0 The package DOESN'T have a %clean section, so it won't build cleanly on systems with old rpm (EL-4 and EL-5, not sure about EL-6). But this package requires a very modern kernels so it won't work on older kernels anyway.
+ The package contains code, or permissible content.
0 No extremely large documentation files.
+ Anything, the package includes as %doc, does not affect the runtime of the application.
0 No C/C++ header files.
0 No static libraries.
0 No pkgconfig(.pc) files.
0 The package doesn't contain library files without a suffix (e.g. libfoo.so) in some of the dynamic linker's default paths.
0 No devel sub-package.
+ The package does NOT contain any .la libtool archives.
0 Not a GUI application.
+ The package does not own files or directories already owned by other packages.
0 At the beginning of %install, the package  does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) so it won't build cleanly on systems with old rpm (EL-4 and EL-5, not sure about EL-6). The same as with %clean section - it's irrelevant for this case.
+ All filenames in rpm packages are valid UTF-8.


So, please, before package uploading and building 

* correct License tag
* claim ownership on two additional libraries


Apart of that I don't see any other issues so this package is 


APPROVED.

Before I posted this, rpmlint was already silent for me.
It still is, even after I upgrade it to F19's, grabbed from Koji.

I am not sure why this is the case =(

In any case: The license is changed to gplv2+, and I claimed ownership over those two extra directories.

I also tried to address the other problems by moving some of the files from standard %{files} sections - but I can't verify the end rpmlint result.

In any case, I've put the updated files at:

http://glommer.net/vzctl-fedora/v4/

Thanks.

You have a go already. You should continue from this:

* https://fedoraproject.org/wiki/Package_SCM_admin_requests

New Package SCM Request
=======================
Package Name: vzctl
Short Description: utility that allows system administrators to control OpenVZ containers
Owners: glauber
Branches: f17 f18

Git done (by process-git-requests).

Closing as NEXTRELEASE, according to step 12 in https://fedoraproject.org/wiki/New_package_process_for_existing_contributors

vzctl-4.1-2.fc17 has been submitted as an update for Fedora 17.
https://admin.fedoraproject.org/updates/vzctl-4.1-2.fc17

vzctl-4.1-2.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/vzctl-4.1-2.fc18

What architectures are supported by openvz.org or vzctl? I can see some #ifdefs in include/vzsyscalls.h but do they mean vzctl really works there?

vzsyscalls.h is the entry point for the OpenVZ kernel, that we don't use in Fedora. (We do keep the package in its full, it would be a lot more trouble to strip it).

As for the upstream kernel, any architecture that has full cgroup + namespace support should work. So although I will be the first to admit that this is only tested in x86 and x86_64, there should not be any fundamental problems in running it in other architectures. We would only need to test it, and in case it blows, fix it.

In the near future, I will introduce container migration through the CRIU project. (http://criu.org). That specific functionality will be available for x86_64 only. But it is also the goal of the criu project to have it supported in multiple architectures. So this is a temporary condition.

Thanks.

Then it would be nice if vzctl could build also on arches without the OpenVZ kernel, right now it fails on s390(x) with (http://s390.koji.fedoraproject.org/koji/taskinfo?taskID=857522)

...
In file included from cpu.c:31:0:
../../include/vzsyscalls.h:66:2: error: #error "no syscall for this arch"
cpu.c: In function 'fairsched_chwt':
cpu.c:37:16: error: '__NR_fairsched_chwt' undeclared (first use in this function)
cpu.c:37:16: note: each undeclared identifier is reported only once for each function it appears in
cpu.c: In function 'fairsched_rate':
cpu.c:47:16: error: '__NR_fairsched_rate' undeclared (first use in this function)
cpu.c: In function 'fairsched_vcpus':
cpu.c:57:16: error: '__NR_fairsched_vcpus' undeclared (first use in this function)
make[2]: *** [cpu.lo] Error 1

and same error should occur on ARM and PPC64. Nothing critical, I've excluded vzctl in s390 build system now, the other secondary arch maintainers will do the same, but nice to have in the future I think.

Ok, I am working in a patch to it right now.

(In reply to comment #26)
> Ok, I am working in a patch to it right now.

Thanks, please ping us on #fedora-{arm,ppc,s390x} if you'll need access to the HW (no public s390x AFAIK).

vzctl-4.1-2.fc18 has been pushed to the Fedora 18 stable repository.

vzctl-4.1-2.fc17 has been pushed to the Fedora 17 stable repository.