notification.info_from_instance should use get()

[Phil Day]

notification.info_from_instance() reads many values from the instance structure, including capacity vales that are populated from the instance_system_metadata table. However there are cases where these values are not present - for example if a deleted instance is passed in then the DB queries do not always do the joins. This results in a KeyError exception.

Whilst such cases are triggered by bugs the notification code should be more robust and use .get() methods instead.

[Jake Liu (jake-liu)]

Hi Phil, can you please give more detail on the problem.

Do you mean that we need change the following to instance_ref.get()?

instance_info = dict(
        # Owner properties
        tenant_id=instance_ref['project_id'],
        user_id=instance_ref['user_id'],

        # Identity properties
        instance_id=instance_ref['uuid'],
        display_name=instance_ref['display_name'],
        reservation_id=instance_ref['reservation_id'],
        hostname=instance_ref['hostname'],

        # Type properties
        instance_type=instance_type_name,
        instance_type_id=instance_ref['instance_type_id'],
        architecture=instance_ref['architecture'],

        # Capacity properties
        memory_mb=instance_ref['memory_mb'],
        disk_gb=instance_ref['root_gb'] + instance_ref['ephemeral_gb'],
        vcpus=instance_ref['vcpus'],
        # Note(dhellmann): This makes th
        ...............

I'm not clear about the following:
notification.info_from_instance() reads many values from the instance structure, including capacity vales that are populated from the instance_system_metadata table.
>>>>>>>>> How does capacity vales get from system metadata in this function?

Thanks.

[Jeffrey Zhang (jeffrey4l)]

1. the info_from_instance() method return a dict object already.
2. the instance_ref should came from DB. And the deletion doesn't remove the data, the just mark it as delete. So your issue should not exist.

[Mathew Odden (locke105)]

Phil, can you confirm this is still an issue?

[Phil Day (philip-day)]

To answer your original question Jake yes that was the code that I thought should be changed to use get.

I don't have any current examples of where this is causing a problem, as the bug that led me to this has a fix that avoids thsi code altogether. However it still feels like an exception waiting to happen

[Sean Dague (sdague)]

Long incomplete bug, marking as Opinion. It might be valid, but it's not anything actionable.