Earlier  
Later
New Search
Posted Nick Remark
#openstack-sdks - 2022-10-27
18:36:55 ard_ hey, I am working on a https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862565 and have several question which I can't answer myself. According to https://docs.openstack.org/api-ref/image/v2/index.html?expanded=update-image-detail there is a set of image properties which can't be updated and this will result in 403 reply. The set I believe is here: https://github.com/openstack/glance/blob/master/etc/schema-image.json
18:36:55 ard_ 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.
18:38:26 ard_ at the same time min_disk and min_ram are not tried to be updated. Am I missing something?
19:37:44 ard_ jm1: regarding image.update_image with id and image object. If you send image object request.body being produced with {'op': 'remove', 'path': '/stores'} and lots of properties which aren't planned to touch. Because they are correct in existing image. openstacksdk==0.102.0 So my change has this strange workaround https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862565/8/plugins/modules/image.py#585
19:53:39 ard_ Here is a little bit more details: https://paste.opendev.org/show/817361/ So does it means that update args should contain all properties to not attempt removing them? Then the logic in the module is not correct. Otherwise there is an issue in sdk version .0.102.0
#openstack-sdks - 2022-10-28
07:05:48 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:34:38 jm1 ard_: o/ lets ask gtema about this :D
07:35:02 jm1 gtema: update functions such as update_image take a image object or id as first parameter
07:35:33 gtema yes, or something like a dict representing object
07:35:43 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:46 gtema {"id": "fakeid", "name": "fakename"}
07:37:08 jm1 gtema: ard_'s issue seems to be that this image+updated-args object is passed to glance api
07:38:09 jm1 gtema: from ard_'s description is looks like glance raises an error because it gets stuff which cannot be updated
07:38:33 jm1 gtema: is this excepted?
07:39:01 gtema not really
07:39:15 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:27 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:40:32 gtema it should not be the case. If you pass as first param whole image it should only update change attrs
07:40:56 gtema but image is specific in that it does things differently and tries to construct json patch
07:41:11 ard_ I tried to debug and see different body in api calls
07:42:16 gtema would be good if you give me some reproduce case (small sdk script)
07:43:07 ard_ the params are actually updated, when those 'op': 'remove' objects are added. It just complains with 409 error from api
07:43:38 ard_ give me 10 minutes
07:43:52 gtema no hurry, I am going for meeting now anyway
07:48:09 jm1 ard_: so we have different issues here? the kernel_id/ramdisk_id issue is separate from the image.id/image issue?
07:48:49 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:52:47 opendevreview Jakob Meng proposed openstack/ansible-collections-openstack master: [DNM] Image update test https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862864
07:53:00 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:54:04 jm1 ard_: this should fail if we read the api correctly https://review.opendev.org/c/openstack/ansible-collections-openstack/+/862864
07:55:36 jm1 ard_: may i alter your patch?
07:56:43 ard_ jm1: of course you can. I came here to get a working solution at the end.
08:01:32 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:55 jm1 ard_: ^ does this answer your question regarding min_disk..? :D
08:06:12 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:40 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:07:32 ard_ jm1: here https://opendev.org/openstack/ansible-collections-openstack/src/branch/master/plugins/modules/image.py#L460
08:12:55 jm1 ard_: why does min_disk not work?
08:14:51 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:23:42 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:24:45 jm1 ard_: i would not mix create and update parameters though, since they are not necessarily the same
08:25:58 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:41:34 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:44:12 jm1 ard_: does this ^ work for you? are we missing anything?
08:44:16 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:58 ard_ jm1: and it might not be min_disk or min_ram. This way you'll get this value reset to 0
08:45:16 jm1 ard_: glance uses 0 as the default for both min_disk and min_ram, so i dropped the default values
08:45:26 ard_ cool
08:46:15 jm1 ard_: those default values werent documented anyway :/
08:47:21 jm1 ard_: have you been able to reproduce this image update issue when passing image instead of image.id?
08:47:49 ard_ jm1: I can on my sandbox
08:48:02 ard_ jm1: just in python shell
08:49:05 jm1 ard_: would you mind sharing the code and sdk's debug output with us (without secrets/tokens/..)?
08:49:17 jm1 so that gtema can have a look
08:49:46 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:51:48 jm1 ard_: ack, fyi sdk has a enable_logging() function which gives all important output, no need to use pdb
08:55:07 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...
09:12:04 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:14:31 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:21:43 jm1 ard_: like this? https://docs.ansible.com/ansible/latest/dev_guide/debugging.html#simple-debugging
09:22:14 ard_ jm1: The script to reproduce https://paste.opendev.org/show/817369/
09:22:59 ard_ jm1: still looking how to sanitize debug sdk output. Even if it is for the sandbox in private network...
09:23:28 jm1 gtema: ^
09:24:28 ard_ jm1: exactly and the same way, just always got tracebacks on during debugging and now I know to use raise instead of print :)
10:00:44 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:06:27 jm1 ard_: oh wow, what is this owner_specified.* stuff? gtema handing over to you :D
10:06:55 gtema owner_specified are the special props of sdk (checksum verification)
10:08:04 ard_ gtema: but it should not request them to be removed?
10:08:15 gtema nope, should not
10:08:46 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:09:15 gtema btw, what is the update_payload?
10:09:15 gtema ah, sorry
10:19:53 gtema ard_ I can say that your script just worked for me
10:20:12 gtema it is definitely wrong that it tries to unset owner_specified*, but it definitely worked
10:20:35 ard_ it change the image, but drop 409
10:21:09 gtema 200 for me, I assume you fall into cloud specifics
10:21:14 jm1 gtema: where does this 'stores' come from and why is it passed to glance api?
10:21:14 ard_ gtema: you mean it is something with my sandbox
10:21:14 gtema which cloud you use?
10:22:17 gtema ard_ or is it real upstream deployed as private cloud?
10:22:46 ard_ it is aio deployment for testing purposes
10:23:08 gtema ok, never tried that
10:28:00 gtema I can reproduce it now once I try to set another property
10:47:30 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:39 gtema this looks like fun
10:51:08 jm1 gtema: incompatibility between jsonpatch lib used in glance and ard_'s venv or bug in glance?
10:52:14 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
12:04:47 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:25:33 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:29:55 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
14:03:28 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:49:19 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
16:05:08 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:06:15 gtema point is that for historical reasons all "custom" properties of images are stored in OSC and SDK under "properties" attribute
16:06:37 gtema as customs all attrs not currently defined under Image resource are meant
16:07:08 gtema so when you want to access such prop - you need to look under image.properties["foo"]
16:07:34 gtema and when you want to update such props you need to use coinn.image.update_image_properties(foo="bar")
16:08:16 arddennis yes, first when I saw it I was a little bit confused, then accepted it.
16:08:28 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:47 gtema problem right now is merging or the nested dicts
16:10:31 gtema but I agree, this is now pretty confusing and not self-explaining from docs

Earlier   Later