PCI devices claimed on compute node during _claim_test()

[Jay Pipes]

The nova.compute.claims.Claim object is used to test whether a set of requested resources can be satisfied by the compute node. In the constructor of the Claim object, the Claim._claim_test() object is called:

    def __init__(self, context, instance, tracker, resources, overhead=None,
                 limits=None):
        super(Claim, self).__init__()
        <snip>
        # Check claim at constructor to avoid mess code
        # Raise exception ComputeResourcesUnavailable if claim failed
        self._claim_test(resources, limits)

If we take a look at _claim_test(), we see pretty clearly that resources are NOT supposed to be actually claimed -- instead, the method should only *check* to see if the request can be fulfilled:

    def _claim_test(self, resources, limits=None):
        """Test if this claim can be satisfied given available resources and
        optional oversubscription limits

        This should be called before the compute node actually consumes the
        resources required to execute the claim.

        :param resources: available local compute node resources
        :returns: Return true if resources are available to claim.
        """
       <snip>
        reasons = [self._test_memory(resources, memory_mb_limit),
                   self._test_disk(resources, disk_gb_limit),
                   self._test_vcpus(resources, vcpus_limit),
                   self._test_numa_topology(resources, numa_topology_limit),
                   self._test_pci()]
        reasons = reasons + self._test_ext_resources(limits)
        reasons = [r for r in reasons if r is not None]
        if len(reasons) > 0:
            raise exception.ComputeResourcesUnavailable(reason=
                    "; ".join(reasons))

Unfortunately, the PCI devices are *actually* claimed in the _test_pci() method:

    def _test_pci(self):
        pci_requests = objects.InstancePCIRequests.get_by_instance_uuid(
            self.context, self.instance.uuid)

        if pci_requests.requests:
            devs = self.tracker.pci_tracker.claim_instance(self.context,
                                                           self.instance)
            if not devs:
                return _('Claim pci failed.')

What this means is that if an instance is attempted to be launched on a compute node and that instance has PCI requests that can be satisfied by the compute host, but say, there isn't enough available RAM on the node, the Claim will raise ComputeResourcesUnavailable which will trigger a Retry operation to the scheduler, but the PCI devices will have already been marked as claimed by that instance in the PCI device tracker:

            devs = self.tracker.pci_tracker.claim_instance(self.context,
                                                           self.instance)

The above code actually marks one or more PCI devices on the compute host as claimed for the instance. This introduces inconsistent state into the system. Making things worse is the fact that the nova.pci.manager.PciDevTracker object uses the nova.pci.stats.PciDevStats object for tracking consumed quantities of "pools" of the PCI device types and both the stats aggregation AND the PciDevTracker.pci_devs PciDeviceList object have their state changed improperly.

[Jay Pipes (jaypipes)]

This really is a data/state-corruption bug, since it erroneously consumes PCI devices on the host even when the Claim does not succeed.

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/285865

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/291847

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/291847
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=66e79e1bbca66a6f0f7be14006a87426c4dda402
Submitter: Jenkins
Branch: master

commit 66e79e1bbca66a6f0f7be14006a87426c4dda402
Author: Jay Pipes <email address hidden>
Date: Fri Mar 11 13:16:40 2016 -0500

    pci - Claim devices outside of Claim constructor

    During the nova.compute.Claim.__init__() call, there are a bunch of
    _test_XXX() methods that are called. These methods should test to see
    whether the requested resources of various types can be satisfied by the
    inventory on the ComputeNode. However, that inventory *should not* be
    claimed for a particular request during the Claim object's constructor.

    The Claim._test_pci() method was *actually* claiming the PCI device for
    the requested instance. Unfortunately, this meant that if an instance
    launch request's demand for a resource like RAM was not able to be
    satisfied by the compute node but the launch request's demand for PCI
    devices *was* able to be satisfied by the compute node, those PCI
    devices were actually claimed for the instance even though the claim
    itself would end up being aborted. This resulted in a data
    corruption/inconsistency where a PCI device would be claimed for an
    instance that actually was not running on the node.

    This patch moves the claim of PCI devices out of the _test_pci() method
    and into the ResourceTracker.instance_claim() method. In the process of
    fixing this bug, it was discovered that the unit tests for the Claim
    object with regards to PCI devices were just plain broken. They were
    testing for nothing at all because of the way the Claim constructor
    works. These unit tests were reworked completely, along with the
    MoveClaim unit tests which similarly were not testing the PCI code paths
    at all. An additional unit test was added on the resource tracker to
    verify that nova.pci.manager.PciDevTracker.claim_instance() is called
    when PCI requests are included and satisfied by the Claim.

    Change-Id: Icf75439a552de84ec31c1a47faeee3caf8a5b0a7
    Closes-bug: #1549984

[OpenStack Infra (hudson-openstack)]

Change abandoned by Jay Pipes (<email address hidden>) on branch: master
Review: https://review.openstack.org/285865
Reason: Fixed in https://review.openstack.org/291847