[PATCH] storage: fix vstorage backend build

Nikolay Shirokovskiy posted 1 patch 3 years, 8 months ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/1594123224-581352-1-git-send-email-nshirokovskiy@virtuozzo.com
src/storage/storage_backend_vstorage.c | 2 ++
1 file changed, 2 insertions(+)
[PATCH] storage: fix vstorage backend build
Posted by Nikolay Shirokovskiy 3 years, 8 months ago
Add headers with declarations of  geteuid/getegid
and virGetUserName/virGetGroupName.

Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com>
---
 src/storage/storage_backend_vstorage.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/storage/storage_backend_vstorage.c b/src/storage/storage_backend_vstorage.c
index 85ab832..6cff9f1 100644
--- a/src/storage/storage_backend_vstorage.c
+++ b/src/storage/storage_backend_vstorage.c
@@ -6,10 +6,12 @@
 #include "storage_backend_vstorage.h"
 #include "virlog.h"
 #include "virstring.h"
+#include "virutil.h"
 #include <mntent.h>
 #include <paths.h>
 #include <pwd.h>
 #include <grp.h>
+#include <unistd.h>
 #include "storage_util.h"
 
 #define VIR_FROM_THIS VIR_FROM_STORAGE
-- 
1.8.3.1

Re: [PATCH] storage: fix vstorage backend build
Posted by Andrea Bolognani 3 years, 8 months ago
On Tue, 2020-07-07 at 15:00 +0300, Nikolay Shirokovskiy wrote:
> Add headers with declarations of  geteuid/getegid
> and virGetUserName/virGetGroupName.

Can you share the exact error message you're hitting? Our CI seems to
be perfectly happy with the current status quo. Specifically, the
x86-centos-7 job, the only one which is supposed to be able to build
the OpenVZ driver, is green.

Looking at the raw log[1] for the latest instance of the job, you can
see that the vz driver is enabled (vz: yes), but the vstorage driver
is not (Virtuozzo storage: no). From the configure output, the reason
seems clear:

  checking for vstorage... no
  checking for vstorage-mount... no

The CentOS 7 container that we use for builds has libprlsdk-devel and
friends installed from the official repository[2]... Although, now
that I look at it there's apparently a newer release[3] out, so we
should probably switch to it.


Another thing: the spec file calls configure with hardcoded

  --without-vz
  --without-storage-vstorage

and there is no binary package definition for anything like

  libvirt-daemon-driver-vz
  libvirt-daemon-driver-storage-vstorage

which would probably be nice to have enabled at least on CentOS 7?
Funnily enough, we actually already have

  %if 0%{?rhel}
    %define with_vz 0
  %endif

so clearly at some point there was the intention to conditionally
include or exclude these drivers from the build.


One thing at a time, though. First, how do we get the vstorage
commands included in the CentOS 7 container? What packages need to
be installed, and from what repository?


As an aside, I'm still very confused by the vz/openvz dichotomy.
AFAICT, the latter can be (and in fact is) built unconditionally,
but the former requires the "Parallels SDK" packages to be installed:
baffingly enough, said SDK is obtained from the repository mentioned
above, which just so happens to include the string "openvz" twice in
its URL...


[1] https://storage.googleapis.com/gitlab-gprd-artifacts/5d/af/5daf0e5fa057744f5bfc6ec3d7ebc435b6fdcd9d40afe5a23cc67756a826d8ee/2020_07_09/631037426/692468732/job.log?response-content-type=text%2Fplain%3B%20charset%3Dutf-8&response-content-disposition=inline&GoogleAccessId=gitlab-object-storage-prd@gitlab-production.iam.gserviceaccount.com&Signature=hiC9XbFsV78D1nFcOg09WWcUAR66M2nbIECVm%2BD0tLBWMviBriGhB7eFpNL3%0A7H1bZ%2Faw0M64HaCJ8PZxw4SO6KeRkd5RZ3HC%2Ff7TWngvtKGy9xp4L9QLNcf7%0AiJVYvFQqPcDurp61C%2F%2B%2B5WNMxJq1Vl1wT6Lk0XlJlnssV59FyV4UHV3UAye%2F%0AiHrjSLcbuwbNIAu19dTAJskQroJ8hfRgAcSWDdU2%2FXWsCb19oCzpLBvwtr2T%0AIOYgLDjVCzntxu1gngIB499EDl%2B38ASB27kEFRkEDp8cEqz0wGOUstN4dAWu%0A4hH3S2%2FuXWoVoJ6ibOU%2Bh8REB1luIzVDQelJfGPblA%3D%3D&Expires=1594308539
[2] https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os
[3] https://download.openvz.org/virtuozzo/releases/openvz-7.0.14-136/x86_64/os
-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] storage: fix vstorage backend build
Posted by Nikolay Shirokovskiy 3 years, 8 months ago

