yahoo-eng-team team mailing list archive

[Sean Dague <sean@xxxxxxxxx>]

While there is definitely debt in here, I'm not convinced that this
artifact helps us move forward on it. I'm going to mark as Opinion. Feel
free to reopen.

** Changed in: nova
       Status: Incomplete => Opinion

-- 
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/964763

Title:
  confusing network manager read_deleted usage

Status in OpenStack Compute (Nova):
  Opinion

Bug description:
  This is related to the fix for bug #939580

  Reading the code:

          # NOTE(francois.charlier): in some cases the instance might be                                                                     
          # deleted before the IPs are released, so we need to get deleted                                                                   
          # instances too                                                                                                                    
          read_deleted_context = context.elevated(read_deleted='yes')
          ....
          try:
              fixed_ips = self.db.fixed_ip_get_by_instance(read_deleted_context,
                                                           instance_id)
          except exception.FixedIpNotFoundForInstance:

  then:

    def fixed_ip_get_by_instance(context, instance_id):
        result = model_query(context, models.FixedIp, read_deleted="no").\
                     filter_by(instance_id=instance_id).\
                     all()

  then:

    def model_query(context, *args, **kwargs):
        ...
        read_deleted = kwargs.get('read_deleted') or context.read_deleted

  It's obvious the read_deleted flag is ignored here. And, in any case,
  we only want to read deleted instances, not deleted fixed IPs.

  Now, I understand what's going on - later, in
  _disassociate_floating_ip(), we do:

      instance = self.db.instance_get(context, fixed_ip['instance_id'])

  and it's *this* call we want to pass the read_deleted flag.

  I started moving the setting of read_deleted on the context closer to
  where we actually needed, but it's hard to do this with confidence
  because there are many DB calls which the author of the fix may have
  intended to pass read_deleted='yes' to.  This is a sign that we've
  seriously confused the code by setting read_deleted='yes' in a fairly
  arbitrary place.

To manage notifications about this bug go to:
https://bugs.launchpad.net/nova/+bug/964763/+subscriptions