Thursday, 2022-09-08

*** brinzhang0 is now known as brinzhang01:36
*** brinzhang0 is now known as brinzhang02:51
gibisean-k-mooney, fungi: I will check that secu fix now06:59
opendevreviewBalazs Gibizer proposed openstack/nova master: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/84998507:19
opendevreviewBalazs Gibizer proposed openstack/nova master: Gracefully ERROR in _init_instance if vnic_type changed  https://review.opendev.org/c/openstack/nova/+/85000307:19
gibisean-k-mooneym, stephenfin, fungi: ^^ needed to rebase and adapt the test to the changes on master. It should be green again07:21
fungithanks!07:36
*** brinzhang_ is now known as brinzhang09:12
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (objects)  https://review.opendev.org/c/openstack/nova/+/83940109:21
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (manila abstraction)  https://review.opendev.org/c/openstack/nova/+/83119409:21
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (drivers and compute manager part)  https://review.opendev.org/c/openstack/nova/+/83309009:21
opendevreviewribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (api)  https://review.opendev.org/c/openstack/nova/+/83683009:21
opendevreviewribaudr proposed openstack/nova master: Bump compute version and check shares support  https://review.opendev.org/c/openstack/nova/+/85049909:21
opendevreviewribaudr proposed openstack/nova master: Add metadata for shares  https://review.opendev.org/c/openstack/nova/+/85050009:21
opendevreviewribaudr proposed openstack/nova master: Add instance.share_attach notification  https://review.opendev.org/c/openstack/nova/+/85050109:21
opendevreviewribaudr proposed openstack/nova master: Add instance.share_detach notification  https://review.opendev.org/c/openstack/nova/+/85102809:21
opendevreviewribaudr proposed openstack/nova master: Add shares to InstancePayload  https://review.opendev.org/c/openstack/nova/+/85102909:21
opendevreviewribaudr proposed openstack/nova master: Add instance.power_on_error notification  https://review.opendev.org/c/openstack/nova/+/85208409:22
opendevreviewribaudr proposed openstack/nova master: Add instance.power_off_error notification  https://review.opendev.org/c/openstack/nova/+/85227809:22
opendevreviewribaudr proposed openstack/nova master: Add helper methods to attach/detach shares  https://review.opendev.org/c/openstack/nova/+/85208509:22
opendevreviewribaudr proposed openstack/nova master: Add libvirt test to ensure metadata are working.  https://review.opendev.org/c/openstack/nova/+/85208609:22
opendevreviewribaudr proposed openstack/nova master: Add virt/libvirt error test cases  https://review.opendev.org/c/openstack/nova/+/85208709:22
opendevreviewribaudr proposed openstack/nova master: Add share_info parameter to reboot method for each driver (driver part)  https://review.opendev.org/c/openstack/nova/+/85482309:22
opendevreviewribaudr proposed openstack/nova master: Support rebooting an instance with shares (compute and API part)  https://review.opendev.org/c/openstack/nova/+/85482409:22
opendevreviewribaudr proposed openstack/nova master: Change microversion to 2.94  https://review.opendev.org/c/openstack/nova/+/85208809:22
sean-k-mooneygibi: cool will re reivew. i forgto tthat while this passes locally that ws because it was not rebased which will be done by zuul when its testing10:17
gibiyepp10:17
gibiI also tend to forget that fact10:18
sean-k-mooneyits rare that that rebase succced but brakes something with out causing a merge conflict10:26
sean-k-mooneyso mostly it does not matter10:26
sean-k-mooneygibi: they look fine, i rehecked the second patch as it failed due to a vm segfault10:34
gibiahh, thanks10:34
opendevreviewMerged openstack/nova master: Reproduce bug 1981813 in func env  https://review.opendev.org/c/openstack/nova/+/84998511:25
Ugglagibi, looking at that comment. https://review.opendev.org/c/openstack/nova/+/833090/16/nova/compute/manager.py#3903 the idea is to get a regular list instead of a ShareMappingList. Is there another way to do that ?12:04
Ugglagibi, forget ^12:47
*** dasm|off is now known as dasm12:56
*** bhagyashris is now known as bhagyashris|ruck14:37
whoami-rajatstephenfin, hey, would it still be viable to get this merged? the nova and novaclient changes have merged https://review.opendev.org/c/openstack/python-openstackclient/+/83101414:51
stephenfinwhoami-rajat: Sure. It won't be in the initial Zed release but we can backport. Bit of work needed on it though.15:09
whoami-rajatstephenfin, ack, one question, do you mean place it above the --hostname or below it? https://review.opendev.org/c/openstack/python-openstackclient/+/831014/5/openstackclient/compute/v2/server.py#310415:34
whoami-rajatI think options are be in sequence of microversion?15:35
stephenfinwhoami-rajat: sorry, below16:12
*** gibi is now known as gibi_off16:16
whoami-rajatstephenfin, ack, updated the patch, thanks16:29
stephenfinI just replied :)16:29
stephenfin--confirm-reimage just isn't descriptive enough, IMO, and we generally need pairs for boolean options16:30
stephenfinwhoami-rajat: and I left a few more comments on there in reply16:43
whoami-rajatstephenfin, I left a comment midway of your comment mentioning a case where the else would not be appropriate16:46
whoami-rajatstephenfin, so we do allow rebuilding volume backed instance if the old and new image is same16:46
whoami-rajatand if we don't put the elif microversion >=2.91 that case would fail16:46
whoami-rajatwill address the other ones16:47
stephenfinso if I call 'openstack server rebuild --image $IMAGE $SERVER' and $IMAGE happens to be the same image that was originally used, it will pass?16:49
stephenfinwhoami-rajat: ^16:49
whoami-rajatstephenfin, yes, it should, the nova side allows it16:50
whoami-rajatat least16:50
stephenfinthat feels super janky :-D16:51
stephenfinaren't you effectively preventing that on newer microversions with this change?16:52
stephenfini.e. if someone was relying this previously, they wouldn't be able to do so with OS_COMPUTE_API_VERSION=2.93 or later unless they also passed '--rebuild-volume' ?16:53
stephenfinperhaps we could have a temporary stop-gap measure before preventing it entirely client side16:53
stephenfinif microversion >= 2.93; block outright16:53
stephenfinif microversion < 2.93; warn that this is unsupported, that it will no longer be allowed in the future, and that nova will reject the request if the image is different from the one originally used, but allow the request to continue (for now)16:54
whoami-rajatyeah but we have implemented a generic case to rebuild any type of volume backed instance, people would prefer that instead of the hacky thing we have had before16:55
whoami-rajatif microversion >= 2.93; block outright: in this case we also block image backed instances16:56
whoami-rajatwhich we don't want16:56
stephenfinno, we keep the check for 'server.image is not None'16:56
whoami-rajatok16:57
whoami-rajatI'm kind of confused with all the conditions ...16:58
stephenfinsec, code is probably easier :)16:58
whoami-rajatwe've 1) confirm_reimage check 2) microversion check 3) image check16:58
whoami-rajatyeah would be helpful that way :)16:59
stephenfinwhoami-rajat: https://paste.opendev.org/show/bpmNKNYMoKN736cxOa7e/17:08
stephenfinwhoami-rajat: that should do the trick. Personally though, I'd really rethink the need for this. People know rebuild is a destructive operation and they'd have to opt-in to the new microversion to even take advantage of this17:10
stephenfinI'm sure the cinder team have discussed this at length though17:10
whoami-rajatI might be thinking too much but if a rebuild related MV is added in future, the first "if" case of the "else" case becomes invalid, Eg: we are passing 2.94 for rebuilding an image backed instance with a new field and this check will fail that operation17:13
whoami-rajatstephenfin, ^17:15
whoami-rajatstephenfin, yeah the spec discussion has been going on for several releases (when I wasn't working on it) and the general comments from both nova and cinder side were to have an additional check17:15
sean-k-mooneywith specifci decenters17:16
sean-k-mooneyie. i did not want the check in nova or the client17:16
sean-k-mooneybut i can live with it in the client17:16
sean-k-mooneyrebuild is ment to be distructive17:17
stephenfinwhoami-rajat: Oh yeah, I need to check if it's BFV in both branches of the else https://paste.opendev.org/show/bveR7gYr02CL4XLBbvVe/17:17
sean-k-mooneyif we want to supporot the other useage we shoudl have explit way to do that that works for all vms not just bfv17:17
whoami-rajatalso a bootable volume can still exist when an instance is destroyed so it's kind of different from the rebuild we refer to for ephemeral cases, at least from a cinder perspective17:17
sean-k-mooneyform a nova persecvitve not really if you want to save the root data after a vm is delete just snapshot it17:18
sean-k-mooneyi dont coniser the nova root disk to be ephemeral17:18
sean-k-mooneynova has a seperate thing called ephmearl disks in addtion to the root disk17:19
sean-k-mooneynova vms by can have 4 types of storage at the same time17:19
sean-k-mooneydisk_gb is the root disk, ephemeral_gb is 0-n addtional disk, swap and cinder volumes17:20
whoami-rajatstephenfin, that looks good, will update it17:20
sean-k-mooneyah yes   if server.image is None:17:21
sean-k-mooneyone slight optimisation17:21
sean-k-mooneycan you change the else and if17:21
sean-k-mooneyto an elif17:21
whoami-rajatack, don't have much insights in nova but cinder folks would be really angry without that check :D17:22
sean-k-mooneyhttps://paste.opendev.org/show/befzjPz2izxuGZtwr1Vs/17:22
sean-k-mooneywhoami-rajat: as long as its not in nova im ok with it but i really hate that we allow the non distrutive case and am sad we have to support it17:23
sean-k-mooneywe cant change history but allowing the metadat to be updated via rebuidl to same image for BFV instance should not have been done17:24
stephenfinsean-k-mooney: I usually avoid doing that to indicate that the two checks (microversion and "is it volume backed?" aren't totally related but that would be less nesting, yeah17:24
sean-k-mooneyhonestly i dont mind etither way i guss17:25
sean-k-mooneyi just dislike wraping on 80 charters so avoid nesting if i can17:25
sean-k-mooneyin this case it does not matter as its only impacting the commnet17:26
stephenfintrue17:26
sean-k-mooneywhoami-rajat: stephenfin  has the +2 rights so follw there prefernce on this17:26
stephenfinfor my own notes, attempting to rebuild a volume-backed server without --image fails currently17:27
stephenfin❯ openstack server rebuild test-server-bfv17:27
stephenfin'str' object has no attribute 'get'17:27
sean-k-mooneythat proably a bug17:27
sean-k-mooneysince rebuild without an image specified is ment to default to same image17:27
stephenfin100%17:27
stephenfinin OSC17:28
sean-k-mooneylet me just check if in the api17:28
whoami-rajatyeah, I kind of like stephenfin approach too, we can ignore saving one LOC for readability17:28
whoami-rajatwill update the patch17:28
sean-k-mooneystephenfin: its not17:29
sean-k-mooneywell there is a osc bug17:29
sean-k-mooneybut image is required17:29
sean-k-mooneyin the api17:29
sean-k-mooneyhttps://docs.openstack.org/api-ref/compute/?expanded=rebuild-server-rebuild-action-detail#rebuild-server-rebuild-action17:29
sean-k-mooneystephenfin: so in osc image should be required17:29
sean-k-mooneyunless this was implemted in osc /nova client17:29
stephenfinyeah, it was17:30
sean-k-mooneyah ok17:30
stephenfinif you don't specify it, we use the same image17:30
sean-k-mooneyright but that is client side17:30
stephenfinwhich is why you're used to that behaviour, I guess17:30
stephenfinsean-k-mooney: yup17:30
sean-k-mooneyack 17:30
stephenfinwhoami-rajat: one point: annoyingly either nova or novaclient returns the empty string if the server is volume-backed, so that 'if server.image is None' needs to be simply 'if not server.image' :(17:31
stephenfinhttps://paste.opendev.org/show/bcl82mF5NEjC4T1uhbnu/17:32
stephenfinyeah, it's nova itself. Ugh17:32
whoami-rajatah ok, will update that part17:32
sean-k-mooneyThe UUID and links for the image for your server instance. The image object will be an empty string when you boot the server from a volume.17:33
sean-k-mooneyits in the api ref17:33
stephenfinJust because it's documented doesn't make it right :-P17:33
sean-k-mooneystephenfin: so its not a bug that just what we chose as the sentinel17:33
sean-k-mooneyyes but its part of the api contract17:33
stephenfinoh, I'm not suggesting changing it. Merely complaining17:34
sean-k-mooney:)17:34
stephenfinyup, confirmed that rebuilding a BFV instance with the same image works, but using a different image results in a failure17:35
stephenfin❯ openstack server rebuild --image cirros-0.5.1-x86_64-disk test-server-bfv17:35
stephenfinImage 781c8aa6-210f-4f55-9964-b9fe5ef07749 is unacceptable: Unable to rebuild with a different image for a volume-backed server. (HTTP 400) (Request-ID: req-945cc80d-129e-448a-bde9-bce5f3a8be88)17:35
stephenfin❯ openstack server rebuild --image cirros-0.5.2-x86_64-disk test-server-bfv17:35
stephenfin+-------------------+----------------------------------------------------------+17:35
stephenfin| Field             | Value                                                    |17:35
stephenfin+-------------------+----------------------------------------------------------+17:35
stephenfin...17:35
stephenfinwhoami-rajat: I think we're on the same page. I'll review that again in the morning. Late for me now o/17:36
stephenfinCheers for the input, sean-k-mooney17:36
sean-k-mooney:)17:36
whoami-rajatthanks stephenfin and sean-k-mooney 17:37
*** dasm is now known as dasm|off21:13

Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!