On 09.07.2020 19:06, Andrea Bolognani wrote:
> On Tue, 2020-07-07 at 15:00 +0300, Nikolay Shirokovskiy wrote:
>> Add headers with declarations of  geteuid/getegid
>> and virGetUserName/virGetGroupName.
> 
> Can you share the exact error message you're hitting? Our CI seems to
> be perfectly happy with the current status quo. Specifically, the
> x86-centos-7 job, the only one which is supposed to be able to build
> the OpenVZ driver, is green.

Hi! Sure.

../../src/storage/storage_backend_vstorage.c: In function 'virStorageBackendVzPoolStart':
../../src/storage/storage_backend_vstorage.c:52:9: error: implicit declaration of function 'geteuid' [-Werror=implicit-function-declaration]
         def->target.perms.uid = geteuid();
         ^
../../src/storage/storage_backend_vstorage.c:52:9: error: nested extern declaration of 'geteuid' [-Werror=nested-externs]
../../src/storage/storage_backend_vstorage.c:54:9: error: implicit declaration of function 'getegid' [-Werror=implicit-function-declaration]
         def->target.perms.gid = getegid();
         ^
../../src/storage/storage_backend_vstorage.c:54:9: error: nested extern declaration of 'getegid' [-Werror=nested-externs]
../../src/storage/storage_backend_vstorage.c:58:5: error: implicit declaration of function 'virGetGroupName' [-Werror=implicit-function-declaration]
     if (!(grp_name = virGetGroupName(def->target.perms.gid)))
     ^
../../src/storage/storage_backend_vstorage.c:58:5: error: nested extern declaration of 'virGetGroupName' [-Werror=nested-externs]
../../src/storage/storage_backend_vstorage.c:58:20: error: assignment makes pointer from integer without a cast [-Werror]
     if (!(grp_name = virGetGroupName(def->target.perms.gid)))
                    ^
../../src/storage/storage_backend_vstorage.c:61:5: error: implicit declaration of function 'virGetUserName' [-Werror=implicit-function-declaration]
     if (!(usr_name = virGetUserName(def->target.perms.uid)))
     ^
../../src/storage/storage_backend_vstorage.c:61:5: error: nested extern declaration of 'virGetUserName' [-Werror=nested-externs]
../../src/storage/storage_backend_vstorage.c:61:20: error: assignment makes pointer from integer without a cast [-Werror]
     if (!(usr_name = virGetUserName(def->target.perms.uid)))
                    ^


> 
> Looking at the raw log[1] for the latest instance of the job, you can
> see that the vz driver is enabled (vz: yes), but the vstorage driver
> is not (Virtuozzo storage: no). From the configure output, the reason
> seems clear:
> 
>   checking for vstorage... no
>   checking for vstorage-mount... no
> 
> The CentOS 7 container that we use for builds has libprlsdk-devel and
> friends installed from the official repository[2]... Although, now
> that I look at it there's apparently a newer release[3] out, so we
> should probably switch to it.

vstorage and vz driver are quite unrelated. For the former vstorage
and vstorage-mount binaries are build requirements.

> 
> 
> Another thing: the spec file calls configure with hardcoded
> 
>   --without-vz
>   --without-storage-vstorage
> 
> and there is no binary package definition for anything like
> 
>   libvirt-daemon-driver-vz
>   libvirt-daemon-driver-storage-vstorage
> 
> which would probably be nice to have enabled at least on CentOS 7?
> Funnily enough, we actually already have
> 
>   %if 0%{?rhel}
>     %define with_vz 0
>   %endif
> 
> so clearly at some point there was the intention to conditionally
> include or exclude these drivers from the build.
> 
> 
> One thing at a time, though. First, how do we get the vstorage
> commands included in the CentOS 7 container? What packages need to
> be installed, and from what repository?

The repo is http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os

vstorage binary is in vstorage-ctl package and vstorage-mount binary
is in vstorage-client package.

However vstorage binary is not used at all the driver. Also the openvz
driver for example does not check for its binaries. Probably
vstorage driver should be changed accordingly as binaries are not
build requisites.


