Bug 1272248 - Review Request: heketi - RESTful based volume management framework for GlusterFS
Review Request: heketi - RESTful based volume management framework for GlusterFS
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
medium Severity medium
: ---
: ---
Assigned To: Nobody's working on this, feel free to take it
Fedora Extras Quality Assurance
:
Depends On: 1269813 1271684 1271805 1272116
Blocks:
  Show dependency treegraph
 
Reported: 2015-10-15 17:14 EDT by Luis Pabón
Modified: 2016-11-08 17:24 EST (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2015-11-13 14:24:48 EST
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
jchaloup: fedora‑review+


Attachments (Terms of Use)
Update license and %files section (1.67 KB, patch)
2015-10-16 06:35 EDT, Jan Chaloupka
no flags Details | Diff
update spec file (3.80 KB, patch)
2015-10-16 08:15 EDT, Jan Chaloupka
no flags Details | Diff
patch Makefile (2.35 KB, patch)
2015-10-16 08:15 EDT, Jan Chaloupka
no flags Details | Diff
update spec file (3.67 KB, patch)
2015-10-16 08:22 EDT, Jan Chaloupka
no flags Details | Diff

  None (edit)
Description Luis Pabón 2015-10-15 17:14:10 EDT
Spec URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi.spec

SRPM URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi-1.0.0-1.fc22.src.rpm

Description: Heketi provides a RESTful management interface which can be used to manage
the life cycle of GlusterFS volumes.  With Heketi, cloud services like
OpenStack Manila, Kubernetes, and OpenShift can dynamically provision
GlusterFS volumes with any of the supported durability types.  Heketi
will automatically determine the location for bricks across the cluster,
making sure to place bricks and its replicas across different failure
domains.  Heketi also supports any number of GlusterFS clusters, allowing
cloud services to provide network file storage without being limited to a
single GlusterFS cluster.

Fedora Account System Username: lpabon

$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src:153: W: rpm-buildroot-usage %build export GOPATH=%{buildroot}/%{gopath}:%{gopath}
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.
Comment 1 Jan Chaloupka 2015-10-16 06:35 EDT
Created attachment 1083553 [details]
Update license and %files section
Comment 2 Jan Chaloupka 2015-10-16 08:15 EDT
Created attachment 1083620 [details]
update spec file
Comment 3 Jan Chaloupka 2015-10-16 08:15 EDT
Created attachment 1083621 [details]
patch Makefile
Comment 4 Jan Chaloupka 2015-10-16 08:22 EDT
Created attachment 1083638 [details]
update spec file
Comment 5 Jan Chaloupka 2015-10-16 08:25:37 EDT
Hi Luis,

spec files looks fine. Just some nits:
- for license, %license tag can be used when redefining the tag explicitly for el6. 
- for %{gopath}/src/%{import_path} directory, it is better to include it in devel.file-list file as the root directory could contain both source files and tests file. Thus resulting in multiple ownership of the directory.
- when building heketi from bundled deps, it fails to build due to missing heketi source codes
- when building with debuginfo, bundled build is missing BUILD_ID
Comment 6 Luis Pabón 2015-10-16 15:20:37 EDT
(In reply to Jan Chaloupka from comment #5)
> Hi Luis,
> 
> spec files looks fine. Just some nits:
> - for license, %license tag can be used when redefining the tag explicitly
> for el6. 
> - for %{gopath}/src/%{import_path} directory, it is better to include it in
> devel.file-list file as the root directory could contain both source files
> and tests file. Thus resulting in multiple ownership of the directory.
> - when building heketi from bundled deps, it fails to build due to missing
> heketi source codes
> - when building with debuginfo, bundled build is missing BUILD_ID

Hi Jan, Do you mind providing line numbers to the first two? Any suggestions on how to fix any of these issues are welcomed.  I used gofed, then based the rest of my changes on etcd.spec.
Comment 7 Luis Pabón 2015-10-17 16:06:27 EDT
Does with_bundled mean "with bundled dependencies"?  If it does, then since Heketi uses glock it does not support that and I probably should remove that switch. 

Is with_bundled needed for centos/rhel?
Comment 8 Jan Chaloupka 2015-10-19 04:49:37 EDT
Hi Luis,

all suggested changes are applied in "update spec file" patch. "patch Makefile" patches Makefile to build binaries with BUILD ID.

Jan
Comment 9 Jan Chaloupka 2015-10-19 04:51:01 EDT
> Is with_bundled needed for centos/rhel?

As RHEL does not build from deps, yes. The same holds for CentOS.
Comment 10 Luis Pabón 2015-10-19 16:35:12 EDT
Thanks for the help Jan. I have a new update available.  I did not need to change the Makefile, but thank you for the diff.  That helped create the appropriate changes to the spec file.

Here is the diff from previews request:
https://github.com/lpabon/heketi-rpm/compare/f3ea4d3c199a506651b00698709fd50475eeeac9...master

## Output from gofed

Spec URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi.spec

SRPM URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi-1.0.0-1.fc22.src.rpm

Description: Heketi provides a RESTful management interface which can be used to manage
the life cycle of GlusterFS volumes.  With Heketi, cloud services like
OpenStack Manila, Kubernetes, and OpenShift can dynamically provision
GlusterFS volumes with any of the supported durability types.  Heketi
will automatically determine the location for bricks across the cluster,
making sure to place bricks and its replicas across different failure
domains.  Heketi also supports any number of GlusterFS clusters, allowing
cloud services to provide network file storage without being limited to a
single GlusterFS cluster.

Fedora Account System Username: lpabon

$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.
Comment 11 Luis Pabón 2015-10-19 16:40:32 EDT
## Output from rpmlint from CentOS 7 using bundled mode:

$ rpmlint /home/lpabon/rpmbuild/SRPMS/heketi-1.0.0-1.el7.centos.src.rpm /home/lpabon/rpmbuild/RPMS/x86_64/heketi-1.0.0-1.el7.centos.x86_64.rpm /home/lpabon/rpmbuild/RPMS/noarch/heketi-devel-1.0.0-1.el7.centos.noarch.rpm /home/lpabon/rpmbuild/RPMS/x86_64/heketi-unit-test-devel-1.0.0-1.el7.centos.x86_64.rpm

heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: incoherent-version-in-changelog 1.0.0-1 ['1.0.0-1.el7.centos', '1.0.0-1.centos']
heketi.x86_64: W: unstripped-binary-or-object /usr/bin/heketi-cli
heketi.x86_64: W: unstripped-binary-or-object /usr/bin/heketi
heketi.x86_64: W: only-non-binary-in-usr-lib
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
4 packages and 0 specfiles checked; 0 errors, 9 warnings.
Comment 12 Jan Chaloupka 2015-10-20 09:45:53 EDT
Add) %if 0%{?with_check} && ! 0%{?with_bundled}

Nice! Applied in specfile generator, thanks.

Add) go test -v %{import_path}/apps/glusterfs

