Extending versioned notifications for searchlight integration

Registered by Balazs Gibizer on 2016-11-17

The Searchlight project reported multiple bugs [1],[2],[3] on nova about
missing data in nova's notifications. Searchlight treats nova notifications
as a complete record of the state of nova entities especially servers.
However the current nova notifications missing some of the fields
therefor Searchlight today needs to query the Nova API to get the full
picture. The current API polling solution does not scale well in Searchlight
therefore we are proposing the following additions to the nova versioned
notifications.

Let's try the following steps

Step1: Add simple fields to instance.create and instance.update

* The list of Tag objects based on instance.tags
* The list of KeyPair objects containing at least the
name field of the original KeyPair objects based on instance.keypairs
* The instance's display_description field
* The instance's locked field
* The instance's auto_disk_config field

This technically means that the InstancePayload needs to be extended with new
fields. This will automatically makes both the instance.create and
instance.update (and all the instance.<action>) content extended.

Step2 Add BDM related data to instance.create and instance.update

The following fields from the BlockDeviceMapping object where
the instance_uuid of the BDM matches the instance's uuid the notification
is sent about:
* volume_id
* delete_on_termination

Only include a BDM into the notification payload if the volume_id of the BDM
is not None. This is needed to keep the notification content consistent
with the nova API response.

To be able to initialize the BDM part of the notifications the BDM needs
to be available in the context of the notification sending. In case of
instance.create it is already available but in case of instance.update this
needs to be queried. So to avoid unnecessary performance impact make sending
the BDM data configurable. Default the new config parameter to False as
far as including BDMs in notifications in the cases that we need to query
them from the database.

[1] https://bugs.launchpad.net/nova/+bug/1637634
[2] https://bugs.launchpad.net/nova/+bug/1637639
[3] https://bugs.launchpad.net/nova/+bug/1637641

Blueprint information

Status:
Complete
Approver:
Matt Riedemann
Priority:
High
Drafter:
Balazs Gibizer
Direction:
Approved
Assignee:
Balazs Gibizer
Definition:
Approved
Series goal:
Accepted for pike
Implementation:
Implemented
Milestone target:
milestone icon pike-3
Started by
Matt Riedemann on 2016-12-07
Completed by
Matt Riedemann on 2017-07-25

Whiteboard

(mriedem): I have a couple of concerns with the BDM part of this. Today the REST API shows attached volumes when showing details on a given server, but only if the volume_id is set (which it should be for an attached volume after it's attached, but might not be during boot from volume where nova is creating the volume and it's not yet attached but you're polling the server for status):

https://github.com/openstack/nova/blob/f8a81807e016c17e6c45d318d5c92ba0cc758b01/nova/api/openstack/compute/extended_volumes.py#L25

You can create a server using boot from volume with an image_id or a volume snapshot_id in which case the volume_id on the incoming block_device_mapping_v2 request won't have the volume_id set. So depending on where the instance.create notification happens, that BDM.volume_id might not be set for the BFV case.

The other thing is when showing volume attachments for a given server, we only show volume id, server id and mountpoint (device_name) in the response:

https://github.com/openstack/nova/blob/f8a81807e016c17e6c45d318d5c92ba0cc758b01/nova/api/openstack/compute/volumes.py#L222

We've been talking about removing device_name from the server create request as we can't honor it in most cases anyway (the libvirt driver ignores it), and unless nova creates the volume the volume_size is optional when creating a server, so I'm not sure why those are necessary in the notification.

Finally, for the case where we need to send instance.update notifications, if we don't have the BlockDeviceMappingList in scope already, as noted we'd have to query the database for it. We had talked about making that configurable, like with notify_on_state_change configuration value. I think that's something we might want to use here too, and default it to False as far as including BDMs in notifications in the cases that we need to query them from the database because anyone not using Searchlight probably doesn't need to incur that performance cost. Granted, if Nova had a way to ask oslo.messaging if notifications were even turned on, we could short-circuit even earlier, but that's an open RFE to oslo.messaging still: https://bugs.launchpad.net/oslo.messaging/+bug/1639287

--

My recommendation would be to start working on the non-BDM parts of this blueprint and then we can hash out the BDM parts when we get there but save that for the end of the series.

--

(gibi): I agree with mriedem's comments so I update the BP text accordingly

--

(mriedem): 2016-11-28: I see that the additional instance fields are coming from bug [1] but I'm wondering about some other fields returned when getting server details with microversion >= 2.3:

http://docs.openstack.org/developer/nova/api_microversion_history.html#maximum-in-kilo

For example, launch_index, root_device_name and userdata. Those were added for the ec2 API split out of nova, but I'm wondering if we want those in the notifications also for parity with the REST API. If Searchlight doesn't need those right now, then I'm fine with just making incremental changes to the instance notification payload as needed so we don't over-complicate this.

Overall I'm OK with this as a starting point though so I'm approving it for Ocata.

Gerrit topic: https://review.openstack.org/#q,topic:bp/additional-notification-fields-for-searchlight,n,z

Addressed by: https://review.openstack.org/407128
    Add instance's locked field in InstancePayload

Gerrit topic: https://review.openstack.org/#q,topic:bp-extending-versioned-notifications,n,z

Addressed by: https://review.openstack.org/407228
    Adding tags to InstancePayload

Addressed by: https://review.openstack.org/413267
    Add key_name field to InstancePayload

Addressed by: https://review.openstack.org/415298
    Change tags to default field in Instance object.

Gerrit topic: https://review.openstack.org/#q,topic:bp/additional-notification-fields-for-searchlight-patch13,n,z

Gerrit topic: https://review.openstack.org/#q,topic:bp/additional-notification-fields-for-searchlight-patch10,n,z

Addressed by: https://review.openstack.org/419185
    Adding auto_disk_config field to InstancePayload

Addressed by: https://review.openstack.org/419730
    Add keypairs field to InstancePayload

We're now past the feature freeze for Ocata so I've deferred this to Pike. -- mriedem 20170128

Addressed by: https://review.openstack.org/435146
    This patch is followup for https://review.openstack.org/#/c/415298/.

Bumping this to high priority since this is needed for nova integrating with Searchlight for listing instances across multiple cells. -- mriedem 20170222

ML thread aboutt the BDM modelling: http://lists.openstack.org/pipermail/openstack-dev/2017-March/113850.html

Addressed by: https://review.openstack.org/448779
    [WIP] Add BDM to InstancePayload

Gerrit topic: https://review.openstack.org/#q,topic:bp/versioned-notification-transformation-pike,n,z

Addressed by: https://review.openstack.org/443686
    Using max api version in notification sample test

Addressed by: https://review.openstack.org/459493
    Add tags to instance.create Notification

Addressed by: https://review.openstack.org/463001
    Add separate instance.create payload type

Addressed by: https://review.openstack.org/463002
    Add key_name field to InstancePayload

Addressed by: https://review.openstack.org/483324
    use already loaded BDM in instance.<action>

Gerrit topic: https://review.openstack.org/#q,topic:bp/support-tag-instance-when-boot,n,z

Addressed by: https://review.openstack.org/483955
    use already loaded BDM in instance.<action> (2)

Addressed by: https://review.openstack.org/483969
    use already loaded BDM in instance.create

Now that server tags are included in the instance.create notifications, I'm going to mark this complete for Pike. -- mriedem 20170724

(?)

Work Items

Dependency tree

* Blueprints in grey have been implemented.

This blueprint contains Public information 
Everyone can see this information.