> 
> 
> As an aside, I'm still very confused by the vz/openvz dichotomy.
> AFAICT, the latter can be (and in fact is) built unconditionally,
> but the former requires the "Parallels SDK" packages to be installed:
> baffingly enough, said SDK is obtained from the repository mentioned
> above, which just so happens to include the string "openvz" twice in
> its URL...

Yeah, naming is confusing. Basically openvz manages system containers thru
vzctl binary. Vz driver manages both VMs/containers thru single connection
using prlsdk. And originally vz driver was called parallels driver but after
company split we have to change the name. Also in the past prlsdk was only
commercially available and now times changes and both vzctl and prlsdk are
available under openvz name which is an umbrella for uncommercial projects.

Nikolay

> 
> 
> [1] https://storage.googleapis.com/gitlab-gprd-artifacts/5d/af/5daf0e5fa057744f5bfc6ec3d7ebc435b6fdcd9d40afe5a23cc67756a826d8ee/2020_07_09/631037426/692468732/job.log?response-content-type=text%2Fplain%3B%20charset%3Dutf-8&response-content-disposition=inline&GoogleAccessId=gitlab-object-storage-prd@gitlab-production.iam.gserviceaccount.com&Signature=hiC9XbFsV78D1nFcOg09WWcUAR66M2nbIECVm%2BD0tLBWMviBriGhB7eFpNL3%0A7H1bZ%2Faw0M64HaCJ8PZxw4SO6KeRkd5RZ3HC%2Ff7TWngvtKGy9xp4L9QLNcf7%0AiJVYvFQqPcDurp61C%2F%2B%2B5WNMxJq1Vl1wT6Lk0XlJlnssV59FyV4UHV3UAye%2F%0AiHrjSLcbuwbNIAu19dTAJskQroJ8hfRgAcSWDdU2%2FXWsCb19oCzpLBvwtr2T%0AIOYgLDjVCzntxu1gngIB499EDl%2B38ASB27kEFRkEDp8cEqz0wGOUstN4dAWu%0A4hH3S2%2FuXWoVoJ6ibOU%2Bh8REB1luIzVDQelJfGPblA%3D%3D&Expires=1594308539
> [2] https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os
> [3] https://download.openvz.org/virtuozzo/releases/openvz-7.0.14-136/x86_64/os
> 

Re: [PATCH] storage: fix vstorage backend build
Posted by Andrea Bolognani 3 years, 8 months ago
On Fri, 2020-07-10 at 10:10 +0300, Nikolay Shirokovskiy wrote:
> On 09.07.2020 19:06, Andrea Bolognani wrote:
> > One thing at a time, though. First, how do we get the vstorage
> > commands included in the CentOS 7 container? What packages need to
> > be installed, and from what repository?
> 
> The repo is http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os
> 
> vstorage binary is in vstorage-ctl package and vstorage-mount binary
> is in vstorage-client package.
> 
> However vstorage binary is not used at all the driver. Also the openvz
> driver for example does not check for its binaries. Probably
> vstorage driver should be changed accordingly as binaries are not
> build requisites.

I'm not sure how the best way to handle the situation is. You should
look at what we do with qemu-img for inspiration.

> > As an aside, I'm still very confused by the vz/openvz dichotomy.
> > AFAICT, the latter can be (and in fact is) built unconditionally,
> > but the former requires the "Parallels SDK" packages to be installed:
> > baffingly enough, said SDK is obtained from the repository mentioned
> > above, which just so happens to include the string "openvz" twice in
> > its URL...
> 
> Yeah, naming is confusing. Basically openvz manages system containers thru
> vzctl binary. Vz driver manages both VMs/containers thru single connection
> using prlsdk. And originally vz driver was called parallels driver but after
> company split we have to change the name. Also in the past prlsdk was only
> commercially available and now times changes and both vzctl and prlsdk are
> available under openvz name which is an umbrella for uncommercial projects.

Thanks for the explanation, but I'm afraid that even with it the
relationship between the various projects and products is only
marginally clearer to me O:-)

Anyway.

Starting from our existing CentOS 7 build environment, I've added

  [vz]
  baseurl=http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os/
  enabled=1
  gpgcheck=1
  priority=90
  includepkgs=vstorage*,libvz*

to /etc/yum.repos.d/vz.repo and tried to install vstorage-client.
That failed with

  Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz)
             Requires: libjson-c.so.2(libjson-c.so.2)(64bit)

