Martin Polednik has posted comments on this change. Change subject: core: containers: add the container support module ......................................................................
Patch Set 25: (15 comments) partialr eview 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/config. 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? PS25, Line 34: subnet='10.1.0.0', Also, this is enough addresses for 65534. How many containers can beefy virtualization host run? 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 module API with testing utilities. PS25, Line 99: root.fire(event_id, dom, *args) As a linux/unix user, I feel unsafe seeing this. :) Can you spare a few words on the name, do you expect it to be a proper hierarchy? 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 consider rmFile wrong, fix it first. PS25, Line 42: def read_file(path): : with open(path, 'rt') as f: : return f.read() I'm unsure of the benefit of having this in a function within containers. 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. PS25, Line 36: Stats = None This could really use class type hint. Not sure if easily possible. PS25, Line 126: pass # we don't support some cgroups Why? 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 if some more sophisticated get/set action is needed. PS25, Line 166: def _read_keyvalue(path, sep=' '): I have a feeling we do already have such function, and if not, many reimplementations of the same thing. Do we really need new one? Why is it not a part of Reader? 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, what about MissingXML? PS25, Line 47: encoding = 'unicode' if six.PY3 else 'utf-8' I'd expect PY4 to keep the unicode. PS25, Line 66: cached Is this really a cache? -- 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 <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: gerrit-hooks <[email protected]> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/admin/lists/[email protected]
