Bug 638647 - Review Request: mom - Dynamically manage system resources on virtualization hosts
Summary: Review Request: mom - Dynamically manage system resources on virtualization ...
Keywords:
Status: CLOSED RAWHIDE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Rex Dieter
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2010-09-29 15:19 UTC by Adam Litke
Modified: 2014-06-19 20:52 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-04-19 12:34:55 UTC
Type: ---
rdieter: fedora-review+
kevin: fedora-cvs+


Attachments (Terms of Use)
Proposed spec cleanup (875 bytes, patch)
2010-10-26 20:29 UTC, Xavier Bachelot
no flags Details | Diff

Description Adam Litke 2010-09-29 15:19:58 UTC
Spec URL: http://github.com/downloads/aglitke/mom/mom.spec
SRPM URL: http://github.com/downloads/aglitke/mom/mom-0.2.1-1.fc13.src.rpm

Description: MOM is a policy-driven tool that can be used to manage overcommitment on KVM hosts. Using libvirt, MOM keeps track of active virtual machines on a host. At a regular collection interval, data is gathered about the host and guests. Data can come from multiple sources (eg. the /proc interface, libvirt API calls, a client program connected to a guest, etc). Once collected, the data is organized for use by the policy evaluation engine. When started, MOM accepts a user-supplied overcommitment policy. This policy is regularly evaluated using the latest collected data. In response to certain conditions, the policy may trigger reconfiguration of the system’s overcommitment mechanisms. Currently MOM supports control of memory ballooning and KSM but the architecture is designed to accommodate new mechanisms such as cgroups.

For more information see the slides and video from KVM Forum 2010 here:
Slides: http://www.linux-kvm.org/wiki/images/e/e8/2010-forum-litke-kvmforum2010.pdf
Video: http://vimeo.com/15223652

Comment 1 Adam Litke 2010-10-11 17:34:47 UTC
It doesn't look like there has been any movement on this review request since I submitted it.  Have I missed some required data or made some other mistake?

Comment 2 Xavier Bachelot 2010-10-11 22:37:44 UTC
A couple comments :
- in file section, %(_bindir)/usr/sbin/momd should be %{_sbindir}/momd
- files in /usr/share/doc/mom/examples should rather be installed in %{_docdir}/%{name}-%{version} and thus you won't need to specify it as %doc
- %doc is missing the COPYING file

trimmed rpmlint output :
mom.src:4: W: mixed-use-of-spaces-and-tabs (spaces: line 4, tab: line 4)
mom.x86_64: E: no-binary
mom.x86_64: E: non-executable-script /usr/lib/python2.6/site-packages/mom/Collectors/GuestNetworkDaemon.py 0644L /usr/bin/env
mom.x86_64: W: service-default-enabled /etc/rc.d/init.d/momd
mom.x86_64: E: malformed-line-in-lsb-comment-block # system resources
mom.x86_64: W: incoherent-subsys /etc/rc.d/init.d/momd $prog
mom.x86_64: W: service-default-enabled /etc/rc.d/init.d/momd
mom-debuginfo.x86_64: E: empty-debuginfo-package


- please fix space/tabs issue
- package should be noarch
- service shouldn't be enabled by default
- remove broken LSB line in initscript


Also, if this is your first package, you need to be sponsored and thus this bug needs to block FE-NEEDSPONSOR.

Comment 3 Xavier Bachelot 2010-10-11 22:40:39 UTC
oh and also, missing release tag in changelog. 0.2.1 should be 0.2.1-1

Comment 4 Adam Litke 2010-10-13 19:47:13 UTC
Thanks for your review Xavier.  I believe I have properly addressed all of your concerns.  Please find the updated files here:

http://github.com/downloads/aglitke/mom/mom.spec
http://github.com/downloads/aglitke/mom/mom-0.2.1-2.fc13.src.rpm

