| *** brinzhang0 is now known as brinzhang | 01:36 | |
| *** brinzhang0 is now known as brinzhang | 02:51 | |
| gibi | sean-k-mooney, fungi: I will check that secu fix now | 06:59 |
|---|---|---|
| opendevreview | Balazs Gibizer proposed openstack/nova master: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/849985 | 07:19 |
| opendevreview | Balazs Gibizer proposed openstack/nova master: Gracefully ERROR in _init_instance if vnic_type changed https://review.opendev.org/c/openstack/nova/+/850003 | 07:19 |
| gibi | sean-k-mooneym, stephenfin, fungi: ^^ needed to rebase and adapt the test to the changes on master. It should be green again | 07:21 |
| fungi | thanks! | 07:36 |
| *** brinzhang_ is now known as brinzhang | 09:12 | |
| opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (objects) https://review.opendev.org/c/openstack/nova/+/839401 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (manila abstraction) https://review.opendev.org/c/openstack/nova/+/831194 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (drivers and compute manager part) https://review.opendev.org/c/openstack/nova/+/833090 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Attach Manila shares via virtiofs (api) https://review.opendev.org/c/openstack/nova/+/836830 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Bump compute version and check shares support https://review.opendev.org/c/openstack/nova/+/850499 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Add metadata for shares https://review.opendev.org/c/openstack/nova/+/850500 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Add instance.share_attach notification https://review.opendev.org/c/openstack/nova/+/850501 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Add instance.share_detach notification https://review.opendev.org/c/openstack/nova/+/851028 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Add shares to InstancePayload https://review.opendev.org/c/openstack/nova/+/851029 | 09:21 |
| opendevreview | ribaudr proposed openstack/nova master: Add instance.power_on_error notification https://review.opendev.org/c/openstack/nova/+/852084 | 09:22 |
| opendevreview | ribaudr proposed openstack/nova master: Add instance.power_off_error notification https://review.opendev.org/c/openstack/nova/+/852278 | 09:22 |
| opendevreview | ribaudr proposed openstack/nova master: Add helper methods to attach/detach shares https://review.opendev.org/c/openstack/nova/+/852085 | 09:22 |
| opendevreview | ribaudr proposed openstack/nova master: Add libvirt test to ensure metadata are working. https://review.opendev.org/c/openstack/nova/+/852086 | 09:22 |
| opendevreview | ribaudr proposed openstack/nova master: Add virt/libvirt error test cases https://review.opendev.org/c/openstack/nova/+/852087 | 09:22 |
| opendevreview | ribaudr proposed openstack/nova master: Add share_info parameter to reboot method for each driver (driver part) https://review.opendev.org/c/openstack/nova/+/854823 | 09:22 |
| opendevreview | ribaudr proposed openstack/nova master: Support rebooting an instance with shares (compute and API part) https://review.opendev.org/c/openstack/nova/+/854824 | 09:22 |
| opendevreview | ribaudr proposed openstack/nova master: Change microversion to 2.94 https://review.opendev.org/c/openstack/nova/+/852088 | 09:22 |
| sean-k-mooney | gibi: 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 testing | 10:17 |
| gibi | yepp | 10:17 |
| gibi | I also tend to forget that fact | 10:18 |
| sean-k-mooney | its rare that that rebase succced but brakes something with out causing a merge conflict | 10:26 |
| sean-k-mooney | so mostly it does not matter | 10:26 |
| sean-k-mooney | gibi: they look fine, i rehecked the second patch as it failed due to a vm segfault | 10:34 |
| gibi | ahh, thanks | 10:34 |
| opendevreview | Merged openstack/nova master: Reproduce bug 1981813 in func env https://review.opendev.org/c/openstack/nova/+/849985 | 11:25 |
| Uggla | gibi, 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 |
| Uggla | gibi, forget ^ | 12:47 |
| *** dasm|off is now known as dasm | 12:56 | |
| *** bhagyashris is now known as bhagyashris|ruck | 14:37 | |
| whoami-rajat | stephenfin, 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/+/831014 | 14:51 |
| stephenfin | whoami-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-rajat | stephenfin, 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#3104 | 15:34 |
| whoami-rajat | I think options are be in sequence of microversion? | 15:35 |
| stephenfin | whoami-rajat: sorry, below | 16:12 |
| *** gibi is now known as gibi_off | 16:16 | |
| whoami-rajat | stephenfin, ack, updated the patch, thanks | 16:29 |
| stephenfin | I just replied :) | 16:29 |
| stephenfin | --confirm-reimage just isn't descriptive enough, IMO, and we generally need pairs for boolean options | 16:30 |
| stephenfin | whoami-rajat: and I left a few more comments on there in reply | 16:43 |
| whoami-rajat | stephenfin, I left a comment midway of your comment mentioning a case where the else would not be appropriate | 16:46 |
| whoami-rajat | stephenfin, so we do allow rebuilding volume backed instance if the old and new image is same | 16:46 |
| whoami-rajat | and if we don't put the elif microversion >=2.91 that case would fail | 16:46 |
| whoami-rajat | will address the other ones | 16:47 |
| stephenfin | so 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 |
| stephenfin | whoami-rajat: ^ | 16:49 |
| whoami-rajat | stephenfin, yes, it should, the nova side allows it | 16:50 |
| whoami-rajat | at least | 16:50 |
| stephenfin | that feels super janky :-D | 16:51 |
| stephenfin | aren't you effectively preventing that on newer microversions with this change? | 16:52 |
| stephenfin | i.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 |
| stephenfin | perhaps we could have a temporary stop-gap measure before preventing it entirely client side | 16:53 |
| stephenfin | if microversion >= 2.93; block outright | 16:53 |
| stephenfin | if 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-rajat | yeah 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 before | 16:55 |
| whoami-rajat | if microversion >= 2.93; block outright: in this case we also block image backed instances | 16:56 |
| whoami-rajat | which we don't want | 16:56 |
| stephenfin | no, we keep the check for 'server.image is not None' | 16:56 |
| whoami-rajat | ok | 16:57 |
| whoami-rajat | I'm kind of confused with all the conditions ... | 16:58 |
| stephenfin | sec, code is probably easier :) | 16:58 |
| whoami-rajat | we've 1) confirm_reimage check 2) microversion check 3) image check | 16:58 |
| whoami-rajat | yeah would be helpful that way :) | 16:59 |
| stephenfin | whoami-rajat: https://paste.opendev.org/show/bpmNKNYMoKN736cxOa7e/ | 17:08 |
| stephenfin | whoami-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 this | 17:10 |
| stephenfin | I'm sure the cinder team have discussed this at length though | 17:10 |
| whoami-rajat | I 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 operation | 17:13 |
| whoami-rajat | stephenfin, ^ | 17:15 |
| whoami-rajat | stephenfin, 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 check | 17:15 |
| sean-k-mooney | with specifci decenters | 17:16 |
| sean-k-mooney | ie. i did not want the check in nova or the client | 17:16 |
| sean-k-mooney | but i can live with it in the client | 17:16 |
| sean-k-mooney | rebuild is ment to be distructive | 17:17 |
| stephenfin | whoami-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-mooney | if we want to supporot the other useage we shoudl have explit way to do that that works for all vms not just bfv | 17:17 |
| whoami-rajat | also 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 perspective | 17:17 |
| sean-k-mooney | form a nova persecvitve not really if you want to save the root data after a vm is delete just snapshot it | 17:18 |
| sean-k-mooney | i dont coniser the nova root disk to be ephemeral | 17:18 |
| sean-k-mooney | nova has a seperate thing called ephmearl disks in addtion to the root disk | 17:19 |
| sean-k-mooney | nova vms by can have 4 types of storage at the same time | 17:19 |
| sean-k-mooney | disk_gb is the root disk, ephemeral_gb is 0-n addtional disk, swap and cinder volumes | 17:20 |
| whoami-rajat | stephenfin, that looks good, will update it | 17:20 |
| sean-k-mooney | ah yes if server.image is None: | 17:21 |
| sean-k-mooney | one slight optimisation | 17:21 |
| sean-k-mooney | can you change the else and if | 17:21 |
| sean-k-mooney | to an elif | 17:21 |
| whoami-rajat | ack, don't have much insights in nova but cinder folks would be really angry without that check :D | 17:22 |
| sean-k-mooney | https://paste.opendev.org/show/befzjPz2izxuGZtwr1Vs/ | 17:22 |
| sean-k-mooney | whoami-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 it | 17:23 |
| sean-k-mooney | we cant change history but allowing the metadat to be updated via rebuidl to same image for BFV instance should not have been done | 17:24 |
| stephenfin | sean-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, yeah | 17:24 |
| sean-k-mooney | honestly i dont mind etither way i guss | 17:25 |
| sean-k-mooney | i just dislike wraping on 80 charters so avoid nesting if i can | 17:25 |
| sean-k-mooney | in this case it does not matter as its only impacting the commnet | 17:26 |
| stephenfin | true | 17:26 |
| sean-k-mooney | whoami-rajat: stephenfin has the +2 rights so follw there prefernce on this | 17:26 |
| stephenfin | for my own notes, attempting to rebuild a volume-backed server without --image fails currently | 17:27 |
| stephenfin | ❯ openstack server rebuild test-server-bfv | 17:27 |
| stephenfin | 'str' object has no attribute 'get' | 17:27 |
| sean-k-mooney | that proably a bug | 17:27 |
| sean-k-mooney | since rebuild without an image specified is ment to default to same image | 17:27 |
| stephenfin | 100% | 17:27 |
| stephenfin | in OSC | 17:28 |
| sean-k-mooney | let me just check if in the api | 17:28 |
| whoami-rajat | yeah, I kind of like stephenfin approach too, we can ignore saving one LOC for readability | 17:28 |
| whoami-rajat | will update the patch | 17:28 |
| sean-k-mooney | stephenfin: its not | 17:29 |
| sean-k-mooney | well there is a osc bug | 17:29 |
| sean-k-mooney | but image is required | 17:29 |
| sean-k-mooney | in the api | 17:29 |
| sean-k-mooney | https://docs.openstack.org/api-ref/compute/?expanded=rebuild-server-rebuild-action-detail#rebuild-server-rebuild-action | 17:29 |
| sean-k-mooney | stephenfin: so in osc image should be required | 17:29 |
| sean-k-mooney | unless this was implemted in osc /nova client | 17:29 |
| stephenfin | yeah, it was | 17:30 |
| sean-k-mooney | ah ok | 17:30 |
| stephenfin | if you don't specify it, we use the same image | 17:30 |
| sean-k-mooney | right but that is client side | 17:30 |
| stephenfin | which is why you're used to that behaviour, I guess | 17:30 |
| stephenfin | sean-k-mooney: yup | 17:30 |
| sean-k-mooney | ack | 17:30 |
| stephenfin | whoami-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 |
| stephenfin | https://paste.opendev.org/show/bcl82mF5NEjC4T1uhbnu/ | 17:32 |
| stephenfin | yeah, it's nova itself. Ugh | 17:32 |
| whoami-rajat | ah ok, will update that part | 17:32 |
| sean-k-mooney | The 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-mooney | its in the api ref | 17:33 |
| stephenfin | Just because it's documented doesn't make it right :-P | 17:33 |
| sean-k-mooney | stephenfin: so its not a bug that just what we chose as the sentinel | 17:33 |
| sean-k-mooney | yes but its part of the api contract | 17:33 |
| stephenfin | oh, I'm not suggesting changing it. Merely complaining | 17:34 |
| sean-k-mooney | :) | 17:34 |
| stephenfin | yup, confirmed that rebuilding a BFV instance with the same image works, but using a different image results in a failure | 17:35 |
| stephenfin | ❯ openstack server rebuild --image cirros-0.5.1-x86_64-disk test-server-bfv | 17:35 |
| stephenfin | Image 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-bfv | 17:35 |
| stephenfin | +-------------------+----------------------------------------------------------+ | 17:35 |
| stephenfin | | Field | Value | | 17:35 |
| stephenfin | +-------------------+----------------------------------------------------------+ | 17:35 |
| stephenfin | ... | 17:35 |
| stephenfin | whoami-rajat: I think we're on the same page. I'll review that again in the morning. Late for me now o/ | 17:36 |
| stephenfin | Cheers for the input, sean-k-mooney | 17:36 |
| sean-k-mooney | :) | 17:36 |
| whoami-rajat | thanks stephenfin and sean-k-mooney | 17:37 |
| *** dasm is now known as dasm|off | 21:13 | |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!