Change in vdsm[master]: WIP: Multiple Gateways Feature

gvallare at redhat.com gvallare at redhat.com
Sun Jun 9 17:01:05 UTC 2013


Giuseppe Vallarelli has posted comments on this change.

Change subject: WIP: Multiple Gateways Feature
......................................................................


Patch Set 10: I would prefer that you didn't submit this

(2 inline comments)

Design can be further improved, by reading the list of methods is clear that you can have a static or a dynamic source route with related operations, this will make classes more cohesive.

....................................................
File vdsm/sourceRoute.py
Line 49:     def _buildRules(self):
Line 50:         return [ipwrapper.Rule(source=self.network, table=self.table),
Line 51:                 ipwrapper.Rule(destination=self.network, table=self.table)]
Line 52: 
Line 53:     def _writeCommands(self, commands, type=''):
Can type be an empty string? My understanding is that it should be one among route and rule.
Line 54:         filePath = os.path.join(netinfo.NET_CONF_DIR,
Line 55:                                 '%s-%s' % (type, self.device))
Line 56:         with open(filePath, 'w') as file:
Line 57:             for command in commands:


Line 90: 
Line 91:         self.routes = self._buildRoutes()
Line 92:         self.rules = self._buildRules()
Line 93: 
Line 94:         if self.isStatic:
This static or dynamic configuration question is common also in another patch, I wonder if it would make sense to make this explicit, looks like an important aspect from a domain perspective, instead of a simple boolean they should be 2 different types routes and rules are build in a completely different way this difference can be seen in other different methods. Basically you have almost 2 methods per action one for static configuration and one for dynamic configuration.
Line 95:             self._addRoutesStatic()
Line 96:             self._addRulesStatic()
Line 97:         else:
Line 98:             self._addRoutesDHCP()


-- 
To view, visit http://gerrit.ovirt.org/15207
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I0224d896724b9cdc44215e92f0da0be71fd19038
Gerrit-PatchSet: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Assaf Muller <amuller at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Giuseppe Vallarelli <gvallare at redhat.com>
Gerrit-Reviewer: Livnat Peer <lpeer at redhat.com>
Gerrit-Reviewer: Mark Wu <wudxw at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server


More information about the vdsm-patches mailing list