| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2024-07-18 | |||
| 13:57:33 | mikal | sean-k-mooney: it passes CI now, so that's nice. On that note I'm off to bed. | |
| 14:49:45 | bauzas | sean-k-mooney: mikal : I have two comments in https://review.opendev.org/c/openstack/nova-specs/+/915190 | |
| 14:49:55 | bauzas | one about testing at least | |
| 14:50:12 | bauzas | I'm okay with accepting this spec, but only provided we're clear on the testing side | |
| 14:51:06 | sean-k-mooney | bauzas: well i suggested tempest testign at the ptg and we said we shoudl not make that a requirement | |
| 14:51:13 | sean-k-mooney | so the expection is unit and fucntional tests only | |
| 14:51:27 | sean-k-mooney | wiht no devstack based tempest job | |
| 14:52:06 | dansmith | did we? | |
| 14:52:07 | sean-k-mooney | i origally asksed for a devstack plugin and to enabel it in one of our jobs but dansmith and other feel that was two high a bar to require in the spec | |
| 14:52:30 | dansmith | I really thought I brought up that we at least sniff test the RFB banner for our existing vnc stuff | |
| 14:52:39 | bauzas | sean-k-mooney: see my comment | |
| 14:53:02 | sean-k-mooney | dansmith: i susggested a temept test like that but i tought you said no to that | |
| 14:53:04 | bauzas | I proposed a fake object in the functional tests that would reproduce some external call | |
| 14:53:14 | sean-k-mooney | dansmith: im not agaisn doing that just liek we do for vnc | |
| 14:53:32 | dansmith | sean-k-mooney: okay I thought the opposite, but it's been a while | |
| 14:53:37 | bauzas | but I'm afraid unittests and API functional tests aren't enough | |
| 14:56:05 | sean-k-mooney | bauzas: sure so i think we can approve the spec as is if you have no other obejctsion and work on a trivial devstack plugin with mikal as part fo the implemnation and add this into one of our exsitign jobs | |
| 14:56:31 | bauzas | sean-k-mooney: I'm not really asking for a devstack plugin | |
| 14:56:35 | sean-k-mooney | the plugin would resided in the kerbside repo or a new dedeicated repo | |
| 14:56:47 | sean-k-mooney | bauzas: well something need to be able to deploy kerbside | |
| 14:56:48 | bauzas | I'm just asking for a fake object that would simulate the HTTP calls | |
| 14:57:00 | bauzas | so we just call we correctly call it | |
| 14:57:14 | sean-k-mooney | we dont call kerbside | |
| 14:57:16 | sean-k-mooney | it calls us | |
| 14:57:43 | sean-k-mooney | we shoudl have tempest coverage for the new api changes | |
| 14:57:48 | sean-k-mooney | or could | |
| 14:58:18 | sean-k-mooney | but im not sure that give use anything over the api sample tests | |
| 14:58:36 | sean-k-mooney | unless we actully deploy with kerbside as well in a tempest job | |
| 14:58:47 | bauzas | hmmmmmmmmm | |
| 14:59:04 | bauzas | the whole spec is about passing a configurable URL to our API | |
| 14:59:16 | bauzas | that's it | |
| 14:59:20 | sean-k-mooney | not entirly | |
| 14:59:29 | sean-k-mooney | it about that + adding new devices to the xml | |
| 14:59:55 | sean-k-mooney | and 2 addtion tional api chagnes | |
| 15:00:00 | sean-k-mooney | 1 to define the console type | |
| 15:00:18 | sean-k-mooney | and another to expsoe the tls_port via the existing admin only console token api | |
| 15:00:24 | bauzas | lemme be clear | |
| 15:00:29 | bauzas | I was confusing | |
| 15:00:37 | bauzas | " | |
| 15:00:42 | bauzas | "The user then fetches that URL, and Kerbside delivers a .vv file with the | |
| 15:00:44 | bauzas | connection information for a SPICE client. Kerbside uses a call to | |
| 15:00:46 | bauzas | `/os-console-auth-tokens/bf2e6883-...` to determine the validity of the | |
| 15:00:48 | bauzas | console authentication token, and the connection information for the console." | |
| 15:01:31 | bauzas | here, I think we can have a functional test that would reproduce the kerbside server by calling the os-console-auth-tokens | |
| 15:01:39 | bauzas | as a mock object | |
| 15:01:42 | sean-k-mooney | sure | |
| 15:01:53 | sean-k-mooney | but thats just a standar fucntional test | |
| 15:01:55 | bauzas | this way, we wouldn't need to depend on kerbside | |
| 15:02:07 | sean-k-mooney | sure but i was edxpecting that already | |
| 15:02:14 | bauzas | right, that's what I'm asking in https://review.opendev.org/c/openstack/nova-specs/+/915190/13/specs/2024.2/approved/libvirt-spice-direct-consoles.rst | |
| 15:02:44 | bauzas | from what I just read, mikal only proposed manual testing | |
| 15:03:07 | sean-k-mooney | we can make that more explit but i was starting form a baseline of "all new feature should have unit and functional tests as our minium baseline" | |
| 15:03:15 | sean-k-mooney | no | |
| 15:03:28 | bauzas | for functional tests, quoted " However, a test for the | |
| 15:03:30 | bauzas | API microversion will be added" | |
| 15:03:56 | bauzas | which is different from having a mock object that would simulate kerbside external call to Nova | |
| 15:04:20 | sean-k-mooney | so i expect the testing section of the spec to call out any special testing behiound our baselien | |
| 15:04:45 | sean-k-mooney | i dont in genrall expect to say "we will add unit and fucntional tests" but if you want that to be updated to included that im fine with that | |
| 15:05:22 | sean-k-mooney | the sepcific thing you asked for however i dont think makes sense for us to test | |
| 15:05:41 | sean-k-mooney | nova has 2 existign apis both fo which have test coverage | |
| 15:06:12 | sean-k-mooney | the console url show rpvoded a url that is templated based on `spice.kerbside_base_url` | |
| 15:06:25 | bauzas_ | okay, then I'll accept the spec but asking for a FUP | |
| 15:06:48 | sean-k-mooney | the url provided to the use rwhen they do show has a console token which kebside will use to call /os-console-auth-tokens | |
| 15:06:58 | sean-k-mooney | so im expeicting 2 seperte tests | |
| 15:07:16 | sean-k-mooney | one to cover the new console type for console url show | |
| 15:07:35 | sean-k-mooney | and one to test the addtion of a tls_port for spice-direct in /os-console-auth-tokens | |
| 15:53:01 | opendevreview | Merged openstack/nova-specs master: Re-propose using extend volume completion action for 2024.2 https://review.opendev.org/c/openstack/nova-specs/+/917133 | |
| 16:05:43 | bauzas | sean-k-mooney: dansmith: gibi: I know this is late, but could you please look at https://review.opendev.org/c/openstack/nova-specs/+/922201 if you have time today ? | |
| 16:05:58 | bauzas | I can also try to look at it before leaving | |
| 16:09:35 | dansmith | there's a ton of unresolved comments on that? | |
| 16:12:25 | bauzas_ | I think fwiesel replied on sean's comments | |
| 16:12:29 | bauzas_ | but sure, I can look at those | |
| 16:13:07 | dansmith | but there's a bunch that reference lines not even in the file anymore and so they're stacked at the top | |
| 16:13:18 | bauzas_ | due to an older revision | |
| 16:13:30 | bauzas_ | but I can clean them up or stick them | |
| 16:13:35 | bauzas_ | lemme look before you go in | |
| 16:16:40 | sean-k-mooney | if the lazy loadign was optional and off by defualt. i could maybe see triling this but im still not entirly sure this the best approch | |
| 16:27:11 | bauzas | sean-k-mooney: can you then just reply to Fabian's comments ? | |
| 16:27:31 | bauzas | I agree with the fact that I wouldn't rush to merge that spec given your concerns | |
| 16:27:47 | bauzas | I'd rather propose that spec to be discussed at the next PTG | |
| 16:27:52 | bauzas | fwiesel ^ | |
| 16:31:16 | dansmith | he does specify it being optional, but not sure about default | |
| 16:31:44 | dansmith | but I agree, I'm really not sure about the claims made in there, as this seems like it would be a gateway to increasing load | |
| 16:32:26 | sean-k-mooney | i woudl actully prefer to reduce the load on neutron by reviving https://review.opendev.org/c/openstack/nova/+/786348 | |
| 16:32:27 | dansmith | it may be better in the one case where you want to re-fetch one thing, but it seems like it opens the ability (and perhaps even required behavior) of generating many more small requests at the behest of a client that may want to DoS the metadata server | |
| 16:33:05 | dansmith | I thought the plan was just to cache that stuff in with metadata instead of always fetching it from neutron, tbh | |
| 16:33:23 | sean-k-mooney | well thats what the older patch is doing | |
| 16:33:24 | dansmith | idk that we need to store it in info_cache directly and I can see reasons why that would be bad and confusing | |
| 16:33:38 | dansmith | the older patch is storing it in info_cache, rihgt? | |
| 16:33:41 | sean-k-mooney | its adding the security groups to the network info cache | |
| 16:33:53 | sean-k-mooney | which speed up both the metadta api and server show | |
| 16:33:57 | dansmith | right, but what I'm saying I thought the plans was, is to just cache it in the built metadata | |
| 16:34:12 | dansmith | yeah I know, but if you immediately add a floating and then server show and it's not there, that's going to suck | |
| 16:34:18 | sean-k-mooney | oh am i was not in the ptg session so i dont know | |
| 16:34:22 | dansmith | which I think is why we're not doing that today, IIRC | |
| 16:34:46 | sean-k-mooney | we do already cache all the metadata once built today btu its time based with no way to triger the invalidation | |
| 16:34:51 | dansmith | network-changed doesn't dispatch for adding a floating right? | |
| 16:35:20 | sean-k-mooney | it might but i dont think applciationcation should be relaying on the metadata contnet to be up to date | |
| 16:35:47 | sean-k-mooney | we do get network-chaged for a number of things but i dont knwo of a list that is written down anywhere to check | |
| 16:35:52 | dansmith | I was talking about server show | |