which is surprising because I have the json-c package installed.
I think that's caused by a bug in your spec file - the name of the
library should not appear in parentheses - but I can't seem to find
the source package for vstorage-client under

  http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/

Moreover, when I try to get information about eg. vstorage-client,
the output of 'yum info' contains

  License : Virtuozzo

Is this an actual open source license that I can find anywhere?

Until the packaging issue has been addressed and the license
situation has been clarified, we can't move forward with building
the vstorage driver as part of our CI pipeline.

I did, however, reproduce the build failure you describe above and
confirm that your patch fixes it in the process of writing this
message, so

  Reviewed-by: Andrea Bolognani <abologna@redhat.com>

to that.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] storage: fix vstorage backend build
Posted by Daniel P. Berrangé 3 years, 8 months ago
On Fri, Jul 10, 2020 at 06:40:07PM +0200, Andrea Bolognani wrote:
> On Fri, 2020-07-10 at 10:10 +0300, Nikolay Shirokovskiy wrote:
> > On 09.07.2020 19:06, Andrea Bolognani wrote:
> > > One thing at a time, though. First, how do we get the vstorage
> > > commands included in the CentOS 7 container? What packages need to
> > > be installed, and from what repository?
> > 
> > The repo is http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os
> > 
> > vstorage binary is in vstorage-ctl package and vstorage-mount binary
> > is in vstorage-client package.
> > 
> > However vstorage binary is not used at all the driver. Also the openvz
> > driver for example does not check for its binaries. Probably
> > vstorage driver should be changed accordingly as binaries are not
> > build requisites.
> 
> I'm not sure how the best way to handle the situation is. You should
> look at what we do with qemu-img for inspiration.
> 
> > > As an aside, I'm still very confused by the vz/openvz dichotomy.
> > > AFAICT, the latter can be (and in fact is) built unconditionally,
> > > but the former requires the "Parallels SDK" packages to be installed:
> > > baffingly enough, said SDK is obtained from the repository mentioned
> > > above, which just so happens to include the string "openvz" twice in
> > > its URL...
> > 
> > Yeah, naming is confusing. Basically openvz manages system containers thru
> > vzctl binary. Vz driver manages both VMs/containers thru single connection
> > using prlsdk. And originally vz driver was called parallels driver but after
> > company split we have to change the name. Also in the past prlsdk was only
> > commercially available and now times changes and both vzctl and prlsdk are
> > available under openvz name which is an umbrella for uncommercial projects.
> 
> Thanks for the explanation, but I'm afraid that even with it the
> relationship between the various projects and products is only
> marginally clearer to me O:-)
> 
> Anyway.
> 
> Starting from our existing CentOS 7 build environment, I've added
> 
>   [vz]
>   baseurl=http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os/
>   enabled=1
>   gpgcheck=1
>   priority=90
>   includepkgs=vstorage*,libvz*
> 
> to /etc/yum.repos.d/vz.repo and tried to install vstorage-client.
> That failed with
> 
>   Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz)
>              Requires: libjson-c.so.2(libjson-c.so.2)(64bit)
> 
> which is surprising because I have the json-c package installed.
> I think that's caused by a bug in your spec file - the name of the
> library should not appear in parentheses - but I can't seem to find
> the source package for vstorage-client under
> 
>   http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/

Yes, this dependancy is clearly broken, whcih is the reason why
we're not using the URL you show above, and instead using the older
release at:

  https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os/

this URL is the last one that doesn't have the broken dependancies.

I asked about this at the time but didn't get any response, so I guess
it was buried in the email torrent and then i forgot to ping about it.

  https://www.redhat.com/archives/libvir-list/2019-December/msg00437.html

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH] storage: fix vstorage backend build
Posted by Andrea Bolognani 3 years, 8 months ago
On Fri, 2020-07-10 at 18:26 +0100, Daniel P. Berrangé wrote:
> On Fri, Jul 10, 2020 at 06:40:07PM +0200, Andrea Bolognani wrote:
> > That failed with
> > 
> >   Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz)
> >              Requires: libjson-c.so.2(libjson-c.so.2)(64bit)
> > 
> > which is surprising because I have the json-c package installed.
> > I think that's caused by a bug in your spec file - the name of the
> > library should not appear in parentheses - but I can't seem to find
> > the source package for vstorage-client under
> > 
> >   http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/
> 
> Yes, this dependancy is clearly broken, whcih is the reason why
> we're not using the URL you show above, and instead using the older
> release at:
> 
>   https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os/
> 
> this URL is the last one that doesn't have the broken dependancies.