Purpose of %gotest macro is portability to secondary architectures where gcc-go is used instead of gc. On golang architectures, %gotest is defined as "go test ...". On gcc-go it is defined as "go test -compiler gcc-go ...". If you are targeting primary architectures, it is fine. For secondary, please use %gotest macro instead and define %gotest macro in %check section before the first use of %gotest as:

%if ! 0%{?gotest:1}
%global gotest go test
%endif

Otherwise the rest looks fine. Thanks Luis for your feedback on gofed. I have learnt new things from you.

Jan
Comment 13 Jan Chaloupka 2015-10-20 09:46:57 EDT
Actually, one more question: what is %{name}-godeps-%{shortcommit}.tar.gz used for? What does it contain?
Comment 14 Luis Pabón 2015-10-20 10:23:24 EDT
%{name}-godeps-%{shortcommit}.tar.gz  is a tar file containing all the dependencies, like Godeps for etcd.  The difference is that Heketi does not use Godeps, instead it uses glock so the repo does not contain the dependencies.  Instead I have provided the glock output as a tar file that is attached to the SRPM for bundled builds.  Is that ok?
Comment 15 Jan Chaloupka 2015-10-20 12:41:59 EDT
Would not it be better to use _vendor or vendor directory for it? And put it all into one tarball?
Comment 16 Jan Chaloupka 2015-10-20 12:58:25 EDT
One one hand creating vendor directory adds responsibility to update the directory at the same time you regenerate GLOCKFILE. On the other separating godeps from the project itself adds responsibility on maintainer of the package in Fedora not to forget to update the godeps.tar.gz file.

I would choose the former. It simplifies the spec file and it is common to provide such directory.

In both cases, if you build you package from bundled deps, you must provide a list of bundled dependencies [1]:

"All packages whose upstreams have no mechanism to build against system libraries may opt to carry bundled libraries, but if they do, they must include Provides: bundled(<libname>) = <version> in their RPM spec file."