Comment 5 Xavier Bachelot 2010-10-14 09:04:16 UTC
I'm not a sponsor, so I won't be able to approve the package, but here
are a couple more comments.

- I'd rather see the patch split into 2 patches, one for the initscript
and one for the spec. Actually, the spec part is useless, as the bundled
spec file is not used.
- The initscript part of the patch turns 2 lines into 1 which then will
span over 80 characters, please fix.
- Patches need to have an upstream reference (bug/commit/...).This is
intended to help keep track of the patches. As you're upstream and this
will probably make it in the leext release, this is not a big deal, but
I thought I'd mention it.
- The %build and %install section are not following the template. The build part should not be done from within the %install section. See /etc/rpmdevtools/spectemplate-python.spec.
- The install of the initscript could be made shorter :
install -Dp contrib/momd.init $RPM_BUILD_ROOT/%_initddir/momd
- No need to copy the LICENSE and README files to the docdir, just using
%doc LICENSE README in the %files section will be enough.
- The INSTALL file doesn't contain any useful content, it's not needed
to include it currently. This might be changed later if/when its content
evolves.
- Use macros everywhere possible. When moving the examples, replace
/usr/share/doc by %{_defaultdocdir}.
- It might be better to alter setup.py to install the doc to the proper
location rather than move them after install.
- Be consistent with macros. You use both %_initddir and
%{_defaultdocdir}. Choose one style and stick with it. The preferred
style is usually %{_sbindir}.
- No need for %{_defaultdocdir}/%{name}-%{version} in the %files
section, this will automatically be picked up.
- Adding a blank line between changelog entries improve readability.
- BuildRoot and %clean are not needed anymore with recent Fedora
releases. However, this is still needed for EL5, so keep it if you want
to also have EPEL branches. In this case, you'll also want to replace
%{_initddir} by %{_initrddir}.
- There are some useless comments in the spec (first line and in %post).
- Please consider providing a default/dummy conf file and mark it as
%config(noreplace) in the %files section.
- You might want to use %{version} and possibly %{name] macros in the
Source0 tag, so you don't have to change the version in 2 places when
upgrading to a later version.

Comment 6 Adam Litke 2010-10-26 17:01:15 UTC
Thank you for your review.  The latest files can be found below:

http://github.com/downloads/aglitke/mom/mom.spec
http://github.com/downloads/aglitke/mom/mom-0.2.1-3.fc13.src.rpm

In reply to comment #5)
> I'm not a sponsor, so I won't be able to approve the package, but here
> are a couple more comments.
> 
> - I'd rather see the patch split into 2 patches, one for the initscript
> and one for the spec. Actually, the spec part is useless, as the bundled
> spec file is not used.

Done.  I want to keep the spec file patch so I can track the changes that need to be committed to the MOM tree once this review process is complete.

> - The initscript part of the patch turns 2 lines into 1 which then will
> span over 80 characters, please fix.

Done.

> - Patches need to have an upstream reference (bug/commit/...).This is
> intended to help keep track of the patches. As you're upstream and this
> will probably make it in the leext release, this is not a big deal, but
> I thought I'd mention it.

Thanks.  These patches are a convenience for me so I don't have to keep changing upstream for each iteration of this review.

> - The %build and %install section are not following the template. The build
> part should not be done from within the %install section. See
> /etc/rpmdevtools/spectemplate-python.spec.

Fixed.

> - The install of the initscript could be made shorter :
> install -Dp contrib/momd.init $RPM_BUILD_ROOT/%_initddir/momd

Ok.  Thanks.

> - No need to copy the LICENSE and README files to the docdir, just using
> %doc LICENSE README in the %files section will be enough.

Got it.

> - The INSTALL file doesn't contain any useful content, it's not needed
> to include it currently. This might be changed later if/when its content
> evolves.

Are you referring to the README file?  I was hoping I could leave it included since I will be updating it soon.  I don't like the idea of simply deleting an installed file.

