Francesco Romani has posted comments on this change. Change subject: core: containers: add the container support module ......................................................................
Patch Set 25: (15 comments) https://gerrit.ovirt.org/#/c/59824/25/lib/vdsm/virt/containers/config/__init__.py File lib/vdsm/virt/containers/config/__init__.py: 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. https://gerrit.ovirt.org/#/c/59824/25/lib/vdsm/virt/containers/config/network.py File lib/vdsm/virt/containers/config/network.py: PS25, Line 34: subnet='10.1.0.0', > why? No good reason, I copied from somewhere so I don't have a clear rationale for that. https://gerrit.ovirt.org/#/c/59824/25/lib/vdsm/virt/containers/events.py File lib/vdsm/virt/containers/events.py: PS25, Line 87: : # for testing purposes : def clear(self): : with self._lock: : self.events.clear() > 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: root.fire(event_id, dom, *args) > As a linux/unix user, I feel unsafe seeing this. :) Can you spare a few wor will add a proper focstring. https://gerrit.ovirt.org/#/c/59824/25/lib/vdsm/virt/containers/fs.py File lib/vdsm/virt/containers/fs.py: 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. https://gerrit.ovirt.org/#/c/59824/25/lib/vdsm/virt/containers/metrics/cgroups.py File lib/vdsm/virt/containers/metrics/cgroups.py: 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 here. 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 :) https://gerrit.ovirt.org/#/c/59824/25/lib/vdsm/virt/containers/xmlfile.py File lib/vdsm/virt/containers/xmlfile.py: 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 same PS25, Line 76: cached same -- To view, visit https://gerrit.ovirt.org/59824 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0fb768ea97dd719cde9bd5e57e1b7cabe4b0f0ae Gerrit-PatchSet: 25 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list -- vdsm-patches@lists.fedorahosted.org To unsubscribe send an email to vdsm-patches-le...@lists.fedorahosted.org