If a file with dependencies is provided, it makes sense to replace <version> with a version which would be provided if the dependency would be packaged in Fedora. However, usually there is no version. Maybe something like %{version}-%{shortcommit} could be used instead. I have opened RFE for it [2]. What do you think about it, Luis?


[1] https://fedorahosted.org/fesco/ticket/1483#comment:17
[2] https://github.com/gofed/gofed/issues/42
Comment 17 Luis Pabón 2015-10-20 15:49:41 EDT
If I create a _vendor directory inside the heketi-{ver}.tar.gz file, wouldn't it have to be then created by someone other than the developer since the repo will not have this direstory?  Could gofed create the godeps directory and add it inside the tar file for those programs that do not include it in their repo and instead use glock?

What do you think about the following updates:
1. add bundled(...) = <version> as you describe in [2] above
2. Keep the godeps file as separate.  When gofed can look at a repo and notice the GLOCKFILE, it can then generate a godeps.tar.gz and place it inside the {repo}.tar.gz for bundle builds.

or 

2. Setup godeps as a _vendor directory inside the heketi-{ver}.tar.gz file

What do you think?
Comment 18 Jan Chaloupka 2015-10-21 06:09:09 EDT
> If I create a _vendor directory inside the heketi-{ver}.tar.gz file, wouldn't
> it have to be then created by someone other than the developer since the repo
> will not have this direstory?

With some automatic scripting it could be created in heketi's repository automatically with every update of glockfile. That was my thought.

> Could gofed create the godeps directory and add it inside the tar file for
> those programs that do not include it in their repo and instead use glock?

I could script it, actually it is a good idea. There is a lot of projects without Godeps/vendor directory suited for this use case.

I really like the first option :). Let's do it. Meantime the bundled(...) list can be generated from GLOCKFILE.

As long as the package is built from dependencies in Fedora, there is no need for bundled list. So right now the spec file conforms to the current packaging guidelines for go. Approving the spec file.

Still, please generate the bundled list under a BuildRequires of main package with:
%if 0%{?with_bundled} && 0%{?fedora}
Provides: bundled...
...
%endif

This policy is currently only for Fedora. Thanks for the discussion.

[1] https://github.com/gofed/gofed/issues/66
Comment 19 Luis Pabón 2015-10-21 09:10:17 EDT
I made the change as you requested above.  I used the shortcommit value only because the version didn't seem to make sense in this model since some of these libraries do not version their source code.  Instead I tried to keep it simple and provide just the shortcommit.

Do you want this model or should I change it to something like: 0-%{shortcommit}?

Diff:
https://github.com/lpabon/heketi-rpm/commit/2e189391e4f9fbe6f86a1eed1c5f85d028168a0f

Output from gofed:
$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.
Comment 20 Luis Pabón 2015-10-21 09:11:06 EDT
Oops I didn't provide the complete gofed output.  Here it is:
Spec URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi.spec

SRPM URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi-1.0.0-1.fc22.src.rpm

Description: Heketi provides a RESTful management interface which can be used to manage
the life cycle of GlusterFS volumes.  With Heketi, cloud services like
OpenStack Manila, Kubernetes, and OpenShift can dynamically provision
GlusterFS volumes with any of the supported durability types.  Heketi
will automatically determine the location for bricks across the cluster,
making sure to place bricks and its replicas across different failure
domains.  Heketi also supports any number of GlusterFS clusters, allowing
cloud services to provide network file storage without being limited to a
single GlusterFS cluster.

Fedora Account System Username: lpabon

$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.
Comment 21 Jan Chaloupka 2015-10-21 09:32:35 EDT
Can you change
bundled(github.com/auth0/go-jwt-middleware)
to
bundled(golang(github.com/auth0/go-jwt-middleware))
for all bundled?

Short commit is fine as well. In future I will be using entire commit as it is then straightforward to download its tarball and run scans on it.

review is on so good luck :)
Comment 22 Luis Pabón 2015-10-21 09:35:41 EDT
Sure, I can make that change.  Also, I can use the entire commit now also.  It is actually easier for me :-).
Comment 23 Luis Pabón 2015-10-21 09:42:46 EDT
Ok, here is the latest:

Diff:
https://github.com/lpabon/heketi-rpm/commit/74a3fcfa46059c498d2032fc68645906a66565bd

Gofed:
Spec URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi.spec

SRPM URL: https://lpabon.fedorapeople.org/reviews/heketi/heketi-1.0.0-1.fc22.src.rpm

