| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-05-25 | |||
| 13:08:15 | sean-k-mooney | bauzas: well i was orginaly suggesting just allowing this by defult | |
| 13:08:49 | bauzas | sean-k-mooney: I proposed three alternatives : one be PATCH, one be adding a new HTTP header | |
| 13:09:01 | bauzas | and the last being to have a specific subresource | |
| 13:09:02 | sean-k-mooney | im not conviced we need to have a flag of any kind to opt into it but im not against having one | |
| 13:09:20 | bauzas | sean-k-mooney: see my comments on PS2, I was thinking like you | |
| 13:09:33 | bauzas | but looks like we agreed at the PTG to sanity check this | |
| 13:09:46 | sean-k-mooney | ya the header occured to me but not sure we have precendent for that | |
| 13:09:52 | sean-k-mooney | we did | |
| 13:10:33 | sean-k-mooney | i did not have stong feels for or against added a sanity check with a slight one for not doign that | |
| 13:10:46 | sean-k-mooney | ill go read your comment and catch up | |
| 13:11:32 | gibi | I'm on the side of either not add a check (but we agreed on the PTG to add it) or if we add then for me the extra parameters is better fits to the QS than to header or body or subresource | |
| 13:11:59 | bauzas | gibi: hence my +1 :) | |
| 13:11:59 | sean-k-mooney | bauzas: if we were to have a subresouce i actully woudl go with PUT /resource-providers/{uuid}/uuid/{new uuid} | |
| 13:12:22 | bauzas | gibi: I don't wanna hold on this, but I don't appreciate the QS param outcome | |
| 13:13:01 | sean-k-mooney | sorry | |
| 13:13:26 | sean-k-mooney | put /resource-providers/{uuid}/parent_uuid/{parent} | |
| 13:13:34 | sean-k-mooney | with an empty body | |
| 13:13:57 | sean-k-mooney | that basically woudl work like patch without using it | |
| 13:14:42 | bauzas | this sounds quite good to me | |
| 13:14:51 | bauzas | but again, I'm just one | |
| 13:14:57 | sean-k-mooney | you also have to expressly opt into that url | |
| 13:16:09 | gibi | bauzas: sorry I don't get why the current PUT is not RESTful | |
| 13:16:16 | gibi | you have to PUT all the attributes | |
| 13:16:26 | gibi | that you POSTed before | |
| 13:16:35 | bauzas | I guess the universe entropy is probably smaller than a discussion of 3 engineers about specing an API endpoint | |
| 13:17:01 | bauzas | count a 4th and you'll wait for the big crunch | |
| 13:17:53 | bauzas | gibi: it's not RESTful in the sense you can't update all the attributes as of now | |
| 13:18:22 | gibi | bauzas: you have restriction of the value of a field yes, but you have to still list all the fields in the PUT | |
| 13:18:24 | sean-k-mooney | bauzas: i think gibi was saying you would get teh current state update teh parent field and then put back the updated state | |
| 13:18:32 | gibi | sean-k-mooney: yes | |
| 13:18:39 | gibi | for me this is the restful put | |
| 13:18:52 | sean-k-mooney | if you do a update of the full resouce yes | |
| 13:18:56 | bauzas | this | |
| 13:19:18 | bauzas | if you just PUT a name, this isn't RESTful | |
| 13:19:37 | gibi | but the current PUT is not partial and I'm not suggesting any partial PUT | |
| 13:19:40 | gibi | either | |
| 13:19:46 | bauzas | and if you PUT a parent uuid which is not None while the resource parent is not None, then it's not RESFul either | |
| 13:20:06 | bauzas | because you depend on the state of the resource | |
| 13:20:23 | bauzas | but we're bikeshedding I guess | |
| 13:20:28 | sean-k-mooney | gibi: right i think bauzas was assuming you wanted to change that | |
| 13:20:43 | sean-k-mooney | bauzas: were you assuming gibi was suggeting using put for a partial update | |
| 13:20:57 | gibi | so today, PUT takes 3 paramters, name, uuid, and parent_uuid. After my change PUT takes the same parameters | |
| 13:21:07 | bauzas | actually, I was wrong | |
| 13:21:11 | gibi | just the value restriciotn of parent_uuid is relaxed | |
| 13:21:12 | sean-k-mooney | gibi: maybe we coudl put json exmaple into the spec to make that clear | |
| 13:21:14 | bauzas | name and uuid aren't optional | |
| 13:21:21 | bauzas | but parent_uuid is | |
| 13:21:37 | gibi | I assume parent_uuid field is not optional, it just allow taking the value of None | |
| 13:21:57 | bauzas | gibi: tbc, I'd be OK with a parent_uuid QS param name | |
| 13:22:05 | sean-k-mooney | gibi: right that is how i think of that too | |
| 13:22:37 | gibi | the API ref says optional, but I'm not sure that it is just due to the microversion dependency | |
| 13:22:39 | bauzas | but I'm not OK with some allow_reparenting name | |
| 13:22:40 | sean-k-mooney | the parent is an intrinic part of the resouce but None is a valid and default value | |
| 13:23:06 | sean-k-mooney | bauzas: you can change the name whenever you like | |
| 13:23:16 | sean-k-mooney | oh sorry | |
| 13:23:19 | sean-k-mooney | misread that | |
| 13:23:37 | sean-k-mooney | bauzas: you ment you are not ok with teh allow_reparenting query arg name | |
| 13:24:24 | sean-k-mooney | bauzas: i would prefer allow_reparenting over force | |
| 13:33:38 | openstackgerrit | Merged openstack/nova-specs master: Set minversion of tox to 3.18.0 https://review.opendev.org/c/openstack/nova-specs/+/791975 | |
| 13:33:39 | sean-k-mooney | gibi: ill get back to the reparenting spec later today. left my toughts on this as comments but i have no stong feeling agaisnt the current query arg approch nesssarly. | |
| 13:33:57 | gibi | bauzas, sean-k-mooney: thanks | |
| 13:46:22 | masterpe | I have placed indexes on the deleted_at column on all the tables on the databases nova, nova_api and nova_cell0, this speeds this nova-manage db archive_deleted_rows with multiple minutes per run. First it was 19 minutes now it is 19 seconds. | |
| 13:46:38 | masterpe | So I think this is a bug. | |
| 13:47:01 | gibi | masterpe: please file a bug. and feel free to propose a patch that adds the index | |
| 13:47:50 | masterpe | Is bugs.launchpad.net/ still used as bug tracker? | |
| 13:47:58 | gibi | for nova, yes | |
| 13:48:12 | gibi | https://bugs.launchpad.net/nova/+filebug | |
| 13:49:18 | gibi | thank you for reporting | |
| 13:49:23 | sean-k-mooney | masterpe: sound like a quick fix. althoguh we dont always index everythin that shoudl be indexed by default | |
| 13:50:18 | sean-k-mooney | deleted should already be indexed yes | |
| 13:50:34 | sean-k-mooney | deleted_at woudl only be imporant if you are using --before | |
| 13:50:48 | sean-k-mooney | i assume we just missed that usage chagne wehn we added --before | |
| 13:51:30 | sean-k-mooney | prior to that deleted_at would be very rarely used in queired as a column we filtered on | |
| 13:52:19 | sean-k-mooney | i think the soft delete code would have been the only thing that checked it really bar a direct user request for deleted instances | |
| 13:59:47 | masterpe | sean-k-mooney: https://bugs.launchpad.net/nova/+bug/1929563 | |
| 13:59:49 | openstack | Launchpad bug 1929563 in OpenStack Compute (nova) "Missing database index deleted_at column" [Undecided,New] | |
| 14:00:36 | masterpe | Do I only need to add the new indexes to ./db/sqlalchemy/models.py ? | |
| 14:02:53 | sean-k-mooney | masterpe: you also need to add a migration to add the indexs i think | |
| 14:03:11 | masterpe | I need to remember how to make commits, so probably it is easyer one someone else does that ;) | |
| 14:04:05 | sean-k-mooney | well if you want to start with the model change then you could | |
| 14:04:39 | sean-k-mooney | and we could pick it up but stephenfin is also looking at swaping out sqlalcamey_migrate for alembic this cycle | |
| 14:04:50 | sean-k-mooney | so how you do that will cahnge soon ish | |
| 14:05:36 | masterpe | That will get into the master branch and not Train ? | |
| 14:06:02 | gibi | bauzas: you are right, PUT /resource_providers/{uuid} is not RESTFul as it allows partial update today. This is very unfortunate :/ | |
| 14:06:53 | gibi | the parent_uuid not need to be passed, and the uuid field is not part of the body just part of the url | |
| 14:07:52 | gibi | the current PUT is very pretty close to PATCH now | |
| 14:23:26 | gibi | stephenfin: we need you on https://review.opendev.org/c/openstack/nova-specs/+/783827 | |
| 14:23:29 | gibi | :0 | |
| 14:23:55 | stephenfin | I'm needed? How wonderful | |
| 14:24:00 | stephenfin | * stephenfin looks | |
| 14:24:22 | gibi | it is your spec where sean-k-mooney has some comments making it pending | |
| 14:36:11 | sean-k-mooney | stephenfin: my main question is do we need OS-EXT-SRV-ATTR:hostname anymore and shoudl we just have hostname instead | |
| 14:36:57 | stephenfin | ah, that point | |
| 14:37:03 | stephenfin | I did see that and thought I had replied | |
| 14:37:54 | stephenfin | I'm easy. Changing it is more work, both on the server side (code and docs) and client (SDK, OSC, novaclient) side | |
| 14:37:55 | sean-k-mooney | we could adress that point in a follow up which is why im currently +1 | |
| 14:38:15 | stephenfin | but on the other hand, it's certainly a saner response | |
| 14:38:17 | sean-k-mooney | it would be but i think it would be a nice UX improvemnt | |
| 14:38:28 | sean-k-mooney | changing it in the futrue woudl need another micorversion bump | |
| 14:38:51 | sean-k-mooney | so basically i we ever want to change it i think it would be nice to do it now ihn this change | |