Note that we're not just talking about different releases available
from the same repository, but about two completely different
repositories: the one we're using is under

  https://download.openvz.org/virtuozzo/releases/

whereas the one Nikolay pointed to for vstorage package is under

  http://repo.virtuozzo.com/vz/releases/

If you look at the list of releases, you'll see that the versions are
similar but always a bit different: I couldn't find a single release
which is available on both sites.

If you look at the list of packages through the provided "repoview"
files, the releases from the second repository contain many more:
additional categories include things like "Readykernel", Virtuozzo
High Availability" and, most interesting to us, "Virtuozzo Storage".

As mentioned in my previous message, some of these packages appear to
be released not under the (L)GPL but under a "Virtuozzo" license that
I haven't been able to find anywhere, and I'm not entirely sure is
actually open source.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] storage: fix vstorage backend build
Posted by Nikolay Shirokovskiy 3 years, 8 months ago

On 10.07.2020 21:17, Andrea Bolognani wrote:
> On Fri, 2020-07-10 at 18:26 +0100, Daniel P. Berrangé wrote:
>> On Fri, Jul 10, 2020 at 06:40:07PM +0200, Andrea Bolognani wrote:
>>> That failed with
>>>
>>>   Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz)
>>>              Requires: libjson-c.so.2(libjson-c.so.2)(64bit)
>>>
>>> which is surprising because I have the json-c package installed.
>>> I think that's caused by a bug in your spec file - the name of the
>>> library should not appear in parentheses - but I can't seem to find
>>> the source package for vstorage-client under
>>>
>>>   http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/
>>
>> Yes, this dependancy is clearly broken, whcih is the reason why
>> we're not using the URL you show above, and instead using the older
>> release at:
>>
>>   https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os/
>>
>> this URL is the last one that doesn't have the broken dependancies.
> 
> Note that we're not just talking about different releases available
> from the same repository, but about two completely different
> repositories: the one we're using is under
> 
>   https://download.openvz.org/virtuozzo/releases/
> 
> whereas the one Nikolay pointed to for vstorage package is under
> 
>   http://repo.virtuozzo.com/vz/releases/
> 
> If you look at the list of releases, you'll see that the versions are
> similar but always a bit different: I couldn't find a single release
> which is available on both sites.
> 
> If you look at the list of packages through the provided "repoview"
> files, the releases from the second repository contain many more:
> additional categories include things like "Readykernel", Virtuozzo
> High Availability" and, most interesting to us, "Virtuozzo Storage".
> 
> As mentioned in my previous message, some of these packages appear to
> be released not under the (L)GPL but under a "Virtuozzo" license that
> I haven't been able to find anywhere, and I'm not entirely sure is
> actually open source.
> 

We have Virtuozzo product and part of it is available as open source
from openvz.org repo. Virtuozzo Storage is not open source but it
can be used with very limited storage size without license keys.
I guess you would want to read license anyway before using Virtuozzo
Storage packages in CI but I'm going to remove these packages
requirements anyway as I wrote in reply to Andrea's message.

Nikolay


Re: [PATCH] storage: fix vstorage backend build
Posted by Nikolay Shirokovskiy 3 years, 8 months ago

On 13.07.2020 09:57, Nikolay Shirokovskiy wrote:
> 
> 
> On 10.07.2020 21:17, Andrea Bolognani wrote:
>> On Fri, 2020-07-10 at 18:26 +0100, Daniel P. Berrangé wrote:
>>> On Fri, Jul 10, 2020 at 06:40:07PM +0200, Andrea Bolognani wrote:
>>>> That failed with
>>>>
>>>>   Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz)
>>>>              Requires: libjson-c.so.2(libjson-c.so.2)(64bit)
>>>>
>>>> which is surprising because I have the json-c package installed.
>>>> I think that's caused by a bug in your spec file - the name of the
>>>> library should not appear in parentheses - but I can't seem to find
>>>> the source package for vstorage-client under
>>>>
>>>>   http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/
>>>
>>> Yes, this dependancy is clearly broken, whcih is the reason why
>>> we're not using the URL you show above, and instead using the older
>>> release at:
>>>
>>>   https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os/
>>>
>>> this URL is the last one that doesn't have the broken dependancies.
>>
>> Note that we're not just talking about different releases available
>> from the same repository, but about two completely different
>> repositories: the one we're using is under
>>
>>   https://download.openvz.org/virtuozzo/releases/
>>
>> whereas the one Nikolay pointed to for vstorage package is under
>>
>>   http://repo.virtuozzo.com/vz/releases/
>>
>> If you look at the list of releases, you'll see that the versions are
>> similar but always a bit different: I couldn't find a single release
>> which is available on both sites.
>>
>> If you look at the list of packages through the provided "repoview"
>> files, the releases from the second repository contain many more:
>> additional categories include things like "Readykernel", Virtuozzo
>> High Availability" and, most interesting to us, "Virtuozzo Storage".
>>
>> As mentioned in my previous message, some of these packages appear to
>> be released not under the (L)GPL but under a "Virtuozzo" license that
>> I haven't been able to find anywhere, and I'm not entirely sure is
>> actually open source.
>>
> 
> We have Virtuozzo product and part of it is available as open source
> from openvz.org repo. Virtuozzo Storage is not open source but it
> can be used with very limited storage size without license keys.
> I guess you would want to read license anyway before using Virtuozzo
> Storage packages in CI but I'm going to remove these packages
> requirements anyway as I wrote in reply to Andrea's message.
> 