> - Use macros everywhere possible. When moving the examples, replace
> /usr/share/doc by %{_defaultdocdir}.

This should be consistent now.

> - It might be better to alter setup.py to install the doc to the proper
> location rather than move them after install.

Will look into this for a future upstream release.

> - BuildRoot and %clean are not needed anymore with recent Fedora
> releases. However, this is still needed for EL5, so keep it if you want
> to also have EPEL branches. In this case, you'll also want to replace
> %{_initddir} by %{_initrddir}.

I chose to go the backward-compatible route.

> - There are some useless comments in the spec (first line and in %post).

Gone now.

> - Please consider providing a default/dummy conf file and mark it as
> %config(noreplace) in the %files section.

I now duplicate one of the example config files and install it as /etc/momd.conf (which agrees with the init script).

> - You might want to use %{version} and possibly %{name] macros in the
> Source0 tag, so you don't have to change the version in 2 places when
> upgrading to a later version.

Agreed.

Comment 7 Xavier Bachelot 2010-10-26 20:28:14 UTC
Thanks for answering my comments, explanations are fine to me. You're almost there, I expect this to be the last round of comments before the package is ready for a formal review.

rpmlint output :

$ rpmlint mom-0.2.1-3.fc13.noarch.rpm
mom.noarch: W: spelling-error Summary(en_US) virtualization -> actualization, visualization, conceptualization
mom.noarch: W: spelling-error %description -l en_US eg -> Eg, eh, e
mom.noarch: W: spelling-error %description -l en_US cgroups -> groups, c groups, Citigroup
mom.noarch: E: non-executable-script /usr/lib/python2.6/site-packages/mom/Collectors/GuestNetworkDaemon.py 0644L /usr/bin/env
mom.noarch: W: no-manual-page-for-binary momd
mom.noarch: W: incoherent-subsys /etc/rc.d/init.d/momd $prog
1 packages and 0 specfiles checked; 1 errors, 5 warnings.

You can ignore spelling-error.
non-executable-script needs to be fixed, remove the useless shebang. See patch.
no-manual-page-for-binary can be fixed by adding a man page if you wish, but that's certainly not a blocker, feel free to ignore.
incoherent-subsys is a false positive, ignore.

Also, you've misunderstood me about the example conf files. I would still move them to %{_defaultdocdir}/%{name}-%{version} and remove %{_defaultdocdir}/%{name}/examples/*. As is, it would left %{_defaultdocdir}/%{name}/examples unowned anyway. See patch.

There's a typo in the %files section for the conf file. See patch.

Last thing, patches names are not correct. You should suffix them with the name of the package rather than fedora. Less chances to collide with other patches name.

Comment 8 Xavier Bachelot 2010-10-26 20:29:04 UTC
Created attachment 455873 [details]
Proposed spec cleanup

Comment 9 Adam Litke 2010-10-26 21:53:26 UTC
Hi Xavier.  Here is the latest version including some of your updates.  But I noticed an issue with the documentation files that required me to change things a little bit:

The %doc macro erases the directory $RPM_BUILD_ROOT/%{_defaultdocdir}/%{name}-%{version} before copying README and LICENSE there so I end up losing the example files that I copy to that directory as part of the install.  Also, %doc does not appear to support a directory hierarchy so I cannot have an examples subdirectory.

I think the solution is to abandon %doc and specify the documentation files manually.  This idea is represented in this round of files:

http://github.com/downloads/aglitke/mom/mom.spec
http://github.com/downloads/aglitke/mom/mom-0.2.1-4.fc13.src.rpm

Comment 10 Xavier Bachelot 2010-10-26 23:41:32 UTC
Sorry for the misleading comments with the docs, I should obviously have tested that. Your solution and the overall package looks good to me. You just need a sponsor, the formal review should be quick.
Now might be a good time to read https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group if you haven't already.
I'd be happy to help with other reviews if you have other packages to submit.

