This service will be undergoing maintenance at 00:00 UTC, 2016-08-01. It is expected to last about 1 hours
Bug 828879 - Review Request: system-storage-manager - A single tool to manage your storage
Review Request: system-storage-manager - A single tool to manage your storage
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Eric Sandeen
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-05 10:19 EDT by Lukáš Czerner
Modified: 2012-09-17 19:58 EDT (History)
5 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-09-17 19:58:19 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
esandeen: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Lukáš Czerner 2012-06-05 10:19:41 EDT
Spec URL: http://people.redhat.com/lczerner/files/system-storage-manager.spec
SRPM URL: http://people.redhat.com/lczerner/files/system-storage-manager-0.2-1.fc16.src.rpm

Description: 
System Storage Manager provides easy to use command line interface to manage
your storage using various technologies like lvm, btrfs, encrypted volumes and
more.

In more sophisticated enterprise storage environments, management with Device
Mapper (dm), Logical Volume Manager (LVM), or Multiple Devices (md) is becoming
increasingly more difficult. With file systems added to the mix, the number of
tools needed to configure and manage storage has grown so large that it is
simply not user friendly. With so many options for a system administrator to
consider, the opportunity for errors and problems is large.

The btrfs administration tools have shown us that storage management can be
simplified, and we are working to bring that ease of use to Linux filesystems
in general.

You should install the ssm if you need to manage your storage with various
technologies via single unified interface.

Fedora Account System Username: lczerner

This is my first package and I need a sponsor.

Thanks!
-Lukas
Comment 1 Fabian Affolter 2012-06-06 07:41:27 EDT
Just some quick comments:

- Please check the guidelines for the python macros
  https://fedoraproject.org/wiki/Packaging:Python#Macros