Description: Heketi provides a RESTful management interface which can be used to manage
the life cycle of GlusterFS volumes.  With Heketi, cloud services like
OpenStack Manila, Kubernetes, and OpenShift can dynamically provision
GlusterFS volumes with any of the supported durability types.  Heketi
will automatically determine the location for bricks across the cluster,
making sure to place bricks and its replicas across different failure
domains.  Heketi also supports any number of GlusterFS clusters, allowing
cloud services to provide network file storage without being limited to a
single GlusterFS cluster.

Fedora Account System Username: lpabon

$ rpmlint heketi-1.0.0-1.fc22.src.rpm heketi-1.0.0-1.fc22.x86_64.rpm heketi-devel-1.0.0-1.fc22.noarch.rpm heketi-unit-test-devel-1.0.0-1.fc22.x86_64.rpm heketi-debuginfo-1.0.0-1.fc22.x86_64.rpm
heketi.src: W: invalid-url Source3: heketi-godeps-3f4a5b1.tar.gz
heketi.x86_64: W: non-standard-uid /var/lib/heketi heketi
heketi.x86_64: W: non-standard-gid /var/lib/heketi heketi
heketi.x86_64: W: no-manual-page-for-binary heketi
heketi.x86_64: W: no-manual-page-for-binary heketi-cli
5 packages and 0 specfiles checked; 0 errors, 5 warnings.
Comment 24 Jan Chaloupka 2015-10-21 09:59:09 EDT
Good to go :)
Comment 25 Fedora Update System 2015-10-23 01:45:07 EDT
heketi-1.0.0-1.el7 has been submitted as an update to Fedora EPEL 7. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-8bb88f3e8a
Comment 26 Fedora Update System 2015-10-23 01:46:04 EDT
heketi-1.0.0-1.el6 has been submitted as an update to Fedora EPEL 6. https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-8ad0c47397
Comment 27 Jan Chaloupka 2015-10-26 08:06:22 EDT
In future, I would like to suggest building epel7 golang projects purely from bundled deps. The current effort was to remove as many golang packages from epel7 as possible. As some projects can be added to RHEL7 extras causing the epel7 package to be obsoleted/removed. Just noting :).
Comment 28 Fedora Update System 2015-10-26 15:50:31 EDT
heketi-1.0.0-1.el6 has been pushed to the Fedora EPEL 6 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update heketi'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-8ad0c47397
Comment 29 Fedora Update System 2015-10-26 17:20:42 EDT
heketi-1.0.0-1.el7 has been pushed to the Fedora EPEL 7 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'yum --enablerepo=epel-testing update heketi'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-EPEL-2015-8bb88f3e8a
Comment 30 Fedora Update System 2015-11-05 09:29:18 EST
heketi-1.0.1-1.fc23 has been submitted as an update to Fedora 23. https://bodhi.fedoraproject.org/updates/FEDORA-2015-4c8a9e5f52
Comment 31 Fedora Update System 2015-11-05 10:30:05 EST
heketi-1.0.1-1.fc22 has been submitted as an update to Fedora 22. https://bodhi.fedoraproject.org/updates/FEDORA-2015-08f184de52
Comment 32 Fedora Update System 2015-11-05 19:50:54 EST
heketi-1.0.1-1.fc22 has been pushed to the Fedora 22 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update heketi'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-08f184de52
Comment 33 Fedora Update System 2015-11-05 21:34:20 EST
heketi-1.0.1-1.fc23 has been pushed to the Fedora 23 testing repository. If problems still persist, please make note of it in this bug report.
If you want to test the update, you can install it with
$ su -c 'dnf --enablerepo=updates-testing update heketi'
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2015-4c8a9e5f52
Comment 34 Fedora Update System 2015-11-13 14:24:46 EST
heketi-1.0.0-1.el7 has been pushed to the Fedora EPEL 7 stable repository. If problems still persist, please make note of it in this bug report.
Comment 35 Fedora Update System 2015-11-13 15:25:45 EST
heketi-1.0.0-1.el6 has been pushed to the Fedora EPEL 6 stable repository. If problems still persist, please make note of it in this bug report.
Comment 36 Fedora Update System 2015-11-13 17:54:17 EST
heketi-1.0.1-1.fc23 has been pushed to the Fedora 23 stable repository. If problems still persist, please make note of it in this bug report.
Comment 37 Fedora Update System 2015-11-13 20:52:40 EST
heketi-1.0.1-1.fc22 has been pushed to the Fedora 22 stable repository. If problems still persist, please make note of it in this bug report.

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