After giving it more thought I think it is useful to have vstorage
m4 script as it is. It is useful to detect vstorage-mount binary
path at build time. Although Virtuozzo Storage is not open source
and can not be build for distributions with different binary paths
by community it is a part of different Virtuozzo products and
theoretically speaking the path can be changed from product to product.
So it is useful to have path not hardcoded and not detected at runtime.

Before we fix our repos please take a look at Virtuozzo license at [1]
(note there is different tabs and EULA tab).

[1] https://www.virtuozzo.com/legal.html


Re: [PATCH] storage: fix vstorage backend build
Posted by Andrea Bolognani 3 years, 8 months ago
On Mon, 2020-07-13 at 11:52 +0300, Nikolay Shirokovskiy wrote:
> On 13.07.2020 09:57, Nikolay Shirokovskiy wrote:
> > On 10.07.2020 21:17, Andrea Bolognani wrote:
> > > If you look at the list of packages through the provided "repoview"
> > > files, the releases from the second repository contain many more:
> > > additional categories include things like "Readykernel", Virtuozzo
> > > High Availability" and, most interesting to us, "Virtuozzo Storage".
> > > 
> > > As mentioned in my previous message, some of these packages appear to
> > > be released not under the (L)GPL but under a "Virtuozzo" license that
> > > I haven't been able to find anywhere, and I'm not entirely sure is
> > > actually open source.
> > 
> > We have Virtuozzo product and part of it is available as open source
> > from openvz.org repo. Virtuozzo Storage is not open source but it
> > can be used with very limited storage size without license keys.
> > I guess you would want to read license anyway before using Virtuozzo
> > Storage packages in CI but I'm going to remove these packages
> > requirements anyway as I wrote in reply to Andrea's message.
> 
> After giving it more thought I think it is useful to have vstorage
> m4 script as it is. It is useful to detect vstorage-mount binary
> path at build time. Although Virtuozzo Storage is not open source
> and can not be build for distributions with different binary paths
> by community it is a part of different Virtuozzo products and
> theoretically speaking the path can be changed from product to product.
> So it is useful to have path not hardcoded and not detected at runtime.

If runtime detection is good enough for qemu-img, I don't see why it
wouldn't be good enough for vstorage too.

> Before we fix our repos please take a look at Virtuozzo license at [1]
> (note there is different tabs and EULA tab).
> 
> [1] https://www.virtuozzo.com/legal.html

Yeah, sorry, but I'm not going to put my name on a patch that results
in our build environments suddenly including proprietary software.
I'm already not a fan that happening based on principles alone, and I
most certainly don't want to deal with any potential fallout from our
container images violating the EULA or whatnot.

-- 
Andrea Bolognani / Red Hat / Virtualization

Re: [PATCH] storage: fix vstorage backend build
Posted by Nikolay Shirokovskiy 3 years, 8 months ago