- A simple comment in %build (like #nothing to build) to calm rpmlint down is sufficient.
- Python 2.7.x was introduced around F14/F15. Maybe drop the Python 2.6.x stuff.
- %clean and BuildRoot tag can be dropped if the packages is not going to EPEL5. If it's going to EPEL5 then you have to add %defattr to the %files section.
Comment 2 Lukáš Czerner 2012-06-06 11:46:18 EDT
Hi Fabian,

thanks for the comments. Here is what I've updated in the spec file.

 - removed custom python macros and now using python_sitelib
 - using __python macro to invoke python interpreter
 - removed echo from %build. Added comment (# nothing to build)
 - removed %if "%{py_version}" == "2.6" condition
 - removed %clean and BuildRoot

Thanks!
-Lukas
Comment 3 Lukáš Czerner 2012-06-14 04:29:57 EDT
Hello,

is there anything I can do to push this forward ?

Thanks!
-Lukas
Comment 4 Eric Sandeen 2012-06-15 12:48:53 EDT
I'll take it for formal review.
Comment 5 Dan Horák 2012-06-15 14:11:36 EDT
(In reply to comment #4)
> I'll take it for formal review.

Eric, please continue with the review, I will sponsor Lukas afterwards.
Comment 6 Eric Sandeen 2012-06-20 10:38:54 EDT
Thanks Dan.  Finally getting the review going today.
Comment 7 Eric Sandeen 2012-06-20 10:53:57 EDT
Ok, editorial nitpicks:

> System Storage Manager provides easy to use command line interface to manage

"provides an easy to use ..."

> technologies via single unified interface.

"technologies via a single unified interface."

> You should install the ssm if you need

Should probably say "You should install system-storage-manager if you need" ...

And maybe 2 spaces (vs 1) after every sentence in the %description?

Now I'll go point by point through review guidelines to make sure I'm not missing anything.  Pretty simple specfile though!
Comment 8 Eric Sandeen 2012-06-20 11:46:19 EDT
All this is OK (the MUSTS):

rpmlint is amazingly quiet ;)
package naming is fine
specfile name matches
AFAICT, it follows python packaging rules, but I am no expert there!
License (GPLv2+) is acceptable for Fedora.
License matches license statements in source
COPYING file contains license and is in %doc
Specfile is legible and in American e=English
Source matches upstream
Upstream source URL is fine
package built fine: http://koji.fedoraproject.org/koji/taskinfo?taskID=4180107
RPM does own the two dirs it creates
File perms seem fine
Macros are consistently used in the specfile
Package contains permissible content (code)
Doc files work properly with --excludedocs; there is no %doc directive but %{_docdir} seems to imply that.
No static or devel libraries, so no issues there.


Questions / suggestions:

For runtime requires, I wonder about the fs utilities it invokes.  If it handles it gracefully when they are missing, probably no need for a runtime requirement,  but if not, perhaps you should require those packages at least for now?

I'll try to grep through & see what all might get executed.
Comment 9 Eric Sandeen 2012-06-20 11:56:34 EDT
Ok, just looking at "command = " lines, we have these things being executed as commands (this is a little crude, not all are actually system commands I think)

blkid
btrfs
cryptsetup
device
dmsetup
filesystem
fsck.{0}
lvcreate
lvm
lvremove
lvresize
mkfs.{0}
mkfs.btrfs
mount
pvremove
remove
resize
resize2fs
subvolume
tune2fs
umount
vgcreate
vgextend
vgreduce
vgremove
which
wipefs
xfs_check
xfs_db
xfs_growfs

which seems to correspond to these packages:

cryptsetup-luks
device-mapper
e2fsprogs
lvm2
util-linux-ng
xfsprogs

Perhaps these should be explicitly listed as runtime requirements until you are sure that it can function reasonably without them in place?
Comment 10 Eric Sandeen 2012-06-20 11:57:02 EDT
Hm package list was from an older system, so s/util-linux-ng/util-linux/ at least.
Comment 11 Eric Sandeen 2012-06-20 12:23:41 EDT
And "which" (from the "which" package) is also invoked.

I'm not sure if things like util-linux & which must be listed as Requires; they are not needed for BuildRequires since they are included in the minimum build environment, but I can't find a similar exception list for Requires... will let you know.
Comment 12 Eric Sandeen 2012-06-20 12:27:37 EDT
Ok, I've confirmed that even which & util-linux should be listed as Requires:
Comment 13 Lukáš Czerner 2012-06-21 07:13:52 EDT
Hi Eric,

thank you very much for the review! I have updated description according your comments in Comment 7.

Regarding the commands used in the system storage manager:

cryptsetup-luks - is not required and if not present crypt backend will not be used. It has been designed this way.

device-mapper (dmsetup) - it is required only by crypt backend and again if it is not present, it will not be used.

lvm2 - it is not required and if not present lvm backend will not be used. It has been designed this way.

btrfs-progs - it is not required and if not present btrfs backend will not be used. It has been designed this way.

util-linux - it already is required in the spec file

which - This have to be added into the required packages. Thanks for pointing this out.

xfsprogs
e2fsprogs
   - Unfortunately I've completely forgot about those and the system storage manager will not gracefully handle the situation when the file system tools are missing. We are already working on a patch, however it might make more sense to just require those two packages since it will probably cover most of the usual setups anyway. I am going to add those packages to required for now.


Spec URL: http://people.redhat.com/lczerner/files/system-storage-manager.spec
SRPM URL: http://people.redhat.com/lczerner/files/system-storage-manager-0.2-1.fc16.src.rpm


Thanks Eric!
-Lukas
Comment 14 Eric Sandeen 2012-06-27 10:48:03 EDT
Thanks for the updates Lukas, looks good to me.
Comment 15 Lukáš Czerner 2012-08-07 09:52:15 EDT
New Package SCM Request
=======================
Package Name: system-storage-manager
Short Description: A single tool to manage your storage
Owners: lczerner
Branches: 
InitialCC:
Comment 16 Jon Ciesla 2012-08-07 09:58:13 EDT
"lczerner" is not in the packager group.
Comment 17 Lukáš Czerner 2012-08-07 10:11:26 EDT
Eric,

as my sponsor could you add me into the packager group ?

Thanks!
-Lukas
Comment 18 Jon Ciesla 2012-08-08 08:23:16 EDT
Git done (by process-git-requests).
Comment 19 Lukáš Czerner 2012-08-15 10:17:10 EDT
Package Change Request
======================
Package Name: system-storage-manager
New Branches: f18
Owners: lczerner
Comment 20 Lukáš Czerner 2012-08-15 10:19:01 EDT
(In reply to comment #19)
> Package Change Request
> ======================
> Package Name: system-storage-manager
> New Branches: f18
> Owners: lczerner

I need the f18 branch to request the bodhi update to add this new package to the updates-testing.

Thanks!
-Lukas
Comment 21 Jon Ciesla 2012-08-15 11:06:26 EDT
Git done (by process-git-requests).
Comment 22 Fedora Update System 2012-08-15 11:39:44 EDT
system-storage-manager-0.2-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/system-storage-manager-0.2-1.fc18
Comment 23 Fedora Update System 2012-08-15 11:58:08 EDT
system-storage-manager-0.2-1.fc18 has been pushed to the Fedora 18 testing repository.
Comment 24 Fedora Update System 2012-09-17 19:58:19 EDT
system-storage-manager-0.2-1.fc18 has been pushed to the Fedora 18 stable repository.

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