flavor show shouldn't read deleted flavors.

[melanie witt]

An instance type is created by:

return db.instance_type_create(context.get_admin_context(), kwargs)

which uses the read_deleted="no" from the admin context.

This means, as seen in nova/tests/test_instance_types.py:

def test_read_deleted_false_converting_flavorid(self):
    """
    Ensure deleted instance types are not returned when not needed (for
    example when creating a server and attempting to translate from
    flavorid to instance_type_id.
    """
    instance_types.create("instance_type1", 256, 1, 120, 100, "test1")
    instance_types.destroy("instance_type1")
    instance_types.create("instance_type1_redo", 256, 1, 120, 100, "test1")

    instance_type = instance_types.get_instance_type_by_flavor_id(
            "test1", read_deleted="no")
    self.assertEqual("instance_type1_redo", instance_type["name"])

flavors with colliding ids can exist in the database.

From the test we see this looks intended, however it results in undesirable results if we consider the following scenario.

For 'show' in the flavors api, it uses read_deleted="yes". The reason for this is if a vm was created in the past with a now-deleted flavor, 'nova show' can still show the flavor name that was specified for that vm creation. The flavor name is retrieved using the flavor id stored with the instance.

Well, if there are colliding flavor ids in the database, the first of the duplicates will be picked, and it may not be the correct flavor for the vm.

This leads me to believe that maybe at flavor create time, colliding ids should not be allowed, i.e. use

return db.instance_type_create(context.get_admin_context(read_deleted="yes"),
                               kwargs)

to prevent the possibility of colliding flavor ids.

[Boris Pavlovic (boris-42)]

I don't think that such approach is good.

If we have another situation.
User has create some flavor than he deleted it, and then he want to create one more time with same flavorid and he couldn't do it... Why?

For example if we are using dashboard then flavor update method do next: (delete old flavor, create new with same name but other parameters) So there will be a lot of problems...

Probably the right way is to store in instances not only flavorid but also instance_type_id but I am not sure...

[Vish Ishaya (vishvananda)]

we do store instance_type_id in the instance so the relationship is still fine. It just means that flavor get will be confusing. We now store all of the important flavor data in system_metadata so probably we need to switch to not allowing show for deleted flavors and instead provide a way to get flavor data for an instance from system_metadata instead of the flavors table.

[Rohan (kanaderohan)]

It seems like we are already pulling flavor data from instance's system_metadata. We can now safely do "read_deleted=no" while showing deleted flavors.

[Rohan (kanaderohan)]

Do we have a consensus on not showing deleted flavors? since showing new and deleted flavors with same flavor_id can be confusing to user.

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[OpenStack Infra (hudson-openstack)]

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

[David Kranz (david-kranz)]

Changing a 200 to a 404 is an explicit no-no according to the api stability policy. Is this issue, which I presume is a longstanding one, really serious enough to violate that policy in a soon-to-be-deprecated api?

[Rohan (kanaderohan)]

The issue being solved here is when a deleted flavor's info has to be shown in the instance details.

The deleted flavor might have been replaced with some other flavor with the same flavor id, hence while showing instance details, the api might fetch completely different flavor details since the flavor id matches.

To solve this, we can do
1) Always fetch flavor details from instance's system_metadata instead of flavor api

2) Fetch flavor details from instance's system_metadata only if the original flavor is marked as deleted or 404 when trying to fetch that old flavor (my current patch does this)

1st option doesnt break flavor api get status from 200 to 404, it doesnt touch the flavor api at all.
2nd option has to rely on flavor api not getting deleted flavors in the flavor get api call.

[Julie Pichon (jpichon)]

Adding the ceilometer folks as I imagine this is affecting Ceilometer too - since "nova show" can sometimes return the wrong flavor, it may well impact some of the meter types.

(It seems to be reproducible quite consistently on devstack, creating a m1.tiny instance as the demo user, deleting the flavor and creating a new flavor with id 1 - "nova show" will show the new flavor rather than the real one for this instance).

[Joe Gordon (jogo)]

nova patch was abandoned, moving back to new to be re-triaged

[melanie witt (melwitt)]

novaclient patch was abandoned, moving back to new to be re-triaged