On 13.07.2020 12:16, Andrea Bolognani wrote:
> On Mon, 2020-07-13 at 11:52 +0300, Nikolay Shirokovskiy wrote:
>> On 13.07.2020 09:57, Nikolay Shirokovskiy wrote:
>>> On 10.07.2020 21:17, Andrea Bolognani wrote:
>>>> If you look at the list of packages through the provided "repoview"
>>>> files, the releases from the second repository contain many more:
>>>> additional categories include things like "Readykernel", Virtuozzo
>>>> High Availability" and, most interesting to us, "Virtuozzo Storage".
>>>>
>>>> As mentioned in my previous message, some of these packages appear to
>>>> be released not under the (L)GPL but under a "Virtuozzo" license that
>>>> I haven't been able to find anywhere, and I'm not entirely sure is
>>>> actually open source.
>>>
>>> We have Virtuozzo product and part of it is available as open source
>>> from openvz.org repo. Virtuozzo Storage is not open source but it
>>> can be used with very limited storage size without license keys.
>>> I guess you would want to read license anyway before using Virtuozzo
>>> Storage packages in CI but I'm going to remove these packages
>>> requirements anyway as I wrote in reply to Andrea's message.
>>
>> After giving it more thought I think it is useful to have vstorage
>> m4 script as it is. It is useful to detect vstorage-mount binary
>> path at build time. Although Virtuozzo Storage is not open source
>> and can not be build for distributions with different binary paths
>> by community it is a part of different Virtuozzo products and
>> theoretically speaking the path can be changed from product to product.
>> So it is useful to have path not hardcoded and not detected at runtime.
> 
> If runtime detection is good enough for qemu-img, I don't see why it
> wouldn't be good enough for vstorage too.
> 
>> Before we fix our repos please take a look at Virtuozzo license at [1]
>> (note there is different tabs and EULA tab).
>>
>> [1] https://www.virtuozzo.com/legal.html
> 
> Yeah, sorry, but I'm not going to put my name on a patch that results
> in our build environments suddenly including proprietary software.
> I'm already not a fan that happening based on principles alone, and I
> most certainly don't want to deal with any potential fallout from our
> container images violating the EULA or whatnot.
> 

Ok, given all the license stuff and the fact we can just use runtime
binary detection let's go with this variant. I'll send the patch.

Nikolay

Re: [PATCH] storage: fix vstorage backend build
Posted by Nikolay Shirokovskiy 3 years, 8 months ago

On 10.07.2020 20:26, Daniel P. Berrangé wrote:
> On Fri, Jul 10, 2020 at 06:40:07PM +0200, Andrea Bolognani wrote:
>> On Fri, 2020-07-10 at 10:10 +0300, Nikolay Shirokovskiy wrote:
>>> On 09.07.2020 19:06, Andrea Bolognani wrote:
>>>> One thing at a time, though. First, how do we get the vstorage
>>>> commands included in the CentOS 7 container? What packages need to
>>>> be installed, and from what repository?
>>>
>>> The repo is http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os
>>>
>>> vstorage binary is in vstorage-ctl package and vstorage-mount binary
>>> is in vstorage-client package.
>>>
>>> However vstorage binary is not used at all the driver. Also the openvz
>>> driver for example does not check for its binaries. Probably
>>> vstorage driver should be changed accordingly as binaries are not
>>> build requisites.
>>
>> I'm not sure how the best way to handle the situation is. You should
>> look at what we do with qemu-img for inspiration.
>>
>>>> As an aside, I'm still very confused by the vz/openvz dichotomy.
>>>> AFAICT, the latter can be (and in fact is) built unconditionally,
>>>> but the former requires the "Parallels SDK" packages to be installed:
>>>> baffingly enough, said SDK is obtained from the repository mentioned
>>>> above, which just so happens to include the string "openvz" twice in
>>>> its URL...
>>>
>>> Yeah, naming is confusing. Basically openvz manages system containers thru
>>> vzctl binary. Vz driver manages both VMs/containers thru single connection
>>> using prlsdk. And originally vz driver was called parallels driver but after
>>> company split we have to change the name. Also in the past prlsdk was only
>>> commercially available and now times changes and both vzctl and prlsdk are
>>> available under openvz name which is an umbrella for uncommercial projects.
>>
>> Thanks for the explanation, but I'm afraid that even with it the
>> relationship between the various projects and products is only
>> marginally clearer to me O:-)
>>
>> Anyway.
>>
>> Starting from our existing CentOS 7 build environment, I've added
>>
>>   [vz]
>>   baseurl=http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os/
>>   enabled=1
>>   gpgcheck=1
>>   priority=90
>>   includepkgs=vstorage*,libvz*
>>
>> to /etc/yum.repos.d/vz.repo and tried to install vstorage-client.
>> That failed with
>>
>>   Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz)
>>              Requires: libjson-c.so.2(libjson-c.so.2)(64bit)
>>
>> which is surprising because I have the json-c package installed.
>> I think that's caused by a bug in your spec file - the name of the
>> library should not appear in parentheses - but I can't seem to find
>> the source package for vstorage-client under
>>
>>   http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/
> 
> Yes, this dependancy is clearly broken, whcih is the reason why
> we're not using the URL you show above, and instead using the older
> release at:
> 
>   https://download.openvz.org/virtuozzo/releases/openvz-7.0.11-235/x86_64/os/
> 
> this URL is the last one that doesn't have the broken dependancies.
> 
> I asked about this at the time but didn't get any response, so I guess
> it was buried in the email torrent and then i forgot to ping about it.
> 
>   https://www.redhat.com/archives/libvir-list/2019-December/msg00437.html