Comment 11 Adam Litke 2010-10-27 14:45:43 UTC
Xavier, thanks a lot for your help!  I will find a sponsor now.

Comment 12 Daniel Veillard 2010-11-18 16:20:56 UTC
Following the Licence check, the thorough reviews by Xavier and
the fact that mom would be an interesting addition to the virtualization
tools of Fedora, I would like to sponsor that package.
However I'm getting completely lost on the documentation on how to
sponsor a new contributor

"Once they've made those changes, you can approve the package and click the appropriate link to sponsor them for access."

I don't see how to "approve" a package from bugzilla, or how to click
the  "appropriate" link that I just can't see,

http://fedoraproject.org/wiki/How_to_sponsor_a_new_contributor#Sponsoring_Someone_for_Fedora_Package_Collection

Daniel

Comment 13 Daniel Veillard 2010-11-18 16:33:28 UTC
Okay, I'm not in the sponsor list, someone from
https://admin.fedoraproject.org/accounts/group/members/packager/*/sponsor?_csrf_token=54a82e5ec35198ab07b106ffa5a098e34ab720b1

need to approve you, I can't,

Daniel

Comment 14 Xavier Bachelot 2010-11-18 16:39:34 UTC
Daniel, take care not to post your _csrf_token value or your session might be stolen.

Comment 15 Adam Litke 2011-01-05 16:18:49 UTC
New Package SCM Request
=======================
Package Name: mom
Short Description: Dynamically manage system resources on virtualization hosts
Owners: aglitke
Branches: devel
InitialCC:

Comment 16 BJ Dierkes 2011-01-06 19:30:14 UTC
It is too early to submit for a SCM module.  You must first find a sponsor (as this is your first package) and the sponsor will perform a formal review before you can make an SCM request.

Comment 17 Adam Litke 2011-01-06 19:47:19 UTC
BJ, I have already been sponsored by Richard Jones (rjones).  Is there a step that he omitted in approving this package?

Comment 18 BJ Dierkes 2011-01-06 20:14:08 UTC
I'm sorry, but I do not see any reference to 'rjones' in this tracker.  If he agreed to be your sponsor, he needs to formally take this review (set the fedora-review flag to '?'), and perform the review until it passes.  At that time he would set the fedora-review flag to '+' meaning it has passed review.

Please ask Richard Jones to visit this ticket, and he can take over from there.

Comment 19 Richard W.M. Jones 2011-01-06 20:18:01 UTC
I'm sponsoring Adam, but I wasn't aware I had to review the package
as well.

However this package is still missing a review, and consequently
there is no fedora-review+ flag.  Xavier can you complete this
review?

Comment 20 BJ Dierkes 2011-01-06 20:47:02 UTC
Ah, oops.  My mistake, I was under the impression that the sponsor did the review as well ... I guess I just assumed that because my sponsor did.  I apologize for the bad information.

Comment 21 Michael Schwendt 2011-01-07 09:59:01 UTC
Richard, you must be the reviewer if you want to be the sponsor:

https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Get_Sponsored

| Review and approval for the first package for new packagers
| must be done by registered sponsors. Subsequent reviews can be done
| by any package maintainer. Informal reviews can always be done by
| anyone interested.

https://fedoraproject.org/wiki/How_to_sponsor_a_new_contributor
https://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group

[...]

