Dan Kenigsberg has posted comments on this change. Change subject: tc: Integrate Host QoS with network addition via setupNetworks ......................................................................
Patch Set 12: (4 comments) http://gerrit.ovirt.org/#/c/30467/12/vdsm/network/configurators/qos.py File vdsm/network/configurators/qos.py: Line 28: StrictVersion('3.5.0')) else 'sfq' Line 29: Line 30: Line 31: def configure_outbound(qosOutbound, top_device): Line 32: """Adds the qosOutbound configuration to the backing device (be it bond "backing device" is a better term than log_dev. Let's use the first instead of the latter. Line 33: or nic). Adds a class and filter for default traffic if necessary""" Line 34: description = top_device.description Line 35: vlan_tag = description.get('vlan_tag') Line 36: device = description['log_dev'] Line 36: device = description['log_dev'] Line 37: qdiscs = list(tc._qdiscs(device)) Line 38: root_qdisc = _root_qdisc(qdiscs) Line 39: class_id = '%x' % (_NON_VLANNED_ID if vlan_tag is None else vlan_tag) Line 40: if not root_qdisc or root_qdisc['kind'] != 'hfsc': 'hfsc' strings appears too often, risking a typo. Could you define a constant for any repeated qos term? Line 41: _fresh_qdisc_conf_out(device, vlan_tag, class_id, Line 42: _qos_to_str_dict(qosOutbound)) Line 43: else: Line 44: _qdisc_conf_out(device, root_qdisc['handle'], vlan_tag, class_id, http://gerrit.ovirt.org/#/c/30467/12/vdsm/network/models.py File vdsm/network/models.py: Line 111: def __repr__(self): Line 112: return 'Nic(%s)' % self.name Line 113: Line 114: @property Line 115: def description(self): To me, "description" is a human-readable text. Not a bag of fact extracted from a device hierarchy. I suppose that this dictionary was introduced to simplify the extraction of information from the underlying devices. Do you plan to add more stuff into there, beyond log_dev and vlan_tag? If not, maybe the "description" mechanism is not worth it. It is also not clear to me if we should place an attribiute like vlan_tag on a device even though it belongs to an interface below it. How about a module-level function that traverses a network to find its vlan tag def get_vlan_tag(network): if bridged: .... and another to report its logical device? I admit they may be have a lot in common, but more fitting the object model. Line 116: """Returns a dictionary with information about the underlying hierarchy Line 117: of the device""" Line 118: return {'log_dev': self.name} Line 119: Line 114: @property Line 115: def description(self): Line 116: """Returns a dictionary with information about the underlying hierarchy Line 117: of the device""" Line 118: return {'log_dev': self.name} "log_dev" is too opaque name for the nic or bond underlying beneath the network. Line 119: Line 120: Line 121: class Vlan(NetDevice): Line 122: MAX_ID = 4094 -- To view, visit http://gerrit.ovirt.org/30467 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I4a5378870a3dac9307a6eef8d9937a9c03a0c819 Gerrit-PatchSet: 12 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