Sorry, I missed that. We'll fix the issues.

Nikolay


Re: [PATCH] storage: fix vstorage backend build
Posted by Nikolay Shirokovskiy 3 years, 8 months ago

On 10.07.2020 19:40, Andrea Bolognani wrote:
> On Fri, 2020-07-10 at 10:10 +0300, Nikolay Shirokovskiy wrote:
>> On 09.07.2020 19:06, Andrea Bolognani wrote:
>>> One thing at a time, though. First, how do we get the vstorage
>>> commands included in the CentOS 7 container? What packages need to
>>> be installed, and from what repository?
>>
>> The repo is http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os
>>
>> vstorage binary is in vstorage-ctl package and vstorage-mount binary
>> is in vstorage-client package.
>>
>> However vstorage binary is not used at all the driver. Also the openvz
>> driver for example does not check for its binaries. Probably
>> vstorage driver should be changed accordingly as binaries are not
>> build requisites.
> 
> I'm not sure how the best way to handle the situation is. You should
> look at what we do with qemu-img for inspiration.

Yeah, qemu-img presence is not checked/required for build so I guess 
I'll just fix vstorage driver to behave the same way.

> 
>>> As an aside, I'm still very confused by the vz/openvz dichotomy.
>>> AFAICT, the latter can be (and in fact is) built unconditionally,
>>> but the former requires the "Parallels SDK" packages to be installed:
>>> baffingly enough, said SDK is obtained from the repository mentioned
>>> above, which just so happens to include the string "openvz" twice in
>>> its URL...
>>
>> Yeah, naming is confusing. Basically openvz manages system containers thru
>> vzctl binary. Vz driver manages both VMs/containers thru single connection
>> using prlsdk. And originally vz driver was called parallels driver but after
>> company split we have to change the name. Also in the past prlsdk was only
>> commercially available and now times changes and both vzctl and prlsdk are
>> available under openvz name which is an umbrella for uncommercial projects.
> 
> Thanks for the explanation, but I'm afraid that even with it the
> relationship between the various projects and products is only
> marginally clearer to me O:-)
> 
> Anyway.
> 
> Starting from our existing CentOS 7 build environment, I've added
> 
>   [vz]
>   baseurl=http://repo.virtuozzo.com/vz/releases/7.0/x86_64/os/
>   enabled=1
>   gpgcheck=1
>   priority=90
>   includepkgs=vstorage*,libvz*
> 
> to /etc/yum.repos.d/vz.repo and tried to install vstorage-client.
> That failed with
> 
>   Error: Package: vstorage-libs-shared-7.10.1.5-1.vz7.x86_64 (vz)
>              Requires: libjson-c.so.2(libjson-c.so.2)(64bit)
> 
> which is surprising because I have the json-c package installed.
> I think that's caused by a bug in your spec file - the name of the
> library should not appear in parentheses - but I can't seem to find
> the source package for vstorage-client under
> 
>   http://repo.virtuozzo.com/vz/releases/7.0/source/SRPMS/
> 
> Moreover, when I try to get information about eg. vstorage-client,
> the output of 'yum info' contains
> 
>   License : Virtuozzo
> 
> Is this an actual open source license that I can find anywhere?
> 
> Until the packaging issue has been addressed and the license
> situation has been clarified, we can't move forward with building
> the vstorage driver as part of our CI pipeline.

So these issues will become irrelevant after the fix proposed above.
Nevertheless we fix our repos to have proper requirements of course.

> 
> I did, however, reproduce the build failure you describe above and
> confirm that your patch fixes it in the process of writing this
> message, so
> 
>   Reviewed-by: Andrea Bolognani <abologna@redhat.com>
> 
> to that.
> 

Pushed. 

Nikolay