Circular dependencies when refactoring functionality from XenPool to Pool

[Bahtiar Gadimov]

Hi,

while working on an lvm storage implementation for the current
qubes-core-admin I noticed that some of the logic used by XenStorage
and XenPool is general enough to be reused. I started moving code from
XenStorage & XenPool to QubesVmStorage & Pool. I prepared a branch
(https://github.com/QubesOS/qubes-core-admin/compare/master...kalkin:storage?expand=1),
but there is one issue stopping me from opening a PR.

One of the changes is moving the vmdir logic from `XenPool` to the
general `Pool` class. Any storage implementation needs this logic for
saving the vm config and not storage related data. Part of this logic
is looking at the the `vm_type` (i.e: `QubesAppVm`, `QubesTemplateVm`,
..) and then deciding in which dir the vm data should be (i.e:
'appvms/, 'vm-templates/').

When importing all the needed vm classes at the top of the file i get
the an error like 'cannot import name QubesAppVm'. I had to import
inside the `Pool.vmdir_path()` method instead of on the top of file
(See https://github.com/QubesOS/qubes-core-admin/compare/master...kalkin:storage?expand=1#diff-1887b5d6907fa270c4a5ebf5507a6ee1R386).

I am not sure i fully understand the whole module magic which
qubes-core-admin does. My suspicion is that the a vm class needs to
call `register_qubes_vm_class()` to register the class inside the
module. For this `QubesVm` has to import the `qubes.storage` module
and this needs to import all the vm classes for the
`Pool.vmdir_path()` method -> Circular dependency. Am I right? If yes,
is the above solution perhaps the right one, even if it is ugly?

Bahtiar `kalkin-` Gadimov

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Wed, Nov 25, 2015 at 02:04:40PM +0100, Bahtiar Gadimov wrote:
> Hi,
>
> while working on an lvm storage implementation for the current
> qubes-core-admin I noticed that some of the logic used by XenStorage
> and XenPool is general enough to be reused. I started moving code from
> XenStorage & XenPool to QubesVmStorage & Pool. I prepared a branch
> (https://github.com/QubesOS/qubes-core-admin/compare/master...kalkin:storage?expand=1),
> but there is one issue stopping me from opening a PR.
>
> One of the changes is moving the vmdir logic from `XenPool` to the
> general `Pool` class. Any storage implementation needs this logic for
> saving the vm config and not storage related data. Part of this logic
> is looking at the the `vm_type` (i.e: `QubesAppVm`, `QubesTemplateVm`,
> ..) and then deciding in which dir the vm data should be (i.e:
> 'appvms/, 'vm-templates/').

Maybe the better option would be to use vm.is_template() instead of
checking direct VM class? This would for example remove need for
specifically list both TemplateVm and TemplateHVm. And maybe some future
classes...
The same for types:
vm.is_netvm()
vm.is_disposablevm()

> When importing all the needed vm classes at the top of the file i get
> the an error like 'cannot import name QubesAppVm'. I had to import
> inside the `Pool.vmdir_path()` method instead of on the top of file
> (See https://github.com/QubesOS/qubes-core-admin/compare/master...kalkin:storage?expand=1#diff-1887b5d6907fa270c4a5ebf5507a6ee1R386).
>
> I am not sure i fully understand the whole module magic which
> qubes-core-admin does. My suspicion is that the a vm class needs to
> call `register_qubes_vm_class()` to register the class inside the
> module. For this `QubesVm` has to import the `qubes.storage` module
> and this needs to import all the vm classes for the
> `Pool.vmdir_path()` method -> Circular dependency. Am I right?

Yes.
Generally I think storage classes shouldn't depend on explicit VM
classes. I we'd like to add some other class in the future that would
mean (in current implementation) need to modify also storage pool for
that...

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWVb2jAAoJENuP0xzK19cspD0H/2FFUBKdWCzwXXQsRz13P+uG
VJ9smk6kN/PW78b3zt7zDbDoXUz7Z17nM24eGahz6Jp/pHjV0WF0/Lrl3TdkBMxU
2lZC1vJ3gGTAiSFl4m28haEzRoI1FnhfY+j/ZIUKnLJYg/wXt5FYAewIbbCoIswb
t9IJ0oCkWBwyal9ZwYgEFIFKY+ezwcIt+YZBLKM1E9Vg6zAwNgjqVCuZdl/BCnlB
WU3Sw5msUdV0Hp6rVsqPjuVLqlIwVorx8ix4tuODCILAy85TEibMIdh60t6dq/lP
LvWT9HV4FmZMVbofRmxMHpwhmS1XmC58nTuhJZe4aFSz+N6KVp8Hvzs+zpBRiKY=
=Duvi
-----END PGP SIGNATURE-----

[Bahtiar Gadimov]

Hi,

thank you for your swift response.

2015-11-25 14:54 GMT+01:00 Marek Marczykowski-Górecki
<marm...@invisiblethingslab.com>:

> On Wed, Nov 25, 2015 at 02:04:40PM +0100, Bahtiar Gadimov wrote:

>> One of the changes is moving the vmdir logic from `XenPool` to the
>> general `Pool` class. Any storage implementation needs this logic for
>> saving the vm config and not storage related data. Part of this logic
>> is looking at the the `vm_type` (i.e: `QubesAppVm`, `QubesTemplateVm`,
>> ..) and then deciding in which dir the vm data should be (i.e:
>> 'appvms/, 'vm-templates/').
>
> Maybe the better option would be to use vm.is_template() instead of
> checking direct VM class? This would for example remove need for
> specifically list both TemplateVm and TemplateHVm. And maybe some future
> classes...
> The same for types:
> vm.is_netvm()
> vm.is_disposablevm()

Thanks that is exactly what i needed.

>> I am not sure i fully understand the whole module magic which
>> qubes-core-admin does. My suspicion is that the a vm class needs to
>> call `register_qubes_vm_class()` to register the class inside the
>> module. For this `QubesVm` has to import the `qubes.storage` module
>> and this needs to import all the vm classes for the
>> `Pool.vmdir_path()` method -> Circular dependency. Am I right?
>
> Yes.
> Generally I think storage classes shouldn't depend on explicit VM
> classes. I we'd like to add some other class in the future that would
> mean (in current implementation) need to modify also storage pool for
> that...

I implemented it right now as you proposed (see PR:12). After merging
this PR it should not be an issue of adding some other class. You
would only need to edit it if it will be saved in another
subdirectory.

BTW: What is the reason for splitting the vms in different
directories? It's not like there could be an foo AppVm and foo
TemplateVm at the same time, or can it?


Thanks
Bahtiar `kalkin-` Gadimov

[Marek Marczykowski-Górecki]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

On Wed, Nov 25, 2015 at 04:17:04PM +0100, Bahtiar Gadimov wrote:
> BTW: What is the reason for splitting the vms in different
> directories? It's not like there could be an foo AppVm and foo
> TemplateVm at the same time, or can it?

No. But this way you can mount different disk for templates and
appvms (for example keep templates on SSD). Also Qubes Live is using it
to more effectively use limited RAM.

- --
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2

iQEcBAEBCAAGBQJWVi62AAoJENuP0xzK19csFsMH/jrBowdynmi78wHYEh8svfky
tZn2geFKcIS8hgUGE4z7mzhrD7uqJHKTDioK3z78HBuuNgqQCDFAlKvPAV+9s2w1
VY9s+mHieLJjbVLaDUy6OH4HVR6SAGca1Fi5FR/lfejiSKejlBIW7zNpG6dY4CTa
25w6wjNGmy7Yfl5+NkHyIquulr9KXhGjN3cS/joIg+4gXR9pM23NMgtfCDHCN3ct
c/Gjs+JBvLlkEP5bVo9gRcLuD9UjVo4b/xzSyZ9/pf1xTqsuAoMRXXBQjtURKdtX
FmloO0ho15V+yDb1CmI82gwwHlY4aDdT7ZEySTmWZLpzkgCsP9atCsFDp7zWDRg=
=yMQG
-----END PGP SIGNATURE-----