yahoo-eng-team team mailing list archive
-
yahoo-eng-team team
-
Mailing list archive
-
Message #01019
[Bug 887692] Re: The QuantumManager could use some refactoring
The quantum interface isn't implemented as a manager anymore, so this
isn't relevant anymore.
** Changed in: nova
Status: Confirmed => Invalid
--
You received this bug notification because you are a member of Yahoo!
Engineering Team, which is subscribed to OpenStack Compute (nova).
https://bugs.launchpad.net/bugs/887692
Title:
The QuantumManager could use some refactoring
Status in OpenStack Compute (Nova):
Invalid
Bug description:
These changes were suggested by Aaron Lee during his review:
Patch Set 9: (5 inline comments)
I'm giving this a 0 because all of my comments are about refactorings
that would have made it easier for me to read what this patch is
doing. The functionality looks good, as far as I can tell, but I think
the code needs to be cleaner.
....................................................
File nova/network/linux_net.py
Line 995: # Don't forward traffic unless we were told to be a gateway
I would rather see the actions within the conditional more focused on the decision of the conditional.
i.e.
guard = "DROP"
if gateway
guard = "ACCEPT"
for direction in ['in', 'out']
iptables_manager.ipv4['filter'].add_rule('FORWARD',
'--$(direction)-interface %(guard)s -j %(bridge)s' % \
locals())
This will make it much easier to fix errors in the generation of those
commands.
....................................................
File nova/network/quantum/manager.py
Line 213: "label": "quantum-net-%s" % quantum_net_id}
I don't think this get network ref logic is a part of allocating for an instance. It would be better stored in it's own method.
Line 234: LOG.info("Using DHCP for network: %s" % network_ref['label'])
This would be much easier to maintain and test in small units if each step was a method, and enable_dhcp just called each individual method. Refactoring like that would also make more granular error handling easier.
Line 449: subnet['network_id'], project_id)
It appears the real meat of this function is these three calls to the driver, it should be separated from populating the network_ref.
This data mutating done on this response from get_subnets_by_net_id is
very similar to the changes made to this data on both other calls to
get_subnets_by_net_id. This is another candidate for refactoring, but
may be out of the scope of this commit.
Line 485: def get_dhcp_hosts_text(self, context, subnet_id, project_id=None):
These methods share 10 lines, maybe the collection of information and iteration over that list could be abstracted?
There are a few others I wanted to do as well.
To manage notifications about this bug go to:
https://bugs.launchpad.net/nova/+bug/887692/+subscriptions