confusing network manager read_deleted usage

[Mark McLoughlin]

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.

[Joe Gordon (jogo)]

Is this still valid? The code has changed a lot since this was filed

[Sean Dague (sdague)]

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.