> %files
> ...
> %{_defaultdocdir}/%{name}-%{version}/*

Notice that directory %{_defaultdocdir}/%{name}-%{version} is not included:
https://fedoraproject.org/wiki/Packaging:UnownedDirectories

Better replace the line with just:
%{_defaultdocdir}/%{name}-%{version}/

which includes the directory and its contents. The trailing slash is just for readability (to make more clear that the entry is for a directory).

Also be aware that explicitly including files in %{_defaultdocdir}/%{name}-%{version} conflicts with using %doc to include documentation files. As it's a common pitfall for packagers, I recommend you add a comment to the %files section that %doc MUST NOT be used to include further documentation files as long as %{_defaultdocdir}/%{name}-%{version} is used. With recent distribution releases there will be an error. With older ones, you would kill files without any error/warning.


> Requires:       libvirt, libvirt-python

Even if it may be obvious to insiders, please add a comment here that explains why these two Explicit Requires are necessary:
https://fedoraproject.org/wiki/Packaging:Guidelines#Explicit_Requires

Comment 22 Adam Litke 2011-01-07 16:06:54 UTC
Michael, thanks for your comments.  I have updated the spec file and srpm to incorporate your suggestions.

https://github.com/downloads/aglitke/mom/mom.spec
https://github.com/downloads/aglitke/mom/mom-0.2.1-5.fc14.src.rpm

Comment 23 Adam Litke 2011-02-02 20:53:47 UTC
Could anyone help out with the official review for this package?  Thanks!

Comment 24 Xavier Bachelot 2011-02-02 21:16:32 UTC
The status of this bug is a bit unclear to me.
First are you sponsored now Adam ? If yes, then the FE-NEEDSPONSOR can be cleared and any sponsored packager can do the formal review. If not, then a sponsor needs to sponsor you and do the review, which leads to the second unclear point.
Afaiu, Richard offered to sponsor you, but declined to do the review, which is somewhat against the rule. Richard, are you willing to sponsor Adam and do the review ? If not, then please clear the review flag so another sponsor can take over.

Comment 25 Adam Litke 2011-02-02 21:30:47 UTC
Thanks Xavier.  I think you have interpreted the situation correctly.

Comment 26 Richard W.M. Jones 2011-02-02 21:31:50 UTC
Review and sponsorship are completely different things, but in
any case I cannot possibly do a review in the next month.

Comment 27 Adam Litke 2011-02-23 15:49:49 UTC
Hi everyone.

I really want to move this process forward and, since I also know everyone is very busy, I am prepared to sweeten the deal for any person willing to help me get this package into Fedora.  In addition to writing software, I also enjoy making beer.  So, following OSS traditions, I am offering a 6-pack of quality home-brewed beer (you pick the style) shipped to the US address of the Fedora Packager who helps me complete the New Package Process for Memory Overcommitment Manager.

Comment 28 Sebastien Pasche 2011-04-09 09:06:00 UTC
Hello,

First of all, I appologie about my possible not perfect argumentation and my English skills which are even less than that ;-).

I would like to say why I think MOM (Not MOM directly, but the problematics MOM is dealing with) is very important to Fedora and very important for futures and moderns virtualization systems.

Actually there is simply not a lot of real solutions to deal efficiently with memory overcommitment. Ballooning and KSM truly working great independently, but together they are sources of many problems and configuration's complications.

There is a real need of memory overcommitment management systems and MOM is maybe not the best one, but it's working well and is available.

As fedora say in its values "Features First" and (I think) as a "new technologies innovator" being able to purpose to its user an "advanced and efficient virtualization environment" is a necessary objective. A tool like MOM and in a more general way, a working solution to the issues of memory overcommitment in virtualization can be a real and great advantages to Fedora.

Another point, today we are dealing with the "green" and the green by one of its definitions mean to use hardware efficiently and to be equipped in a well-balanced manner compared to its need. In this case, be able to deal with memory will help running as many virtual machines as possible (and keeping them working properly, qa, etc.) on a single system.

Of course there is also the question of the CPU overcommitment (which tools like niceload or simply nice gives basics workaround, but they are realy not perfect to solve that kinds of problems) still need to be solved.

And for all this reason if someone would like to help this project to grown by giving it a chance to be in the fedora world I would be truly grateful to him.

A MOM satisfied user

Seb

Comment 29 MERCIER Jonathan 2011-04-18 10:49:40 UTC
I will take a look soon thanks for your works

Comment 30 Xavier Bachelot 2011-04-18 12:01:58 UTC
(In reply to comment #29)
> I will take a look soon thanks for your works

Jonathan, are you a sponsor ? It seems not and this bug blocks FE-NEEDSPONSOR.

Comment 31 Sebastien Pasche 2011-10-15 07:26:09 UTC
Any news ?

...

Comment 32 Xavier Bachelot 2011-10-15 10:47:13 UTC
I guess until a sponsor takes the review, it will not move, unless Adam has been sponsored meanwhile. Anyway, the status of this review is rather unclear, and as it is now, I feel sorry for Adam to be trapped here for so long, so I'm un-assigning it and resetting the review flag, so someone can take the review for good. Indeed, if I'm mistaken, feel free to revert my changes.

Comment 33 Rex Dieter 2011-10-15 13:21:59 UTC
I'd be happy to officially sponsor/review.

As an aside, IMO, the "sponsor has to be the initial reviewer" should be more a guideline than a hard-and-fast rule, but meh...

Comment 34 Rex Dieter 2011-10-15 13:40:22 UTC
thanks to all the pre-reviews...

naming: ok
license: ok
macros: ok
scriptlets: ok

SHOULD: for f16+ anyway, would be nice to include native systemd support rather than legacy init script, see also:
https://fedoraproject.org/wiki/Packaging:Systemd

otherwise, fairly simple and clean, APPROVED.


Wrt sponsorship, looks like you've already been included in packager group (sponsor rjones ), removing blocker.  feel free to poke either of us if you have questions or need help with anything.

Comment 35 Adam Litke 2011-10-17 13:06:45 UTC
Thanks guys for helping me to move this along.  I'll take a look at systemd and work to get something going in the next few days.

Comment 36 Rex Dieter 2011-10-17 13:31:55 UTC
Please don't consider the systemd thing to be a review blocker.  imo, move along with the process, and get this imported into fedora git (then, work on improving things).

Comment 37 Adam Litke 2011-10-25 15:46:35 UTC
New Package SCM Request
=======================
Package Name: mom
Short Description: Dynamically manage system resources on virtualization hosts
Owners: aglitke
Branches: f15 f16 f17
InitialCC:

Comment 38 Gwyn Ciesla 2011-10-25 15:56:31 UTC
Git done (by process-git-requests).

Removed f17, as f17==devel at the moment.

Comment 39 Adam Litke 2012-01-12 23:55:54 UTC
Package Change Request
======================
Package Name: mom
New Branches: f17
Owners: aglitke
InitialCC:

Comment 40 Gwyn Ciesla 2012-01-13 01:26:53 UTC
f17==devel.  We've not branched yet.

Comment 41 Adam Litke 2012-01-16 19:12:52 UTC
Package Change Request
======================
Package Name: mom
New Branches: el6
Owners: aglitke
InitialCC:

Comment 42 Gwyn Ciesla 2012-01-17 01:11:21 UTC
Git done (by process-git-requests).

Comment 43 Rex Dieter 2012-04-19 12:34:55 UTC
$ koji latest-pkg f18 mom
Build                                     Tag                   Built by
----------------------------------------  --------------------  ----------------
mom-0.2.2-1.fc17                          f17                   aglitke

looks like we can close this out.

Comment 44 Adam Litke 2012-12-12 16:48:28 UTC
Package Change Request
======================
Package Name: mom
New Branches: el7
Owners: aglitke
InitialCC:

Comment 45 Gwyn Ciesla 2012-12-12 16:55:14 UTC
There is no EL-7 at the moment.

Comment 46 Adam Litke 2014-06-18 18:51:03 UTC
Package Change Request
======================
Package Name: mom
New Branches: el7
Owners: aglitke
InitialCC:

Comment 47 Kevin Fenzi 2014-06-19 20:52:03 UTC
Git done (by process-git-requests).


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