| opendevreview | David Hill proposed openstack/nova stable/victoria: Ensure MAC addresses characters are in the same case https://review.opendev.org/c/openstack/nova/+/816927 | 00:12 |
|---|---|---|
| *** gouthamr_ is now known as gouthamr | 04:07 | |
| *** erlon_ is now known as erlon | 04:07 | |
| opendevreview | Rajat Dhasmana proposed openstack/nova-specs master: Add spec for volume backed server rebuild https://review.opendev.org/c/openstack/nova-specs/+/809621 | 05:02 |
| *** ministry is now known as __ministry | 06:55 | |
| EugenMayer | good morning. the default_availability_zone setting, where does it belong, on the nova-schedular service or nova-compute on the compute? Kind of make sense on the former | 07:52 |
| EugenMayer | maybe in common, when reading https://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.default_availability_zone - how to understand, for each of those values, which service use them? Since i use kolla, i have a lot of nova.conf files and default_schedule_zone could go on potentially several of those. How would i find those | 07:55 |
| EugenMayer | informations in the docs? | 07:55 |
| gibi | EugenMayer: for some of the config the docs states which service uses them, but some it does not. In the latter case you need to look into the code | 08:55 |
| gibi | EugenMayer: based on a quick grep for CONF.default_availability_zone it is used by the scheduler and the api service | 09:01 |
| bauzas | good morning | 09:12 |
| bauzas | gibi: EugenMayer: correct, we use the default AZ value in the API | 09:13 |
| bauzas | if none AZ is provided, then we look at the default one, which is 'None' | 09:13 |
| gibi | bauzas: o/ | 09:14 |
| bauzas | if an operator sets the default_az value, then any instance which would be created will have an AZ | 09:14 |
| bauzas | either the default AZ if the user doesn't ask for it, or then the AZ that the user provides with --az | 09:14 |
| EugenMayer | bauzas / gibi thank you. The question is in which group? | 09:21 |
| opendevreview | Ilya Popov proposed openstack/nova master: Fix to use NUMA cell with free resources first https://review.opendev.org/c/openstack/nova/+/805649 | 09:27 |
| bauzas | EugenMayer: sorry, missed your ping | 10:06 |
| bauzas | EugenMayer: you ask about the config group ? | 10:06 |
| EugenMayer | yes since beside one does not know which service, one does not know the group bauzas - kind of heavily lifting expected to look this up in the source code but well, i guess i could just contribute the docs and stop complaining :) | 10:07 |
| opendevreview | Martin Kopec proposed openstack/nova master: Update Interop doc https://review.opendev.org/c/openstack/nova/+/816980 | 10:12 |
| opendevreview | Jan Hartkopf proposed openstack/nova master: add support for updating server's user_data https://review.opendev.org/c/openstack/nova/+/816157 | 10:48 |
| *** tobias-urdin5 is now known as tobias-urdin | 11:06 | |
| opendevreview | wujian proposed openstack/nova master: Fix instance action event result inconsistent https://review.opendev.org/c/openstack/nova/+/816990 | 11:20 |
| *** lbragstad7 is now known as lbragstad | 11:41 | |
| opendevreview | David Hill proposed openstack/nova stable/victoria: Ensure MAC addresses characters are in the same case https://review.opendev.org/c/openstack/nova/+/816927 | 11:48 |
| gibi | lyarwood: hi! could please look at the stable/victoria backport of https://review.opendev.org/q/topic:bug/1944759 elodilles already has +2 on it | 12:14 |
| * lyarwood clicks | 12:14 | |
| lyarwood | gibi: ACK'd | 12:15 |
| gibi | thanks! | 12:15 |
| gibi | bauzas: replied to your question in https://review.opendev.org/c/openstack/nova/+/815690/comment/764e7a47_1cdc2d2d/ | 12:22 |
| sean-k-mooney | stephenfin: im going to respin this shortly can you remove your -2 on https://review.opendev.org/c/openstack/nova/+/804292 | 12:29 |
| sean-k-mooney | lyarwood: ^ that is what we agreed to do at the ptg by the way so that will allow you to avoid issue with 3.10 | 12:31 |
| lyarwood | ah cool sorry missed that | 12:31 |
| sean-k-mooney | although we proably need to start testing with 3.10 this cycle at least non voting | 12:31 |
| sean-k-mooney | im going to quickly repin my autopep8 patch and then that | 12:32 |
| opendevreview | sean mooney proposed openstack/nova master: Add autopep8 to tox and pre-commit https://review.opendev.org/c/openstack/nova/+/806182 | 12:37 |
| opendevreview | Takashi Kajinami proposed openstack/nova stable/xena: Clean up allocations left by evacuation when deleting service https://review.opendev.org/c/openstack/nova/+/816954 | 12:38 |
| sean-k-mooney | lyarwood: gibi if we can merge ^ relitivily quickly that woudl be great as its enforcing that all patch pass autopep8 without needing file changes | 12:38 |
| opendevreview | sean mooney proposed openstack/nova master: This change replaces all hardcoded tox enve with generative envs https://review.opendev.org/c/openstack/nova/+/804292 | 12:44 |
| lyarwood | sean-k-mooney: is that mostly picking up a rule we skip with flake8? | 12:46 |
| sean-k-mooney | autopep8 actully read the tox file for what we skip | 12:46 |
| sean-k-mooney | but yes | 12:46 |
| sean-k-mooney | i dont know how to ignore that 1 new line for doc stings | 12:47 |
| sean-k-mooney | that basicly the only change it really makes today | 12:47 |
| lyarwood | ah right that's why I was asking, I couldn't tell if it was picking up our skip list or not | 12:47 |
| sean-k-mooney | ya it reads the flake8 section | 12:48 |
| sean-k-mooney | so it should respect ignore = E121,E122,E123,E124,E125,E126,E127,E128,E129,E131,E251,H405,W504,E731,H238 | 12:49 |
| sean-k-mooney | if we know what rule was adding the new line we could add it to the list | 12:49 |
| lyarwood | tbh I'd rather do that and avoid the huge diff | 12:50 |
| * lyarwood will look at https://www.flake8rules.com/ after lunch | 12:50 | |
| lyarwood | brb | 12:50 |
| sean-k-mooney | well its much smaller then black ecta but sure | 12:51 |
| sean-k-mooney | looking at https://github.com/hhatto/autopep8#features there are seveeral that are fixing indentaiton or addign missing blank lines | 12:51 |
| sean-k-mooney | it might be comeing form https://www.python.org/dev/peps/pep-0257/ | 12:58 |
| sean-k-mooney | """Insert a blank line after all docstrings (one-line or multi-line) that document a class -- generally speaking, the class's methods are separated from each other by a single blank line, and the docstring needs to be offset from the first method by a blank line.""" | 13:01 |
| sean-k-mooney | https://www.python.org/dev/peps/pep-0257/#multi-line-docstrings | 13:01 |
| sean-k-mooney | lyarwood: ^ that is where that si cominng form | 13:01 |
| sean-k-mooney | ah and its called out in the autopep8 | 13:02 |
| sean-k-mooney | Put a blank line between a class docstring and its first method declaration. (Enabled with E301.) | 13:02 |
| sean-k-mooney | E301 is inconsitent tabs and spaces | 13:03 |
| sean-k-mooney | https://www.flake8rules.com/rules/E101.html | 13:03 |
| lyarwood | and that's enabled so why isn't flake8 picking it up already? | 13:08 |
| sean-k-mooney | its not part of pep8 | 13:08 |
| lyarwood | ah | 13:08 |
| sean-k-mooney | E301 does not specify the docstring behavior | 13:08 |
| sean-k-mooney | auto pep8 impleted as part of the E301 fixing | 13:09 |
| lyarwood | sorry I thought you said it did, I see what you're sayingnow | 13:09 |
| sean-k-mooney | lyarwood: summerised it in the review https://review.opendev.org/c/openstack/nova/+/806182/3#message-f37506ed0411c71d9ce66de251196048da171b34 | 13:13 |
| gibi | sean-k-mooney: I have one question in autopep8 https://review.opendev.org/c/openstack/nova/+/806182/comment/e15200f9_a50f0219/ | 13:15 |
| sean-k-mooney | we can add it to test-requiremetns yes | 13:16 |
| sean-k-mooney | i just did not to avoid installing it in enves that did not need it | 13:16 |
| sean-k-mooney | but if you prefer that i can update it shortly | 13:16 |
| gibi | hm, will we ever need to pin the version of autopep8? | 13:16 |
| sean-k-mooney | this can be done with upperconstraitns | 13:17 |
| sean-k-mooney | or inline | 13:17 |
| sean-k-mooney | i dont think so but its a vaild question | 13:17 |
| gibi | currently it can only be inline afaik. but anyhow we don't pin it now, and we can move it to test-requirements when we need to pin it | 13:17 |
| gibi | so I'm +2 | 13:17 |
| sean-k-mooney | for line 69 yes since that does not include the base test env dep for the pep8 env the upper constratis form the base env still apply | 13:19 |
| lyarwood | okay LGTM | 13:22 |
| * lyarwood heads to the shops, back in ~30mins. | 13:23 | |
| lyarwood | oh wait, downstream meeting :| | 13:23 |
| lyarwood | helps if I look at the correct day in my cal | 13:23 |
| opendevreview | Merged openstack/nova master: db: Remove legacy placement models https://review.opendev.org/c/openstack/nova/+/812146 | 13:26 |
| opendevreview | Merged openstack/nova master: objects: Stop querying the main DB for keypairs https://review.opendev.org/c/openstack/nova/+/812147 | 13:26 |
| bauzas | gibi: +w'd your series | 13:50 |
| gibi | bauzas: thanks | 13:50 |
| gibi | melwitt: hi! When you are up, we are waiting for you +A in https://review.opendev.org/c/openstack/nova/+/815689/2 so if you have time please check | 14:13 |
| opendevreview | Merged openstack/nova master: Revert "tox: Encode specific Python versions" https://review.opendev.org/c/openstack/nova/+/804168 | 14:23 |
| EugenMayer | When doing a nova backup, it seems like ephemeral disks are entirely excluded. is this to be expected? | 14:30 |
| sean-k-mooney | EugenMayer: yes | 14:30 |
| sean-k-mooney | EugenMayer: snapshots and backups are only done of the root disk | 14:31 |
| EugenMayer | wow, that is a huge bummer ;/ | 14:31 |
| EugenMayer | Is there any way to workarround that? somehow using glance directly and creating an image per disk? | 14:31 |
| sean-k-mooney | ephemeral is not intended to be used for storing data that cant be recreated or safely lost | 14:31 |
| sean-k-mooney | no it was exluded by design and cant be worked around | 14:32 |
| sean-k-mooney | without code changes | 14:32 |
| EugenMayer | Well, the problem is, there is no way to have multi-disk environment without defining one as ephemeral, right? | 14:32 |
| sean-k-mooney | not without cinder | 14:32 |
| EugenMayer | yes, those are volumes then | 14:33 |
| sean-k-mooney | with openstack and most cloud plathform the expected and recommended usecase is to have only 1 disk with any addtional storage provided via volumes | 14:33 |
| kevko | Hi nova team :) , testing wallaby and when I'm creating heat stack ..from time to time I see this error in nova .. is it known bug ? | 14:34 |
| kevko | https://paste.opendev.org/show/810848/ | 14:34 |
| sean-k-mooney | not in nova that normally means you have an issue with neutron | 14:34 |
| EugenMayer | sean-k-mooney i understand that since one scale that one disk easily, no need to add others. But those VMs are legacy and partially it is part of the application desing to have 2 disks (on premise things). But well, still that now is somewhat a huge showstopper is was not accounting for yet | 14:34 |
| dansmith | kevko: yeah, means neutron failed to send the even to nova to indicate that networking is ready | 14:35 |
| sean-k-mooney | kevko: if you create really large heat stacks and your neutron installation is slow then it can time out wireing up the ports | 14:35 |
| sean-k-mooney | you can workaround it by extending the timeout but the real fix is to tune your neutron deployment to support the workload | 14:36 |
| kevko | well, it's really small heat template | 14:37 |
| sean-k-mooney | is this in the upstream ci or a production issue in your deployment by the way | 14:37 |
| sean-k-mooney | ah you said you were testing it so i guess its in your lab | 14:38 |
| sean-k-mooney | what network backend are you using? | 14:38 |
| EugenMayer | sean-k-mooney thank you for givin the definit answer. Not sure what i make out of it in terms of options/solutions but at least good to know it is intended and road blocked | 14:38 |
| kevko | well, it is production problem also on client's side .. | 14:38 |
| kevko | so i'm trying to replicate issue on lab | 14:38 |
| kevko | i can't see relevant errors in neutron | 14:38 |
| sean-k-mooney | kevko: ack is this ml2/ovn? ml2/ovs? cisco aci perhaps? | 14:39 |
| sean-k-mooney | kevko: on the neutron side you likely wont see an error more it wont have sent the event before the timeout exired | 14:39 |
| sean-k-mooney | kevko: if its close but not quite longenough you might actully see the network-vif-plugged event in the nova log after the timeout exired as an unexpected event | 14:40 |
| dansmith | sean-k-mooney: we should debug log the time we waited each time so that people can see if they're close to the deadline normally, or if it's normally 5 seconds, one 300 timeout is obviously an event that's never coming | 14:41 |
| kevko | ml2/ovs | 14:41 |
| sean-k-mooney | if the env is using ml2/ovs you would need to check the neutron l2 agent log to see if it processed the port. for the other two you would hae to check the neutron server log i belive | 14:41 |
| sean-k-mooney | dansmith: ya that is not a bad idea, on a related note i was speaking to bogdando downstream about also potentially queueing the event in the compute agent if no handeler with a TTL that might help in case that the event come before we start waiting but there are downsides to that. | 14:43 |
| dansmith | sean-k-mooney: the entire point of that infrastructure is to make it hard to allow such things :) | 14:44 |
| dansmith | sean-k-mooney: we prepare the event before we do the thing that could cause it to be fired | 14:44 |
| dansmith | otherwise we turn it into a statistical race condition problem | 14:44 |
| sean-k-mooney | yep i know i mentioned that we did not do that for a reason and we discused it before and rejected it | 14:44 |
| * dansmith nods | 14:45 | |
| sean-k-mooney | am by the way i would like your in put on the healthcheck design at some point | 14:45 |
| dansmith | is something up to review? | 14:45 |
| kevko | sean-k-mooney: regarding above ... timeout for rpc response ? or which timeout ..sorry :/ | 14:46 |
| sean-k-mooney | not yet ill likely start on the spec tomorrow or later today but the open question i have is should the datastucture be storge in a module gobal or passed via the context to each fucntion | 14:46 |
| sean-k-mooney | kevko: ill get you the link to the docs but there is a vif_plug_timeout you should increase | 14:47 |
| sean-k-mooney | kevko: that is really jsut a workaround however | 14:47 |
| kevko | so, bump number of neutron workers ? | 14:47 |
| sean-k-mooney | https://docs.openstack.org/nova/latest/configuration/config.html#DEFAULT.vif_plugging_timeout | 14:47 |
| dansmith | kevko: in all likelihood, you're not just a couple seconds late for a *5* minute timeout.. much more likely that it's never coming and changing the timeout will just make things more painful | 14:48 |
| sean-k-mooney | yep | 14:48 |
| sean-k-mooney | kevko: what you really need to do is look at the neutron agent logs on the compute | 14:48 |
| dansmith | kevko: you should see messages in the nova-compute log saying that the event actually showed up but was discarded - if you don't, then changing the timeout just makes your system worse | 14:48 |
| sean-k-mooney | and see when the port was added and how far it got in wiring it up | 14:48 |
| kevko | sean-k-mooney: same log + neutron-ovs-agent 10 minutes +/- ago https://paste.opendev.org/show/810850/ | 14:52 |
| sean-k-mooney | kevko: are you using iptbals by the way or openvswtich firewall driver | 14:54 |
| kevko | ovs | 14:54 |
| * gibi has to drop off early today o/ | 14:54 | |
| sean-k-mooney | ok in that case in wallaby the port is plugged by libvirt as part of the instance creation | 14:54 |
| sean-k-mooney | gibi: o/ | 14:54 |
| opendevreview | Dan Smith proposed openstack/nova master: Log VIF event wait times https://review.opendev.org/c/openstack/nova/+/817030 | 14:55 |
| dansmith | sean-k-mooney: ^ | 14:55 |
| sean-k-mooney | kevko: at least presently since i have not backported the change to delegate taht to os-vif yet | 14:55 |
| sean-k-mooney | oh you are using the Stopwatch functionality instead of time.now and subtracting ya that is nicer | 14:56 |
| kevko | sean-k-mooney: what does it mean ? :/ | 14:56 |
| kevko | checking bad logs ? | 14:56 |
| sean-k-mooney | kevko: that was to dansmith he has already created a patch to log the elapsed time | 14:57 |
| sean-k-mooney | kevko: https://review.opendev.org/c/openstack/nova/+/817030/1/nova/compute/manager.py#491 | 14:57 |
| sean-k-mooney | kevko: oh you ment about libvirt plugging the interafce not os-vif | 14:57 |
| kevko | yeah | 14:57 |
| sean-k-mooney | in wallaby when we call plug on os-vif for port wiht hybrid-plug=false, such as when using the ovs firewall, os-vif just ensure the ovs bridge exists and the port is actully added to the bridge by libvirt | 14:58 |
| sean-k-mooney | so the port wont be added until libvirt tries to start the vm | 14:59 |
| sean-k-mooney | the neutron agent only starts wiring up the port after its created by libvirt but in this code path it will happen only after libvirt starts the vm | 15:00 |
| sean-k-mooney | in the pasued state | 15:00 |
| sean-k-mooney | if you were usign a differnt network backedn the behaivor would be differnt in nova | 15:00 |
| sean-k-mooney | well potentially in any case which is why i asked | 15:01 |
| sean-k-mooney | with your configuration the port will only be created at this point https://github.com/openstack/nova/blob/400d25fdeb45fe53be1069996ffaa3783eb4402b/nova/virt/libvirt/driver.py#L7209-L7212 | 15:03 |
| sean-k-mooney | for other configurtion it would happen eairler here https://github.com/openstack/nova/blob/400d25fdeb45fe53be1069996ffaa3783eb4402b/nova/virt/libvirt/driver.py#L7205 | 15:03 |
| sean-k-mooney | kevko: if creating the guest took a long time its possible that we woudl time out waiting for the even but that is a low proablity | 15:05 |
| sean-k-mooney | kevko: if you put the neutron l2 agent in debug mode it will print the addtion and removal of the port in the ovsdb and also message as it configures them | 15:06 |
| sean-k-mooney | *debug log level | 15:06 |
| kevko | hmm, so issue is not in workers .. | 15:06 |
| sean-k-mooney | kevko: likely not | 15:07 |
| kevko | ok, i will try to turn on neutron debug log and see | 15:07 |
| sean-k-mooney | kevko: i would suggest putting noa and neutron into debug mode on the host then booting another vm and check if while its waitign the domain is succesfully created in libvirt and the tap device is added to ovs | 15:07 |
| sean-k-mooney | if it is then you shoudl check the agent log to see if it deteched it and started to process it | 15:08 |
| kevko | thank you sean-k-mooney, probably i will ask you tomorrow | 15:16 |
| sean-k-mooney | lyarwood: by the way im just stack a clean devstack then im going to work on the qemu wapper that we discussed last week. ill ping you when i have something | 15:17 |
| opendevreview | Elod Illes proposed openstack/nova stable/stein: Reject open redirection in the console proxy https://review.opendev.org/c/openstack/nova/+/802935 | 15:21 |
| lyarwood | sean-k-mooney: ack sounds good | 15:29 |
| opendevreview | Elod Illes proposed openstack/nova stable/stein: address open redirect with 3 forward slashes https://review.opendev.org/c/openstack/nova/+/817037 | 15:29 |
| dansmith | sean-k-mooney: gmann: So, going back to last week's discussion about extra specs... | 15:56 |
| dansmith | sounds like system reader and project admin should be able to see extra specs at this point to be the most compatible with existing stuff yeah? | 15:57 |
| dansmith | (i.e. any system user can see them, only admin on the project side can see them) | 15:57 |
| sean-k-mooney | i think project reader shoudl be able to see extra specs | 15:58 |
| sean-k-mooney | i dont think you should need project admin | 15:59 |
| dansmith | I know you do, but today regular users can't right? | 15:59 |
| sean-k-mooney | they can | 15:59 |
| sean-k-mooney | its systrem_reader_or_project_reader i think today | 15:59 |
| dansmith | oh, maybe that explains why the test is so weird | 15:59 |
| sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/policies/flavor_extra_specs.py#L78 | 16:00 |
| dansmith | heh yeah okay | 16:00 |
| sean-k-mooney | adding/removing obviously should be system scoped i think | 16:00 |
| dansmith | the test conflates a lot of stuff, I guess for that reason, so I'll have to split things apart a bit because system can't see the servers, and thus can't see the embedded flavor | 16:01 |
| sean-k-mooney | ah right | 16:01 |
| dansmith | hrm, | 16:12 |
| dansmith | pretty sure there's a bug in the create test that is testing index perms for create | 16:12 |
| dansmith | https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/tests/unit/policies/test_flavor_extra_specs.py#L235-L254 | 16:13 |
| dansmith | the test purports to check update, | 16:13 |
| melwitt | gibi: ack, will look | 16:13 |
| dansmith | but it's actually checking index, which is rule_name instead of rule | 16:13 |
| dansmith | and it's stubbing out update for everybody instead of index | 16:14 |
| opendevreview | Merged openstack/nova stable/victoria: Reproduce bug 1944759 https://review.opendev.org/c/openstack/nova/+/810914 | 16:14 |
| dansmith | and the test actually asserts that system reader can update extra specs | 16:15 |
| lyarwood | elodilles / gibi ; https://review.opendev.org/q/I26b2a14e0b91c0ab77299c3e4fbed5f7916fe8cf do either of you recall why we don't need this on >= stable/victoria ? | 16:18 |
| lyarwood | seeing some weird behaviour downstream where we hit the 2to3 issues on py39 thanks to virtualenv and setuptools versions but upstream it looks like UC is downgrading setuptools for us during the run | 16:21 |
| kashyap | frickler: lyarwood: Unrelated: just for info: MichalP from libvirt has completed both things: switching to '-accel' by default, and wiring up tb-cache. I tested his v2 series, and looks good: https://listman.redhat.com/archives/libvir-list/2021-November/msg00236.html | 16:22 |
| lyarwood | awesome thanks | 16:23 |
| * kashyap will wire that up to the Nova spec-less bp that he posted the other day | 16:24 | |
| elodilles | lyarwood: i think it is related to victoria and newer runs over focal on gate and we haven't encounter the use_2to3 errors there | 16:31 |
| lyarwood | elodilles: interesting, I get the feeling this might have something to do with the pip version tbh | 16:36 |
| lyarwood | elodilles: the weirdness downstream where we don't downgrade setuptools during the run as we do upstream that is | 16:36 |
| gmann | dansmith: we need to have separate policy now for this. for listing via 1. GET /flavors/{flavor_id}/os-extra_specs/ - system-reader-or-project-reader 2. showing extraspec in GET /servers APIs response we need project_reader 3. PUT/rebuild /servers we need to add project_member | 16:53 |
| dansmith | gmann: yeah, but I think the existing tests are wrong | 16:53 |
| elodilles | lyarwood: so the issue in nova was with the l-c job as it used decorator==3.4.0, which uses the use_2to3 from setuptools. In victoria the l-c.txt has decorator==4.1.0 set, which is not having the use_2to3 anymore | 16:53 |
| dansmith | gmann: for both create and update | 16:53 |
| gmann | is it? | 16:53 |
| dansmith | gmann: if not I need help understanding this: https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/tests/unit/policies/test_flavor_extra_specs.py#L235-L254 | 16:53 |
| dansmith | gmann: we're checking update, but we're making policy for update be @ | 16:53 |
| dansmith | gmann: and we're running the check against index | 16:54 |
| dansmith | if I run the check against update and don't stub update with @, I get a fail | 16:54 |
| lyarwood | elodilles: sorry was on a call | 16:56 |
| gmann | dansmith: yeah, so we are checking this policy https://github.com/openstack/nova/blob/master/nova/api/openstack/compute/flavor_manage.py#L124 | 16:56 |
| lyarwood | elodilles: https://bugs.launchpad.net/designate/+bug/1946340 is the issue that we are hitting downstream, same root cause as the decorator problem just a different package | 16:56 |
| gmann | dansmith: so first policy check of update has to be @ | 16:56 |
| lyarwood | elodilles: and with stable/wallaby upstream we don't appear to be hitting it because our upper constraints correctly downgrades setuptools during a run | 16:57 |
| gmann | dansmith: so that we allow update policy for everyone and see if flavor extraspec in flavor update API response is included as per extra spec policy | 16:57 |
| dansmith | gmann: ...but then you're just asserting that the user can run index, not that update is properly checking the thing right? | 16:57 |
| lyarwood | elodilles: I'm just lost as to why this isn't happening downstream | 16:57 |
| dansmith | gmann: the test is asserting that project reader can create/update flavor extra specs | 16:58 |
| johnsom | lyarwood We removed the EOL driver that needed suds-jurko from Designate: https://review.opendev.org/c/openstack/designate/+/813380 | 16:58 |
| gmann | dansmith: this test is for 'updating flavor return the extra specs if policy allow' | 16:58 |
| johnsom | stable branches will need to be pinned | 16:58 |
| lyarwood | johnsom: right it's a dep of oslo.vmware as well so it's still pulled in by nova during a unit/functional run | 16:59 |
| gmann | dansmith: for create/update flavor is separate tests | 16:59 |
| dansmith | gmann: okay I see test_update_flavor_extra_specs_policy and test_flavor_update_with_extra_specs_policy | 16:59 |
| dansmith | gmann: are you saying the latter is for the "and can see the result" variant? | 17:00 |
| dansmith | if so, that's majorly confusing :) | 17:00 |
| gmann | dansmith: yeah, i should have name it something like test_flavor_update_return_extra_specs_policy | 17:00 |
| gmann | or more clear | 17:01 |
| dansmith | I have a hard time reasoning about these tests, with very few comments and sparse naming, | 17:02 |
| dansmith | because they're trying to replicate things super deep in an api request (as in this case) | 17:02 |
| gmann | dansmith: yeah,multi-policy operation it is confusing but I agree I should have add more comments there | 17:03 |
| elodilles | lyarwood: nova had only this l-c job failure with 'decorator', but other projects had different other packages that failed and had to be replaced/updated. Most probably some packages' version are older at your downstream job than the upstream. | 17:12 |
| kevko | sean-k-mooney: is this related to mi issue ? :/ https://paste.opendev.org/show/810855/ ? | 17:59 |
| sean-k-mooney | kevko: you get the error if the port has been created in ovs but the tapdevice is not present on the host in the same network namespace as ovs | 18:51 |
| sean-k-mooney | actully no that is a slightly idffernt error | 18:52 |
| sean-k-mooney | in this case the port has been revomed form the ovsdb. | 18:52 |
| sean-k-mooney | this might be a race with the vm deletion | 18:52 |
| dansmith | gmann: are you okay with this? https://pastebin.com/hGzKL2ap | 19:33 |
| dansmith | gmann: it makes the tests much less verbose, easier to reason about (IMHO) and easier to refactor when we move things around | 19:34 |
| gmann | dansmith: sure, looks good to me. | 19:39 |
| dansmith | gmann: okay cool :) | 19:39 |
| dansmith | gmann: why is this index and not show? https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/api/openstack/compute/views/servers.py#L236-L237 | 19:49 |
| gmann | dansmith: because we used single policy for listing all extraspecs in flavor APi and in server API response also. | 19:50 |
| gmann | but with new granular one we can name them like servers:show:flavor_extraspecs | 19:51 |
| dansmith | gmann: right, but index means "can you list" and show means "can you see the actual thing" right? | 19:51 |
| gmann | dansmith: yes | 19:51 |
| dansmith | seems like the server-embedded flavor is more of a "show" than "index" to me | 19:51 |
| dansmith | ah, | 19:53 |
| dansmith | I guess it's because a show on a flavor including extra specs means "listing" the extra specs inside | 19:53 |
| gmann | dansmith: but that is list right? not single extraspec. we usually used show for getting single resource details and index for list | 19:53 |
| dansmith | I think that's probably just an implementation detail about how we store those..seems confusing from the outside, but I see now | 19:53 |
| gmann | this one https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/api/openstack/compute/flavors_extraspecs.py#L59 | 19:54 |
| dansmith | yep, I understand now | 19:55 |
| dansmith | makes sense if you consider extra specs their own thing and a flavor as a container of them. I know that's how we do it, it just a little confusing if you think of the flavor as a whole thing :) | 19:56 |
| gmann | yeah | 19:56 |
| gmann | dansmith: i am thinking do we need separate policy for showing extra-specs? | 19:57 |
| dansmith | showing individual specs? | 19:58 |
| gmann | I mean if flavor can be seen by anyone then we show extraspecs also to them or have policy to show flavor detail itself? | 19:59 |
| dansmith | oh, I thought that came from some people wanting to be able to exclude extra_specs from the view of regular users, without removing the rest of the flavor | 19:59 |
| dansmith | sean-k-mooney: ? | 19:59 |
| sean-k-mooney | dansmith: no i was suggesting we might want to allow that in the futre currently some public cloud hide all extra specs form there users there is not filterign today | 20:00 |
| dansmith | sean-k-mooney: there's a separate rule for showing them now | 20:01 |
| sean-k-mooney | in the server reponce or in flavor list | 20:01 |
| sean-k-mooney | i tought there was just one for both | 20:02 |
| gmann | in flavor and server response both | 20:02 |
| gmann | yeah currently one for both | 20:02 |
| gmann | and default to reader so kind of everyone as default | 20:02 |
| sean-k-mooney | what i think woudl be nice at some point woudl be to be able to filter out any non namespace extra spec an any namespaced on related to filtering | 20:03 |
| sean-k-mooney | that way operators that want to hide infra detail can do so but still expsoe the rest fo the extra specs | 20:03 |
| sean-k-mooney | some extra specs ar thigns a user really want to know like does this have a gpu | 20:04 |
| sean-k-mooney | other are really things that are important to the admin and schduler only | 20:04 |
| gmann | that is little complex to handle but anyways let's leave them as it is then if it get updated in future | 20:06 |
| sean-k-mooney | gmann: well that would not be implemeted as a policy it would likely need to be a new api microverions | 20:07 |
| sean-k-mooney | or at least a code change that would implement the filtering | 20:07 |
| gmann | sean-k-mooney: but you said that should be configurable right not hard-coded is_admin check? | 20:08 |
| sean-k-mooney | right now if operator use extra specs for filtering and want to hide that form user there only option is to hide all the extra specs which is an interoperatty issue | 20:08 |
| sean-k-mooney | gmann: good quetion i assuem configurble jsut for interop but admin only for some might make sense. anyway in the context of dans patch we shoudl nto change anythign form what we have today | 20:09 |
| sean-k-mooney | gmann: i just dont really think the exsitg policy fully solve that use case but operators are not complaing so lets not change it for now | 20:09 |
| gmann | sean-k-mooney: or they are not even know it :) as it is allow to show extra specs to everyone as default | 20:10 |
| gmann | but yes if someone has overridden the policy then it make sense | 20:10 |
| gmann | let's leave those as it is. | 20:11 |
| gmann | dansmith: sean-k-mooney so in that case, we can leave it as single policy itself instead of granular i suggested earlier | 20:11 |
| gmann | making them granular for flavor and servers response might confuse and make things complex | 20:12 |
| dansmith | gmann: still need a different one for server/flavor but yep | 20:12 |
| dansmith | oh, no we definitely need those two, | 20:12 |
| gmann | dansmith: because of default ? | 20:12 |
| dansmith | because of the scope_types | 20:12 |
| sean-k-mooney | i can see where scope_types come into other issue by why is it relevent here? you want to not check scope types on the server endpoint? | 20:13 |
| gmann | and with flavor one default as system-project-reader and server one as project-reader ? | 20:14 |
| dansmith | flavor one is project,system but the embedded one is just project | 20:14 |
| gmann | embedded in server reposne right? | 20:14 |
| sean-k-mooney | isned the embeded on contole by the server detail policy | 20:14 |
| sean-k-mooney | e.g. its only expose via server detail | 20:15 |
| gmann | sean-k-mooney: no, as separate policy after detail policy | 20:15 |
| sean-k-mooney | not /server/uuid/extra_specs right | 20:15 |
| dansmith | sure, we could rely on that, but all these tests are disabling the parent check to obsess over the child one being right, | 20:15 |
| dansmith | which would imply that they need to be different | 20:15 |
| gmann | yeah, like if operator want to show server detail to their owner but not extra-specs | 20:16 |
| dansmith | the test will thus assert that system:reader can view the embedded one, but it can't actually | 20:16 |
| sean-k-mooney | gmann: right but if they showed the falavor name you could look it up or at least what it is now | 20:16 |
| sean-k-mooney | you could not compare embeeded to currnt but not sure how much that is relevent | 20:17 |
| gmann | as long as we allow project for both flavor or embedded it is ok i think | 20:17 |
| sean-k-mooney | dansmith: i have not looked at the test so ill trust your judgement if you think havign two woudl be useful but im not sure when it would make sense to have tehm set differently | 20:17 |
| gmann | sean-k-mooney: like we will not add system scope for embedded one as system cannot GET /servers/server-id anyways | 20:19 |
| dansmith | getting the two to work is also making me hate life, so maybe a note in the test would be better | 20:19 |
| gmann | so any extra things to show in sevrer reponse has to be moved to project=scoped only as parent policy is project-scoped only | 20:20 |
| dansmith | these tests are hard to debug sometimes, figuring out which context can do a thing that shouldn't be allowed | 20:20 |
| gmann | dansmith: I tried too be more optimize there may be. instead those can be simple/readable with more separate tests for allowed and not-allowed | 20:22 |
| dansmith | gmann: they're very obsessive which is good, they're just hard to debug, but more infra around them can make it easier | 20:24 |
| gmann | sure | 20:25 |
| opendevreview | Alexey Stupnikov proposed openstack/nova master: Test aborting queued live migration https://review.opendev.org/c/openstack/nova/+/776250 | 20:34 |
| opendevreview | Alexey Stupnikov proposed openstack/nova master: Test aborting queued live migration https://review.opendev.org/c/openstack/nova/+/776250 | 20:46 |
| dansmith | gmann: why does this not include other_project_* and legacy_admin ? https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/tests/unit/policies/test_flavor_extra_specs.py#L399 | 20:53 |
| dansmith | gmann: is it because we're not passing a context to the index check so we're not *actually* preventing those users from doing the flavor extra specs index on servers, because the server rule should have stopped them? | 20:54 |
| dansmith | gmann: by the way, this is what I'm trying to get the tests to (only showing the context setup): https://pastebin.com/UKcd62Th | 20:56 |
| dansmith | which I think is a lot easier to read, especially with the comments | 20:56 |
| gmann | dansmith: RE: other_project_* and legacy_admin - yes as server rule will take care of accessing server | 23:10 |
| dansmith | gmann: ack | 23:11 |
| gmann | and in flavor, as there is no project_id there so anyone can access | 23:11 |
| dansmith | gmann: well, right but in the server tests, I think the casual reader (and definitely the policy-editing operator) would expect that to be the server's project_id | 23:12 |
| dansmith | meaning, when accessing the flavor on a server | 23:13 |
| dansmith | i.e. here https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/tests/unit/policies/test_flavor_extra_specs.py#L327 | 23:14 |
| dansmith | so I will put a note in there, but that's probably another reason to have the rules eventually be separate and to pass a target with the proper project_id in it | 23:14 |
| gmann | dansmith: yeah, we can put project_id in current rule too for safer side (if server can be seen by anyone but extra specs not) here - https://github.com/openstack/nova/blob/171138146a648d22474b7021ac730e26f03455f8/nova/api/openstack/compute/views/servers.py#L236 | 23:16 |
| dansmith | yeah | 23:17 |
| dansmith | anyway, note added | 23:17 |
| dansmith | I will try to finish this up tomorrow and get another version posted with more of it cleaned up | 23:17 |
| gmann | dansmith: and on this: https://pastebin.com/UKcd62Th | 23:18 |
| gmann | in ScoptType: self.admin_authorized_contexts need to include project_admin also right? | 23:19 |
| dansmith | gmann: no? system-only for creating flavors right? | 23:20 |
| gmann | or you can name self.system_admin_authorized_contexts and self.project_admin_authorized_contexts | 23:20 |
| dansmith | well, flavor extra_specs anyway | 23:20 |
| gmann | dansmith: ohk, so only for flavor or generically ? | 23:20 |
| dansmith | I guess in the new way, project admin should be able to do that for project-private flavors, since admin==operator | 23:21 |
| dansmith | I didn't change the policy on create/update (yet) so that's why it's sytem-only there | 23:21 |
| gmann | ohk | 23:22 |
| dansmith | I left it that way because I haven't done flavor yet, only flavor extra_specs because the servers change broke those tests, | 23:22 |
| dansmith | so I'm a little out of order, | 23:22 |
| gmann | got it | 23:22 |
| dansmith | so I just didn't tweak that yet | 23:22 |
| gmann | i can take those. this way it will be more clear on who access what. | 23:23 |
| gmann | and I am almost done with listing all rule in wiki table, might be ready by tonight or tomorrow | 23:24 |
| dansmith | okay, I will try to get this cleaned up and updated in gerrit tomorrow so maybe we can use it as a pattern going forward | 23:24 |
| gmann | +1, thanks | 23:24 |
| clarkb | nova should consider removing nova-live-migration-ceph from the gate queue as the job is non voting | 23:41 |
| opendevreview | Merged openstack/nova master: Add autopep8 to tox and pre-commit https://review.opendev.org/c/openstack/nova/+/806182 | 23:54 |
Generated by irclog2html.py 2.17.2 by Marius Gedminas - find it at https://mg.pov.lt/irclog2html/!