| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-06-18 | |||
| 13:37:48 | sean-k-mooney | not today but i can proably look at adding a small script that will check for this in the ci | |
| 13:38:12 | gibi | good idea | |
| 13:38:20 | gibi | on the CI script | |
| 13:38:40 | artom | lyarwood, done | |
| 13:38:41 | sean-k-mooney | get the added filename, strip the rst, grep and see if there is a url that end with that and curl it to make sure it does not have a 404 | |
| 13:43:49 | lyarwood | thanks | |
| 13:45:48 | stephenfin | an interesting Python problem | |
| 13:45:51 | stephenfin | import json | |
| 13:45:51 | stephenfin | class Foo: | |
| 13:45:51 | stephenfin | @property | |
| 13:45:51 | stephenfin | def bar(self): | |
| 13:45:51 | stephenfin | return json.loads(self._bar) | |
| 13:45:53 | stephenfin | @bar.setter | |
| 13:45:54 | stephenfin | def bar(self, value): | |
| 13:45:56 | stephenfin | self._bar = json.dumps(value) | |
| 13:45:58 | stephenfin | foo = Foo() | |
| 13:46:02 | stephenfin | foo.bar = {} | |
| 13:46:04 | stephenfin | foo.bar['test'] = 'hello' | |
| 13:46:06 | stephenfin | assert foo.bar == {'test': 'hello'} | |
| 13:46:08 | stephenfin | ^ that fails | |
| 13:46:31 | sean-k-mooney | yes | |
| 13:46:32 | sean-k-mooney | that is expected | |
| 13:46:35 | sean-k-mooney | but you can fix that | |
| 13:46:37 | stephenfin | I get why (the setter is called for setting the attribute itself, not attributes of the attribute) but I don't know how to fix it | |
| 13:47:02 | sean-k-mooney | foo.bar is returing a copy of the data in self._bar | |
| 13:47:07 | sean-k-mooney | which is a dict | |
| 13:47:25 | sean-k-mooney | acn dyou cant do {} = {"key":"val"} | |
| 13:48:28 | stephenfin | well I need to fix it, because I've got a bug here https://github.com/openstack/nova/blob/master/nova/objects/migrate_data.py#L71-L89 | |
| 13:48:51 | stephenfin | that's the pattern I used there and it doesn't work - the 'OS_VIF_DELEGATION' attribute of the embedded profile is never set :-( | |
| 13:49:24 | sean-k-mooney | i fixed this in one of my pathces | |
| 13:49:46 | sean-k-mooney | well for the fiels i added | |
| 13:51:29 | stephenfin | if you can find that I'd like to have a look at it, because right now I'm stumped | |
| 13:51:57 | sean-k-mooney | i tough tit was that to be honnest | |
| 13:51:57 | jkulik | foo.bar = foo.bar.update({'test': 'hello'}) ? | |
| 13:52:26 | jkulik | too far off from the initial intention? | |
| 13:52:36 | sean-k-mooney | jkulik: well the fix is to inste do the update on the underling dict | |
| 13:53:09 | sean-k-mooney | so yes that works | |
| 13:54:00 | stephenfin | dict.update doesn't return anything, so the idea is sound but it needs a slight tweak | |
| 13:54:34 | sean-k-mooney | right not update but you read modify write | |
| 13:54:42 | gibi | yepp the problem is that you have a getter and a setter but foo.bar['test'] only use the getter but not the setter :) | |
| 13:54:50 | stephenfin | foo.bar = dict(foo.bar, **{'test': 'hello'}) | |
| 13:55:04 | stephenfin | will do the trick. Still not as pretty but at least it works | |
| 13:55:16 | sean-k-mooney | https://github.com/openstack/nova/blob/master/nova/objects/migrate_data.py#L81-L89 should work though | |
| 13:55:28 | stephenfin | gibi: Yeah. Slightly amazed that I haven't (that I can recall) hit this in all my years writing Python :D | |
| 13:55:46 | sean-k-mooney | i hit a proably with it and change it | |
| 13:55:52 | stephenfin | sean-k-mooney: the getter works, the setter does not | |
| 13:55:56 | gibi | I think we need get rid of the foo.bar['aaa'] by returning a frozen dict | |
| 13:55:56 | stephenfin | at least not in tests | |
| 13:56:28 | sean-k-mooney | stephenfin: its suoudl this was one of the things i had to workaround when writhing tha tpatch | |
| 13:56:31 | gibi | as the getter returns a copy of the underlining data not a reference for it | |
| 13:56:48 | gibi | so we have to tell the caller that it is copy | |
| 13:56:52 | gibi | not a live ref | |
| 13:56:55 | jkulik | frozen dict sounds good. then nobody can accidentally use the non-working pattern | |
| 13:57:49 | sean-k-mooney | frozen dict wont work | |
| 13:58:03 | sean-k-mooney | the issue is we are storing a json string | |
| 13:58:19 | gibi | sean-k-mooney: it does not solve the problem, it forces the caller to avoid modifying what is returned | |
| 13:58:23 | stephenfin | well it would indicate that you can't update it | |
| 13:58:25 | sean-k-mooney | and then we are returning it as a dict and we want update to that property to modify the json | |
| 13:58:37 | sean-k-mooney | gibi: right but we want them to be able to do that | |
| 13:58:38 | stephenfin | however, there's no such thing in stdlib | |
| 13:58:43 | gibi | sean-k-mooney: I don't :) | |
| 13:58:56 | sean-k-mooney | well for this code we are talkign too we do | |
| 13:59:10 | sean-k-mooney | other wise we shoudl jsut remove the property | |
| 13:59:21 | sean-k-mooney | and force use to work direclty on the json | |
| 13:59:43 | opendevreview | Merged openstack/nova-specs master: Fix the bp link in the cyborg admin token spec https://review.opendev.org/c/openstack/nova-specs/+/795493 | |
| 13:59:45 | sean-k-mooney | the only reason the properties exist is to to give a dict like interface to the json blobs | |
| 14:00:00 | sean-k-mooney | stephenfin: what test is failing | |
| 14:00:08 | gibi | sean-k-mooney: then we need to make it one level deper allowing to set per key | |
| 14:00:24 | gibi | like foo.bar.test = 'aaa' | |
| 14:00:25 | stephenfin | none currently, because we don't have tests covering this code path | |
| 14:00:46 | gibi | sean-k-mooney: but that needs more work on the implementation side | |
| 14:01:16 | sean-k-mooney | stephenfin: the quick fix is as follows | |
| 14:01:18 | sean-k-mooney | self.profile[OS_VIF_DELEGATION] = supported | |
| 14:01:24 | sean-k-mooney | becomes | |
| 14:01:38 | kashyap | Am I hallucinating, or were the check marks of x, ✔, and ? used to be in colour on the support-matrix page? - https://docs.openstack.org/nova/wallaby/user/support-matrix.html | |
| 14:02:09 | sean-k-mooney | data = jsonutils.loads(self.profile_json); data[OS_VIF_DELEGATION]=supported; self.profile_json = jsonutils.dumps(data); | |
| 14:02:27 | sean-k-mooney | stephenfin: here https://github.com/openstack/nova/blob/master/nova/objects/migrate_data.py#L89 | |
| 14:03:01 | sean-k-mooney | kashyap: they were yes | |
| 15:14:23 | gibi | stephenfin: could you please hit https://review.opendev.org/c/openstack/osc-placement/+/794276 when you have time | |
| 15:54:47 | gmann | stephenfin: lyarwood if that broken on fedora having py3.9 ? but we do have py3.9 job running successfully though those are n-v | |
| 15:56:32 | gmann | is that | |
| 16:22:21 | opendevreview | Merged openstack/nova master: Handle OPERATION_FAILED error during detach https://review.opendev.org/c/openstack/nova/+/796255 | |
| 17:15:25 | stephenfin | lyarwood: Good thing you asked for that test. This code is doing nothing currently 😇 | |
| 17:15:54 | opendevreview | Stephen Finucane proposed openstack/nova master: objects: Fix VIFMigrateData.supports_os_vif_delegation setter https://review.opendev.org/c/openstack/nova/+/797142 | |
| 17:15:59 | stephenfin | sean-k-mooney: ^ | |
| 17:16:24 | stephenfin | I haven't run those tests locally. I want to push it to the gate and see if the interfaces are correctly created in the Tempest job | |
| 17:16:53 | stephenfin | gmann: The main issue I was seeing is that the deps in lower-constraints don't work with Python 3.9 | |
| 17:17:20 | stephenfin | gmann: However, in the past the functional tests didn't work with Python 3.9. It could be possible that things have been fixed since | |
| 17:18:05 | stephenfin | gmann: However, regardless, there's no reason we should be running different things locally and in the CI. Something could conceivably pass locally (where we're using Python 3.9) but fail in the gate (using Python 3.8) | |
| 17:18:36 | stephenfin | gibi: done | |
| 17:18:40 | gmann | stephenfin: yeah, l-c can be dropped :) which i am not much worried about. | |
| 17:19:36 | gmann | stephenfin: cases like passing py3.9 and failing py3.8 should not be much right as at next cycle we want all code to run on both | |
| 17:20:12 | gmann | stephenfin: i am not against of that change to test py3.8 as default locally but it just add extra work you mentioned in commit msg | |
| 17:20:18 | stephenfin | Sure, but what about when Fedora introduces Python 3.10 | |
| 17:20:41 | stephenfin | Fedora is bleeding edge, and there will always be a delay between when Fedora introduces a Python version and when nova supports it | |
| 17:21:07 | stephenfin | we already have to update setup.cfg to state our supported versions so this is minimal extra work IMO | |
| 17:21:42 | gmann | yeah that is automated in release script i think and may we can add tox basepython update also.. | |
| 17:22:02 | stephenfin | that would be helpful | |
| 17:25:16 | gmann | stephenfin: we can merge that I am not -1 on that. I will see if we can automate in release script sometime later | |
| 17:27:51 | opendevreview | Stephen Finucane proposed openstack/nova stable/wallaby: libvirt: Delegate OVS plug to os-vif https://review.opendev.org/c/openstack/nova/+/790447 | |