← Back to team overview

yahoo-eng-team team mailing list archive

[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