Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
Summary: Review Request: vhostmd - Virtualization host metrics daemon
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Summary: Review Request: vhostmd - Virtualization host metrics daemon Product: Fedora Version: rawhide Platform: All OS/Version: Linux Status: NEW Severity: medium Priority: medium Component: Package Review AssignedTo: nobody@fedoraproject.org ReportedBy: rjones@redhat.com QAContact: extras-qa@fedoraproject.org CC: notting@redhat.com, fedora-package-review@redhat.com Estimated Hours: 0.0 Classification: Fedora Target Release: ---
Spec URL: http://www.annexia.org/tmp/vhostmd.spec SRPM URL: http://www.annexia.org/tmp/vhostmd-0.2-1.fc11.src.rpm Description: vhostmd provides a "metrics communication channel" between a host and its hosted virtual machines, allowing limited introspection of host resource usage from within virtual machines.
Koji scratch build: http://koji.fedoraproject.org/koji/taskinfo?taskID=1604955
rpmlint output: vhostmd.x86_64: W: conffile-without-noreplace-flag /etc/vhostmd/vhostmd.dtd vhostmd.x86_64: W: conffile-without-noreplace-flag /etc/vhostmd/metric.dtd (Probably the upstream package shouldn't be placing the DTD files in that location. In any case I don't think those files should be edited by the user).
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Russell Doty rdoty@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |rdoty@redhat.com Blocks| |511263(SAP5.5)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Matthew Booth mbooth@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |mbooth@redhat.com AssignedTo|nobody@fedoraproject.org |mbooth@redhat.com Flag| |fedora-review?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #1 from Matthew Booth mbooth@redhat.com 2009-10-07 12:11:27 EDT --- MUST items: *** rpmlint reports errors shown above [1] Package is named according to the package naming guidelines spec file name matches the base package name Package meets the packaging guidelines Package is licensed with a Fedora approved license License field in spec file matches actual license Spec file is written in American English Spec file is legible *** Upstream sources are not available [2] Package successfully builds on i686 and x86_64 *** Package uses ExclusiveArch [3] All build dependencies are listed in BuildRequires Upstream does not contain localisation ldconfig is called in %post and %postun for vm-dump-metrics Package does not bundle copies of system libraries Package is not relocatable Package owns all directories that it creates No files are listed more than once in %files Permissions on files are set properly Package has a %clean section containing rm -rf $RPM_BUILD_ROOT Package uses macros consistently Package contains only code Package does not include 'large documentation files' %doc files are not required for correct function Header files are in -devel package Static libraries are not built Package does not include pkgconfig files (but autoconf still uses pkgconfig) Bare .so file is in -devel pacakge -devel subpackage requires parent subpackage Package does not contain .la libtool archives Package does not contain a gui application Package does not own files or directories owned by other packages %install runs rm -rf $RPM_BUILD_ROOT All filenames are valid UTF-8
SHOULD items: Source package includes license text *** Description and summary sections do not contain translations (not blocker) Package builds in mock (see koji build in original submission) Package compiles and builds for supported architectures (but see [3]) *** Package caused system to become unstable requiring reboot [4] Scriptlets are sane Subpackage does not need to require base package in this case Package does not contain pkgconfig files Package does not have file dependencies
Misc: Did you consider any of the other approaches to removing the use of rpath in the spec file?
Notes: [1] If the 2 DTDs are genuinely not configuration files, they should be removed. I note that they're already in %doc, which seems appropriate.
[2] It's not clear how the tarball was obtained or can be reproduced exactly from source. I'm not able to verify this.
[3] I'm taking ExclusiveArch here to be semantically equivalent to ExcludeArch. I note that xen (which is a dependency), also has an ExclusiveArch. libvirt has a solution to this which involves not building against xen where it is not available. Is this possible in this package?
[4] Starting vhostmd on my F-11 host flooded /var/log/messages with: Oct 7 14:51:06 mbooth libvirtd: 14:51:06.616: error : qemudDomainLookupByID:3028 : Domain not found: no domain with matching id 0 Oct 7 14:51:06 mbooth libvirtd: 14:51:06.619: error : qemudDomainLookupByName:3080 : Domain not found: no domain with matching name '0' Oct 7 14:53:51 mbooth vhostmd: Error retrieving metric UsedMem Oct 7 14:53:53 mbooth vhostmd: Failed to collect vm metrics during update
and the following 2 AVC messages: Oct 7 14:42:33 mbooth kernel: type=1400 audit(1254922953.004:17101): avc: denied { read write } for pid=32590 comm="virsh" path="/dev/shm/vhostmd/disk0" dev=tmpfs ino=2946820 scontext=unconfined_u:system_r:xm_t:s0 tcontext=unconfined_u:object_r:tmpfs_t:s0 tclass=file Oct 7 14:42:33 mbooth kernel: type=1400 audit(1254922953.012:17102): avc: denied { getsched } for pid=32590 comm="virsh" scontext=unconfined_u:system_r:xm_t:s0 tcontext=unconfined_u:system_r:xm_t:s0 tclass=process
The system became sluggish after only a few seconds, and unresponsive requiring a reboot shortly after that. This may be a problem with SELinux policy or the default configuration file. Either way, I'd be uncomfortable shipping the package with this as the default behaviour. Is this still a problem on rawhide? I didn't test there.
-----
Rich,
Can you respond to Misc and 4 Notes above first?
Matt
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Peter Lemenkov lemenkov@gmail.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #2 from Richard W.M. Jones rjones@redhat.com 2009-10-12 09:26:47 EDT --- rpaths: No, I just used chrpath. Fedora recommends passing "--disable-rpath" to configure, but AFAIK that is not a standard configure option, and in any case I checked and it's definitely not supported by vhostmd's upstream configure.ac.
Note [4]:
The out of memory issue was a memory leak in libvirtd (bug 528162).
"Domain not found: no domain with matching id 0" is a warning with the default configuration file supplied by upstream. I'll supply an alternative configuration which should work better with KVM.
"Error retrieving metric UsedMem" - idem.
AVCs: We need an selinux policy update to cope with this, but we can't do that until the package is in Fedora.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #3 from Matthew Booth mbooth@redhat.com 2009-10-12 09:44:38 EDT --- rpaths: OK.
Note 4: I'll take your word that this is fixed :) There must be a way to engage the SELinux folks early on this, but I don't see that this should be a blocker.
Any response to notes 1-3?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #4 from Richard W.M. Jones rjones@redhat.com 2009-10-12 09:54:14 EDT --- Thanks for looking at this.
[1] DTDs: These aren't configuration files, but vhostmd.dtd at least is required and has to stay at that location because it is used to validate the XML configuration file. Stupid stupid stupid.
[2] Tarball is now packaged upstream, but the URL is still unusable. This is because gitorious doesn't really know how to host tarballs properly. Upstream is looking for alternate hosting.
[3] Change made as suggested.
Spec URL: http://www.annexia.org/tmp/vhostmd.spec SRPM URL: http://www.annexia.org/tmp/vhostmd-0.3-3.fc11.src.rpm
* Mon Oct 12 2009 Richard W.M. Jones rjones@redhat.com - 0.3-3 - Remove metric.dtd file from /etc (fixes rpmlint warning), but vhostmd.dtd has to remain because it is needed to validate the XML configuration file. - Remove ExclusiveArch, instead conditionally depend on xen-devel. - Use a better, less noisy, more minimal configuration file which doesn't depend on Xen.
* Thu Oct 8 2009 Richard W.M. Jones rjones@redhat.com - 0.3-1 - New upstream version 0.3.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #5 from Matthew Booth mbooth@redhat.com 2009-10-12 11:15:37 EDT --- [1] DTD: OK.
[2] Tarball: I appreciate the upstream hosting issue. It still looks odd, though. The srpm contains vhostmd-vhostmd-0.3.tar.gz (note the repeated vhostmd-). make dist on the source creates the expected vhostmd-0.3.tar.gz. Is this intentional?
Also, there are no tags in the git repo, but make dist on what appears to be the release commit creates a tarball with a different md5, at least on my box.
[3] Unfortunately this now fails to build on ppc:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1742110
You may need some additional configure magic when xen-devel isn't available.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #6 from Richard W.M. Jones rjones@redhat.com 2009-10-12 12:20:20 EDT --- (In reply to comment #5)
[1] DTD: OK.
[2] Tarball: I appreciate the upstream hosting issue. It still looks odd, though. The srpm contains vhostmd-vhostmd-0.3.tar.gz (note the repeated vhostmd-). make dist on the source creates the expected vhostmd-0.3.tar.gz. Is this intentional?
Yeah, this is all fscked up. The upstream issue is being discussed here:
http://lists.opensuse.org/vhostmd/2009-10/msg00007.html
Also, there are no tags in the git repo, but make dist on what appears to be the release commit creates a tarball with a different md5, at least on my box.
Ditto above. It generates a new tarball each time :-(
[3] Unfortunately this now fails to build on ppc:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1742110
You may need some additional configure magic when xen-devel isn't available.
OK I get this now. It's actually a feature of vhostmd that I wasn't aware of up to now. As well as using the metrics disk to export data, it can also export it through xenstore.
Unfortunately the current code doesn't allow that to be configured out at compile time (and thus always requires the Xenstore client library to be available).
I think the best thing here is to add back the ExclusiveArch temporarily until I can get it fixed upstream.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #7 from Matthew Booth mbooth@redhat.com 2009-10-13 05:51:07 EDT --- (In reply to comment #6)
[2] Tarball: I appreciate the upstream hosting issue. It still looks odd, though. The srpm contains vhostmd-vhostmd-0.3.tar.gz (note the repeated vhostmd-). make dist on the source creates the expected vhostmd-0.3.tar.gz. Is this intentional?
Yeah, this is all fscked up. The upstream issue is being discussed here:
Ok, it doesn't sound like the gitorious solution will fly. Can you do a 'make dist' on the source yourself and add a comment to the spec identifying the commit the dist is based on (presumably a92f35045ab8c7b88c714a44623219ffac3caea1 in this case). This should at least be verifiable and reproducible. Of course, this would be easier if upstream would add a tag to this commit, then you could just document the name of the tag.
[3] Unfortunately this now fails to build on ppc:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1742110
You may need some additional configure magic when xen-devel isn't available.
OK I get this now. It's actually a feature of vhostmd that I wasn't aware of up to now. As well as using the metrics disk to export data, it can also export it through xenstore.
Unfortunately the current code doesn't allow that to be configured out at compile time (and thus always requires the Xenstore client library to be available).
I think the best thing here is to add back the ExclusiveArch temporarily until I can get it fixed upstream.
Agreed.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #8 from Richard W.M. Jones rjones@redhat.com 2009-10-13 06:31:55 EDT --- So what I've done is to move to a pre-release of 0.4, which also includes the changes to remove the dependency on xenstore on non-Xen platforms.
Spec URL: http://www.annexia.org/tmp/vhostmd.spec SRPM URL: http://www.annexia.org/tmp/vhostmd-0.4-0.1.git326f0012172.fc11.src.rpm
* Tue Oct 13 2009 Richard W.M. Jones rjones@redhat.com - 0.4-0.1.git326f0012172 - Move to pre-release of 0.4, self-built tarball. - Disable xenstore on non-x86 platforms. - Add patch to fix --without-xenstore option.
Koji scratch build here:
http://koji.fedoraproject.org/koji/taskinfo?taskID=1743279
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #9 from Matthew Booth mbooth@redhat.com 2009-10-13 07:55:16 EDT --- Created an attachment (id=364576) --> (https://bugzilla.redhat.com/attachment.cgi?id=364576) %define have_xen to abstract architecture ugliness
Not a blocker. Would you consider the attached patch to the spec file which I think reads slightly better? It puts the architecture stuff at the top with a brief comment about why it's there.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Matthew Booth mbooth@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-review? |fedora-review+
--- Comment #10 from Matthew Booth mbooth@redhat.com 2009-10-13 08:02:11 EDT --- The only remaining issue is the lack of a released tarball. However, this is an upstream issue and I'm happy that the package does the best currently possible.
ACK.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #11 from Richard W.M. Jones rjones@redhat.com 2009-10-13 08:14:12 EDT --- Yes, I'll add that patch, thanks.
(Of course using %global, not %define !)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Richard W.M. Jones rjones@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag| |fedora-cvs?
--- Comment #12 from Richard W.M. Jones rjones@redhat.com 2009-10-13 08:15:55 EDT --- New Package CVS Request ======================= Package Name: vhostmd Short Description: Virtualization host metrics daemon Owners: Branches: F-12 EL-5 InitialCC:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Kevin Fenzi kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
--- Comment #13 from Kevin Fenzi kevin@tummy.com 2009-10-13 12:24:04 EDT --- I assume you meant rjones as owner? :)
cvs done.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #14 from Fedora Update System updates@fedoraproject.org 2009-10-13 14:14:22 EDT --- vhostmd-0.4-0.1.git326f0012172.fc12 has been submitted as an update for Fedora 12. http://admin.fedoraproject.org/updates/vhostmd-0.4-0.1.git326f0012172.fc12
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Daniel Walsh dwalsh@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |dwalsh@redhat.com
--- Comment #15 from Daniel Walsh dwalsh@redhat.com 2009-10-13 16:43:12 EDT --- Why are we letting in packages that break when SELinux is enforcing mode? The apps should be fixed or work out with the policy maintainers the fixes before the package gets accepted. Any package that forces the user of Fedora to be put in to permissive/disabled mode should not be accepted.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #16 from Matthew Booth mbooth@redhat.com 2009-10-14 05:31:31 EDT --- Dan,
I agree entirely (and made that point!). However, fixing it does seem to be a chicken/egg problem. What's the correct process here?
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Richard W.M. Jones rjones@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs+ |fedora-cvs?
--- Comment #17 from Richard W.M. Jones rjones@redhat.com 2009-10-14 05:51:18 EDT --- Sorry, someone just requested that this is built for F11 too :-(
Package Change Request ====================== Package Name: vhostmd New Branches: F-11 Owners:
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #18 from Daniel Walsh dwalsh@redhat.com 2009-10-14 09:02:50 EDT --- Matthew, I think you did the right thing in contacting me. I have asked Miroslav to begin writing policy for vhostmd. And I will put it in F12 and F11 when it is available.
I will talk to the Fedora team about making this a more formal part of the review.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Kevin Fenzi kevin@tummy.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Flag|fedora-cvs? |fedora-cvs+
--- Comment #19 from Kevin Fenzi kevin@tummy.com 2009-10-15 13:22:57 EDT --- cvs done.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Frank Danapfel fdanapfe@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- CC| |fdanapfe@redhat.com
--- Comment #20 from Frank Danapfel fdanapfe@redhat.com 2009-10-21 11:44:49 EDT --- A F11 test system with 2 VMs and the latest version of vhostmd was provided to SAP so that they can verify that vhostmd delivers the information they require.
SAP has confirmed that their tools are able to read the data in a VM and that vhostmd provides the correct data.
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
--- Comment #21 from Russell Doty rdoty@redhat.com 2009-11-05 15:42:38 EDT --- Frank Danapfel reports that the package has been tested and meets SAP's requirements.
Please proceed with building the package for RHEL 5.4 (and above) and release it into the SAP RHN channel.
Russell Doty
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Beth Zeranski bzeranski@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Blocks| |533941(5.5EPMFlatAll)
Please do not reply directly to this email. All additional comments should be made in the comments box of this bug.
https://bugzilla.redhat.com/show_bug.cgi?id=517488
Richard W.M. Jones rjones@redhat.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |CLOSED Resolution| |CURRENTRELEASE
--- Comment #23 from Richard W.M. Jones rjones@redhat.com 2010-06-08 08:54:18 EDT --- I noticed this bug should be closed, since vhostmd has been added to Fedora.
package-review@lists.fedoraproject.org
-
Red Hat Bugzilla