Francesco Romani has posted comments on this change.

Change subject: core: containers: add the container support module

Patch Set 25:

File lib/vdsm/virt/containers/config/

PS25, Line 26: class AttrDict(MutableMapping):
> Do we *really* **need** this? Also, it's scope is broader than containers/c
Surely not, it was just nicer to me. It is a relic of the times on which 
convirt was an external, isolated project. Let me try to kill it.
File lib/vdsm/virt/containers/config/

PS25, Line 34: subnet='',
> why?
No good reason, I copied from somewhere so I don't have a clear rationale for 
File lib/vdsm/virt/containers/

PS25, Line 87: 
             :     # for testing purposes
             :     def clear(self):
             :         with self._lock:
> I don't think it's my first time saying this, but we should not pollute mod
surely it is not your first time, will clear this up

PS25, Line 99:, dom, *args)
> As a linux/unix user, I feel unsafe seeing this. :) Can you spare a few wor
will add a proper focstring.
File lib/vdsm/virt/containers/

PS25, Line 31: def rm_file(target):
> Why separate module within containers? There is utils rmFile. If you consid
(applies to the following comment too)
again, relic of times on which convirt was external module. We don't actually 
need this module anymore, will remove it.
File lib/vdsm/virt/containers/metrics/

PS25, Line 24: cgroups is linux-specific
> So is VDSM.
Good point, will remove this comment.

PS25, Line 36: Stats = None
> This could really use class type hint. Not sure if easily possible.
Will look into this.

PS25, Line 126: pass  # we don't support some cgroups
> Why?
it is a pending TODO :)

PS25, Line 152:     @property
              :     def cgroups(self):
              :         return self._cgroups
              :     @property
              :     def pid(self):
              :         return self._pid
> There is no reason for these to be property. We can move them to properties
Right. I just dislike those to be externally modifiable. But let's trust people 

PS25, Line 166: def _read_keyvalue(path, sep=' '):
> I have a feeling we do already have such function, and if not, many reimple
Will kill duplication.

Part of reader: doesn't use any of the Reader state, so one could argue it 
doesn't belong there :)
File lib/vdsm/virt/containers/

PS25, Line 35: class UnconfiguredXML(Exception):
> Uhm, to avoid having to read the docstring to understand the exception, wha
Better, will rename

PS25, Line 47: encoding = 'unicode' if six.PY3 else 'utf-8'
> I'd expect PY4 to keep the unicode.
right, let's reverse this check.

PS25, Line 66: cached
> Is this really a cache?
This - no. So the message is confusing and will be removed.
The point is that this XML is saved once the VM is started and never changed; 
the FS acts as a cache here.

PS25, Line 71: cached

PS25, Line 76: cached

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I0fb768ea97dd719cde9bd5e57e1b7cabe4b0f0ae
Gerrit-PatchSet: 25
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <>
Gerrit-Reviewer: Francesco Romani <>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <>
Gerrit-Reviewer: gerrit-hooks <>
Gerrit-HasComments: Yes
vdsm-patches mailing list --
To unsubscribe send an email to

Reply via email to