| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2022-05-24 | |||
| 17:15:00 | sean-k-mooney | i guess i can live with new microversion always reimages and old perserves current behaivor | |
| 17:15:04 | bauzas | ok, looks like we then have a consensus | |
| 17:15:07 | dansmith | "There is a known limitation where the root disk is not replaced for volume-backed instances during a rebuild." | |
| 17:15:11 | dansmith | ^ in the api-ref | |
| 17:15:14 | sean-k-mooney | dansmith: does that work for you. | |
| 17:15:27 | bauzas | dansmith: heh, I'm glad I remind this correctly | |
| 17:15:29 | sean-k-mooney | dansmith: ya but i was expecting an error in that case | |
| 17:15:44 | bauzas | sean-k-mooney: are you okay with the direction ? | |
| 17:15:51 | bauzas | personnally, I'm all good | |
| 17:15:58 | gibi | I'm ok with a new microversion | |
| 17:15:58 | dansmith | sean-k-mooney: yes, but I think that it would probably be prudent for the _client_ to have some flag if you provide the same image to make sure you really mean it | |
| 17:16:21 | dansmith | because most people are just going to be firing the client the same way and not realize the change | |
| 17:16:50 | sean-k-mooney | i dont really liek that | |
| 17:16:55 | bauzas | dansmith: that's the whole purpose of microversions, no ? | |
| 17:16:59 | sean-k-mooney | as every custoemr will then have to use a new microver and flag | |
| 17:17:04 | sean-k-mooney | and heat/ansibel ectra | |
| 17:17:17 | sean-k-mooney | will have to then always know if its a bfv instnace or not | |
| 17:17:28 | dansmith | bauzas: it is, but I think most people "opt into" microversions because they try to do something, google for why it's not working and someone says "oh pass this cryptic version flag" | |
| 17:17:50 | sean-k-mooney | BFV is not a flav show on instance show | |
| 17:17:54 | whoami-rajat | if you request the rebuild and it gets rejected, the message will clearly mention that you need to pass ``reimage_boot_volume`` with the request | |
| 17:17:57 | dansmith | but fine, if it's not palatable, then whatever, it's a risk | |
| 17:18:15 | sean-k-mooney | so you need to do some deep introspecation fo the instnace to fiture out if the root voluem is on cinder or not | |
| 17:18:16 | dansmith | whoami-rajat: they're asking for *not* that | |
| 17:18:20 | sean-k-mooney | and we dodnt provde the bdm either | |
| 17:18:40 | whoami-rajat | ok | |
| 17:18:53 | dansmith | sean-k-mooney: the client could always require that flag if the image is not changing, even for non-BFV instances, but I understand the concern | |
| 17:19:25 | dansmith | just saying, someone's going to lose their data.. granted because they weren't paying close enough attention, but .. it's a pet root volume so I just think it's worth being careful here | |
| 17:19:34 | dansmith | but the microversion is the important part to me for sure | |
| 17:19:34 | sean-k-mooney | so new microver alwasy require you to pass yes-i-really-really-mean-it | |
| 17:20:01 | dansmith | sean-k-mooney: that's what whoami-rajat has, at the API level (IIRC), but I was suggesting make that just a client shell behavior | |
| 17:20:26 | sean-k-mooney | dansmith: i think it was only for bfv instnaces in the spec currently | |
| 17:20:28 | dansmith | like, the client will refuse to use microversion 2.x *and* the same image_ref, unless you provide -yes-really | |
| 17:20:35 | dansmith | sean-k-mooney: yes, in the spec currently | |
| 17:20:53 | sean-k-mooney | if it was for all instance i think it woudl be more palletable | |
| 17:21:15 | sean-k-mooney | becasue as an end user i dont need to care if its bfv or not | |
| 17:21:19 | dansmith | right, I was suggesting making it consistent for all to avoid having to know if it's bfv or not | |
| 17:21:44 | sean-k-mooney | ya that im ok with even if it will break people that blindly use latest | |
| 17:21:54 | sean-k-mooney | it will break them by being safer | |
| 17:21:58 | dansmith | right | |
| 17:22:07 | sean-k-mooney | but that will mean rebuidl with nova client wont work anymore | |
| 17:22:20 | sean-k-mooney | since we are nolonger updatign the cli | |
| 17:22:31 | dansmith | sean-k-mooney: again I'm saying make it a client behavior, not an API one | |
| 17:22:47 | sean-k-mooney | so what woudl the api behavior be | |
| 17:22:56 | sean-k-mooney | new microversion just reimage | |
| 17:23:05 | sean-k-mooney | but gaurd in osc | |
| 17:23:07 | dansmith | the api behavior would be "if new, always destroy" | |
| 17:23:09 | dansmith | right | |
| 17:23:15 | sean-k-mooney | ok i like that more | |
| 17:23:32 | dansmith | osc logic is: "if new_version and image_ref==server.image_ref and args.yes_really: then do_it" | |
| 17:23:55 | dansmith | maybe we don't already have the server there in osc I guess, but we do validations like that in other places right? | |
| 17:24:19 | dansmith | sorry, that's not the right logic, let me try again: | |
| 17:24:48 | dansmith | if image_ref==server.image_ref and new_version: if not args.yes_really: explode with warning | |
| 17:25:05 | dansmith | only require the flag if new version and the image is not changing | |
| 17:25:06 | sean-k-mooney | or just check for new version | |
| 17:25:09 | whoami-rajat | but if someone directly curls the API (not from client), don't we require the validation of additional parameter? | |
| 17:25:24 | dansmith | could do that, but then everyone always has to do that, and the image not changing is so niche | |
| 17:25:31 | dansmith | whoami-rajat: right | |
| 17:25:38 | sean-k-mooney | whoami-rajat: i really dont think that heat should have to do differnt thigns for bfv or not | |
| 17:26:00 | dansmith | sean-k-mooney: and also, heat had better know the impact of the new microversion if they opt into it | |
| 17:26:11 | sean-k-mooney | ya | |
| 17:26:37 | sean-k-mooney | this comes back to not blindly using latest | |
| 17:26:44 | dansmith | heh | |
| 17:27:15 | dansmith | people already blindly paste the shell code from the first answer on stackexchange into a root terminal, | |
| 17:27:26 | sean-k-mooney | so from a tempest point of view we would want test for the new and old microverion too right | |
| 17:27:26 | dansmith | we're pretty well sunk on making them carefully consider microversions :) | |
| 17:27:32 | dansmith | yes | |
| 17:28:28 | sean-k-mooney | am so do you want to summerise what you propsoe we do | |
| 17:29:46 | dansmith | I don't "want" to, but I will | |
| 17:29:46 | sean-k-mooney | new micorversion -> always reimage, old -> preseve data for bfv reimage for not bfv, evac-> alwasy preserve data if on share starge regardless of micoversion(no change) | |
| 17:30:33 | whoami-rajat | since it was mentioned, tempest test for new behavior https://review.opendev.org/c/openstack/tempest/+/831018 | |
| 17:30:41 | whoami-rajat | still in progress though | |
| 17:32:33 | bauzas | folks, btw. I forgot to remember that I'll off from tonight until Monday | |
| 17:32:34 | bauzas | thanks | |
| 17:34:53 | dansmith | sean-k-mooney: https://review.opendev.org/c/openstack/nova-specs/+/840155 | |
| 17:36:08 | sean-k-mooney | dansmith: ack just reading it that sounds good to me | |
| 17:36:15 | dansmith | cool | |
| 17:36:19 | sean-k-mooney | whoami-rajat: reading the tempest test its doing some thing i think shoudl not be in the test | |
| 17:36:34 | sean-k-mooney | and its not valdiating eveythign i think ti shoudl be validating | |
| 17:36:54 | dansmith | sean-k-mooney: which is what? | |
| 17:37:24 | dansmith | sean-k-mooney: I added the bit to create and file and assert that it's gone after the rebuild, because initially we were supposed to be rebuild and we weren't -- the file was still present after the rebuild | |
| 17:37:27 | dansmith | not sure if that is resolved now | |
| 17:37:36 | sean-k-mooney | dansmith: the old micoversion behavior | |
| 17:37:44 | dansmith | ah for sure | |
| 17:37:49 | sean-k-mooney | also https://review.opendev.org/c/openstack/tempest/+/831018/12/tempest/api/compute/servers/test_server_actions.py#917= | |
| 17:37:59 | whoami-rajat | sean-k-mooney, ack, happy to have feedback on the test | |
| 17:38:02 | sean-k-mooney | i dont think adding a cleanup that rebuild to the old image is a good idea | |
| 17:38:15 | sean-k-mooney | it just add another failure mode in the test cleanup | |
| 17:38:16 | dansmith | yeah, not sure what that's about | |
| 17:38:35 | dansmith | comment says "not needed" | |
| 17:38:52 | whoami-rajat | dansmith, I did some changes in the nova code so the errors from logs are gone but it still somehow is not able to do it, I tested manually and the file never exists after the rebuild but somehow in this test, it stays there | |
| 17:39:13 | whoami-rajat | I'm working on a new job with two different images and will take input from there to fix it | |
| 17:39:25 | dansmith | whoami-rajat: ack, well, glad to have that assertion in there then :) | |
| 17:39:35 | dansmith | it was not rebuilding when I tried locally - the file was still present | |
| 17:40:11 | whoami-rajat | sean-k-mooney, ack, yeah that i added from the original rebuild test, can remove that part | |
| 17:40:24 | whoami-rajat | i think it was for an instance that is shared so reverted back to original image | |
| 17:40:45 | whoami-rajat | dansmith, if you try with latest code, it should work, at least it works for me and i tried 3-4 times | |
| 17:40:55 | dansmith | whoami-rajat: but not in the gate right? | |
| 17:40:59 | whoami-rajat | from nova side (in-use) and also from cinder side (available volume) | |
| 17:41:07 | whoami-rajat | yep, not in gate | |
| 17:41:12 | dansmith | okay | |