Targeted Refresh for Cloud VMs #74
Conversation
|
@Ladas could you help us take a look at this? Thanks! |
| def cloud_networks | ||
| return [] if references(:cloud_networks).blank? | ||
| @cloud_networks = references(:cloud_networks).collect do |network_id| | ||
| safe_get { network_service.networks.get(network_id) } |
There was a problem hiding this comment.
hm, could we use a filtered handled list? OpenStack should support list filtering by UUIDs right? In larger envs and e.g. bigger stack deployment, we can have hundreds of entities here. So that would be a lot of API calls
There was a problem hiding this comment.
Are you thinking of making a call to pull the whole inventory and then filtering that, or is there actually API support for requesting a set of UUIDs?
There was a problem hiding this comment.
Yeah we should be able to filter N records from the API, to avoid N+1 API requests. E.g. check server list https://developer.openstack.org/api-ref/compute/#list-servers , the parameter uuid can be used to filter by the instance uuid. And we should be able to provide more of them I think
There was a problem hiding this comment.
There is API docs mentioning IN query. But also that nova does not support OR filter. https://specs.openstack.org/openstack/api-wg/guidelines/pagination_filter_sort.html#filtering
But seem like these are just some guidelines, so not sure who actually follows them. :-)
There was a problem hiding this comment.
I suppose we can merge this as a first version and optimize it where possible later. If we can't get rid of the N+1 queries though, we will need to put back the condition, that will run a full refresh based on the threshold. (Best placement might be the Inventory builder) The old refresh condition is here https://github.com/Ladas/manageiq/blob/04f82ac2f4601144370d760d6f5d221f1cc88ca5/app/models/ems_refresh/refreshers/ems_refresher_mixin.rb#L64
I don't have that condition for AWS, since the API support the filtering everywhere. :-)
There was a problem hiding this comment.
Oh, that is awesome. I didn't know it was possible to query like that. Should be easy enough to figure out what's actually supported and fix this PR to get rid of the N+1 requests.
There was a problem hiding this comment.
Yeah, lets do it in the next PR, you will probably need to experiment. I fear the OpenStack API is still heavily inconsistent at this. :-(
There was a problem hiding this comment.
Will do, thanks for the advice.
There was a problem hiding this comment.
This is definitely going to be more tricky than hoped, Nova doesn't seem to respect the "in:" directive.
There was a problem hiding this comment.
yeah, I wonder which service will do :-) btw. the guideline was mentioning nova might not support any kind of OR condition :-(
| @@ -863,5 +863,107 @@ def expect_sync_cloud_tenants_with_tenants_is_queued | |||
| expect(sync_cloud_tenant.method_name).to eq("sync_cloud_tenants_with_tenants") | |||
| expect(sync_cloud_tenant.state).to eq(MiqQueue::STATE_READY) | |||
| end | |||
|
|
|||
| def assert_targeted_vm(vm_name, attributes) | |||
There was a problem hiding this comment.
would be nice to have a test using existing Vm asserts, I do that in AWS to make sure Vm is the same for targeted and full refresh
There was a problem hiding this comment.
I'm not using exactly the same, since it seemed like it was not possible to acquire the external network information from the information available from just the VM. It seemed like it would take basically a full pull of inventory from Neutron to get some of it, so I left the external network stuff out of the targeted refresh and dropped the external network checks from the test.
There was a problem hiding this comment.
Right so what I do in AWS is a full VM refresh now, so I expand more targets out of the Vm, so the result is a Vm with all network connections(IPs) and volumes.
There was a problem hiding this comment.
Sorry, what do you mean by "full VM refresh"? I'm not sure if you're referring to a different sort of approach, or just using a different name. :)
There was a problem hiding this comment.
So for AWS, every time there is a reference to Vm (aka. Vm target), we unpack the list of references by looking at the DB:
and by looking at the API:
https://github.com/Ladas/manageiq-providers-amazon/blob/6938025f2cd6a4b0504909a1be22a87e30dbc7c0/app/models/manageiq/providers/amazon/inventory/collector/target_collection.rb#L229
So we basically send more Targets to the refresh, by scanning edges of the Vm.
Then the Vm full refresh means that after 1 targeted refresh, you will get a full info about the Vm. Where the most important relations are probably IP addresses and volumes. Then it's easier to use this e.g. in a automate workflow, where 1 step is the sync refresh of the Vm and you want to work with the data in the next step.
Also for other releases, the refresh should be configurable, so we can invoke targeted Vm or full targeted Vm refresh, based on the needs.
Also this solves the fact that events might come in a wrong order, then the graph refresh will not be able to populate some edges. (for the 5.0 release, we will be solving these by a skeletal pre-create and a post processing where the missing edges will be planned for the next refresh)
There was a problem hiding this comment.
Oh, I think I see what you mean. "Targeted VM Refresh" just being the VM object itself, and "Full Targeted VM Refresh" being the VM and its relationships. Makes sense now. I'm refreshing the VM and its relationships, but it seems like not all the relevant information to populate the network stuff was available. (Since you only get the IP address not it's ID, and you don't have enough port information to reconstruct the whole network just from the VM and its relationships due to objects from the Openstack API not having references in both directions). Though if Neutron supports filtered querying like you mentioned, maybe I can get around that. I was working on this before 2 weeks of PTO so my memory is fuzzy about the exact nature of the problem.
There was a problem hiding this comment.
it will require some tweaking, yo ucan also do more API queries to fetch e.g. the network_ports, given you will be able to filter that API using e.g. a Vm uuid
|
@mansam it's great, just 1 concern with get vs filtered list API For the specs, we should add moooore in next PRs, so every possible target has at least some coverage. :-D In aws I have https://github.com/Ladas/manageiq-providers-amazon/blob/cd02dc09e805424ef90cf30f082f710000c09319/spec/models/manageiq/providers/amazon/cloud_manager/vcr_specs/targeted_refresh_spec.rb#L32 next steps, analyzing events and calling refresh from automate. I have not yet merged handler here it calls and using event parser to extract targets out of the event: then from automate event handler we just call: comparing the the old way: |
|
@mansam just call |
mansam
force-pushed
the
targeted-refresh
branch
from
2388aa1
to
a788440
Compare
mansam
force-pushed
the
targeted-refresh
branch
from
a788440
to
2087eec
Compare
|
@mansam are any of the codeclimate issues relevant? |
|
@tzumainn I've fixed the only ones that were relevant, so this should be ok to merge once the tests go green again. |
|
Some comments on commits mansam/manageiq-providers-openstack@2087eec~...670c192 spec/models/manageiq/providers/openstack/cloud_manager/stubbed_refresher_spec.rb
|
|
Checked commits mansam/manageiq-providers-openstack@2087eec~...670c192 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/models/manageiq/providers/openstack/inventory/collector/target_collection.rb
|
This pull request introduces the scaffold for targeted refresh and implements it for cloud VMs. This is based significantly on @Ladas's work on the Amazon provider.