| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-08-26 | |||
| 14:17:28 | dansmith | sean-k-mooney: yeah, we knew it was going to need updates, and I'm not complaining about the comments (at all) | |
| 14:18:04 | dansmith | sean-k-mooney: I think that's probably unintentional and I didn't even catch it | |
| 14:18:12 | gibi | dansmith: I think I could do better if there would be clear and agreed priority which feature to review first. As I think i filled my time with plenty of reviews in general | |
| 14:18:25 | sean-k-mooney | ack we can just call that out as a limiation in the docs and fix it next cycle | |
| 14:18:44 | dansmith | gibi: oh I know, I'm certainly not complaining about the amount of reviews being done :) | |
| 14:18:52 | sean-k-mooney | its just because ironic does not use the default implementation of the rebuild fucntion | |
| 14:18:55 | sean-k-mooney | it has its own | |
| 14:19:15 | dansmith | sean-k-mooney: yeah I know, but I also didn't know we had bfv with ironic :) | |
| 14:21:50 | JayF | Good morning folks o/. Just wanted to bump my three outstanding stable ironic driver patches for review. https://review.opendev.org/c/openstack/nova/+/853546 https://review.opendev.org/c/openstack/nova/+/821351 https://review.opendev.org/c/openstack/nova/+/854257 all three are clean backports, and all but one already have one +2 | |
| 14:23:53 | gibi | dansmith: I slept on your comment about split the PCI feautre at the point where all the compute related part is ready. I figured that the current patch order does not really matches with that. I currently I have the other i) inventory healing, ii) allocation healing, ii) scheduling. But the scheduling part of the feauture needs compute side changes: 1) to driver the PCI claim based on the placment | |
| 14:23:59 | gibi | allocation 2) the pci and numa fitting logic is shared between the scheduler and the compute | |
| 14:24:49 | dansmith | gibi: I wasn't really suggesting a reorder, I was more just commenting on what seams are flexible for backports and which aren't :) | |
| 14:24:55 | gibi | so even if we could merge only the inventory and allocation healing without the scheduling support, backporting the scheduling support later is not really feasible | |
| 14:25:18 | gibi | or at least pretty shaky business | |
| 14:25:43 | gibi | fortunately I don't have to think about feature backport in this chat window :D | |
| 14:26:33 | opendevreview | Merged openstack/nova master: blockinfo: Add encryption details to the disk_info mappings when provided https://review.opendev.org/c/openstack/nova/+/772272 | |
| 14:26:42 | opendevreview | Merged openstack/nova master: imagebackend: Add disk_info_mapping as an optional attribute of Image https://review.opendev.org/c/openstack/nova/+/826530 | |
| 14:26:50 | opendevreview | Merged openstack/nova master: libvirt: Consolidate create_cow_image and create_image https://review.opendev.org/c/openstack/nova/+/846246 | |
| 14:26:59 | dansmith | gibi: :) | |
| 14:27:00 | opendevreview | Merged openstack/nova stable/wallaby: add regression test case for bug 1978983 https://review.opendev.org/c/openstack/nova/+/853811 | |
| 14:28:05 | dansmith | gibi: sean-k-mooney: isn't there a planned train for service/microversions somewhere? I wonder if I could at least rebase this on the right thing to get it lined up for whoami-rajat | |
| 14:33:10 | gibi | dansmith: https://etherpad.opendev.org/p/nova-zed-microversions-plan | |
| 14:33:49 | dansmith | yeah, thanks | |
| 14:34:18 | gibi | I'm not against to reorder the next to microversion | |
| 14:34:37 | dansmith | is that 2.93 one likely to get the review and attention it needs? | |
| 14:34:42 | gibi | if the rebuild bfv become ready before the user_data update | |
| 14:35:02 | gibi | dansmith: I'm actively helping 2.39 and melwitt too | |
| 14:35:18 | dansmith | looks like it's getting attention.. yeah okay cool | |
| 14:36:04 | gibi | I suggest to make rebuild bfv ready independently from the fact which microversion will it get. and it is ready before the user_data feature then we can switch the order in couple of hours | |
| 14:36:19 | dansmith | oh yeah for sure, | |
| 14:36:29 | dansmith | I just wanted to make sure that this was still the ordering before I rebase | |
| 14:36:55 | gibi | I have no reason to reorder now as both feautre needs work | |
| 14:37:08 | dansmith | yup | |
| 14:37:11 | gibi | also don't want to step over our PTL :) | |
| 14:38:01 | dansmith | again, I just wanted to make sure I knew the right thing to rebase on, nothing more :) | |
| 14:38:30 | gibi | no worried :) | |
| 14:40:33 | gmann | gibi: do you know till when bauzas is on PTO? | |
| 14:40:41 | gibi | let me double check | |
| 14:41:08 | gibi | he is back on the 30th | |
| 14:41:29 | gibi | based on the RH internal calendar | |
| 14:41:48 | gmann | ok, just one day before PTL nomination close. let me rebase his nomination patch which he added before going to PTO. | |
| 14:42:01 | gibi | ack | |
| 14:46:24 | dansmith | looks like user_data is pretty far behind master at this point too, so I hope that gets rebased when it is updated | |
| 14:48:46 | gibi | ahh you already commented that to review so I don't need to :) | |
| 14:49:12 | dansmith | yeah just did, typo and all | |
| 14:55:01 | opendevreview | Merged openstack/nova stable/wallaby: For evacuation, ignore if task_state is not None https://review.opendev.org/c/openstack/nova/+/853812 | |
| 14:59:55 | dansmith | sean-k-mooney: your comment here says 64: https://review.opendev.org/c/openstack/nova/+/820368/32/nova/objects/service.py | |
| 15:00:07 | dansmith | ah, nevermind | |
| 15:01:37 | sean-k-mooney | nice power of 2 | |
| 15:01:40 | gibi | now we can infer that your brain uses 6 bit ints internally | |
| 15:02:59 | dansmith | okay I rebased that stack plus user data on master, but won't push user data of course | |
| 15:03:10 | dansmith | but this should apply cleanly once the user data author does | |
| 15:03:31 | gibi | cool | |
| 15:04:01 | dansmith | some conflicts in exception.py too | |
| 15:04:23 | dansmith | is that author on irc? | |
| 15:05:51 | gibi | the author's IRC nick was jhartkopf in the past | |
| 15:06:06 | gibi | he was on nova before but not at the moment | |
| 15:06:07 | dansmith | ah | |
| 15:07:08 | sean-k-mooney | the os traits release i think is still pending but that should happen soon i hope | |
| 15:07:44 | gibi | sean-k-mooney: I talked about that with elodilles and he expect it to happen today | |
| 15:08:12 | sean-k-mooney | cool | |
| 15:09:02 | sean-k-mooney | dansmith: so the docs change i reqestied in the last patch could be in a followup which can merge after FF | |
| 15:09:19 | sean-k-mooney | am i might even be able to jsut write that for them | |
| 15:09:42 | sean-k-mooney | we can call out this does not work for ironic htere too | |
| 15:09:48 | sean-k-mooney | and adress that next cycle | |
| 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, | |