| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2019-03-05 | |||
| 15:35:16 | mriedem | grumble | |
| 15:35:18 | sean-k-mooney | gibi: in that case it would not be in the cache and we can fallback to hitting the api | |
| 15:35:33 | sean-k-mooney | so check the cache if its there use it if not hit api | |
| 15:35:37 | mriedem | sean-k-mooney: but then you're going to always hit the api because not all instances have these types of ports | |
| 15:35:44 | mriedem | so... | |
| 15:35:58 | mriedem | you'd have to condition based on the instance.vm_state, but anyway, let's not worry about this right now | |
| 15:36:34 | openstackgerrit | Andrey Volkov proposed openstack/nova stable/queens: Test for plug_contrail_vif with utf8 VM name https://review.openstack.org/641048 | |
| 15:36:49 | sean-k-mooney | ok there is a comment in the code to optimise this anyway so ya we can look at that in train | |
| 15:37:19 | gibi | if in train we can implement the move operations then we can drop the whole check from the aPI | |
| 15:37:34 | gibi | bah, not ture | |
| 15:37:35 | gibi | true | |
| 15:37:49 | mriedem | not true b/c older microversions need to fail right? | |
| 15:37:55 | gibi | yeah | |
| 15:38:03 | gibi | we need to support old microversions | |
| 15:38:19 | gibi | but we have the microversion check in place | |
| 15:38:31 | gibi | so on old microversions we can simply check the microversion not the port allocations | |
| 15:38:50 | gibi | we only need the current check as the current 2.72 is not complete | |
| 15:39:11 | gibi | (or we can go back to the discussion how many microversin we need for this feature) | |
| 15:39:47 | sean-k-mooney | gibi well for old microverion you still need the check as we are only failing move operation if you have a port resouce request | |
| 15:39:55 | mriedem | correct ^ | |
| 15:40:15 | mriedem | just because 2.80 supports moving those types of servers doesn't mean that all of a sudden 2.1 does also | |
| 15:41:21 | gibi | assume we merge the create support in 2.72. and the in Train we fix the move with a bugfix. then if the microversion < 2.72 then we have to check if the instance has ports with allocation | |
| 15:41:33 | gibi | so yeah we cannot drop that check | |
| 15:41:38 | gibi | that looks into the ports | |
| 15:41:56 | gibi | but we can make it smarter by checking the vm state and the info_cache | |
| 15:42:13 | sean-k-mooney | ya but we shoudl definetly be about to optimise it when we have more time | |
| 15:42:27 | sean-k-mooney | *able to | |
| 15:42:44 | mriedem | did we agree to support moves without a new microversion? | |
| 15:43:01 | gibi | mriedem: no we haven't yet. | |
| 15:43:05 | mriedem | ok | |
| 15:43:09 | gibi | mriedem: we postponed that decision to Train | |
| 15:43:19 | mriedem | we would at least need a minimum compute service version check | |
| 15:43:30 | sean-k-mooney | i think that is dansmith's perference e.g. non new micro | |
| 15:43:56 | gibi | mriedem: is the compute version check due to the virt driver impact? | |
| 15:44:36 | mriedem | gibi: the compute service is what unbinds and binds the ports and stuff right during a move | |
| 15:45:02 | gibi | mriedem: ohh so we need a new compute on both end of the migration for bind/unbind | |
| 15:45:04 | mriedem | so anything you'd need to add to compute in train to support migrations would require an ABI version bump | |
| 15:45:08 | mriedem | correct | |
| 15:45:12 | gibi | understood | |
| 15:46:33 | sean-k-mooney | that would be a check in the conductor liek we do for the multiple port bindigns or like we will be doing for numa and sriov migration | |
| 15:47:32 | sean-k-mooney | effectivly if both computes are not new enough reject the migration | |
| 15:48:01 | gibi | sean-k-mooney: ack | |
| 15:50:00 | mriedem | need another novaclient core to hit this https://review.openstack.org/#/c/640657/ | |
| 15:51:03 | mriedem | jaypipes: are you coming back on the libvirt reshaper change today? https://review.openstack.org/#/c/599208/ | |
| 15:51:51 | jaypipes | mriedem: yes. it's on my screen right now. | |
| 15:57:04 | cfriesen | mriedem: should I include the blueprint for the PciInvalidAlias commit, or leave it standalone? | |
| 15:59:08 | mriedem | cfriesen: that's fixing a separate bug right? | |
| 15:59:21 | gibi | mriedem: +Ad https://review.openstack.org/#/c/640657/ | |
| 15:59:39 | mriedem | iow, handling PciInvalidAlias separately from the blueprint is so it could be backported right? | |
| 15:59:48 | cfriesen | mriedem: yep | |
| 15:59:54 | mriedem | cfriesen: if not, and it doesn't result in a 500 response today, then we don't need to backport it | |
| 16:00:46 | mriedem | cfriesen: btw are you sure that's not handled today? | |
| 16:00:49 | mriedem | by this https://github.com/openstack/nova/blob/c43c1d3fb9da5dd0a13e1f15623a696212f095ff/nova/api/openstack/wsgi.py#L367 | |
| 16:00:56 | mriedem | PciInvalidAlias extends Invalid | |
| 16:01:14 | mriedem | so that should return a 400 | |
| 16:01:43 | cfriesen | yeah, was just trying to track that down | |
| 16:02:20 | mriedem | if it is handled then i don't think we need anything separate | |
| 16:02:48 | mriedem | https://review.openstack.org/#/c/620706/27/nova/api/openstack/compute/servers.py@a857 | |
| 16:02:59 | cfriesen | so Fault will end up being HTTPBadRequest? | |
| 16:03:38 | cfriesen | or I guess Fault(exception.ConvertedException( | |
| 16:04:15 | mriedem | without any testing for it it's hard to say | |
| 16:05:31 | mriedem | so we get that from get_pci_requests_from_flavor which is called today from _validate_and_build_base_options in the api on server create, | |
| 16:05:45 | mriedem | i don't see any existing tests that handle the PciInvalidAlias if it's raised from there | |
| 16:05:58 | mriedem | i suspect it is handled and converted to a 400 response | |
| 16:06:05 | mriedem | but we don't have any testing for that | |
| 16:06:09 | cfriesen | I'll try a test without specially adding it to the list | |
| 16:06:16 | mriedem | unlike https://review.openstack.org/#/c/640841/2/nova/tests/unit/api/openstack/compute/test_serversV21.py | |
| 16:10:45 | cfriesen | mriedem: looks like it fails: http://paste.openstack.org/show/747293/ | |
| 16:11:41 | cfriesen | if I add PciInvalidAlias to the list in ServersController.create() , it passes | |
| 16:11:52 | mriedem | i assume those tests aren't using https://github.com/openstack/nova/blob/c43c1d3fb9da5dd0a13e1f15623a696212f095ff/nova/api/openstack/wsgi.py#L367 | |
| 16:12:03 | mriedem | those are hitting the handler directly | |
| 16:12:10 | cfriesen | yeah, just realized that | |
| 16:12:10 | mriedem | whereas ^ wraps the handler | |
| 16:13:07 | cfriesen | okay, need to hack devstack then | |
| 16:13:09 | mriedem | anywho, i think PciInvalidAlias is probably handled as a 400 and we're OK, and for completeness you could add it to that big ass list of exceptions for the validation stuff | |
| 16:13:16 | cfriesen | works | |
| 16:13:42 | mriedem | you got a 400 for an invalid alias in devstack? | |
| 16:13:52 | cfriesen | haven't done it yet | |
| 16:14:01 | mriedem | oh | |
| 16:16:59 | cfriesen | I get: ERROR (ClientException): Unknown Error (HTTP 500) | |
| 16:17:35 | mriedem | is there a stacktrace in the api logs? | |
| 16:18:39 | cfriesen | whoops...typo. I had "faise" istead of "raise" | |
| 16:19:44 | cfriesen | okay, for real this time I get this: http://paste.openstack.org/show/747294/ checking logs | |
| 16:20:38 | cfriesen | here are the logs: http://paste.openstack.org/show/747295/ | |
| 16:21:37 | cfriesen | adding the exception to the list makes it a 400 as expected | |
| 16:22:40 | cfriesen | report a bug or just put it in the git commit message? | |
| 16:27:01 | gibi | mriedem: regarding https://review.openstack.org/#/c/636360/21/releasenotes/notes/support-neutron-ports-with-resource-request-cb9ad5e9757792d0.yaml@21 I think unshelving is possible after a simple shelve without offload. And that operation is supported with bandwith as that does not need a new scheduling | |
| 16:29:37 | mriedem | efried: can we do an assertEqual(1, len(mdevs)) check in here instead? https://review.openstack.org/#/c/639854/4 | |
| 16:29:42 | jaypipes | mriedem: off it goes. | |
| 16:29:58 | efried | ... | |
| 16:30:19 | mriedem | gibi: yeah true | |
| 16:32:35 | jaypipes | wouldn't it be great if Nvidia developers actually participated in the community of people trying to enable their hardware? :( | |
| 16:33:46 | efried | mriedem: responded | |
| 16:35:15 | mriedem | efried: fwiw i also commented in bauzas's change that we should just do that mdev naming conversion from dashes to underscores in _get_mediated_device_information | |
| 16:35:24 | mriedem | so callers, like the libvirt driver and your test, don't need to care about that conversion | |
| 16:35:37 | mriedem | gah https://review.openstack.org/#/c/639647/ | |
| 16:35:49 | mriedem | i was going to -1 because there are other follow ups from this series i'd like to see | |
| 16:35:51 | mriedem | i guess i'll do those | |
| 16:36:38 | efried | mriedem: So do you want me to fix the test patch, or are you going to fup? | |
| 16:37:34 | mriedem | i'm gonna fup all night long | |