| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-26 | |||
| 15:10:26 | sean-k-mooney | if you have fixed the compute service version then i think that was the main thing that need to be adress to resolve the merge conflict | |
| 15:10:46 | dansmith | tbh, I'm not sure we even need to pass that to the virt driver anymore, | |
| 15:10:56 | dansmith | that might be a holdover from a previous approach to this | |
| 15:11:09 | dansmith | oh right, envermind, | |
| 15:11:18 | dansmith | because the default impl is in compute manager | |
| 15:11:25 | sean-k-mooney | yep | |
| 15:11:26 | dansmith | I was eye-grepping for virt/* | |
| 15:11:37 | sean-k-mooney | ironic has an imple of it | |
| 15:11:41 | sean-k-mooney | but i think only it does | |
| 15:11:48 | sean-k-mooney | so it does not fallback | |
| 15:12:27 | dansmith | that bottom patch fails unit tests with a missing trait, but does not depends-on anything else.. I assume that's because we're waiting for the traits release? | |
| 15:12:30 | sean-k-mooney | its why ironic supprot rebuild witout erasing the epmeeral disk while reimaing the root disk | |
| 15:12:43 | sean-k-mooney | yes | |
| 15:13:11 | dansmith | having this behave differently for one virt driver does not seem like a very good user experience, even though ironic is weird | |
| 15:13:37 | sean-k-mooney | right ironic already had specific beahivor | |
| 15:13:48 | dansmith | sigh | |
| 15:13:56 | sean-k-mooney | im not sure it would take much to add ironci supprot but testin gwould be hard | |
| 15:14:23 | sean-k-mooney | the ironic specific behavior is that it has an api parmater that allows the ephemerl disk to not be erased when you rebild with a new image | |
| 15:14:47 | sean-k-mooney | we could support that for libvirt too but we dotn since the default impl does not support it | |
| 15:15:14 | dansmith | SIGH | |
| 15:16:17 | dansmith | so yeah, so much of rebuild is done by ironic, perhaps just refusing to do rebuild on bfv if we're instructed to wipe the root disk is the right approach | |
| 15:16:31 | dansmith | but we'll really need that implemented | |
| 15:17:09 | sean-k-mooney | well we can check that in the api fairly simplely | |
| 15:17:18 | sean-k-mooney | we can check the hypervior type i belive | |
| 15:17:38 | dansmith | which would be terrible | |
| 15:17:40 | sean-k-mooney | and just reject it with a 400 | |
| 15:17:55 | dansmith | we should not have the API behaving differently depending on the virt driver | |
| 15:18:17 | sean-k-mooney | it also should not behave differntly based on if the instnace is boot form volume | |
| 15:18:18 | dansmith | that's compute stuff, and while it sucks to fail late like that, I much prefer that than building virt-specific stuff into the api | |
| 15:18:22 | sean-k-mooney | but that the situration we are in | |
| 15:18:45 | sean-k-mooney | due to the hacky (only update metadat if the image is the same and its BFV) legacy | |
| 15:19:03 | dansmith | the api has to behave differently for bfv because it historically did, but we shouldn't be building *new* stuff that drags virt specifics into the api | |
| 15:19:26 | sean-k-mooney | ack i agree with that | |
| 15:19:38 | sean-k-mooney | the preserve_ephemeral filed looks like it predates microveriosn by the way | |
| 15:19:45 | dansmith | basically all the functionals on that user data patch fail because of that missing trait, | |
| 15:19:47 | sean-k-mooney | i was trying to figure out when we added that | |
| 15:20:06 | dansmith | which makes it hard to work on the functional tests | |
| 15:20:13 | sean-k-mooney | i belvie the pased in the previsous version of the patch that did not have the trait | |
| 15:20:40 | sean-k-mooney | locally we can just pip install the os-traits git repo | |
| 15:20:51 | sean-k-mooney | but ya that why i was hopign to do the release yesterday | |
| 15:20:53 | dansmith | yeah, I'll have to do that | |
| 15:21:37 | sean-k-mooney | elodilles: any chance we can merge https://review.opendev.org/c/openstack/releases/+/854617 | |
| 15:21:49 | sean-k-mooney | to adress ^ | |
| 15:22:00 | elodilles | sean-k-mooney: ack, will look into it | |
| 15:22:10 | opendevreview | Merged openstack/nova master: Add locked_memory extra spec and image property https://review.opendev.org/c/openstack/nova/+/778347 | |
| 15:22:15 | sean-k-mooney | elodilles++ thanks very much :) | |
| 15:24:54 | elodilles | sean-k-mooney: do you need to release it ASAP? I'm asking because if really needed then I'll +W with single +2 (i'm the only release manager on duty today o:)) otherwise we need to wait till Monday | |
| 15:27:45 | sean-k-mooney | am ideally yes. it could wait but we have 2 api changes blocked by it currently. | |
| 15:28:11 | sean-k-mooney | its at your disgression ultimately dansmith or gibi might have a stonger prefernce | |
| 15:28:52 | gibi | I'm fine releasing that today with a single release mgmt +2 | |
| 15:29:07 | dansmith | yeah, it'd be a lot better if it could be today, unfortunately | |
| 15:29:45 | gibi | PTO + FF is not a good combination | |
| 16:06:05 | elodilles | sean-k-mooney gibi dansmith: ack, os-traits release is on the way | |
| 16:06:13 | dansmith | elodilles: thanks! | |
| 16:06:15 | gibi | elodilles: thank oyu | |
| 16:06:16 | gibi | you | |
| 16:06:23 | elodilles | np | |
| 16:07:09 | sean-k-mooney | once that is release we shuld bump our min required version https://github.com/openstack/nova/blob/master/requirements.txt#L56 right in the change that adds the userdata feature | |
| 16:07:56 | sean-k-mooney | like i know we will get it automaticaly but we should raise our dep in the feature that requires the new trait | |
| 16:09:15 | opendevreview | Dan Smith proposed openstack/nova master: DNM: Test for rosmaita https://review.opendev.org/c/openstack/nova/+/854815 | |
| 16:10:02 | opendevreview | Dan Smith proposed openstack/nova master: DNM: Test for rosmaita https://review.opendev.org/c/openstack/nova/+/854815 | |
| 16:10:55 | gibi | the gate queue is 53 patches long, nice | |
| 16:12:18 | elodilles | :-o | |
| 16:12:42 | sean-k-mooney | oh the oauth 2 feature is mergin in keystone | |
| 16:12:54 | sean-k-mooney | thats cool | |
| 16:15:09 | dansmith | sean-k-mooney: I'm getting functional fails on missing api samples for 2.93 | |
| 16:15:34 | dansmith | looks like the author dropped some of them from a previous version? | |
| 16:15:48 | sean-k-mooney | on hte user data patch lets see | |
| 16:16:04 | dansmith | yeah | |
| 16:16:17 | sean-k-mooney | ... i jsut realsied i review version 8 not 9 just now | |
| 16:16:21 | sean-k-mooney | and yes | |
| 16:16:25 | sean-k-mooney | https://review.opendev.org/c/openstack/nova/+/816157/8..9 | |
| 16:16:38 | sean-k-mooney | there were more removed then added | |
| 16:17:26 | sean-k-mooney | im stongly tempeted to say we shoudl swap those two if the author of the user data one has not updated it by monday | |
| 16:17:58 | sean-k-mooney | im tempeted to say we do it now but its kind of a pain to update all the micorversion refrences | |
| 16:20:46 | sean-k-mooney | ya so it passed on 8 https://review.opendev.org/c/openstack/nova/+/816157/9#message-87c965d558c8c7b25b2c4d1ebd3232c2627f444a and likely because of os-traits not being released they did not realise it failed on v9 | |
| 16:20:59 | sean-k-mooney | thats a pian | |
| 16:21:03 | dansmith | yeah | |
| 17:07:58 | opendevreview | Merged openstack/nova master: Retry /reshape at provider generation conflict https://review.opendev.org/c/openstack/nova/+/851358 | |
| 17:36:38 | dansmith | gibi: still around? | |
| 17:37:06 | gibi | dansmith: yes, but with limited brain power | |
| 17:37:55 | dansmith | gibi: on the error path.. if we created the new attachment, saved it to the bdm, but failed to delete the old one, is there any reason not to just keep rolling? | |
| 17:38:08 | dansmith | like what does restoring the old attachment_id and aborting get us there? | |
| 17:38:35 | sean-k-mooney | so the db save failed | |
| 17:38:41 | dansmith | no | |
| 17:38:46 | dansmith | the db save of the new one completed | |
| 17:38:56 | dansmith | the delete of the old one failed (in cinderclient) | |
| 17:38:58 | sean-k-mooney | but the delete in cinder failed | |
| 17:39:01 | sean-k-mooney | ah ok | |
| 17:39:17 | sean-k-mooney | am well we could leak it but other then that i think its ok | |
| 17:39:23 | sean-k-mooney | if we have our side in order | |
| 17:39:44 | dansmith | right, we leak an attachment, but presumably we're already sunk if we fail to delete | |
| 17:39:49 | gibi | dansmith: please point me the case in the code | |
| 17:40:42 | dansmith | gibi: https://review.opendev.org/c/openstack/nova/+/820368/32/nova/compute/manager.py in L3437 | |
| 17:40:44 | sean-k-mooney | dansmith: at that point have we "bound" the new attachmet propelry and got all the connector info so we can update the xml and proceed | |
| 17:41:24 | sean-k-mooney | presuamable deleteing the attcoemnt woudl only fail if cidner was unaviable or something like that | |
| 17:41:28 | dansmith | sean-k-mooney: well this is before the reimage, but yeah, I'm saying if we've recorded the new attachment and can't delete the old one, I'm not sure it gets us much to revert our own db record to the old attachment and then try to delete the new one | |
| 17:41:49 | sean-k-mooney | ah | |
| 17:41:55 | gibi | dansmith: so the root_bdm.attachment_id points to the new attachement_id and we just deleted that in cinder | |
| 17:42:06 | dansmith | reverting to the old one and deleting the new one is a more complicated cleanup, which I can do, but I just want to make sure it's worth it | |