| opendevreview | Denys Mishchenko proposed openstack/ansible-collections-openstack master: Existing images update name, visibility etc https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862565 | 07:05 |
|---|---|---|
| jm1 | ard_: o/ lets ask gtema about this :D | 07:34 |
| jm1 | gtema: update functions such as update_image take a image object or id as first parameter | 07:35 |
| gtema | yes, or something like a dict representing object | 07:35 |
| jm1 | gtema: iiuc when an image object is given, then proxy class will take all other update_image args and assign them to the image object | 07:35 |
| gtema | {"id": "fakeid", "name": "fakename"} | 07:35 |
| jm1 | gtema: ard_'s issue seems to be that this image+updated-args object is passed to glance api | 07:37 |
| jm1 | gtema: from ard_'s description is looks like glance raises an error because it gets stuff which cannot be updated | 07:38 |
| jm1 | gtema: is this excepted? | 07:38 |
| gtema | not really | 07:39 |
| gtema | https://opendev.org/openstack/openstacksdk/src/branch/master/openstack/tests/functional/image/v2/test_image.py#L69 - we test changing image name in func tests | 07:39 |
| jm1 | gtema: maybe the issue is here: "and this does include kernel_id and ramdisk_id . At the same time version of the script tries to change them if such params defined and differs from the present image." | 07:39 |
| gtema | it should not be the case. If you pass as first param whole image it should only update change attrs | 07:40 |
| gtema | but image is specific in that it does things differently and tries to construct json patch | 07:40 |
| ard_ | I tried to debug and see different body in api calls | 07:41 |
| gtema | would be good if you give me some reproduce case (small sdk script) | 07:42 |
| ard_ | the params are actually updated, when those 'op': 'remove' objects are added. It just complains with 409 error from api | 07:43 |
| ard_ | give me 10 minutes | 07:43 |
| gtema | no hurry, I am going for meeting now anyway | 07:43 |
| jm1 | ard_: so we have different issues here? the kernel_id/ramdisk_id issue is separate from the image.id/image issue? | 07:48 |
| jm1 | ard_: reading the glance api doc i agree with you that updating kernel_id/ramdisk_id is not going to work: "An attempt to modify any of the “base” image properties that are managed by the Image Service. These are the properties specified as read only in the Image Schema." | 07:48 |
| opendevreview | Jakob Meng proposed openstack/ansible-collections-openstack master: [DNM] Image update test https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862864 | 07:52 |
| ard_ | jm1: yes, sorry that I brought multiple questions at the same time. Yes, it is separate and should not be tried to be updated. This is the question 1. Question 2: min_disk and min_ram - they are allowed to be changed | 07:53 |
| jm1 | ard_: this should fail if we read the api correctly https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862864 | 07:54 |
| jm1 | ard_: may i alter your patch? | 07:55 |
| ard_ | jm1: of course you can. I came here to get a working solution at the end. | 07:56 |
| opendevreview | Jakob Meng proposed openstack/ansible-collections-openstack master: Existing images update name, visibility etc https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862565 | 08:01 |
| jm1 | ard_: ^ does this answer your question regarding min_disk..? :D | 08:01 |
| opendevreview | Jakob Meng proposed openstack/ansible-collections-openstack master: Existing images update name, visibility etc https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862565 | 08:06 |
| ard_ | jm1: yes, this does answer it. I didn't put it in myself, wanted to discuss first as there might been a reason for this. Especially with the kernel parameter present. Also, min_disk has a default value of 0, so you can't just do like you did. It's default value should be moved to _build_params function | 08:06 |
| *** JqckB_ is now known as JqckB | 08:07 | |
| *** TheJulia_ is now known as TheJulia | 08:07 | |
| *** gouthamr_ is now known as gouthamr | 08:07 | |
| *** PrinzElvis_ is now known as PrinzElvis | 08:07 | |
| ard_ | jm1: here https://opendev.org/openstack/ansible-collections-openstack/src/branch/master/plugins/modules/image.py#L460 | 08:07 |
| jm1 | ard_: why does min_disk not work? | 08:12 |
| ard_ | if you run update image without this value it will be defaulted to 0 and if image had a limit, it will be reset to 0 | 08:14 |
| jm1 | ard_: sure which is to be expected with default values. but i agree we should think about removing the default values for min_disk and min_ram | 08:23 |
| jm1 | ard_: i would not mix create and update parameters though, since they are not necessarily the same | 08:24 |
| jm1 | ard_: actually i see more stuff which could be refactored... lets focus on the important parts here and i will refactor this one later | 08:25 |
| opendevreview | Jakob Meng proposed openstack/ansible-collections-openstack master: Existing images update name, visibility etc https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862565 | 08:41 |
| jm1 | ard_: does this ^ work for you? are we missing anything? | 08:44 |
| ard_ | jm1: I guess having min_disk and min_ram set to 0 if they were not defined is a good idea during image creation, but it is not a good idea during image change. You might try to change only one attribute of the image. | 08:44 |
| ard_ | jm1: and it might not be min_disk or min_ram. This way you'll get this value reset to 0 | 08:44 |
| jm1 | ard_: glance uses 0 as the default for both min_disk and min_ram, so i dropped the default values | 08:45 |
| ard_ | cool | 08:45 |
| jm1 | ard_: those default values werent documented anyway :/ | 08:46 |
| jm1 | ard_: have you been able to reproduce this image update issue when passing image instead of image.id? | 08:47 |
| ard_ | jm1: I can on my sandbox | 08:47 |
| ard_ | jm1: just in python shell | 08:48 |
| jm1 | ard_: would you mind sharing the code and sdk's debug output with us (without secrets/tokens/..)? | 08:49 |
| jm1 | so that gtema can have a look | 08:49 |
| ard_ | jm1: and as I said before, I see the difference in formed body. I used pdb to trace what is tried to be sent. I was on the meeting and delayed a bit, but soon I will give you example of the script. | 08:49 |
| jm1 | ard_: ack, fyi sdk has a enable_logging() function which gives all important output, no need to use pdb | 08:51 |
| ard_ | jm1: yeah, but sometimes you want to run commands and check how the vars will work in a functions by your hand. I will check enable_logging() I am new to this, so using only tools I used before... | 08:55 |
| jm1 | ard_: just mentioning it because you can use this with ansible modules too which are otherwise hard to debug: https://review.opendev.org/c/openstack/bifrost/+/859430/11/playbooks/roles/bifrost-configdrives-dynamic/tasks/main.yml#38 | 09:12 |
| ard_ | jm1: thanks for this as well. I used epdb to connect to the process on remote systems running ansible tasks. This way I debugged module work. Just had small dependency in venv for the ansible. | 09:14 |
| jm1 | ard_: like this? https://docs.ansible.com/ansible/latest/dev_guide/debugging.html#simple-debugging | 09:21 |
| ard_ | jm1: The script to reproduce https://paste.opendev.org/show/817369/ | 09:22 |
| ard_ | jm1: still looking how to sanitize debug sdk output. Even if it is for the sandbox in private network... | 09:22 |
| jm1 | gtema: ^ | 09:23 |
| ard_ | jm1: exactly and the same way, just always got tracebacks on during debugging and now I know to use raise instead of print :) | 09:24 |
| ard_ | jm1: gtema: here is a log output https://paste.opendev.org/show/bpAMU2si0ezvN8Ckf3KP/ I tried to sanitize it... but it is on the sandbox in a private network, so should not have any sensitive data. | 10:00 |
| jm1 | ard_: oh wow, what is this owner_specified.* stuff? gtema handing over to you :D | 10:06 |
| gtema | owner_specified are the special props of sdk (checksum verification) | 10:06 |
| ard_ | gtema: but it should not request them to be removed? | 10:08 |
| gtema | nope, should not | 10:08 |
| gtema | this means to me (on the first sight) that those props were not returned in the get and therefore it tries to drop them | 10:08 |
| gtema | btw, what is the update_payload? | 10:09 |
| gtema | ah, sorry | 10:09 |
| gtema | ard_ I can say that your script just worked for me | 10:19 |
| gtema | it is definitely wrong that it tries to unset owner_specified*, but it definitely worked | 10:20 |
| ard_ | it change the image, but drop 409 | 10:20 |
| gtema | 200 for me, I assume you fall into cloud specifics | 10:21 |
| jm1 | gtema: where does this 'stores' come from and why is it passed to glance api? | 10:21 |
| ard_ | gtema: you mean it is something with my sandbox | 10:21 |
| gtema | which cloud you use? | 10:21 |
| gtema | ard_ or is it real upstream deployed as private cloud? | 10:22 |
| ard_ | it is aio deployment for testing purposes | 10:22 |
| gtema | ok, never tried that | 10:23 |
| gtema | I can reproduce it now once I try to set another property | 10:28 |
| *** arxcruz is now known as arxcruz|ruck | 10:36 | |
| gtema | one awesome finding here: glance responds `Invalid operation: `move`. It must be one of the following: add, remove, replace.` and patch is generated by jsonpatch lib | 10:47 |
| gtema | this looks like fun | 10:47 |
| jm1 | gtema: incompatibility between jsonpatch lib used in glance and ard_'s venv or bug in glance? | 10:51 |
| gtema | no clue. It doesn't mean every user will face it, since you need to have situation where jsonpatch desides attribute is "renamed" keeping the content | 10:52 |
| gtema | jm1 ard_ - I am able to reproduce it now in the unittest, still digging where is it coming from. It is definitely a victim of magic properties handling | 12:04 |
| jm1 | ard_: look at that, you can actually update the ramdisk and kernel in devstack https://storage.bhs.cloud.ovh.net/v1/AUTH_dcaab5e32b234d56b626f72581e3644c/zuul_opendev_logs_e3c/862864/1/check/ansible-collections-openstack-functional-devstack/e3c2883/job-output.txt | 12:25 |
| opendevreview | Jakob Meng proposed openstack/ansible-collections-openstack master: Existing images update name, visibility etc https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862565 | 12:29 |
| *** njohnsto- is now known as njohnsto_ | 14:02 | |
| arddennis | jm1: yes you can probably update kernel and ramdisk, but at some point of time you could loose this ability. And the question was more to highlight my misunderstanding of the situation. Where the property which is supposed to be changeable is not and vise versa. | 14:03 |
| gtema | ard_ jm1: instead of invoking update_image a call to update_image_properties can be used (https://paste.opendev.org/show/beYgrcpmaEw1pIM6cFtU/). This is what is currently used in the cloud layer and is working. I am still trying to find out why update_image itself is corrupting extra params | 14:49 |
| arddennis | gtema jm1: Do you think it is reasonable to move update for the properties out of the image update and make two api requests instead of one if properties are defined? It isn't hard to implement. | 16:05 |
| gtema | point is that for historical reasons all "custom" properties of images are stored in OSC and SDK under "properties" attribute | 16:06 |
| gtema | as customs all attrs not currently defined under Image resource are meant | 16:06 |
| gtema | so when you want to access such prop - you need to look under image.properties["foo"] | 16:07 |
| gtema | and when you want to update such props you need to use coinn.image.update_image_properties(foo="bar") | 16:07 |
| arddennis | yes, first when I saw it I was a little bit confused, then accepted it. | 16:08 |
| gtema | if you try to pass conn.image.update_image(foo="bar") foo is identified as custom property and then SDK does not know - what do you want to do with other custom properties - only change single one or reset them all | 16:08 |
| gtema | problem right now is merging or the nested dicts | 16:08 |
| gtema | but I agree, this is now pretty confusing and not self-explaining from docs | 16:10 |
| opendevreview | Jakob Meng proposed openstack/ansible-collections-openstack master: Revert openstack.cloud.server parameter user_data to userdata https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862935 | 16:28 |
| servagem | Hi, could someone review this patch? It is missing one approval. Thank you. https://review.opendev.org/c/openstack/python-openstackclient/+/862513 | 16:38 |
| jm1 | arddennis: would you please review the latest patchset? https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862565/ | 17:20 |
Generated by irclog2html.py 2.17.3 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!