| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-24 | |||
| 14:32:35 | dansmith | I understand the desire for consistency, | |
| 14:32:45 | dansmith | but it's pretty late to be changing that, and it's been that way since forever | |
| 14:32:58 | dansmith | so I'm pretty meh about it, as you might expect :) | |
| 14:33:30 | bauzas | yeah :( | |
| 14:34:07 | bauzas | I honestly haven't thought about changing this before | |
| 14:34:20 | bauzas | and the fact I was redefining what would be the compute RPC | |
| 14:34:34 | bauzas | I just looked at what was changed | |
| 14:35:05 | bauzas | and now, looks like it's difficult for me to do it as we would need to change a lot of stuff here | |
| 14:35:11 | dansmith | the client can look at the version to determine which param to send, and the v5 proxy can just convert the old to the new | |
| 14:35:31 | dansmith | I don't really know how much test damage there will be, since those usually dispatch positionally | |
| 14:35:47 | dansmith | but I just think it's pretty late to jam that in here | |
| 14:40:57 | bauzas | dansmith: surely, I'm not saying this is impossible | |
| 14:41:06 | bauzas | dansmith: just the fact that the ship mostly sailed | |
| 14:41:31 | bauzas | and yes, this would mean the client would have to pass different arguments based on the version it discovers | |
| 14:41:41 | bauzas | something we usually do | |
| 14:41:49 | bauzas | but something we need to amend tests as well | |
| 14:41:55 | dansmith | yeah | |
| 14:42:32 | bauzas | dansmith: we could tho deprecate instance_type in a 6.1 and pass a new arg named flavor, right? | |
| 14:42:57 | bauzas | so, later with 7.0, we would just get rid of the deprecated instance_type | |
| 14:43:00 | sean-k-mooney | given this is internal an we all know that instance_type is the flavor | |
| 14:43:06 | sean-k-mooney | im not sure there is much gain form that | |
| 14:43:13 | dansmith | bauzas: that's a bad idea, IMHO | |
| 14:43:14 | sean-k-mooney | but we could i guess | |
| 14:43:25 | dansmith | sean-k-mooney: I agree, this is not a huge deal | |
| 14:43:39 | bauzas | dansmith: well, that's what we do when we want to rename o.vo fields, nope ? | |
| 14:43:54 | dansmith | sean-k-mooney: we could even docstring it to explain, or just set flavor=instance_type as the first line of the method :) | |
| 14:43:54 | bauzas | deprecating the old field and adding a new one | |
| 14:43:59 | sean-k-mooney | dansmith: yep | |
| 14:44:08 | bauzas | dansmith: that's the easy part | |
| 14:44:14 | sean-k-mooney | bauzas: am not quite | |
| 14:44:18 | sean-k-mooney | we do add a new filed | |
| 14:44:22 | dansmith | bauzas: yeah, but you just have to handle both until the next major version.. we usually do that for a _format_ change not just a name change | |
| 14:44:24 | sean-k-mooney | but we transrom form one to the other | |
| 14:44:28 | sean-k-mooney | we dont really deprecate | |
| 14:44:59 | bauzas | dansmith: well, you're right, sounds a bit nitty to do this for just a param change | |
| 14:45:01 | sean-k-mooney | we add support for converting in make compatible so we normally wont set both for example | |
| 14:45:26 | bauzas | but yeah, in Xena we could s/instance_type/flavor in the inner of the method and just leave the wire unchanged | |
| 14:46:02 | bauzas | stephenfin: does this sound acceptable to you ? | |
| 14:56:25 | tosky | melwitt: hi, do you remember if this workaround can be removed, or at least restricted to queens and rocky (or just queens)? https://opendev.org/openstack/devstack-plugin-ceph/src/branch/master/devstack/plugin.sh#L33 | |
| 15:04:17 | melwitt | tosky: hm, originally that was only for queens but I see this commit added the master branch https://opendev.org/openstack/devstack-plugin-ceph/commit/39de6df04130cf2f221fb5ba2a9b5ff597de332a so I think you need to ask frickler whether we still need master branch | |
| 15:08:50 | melwitt | gibi: thanks for finding and fixing the wsgi app unit test bug! | |
| 15:23:06 | stephenfin | bauzas: dansmith: Yeah, that's fine if it's way too much work. With that said, I'm surprised that it _is_ a lot of work. I expected it would just be a shim, and iirc there are only a handful of places where we explicitly set the 'instance_type' kwarg directly in tests | |
| 15:23:35 | stephenfin | I also have a patch ready to replace instance_type everywhere else, if that was a blocker, though making it a dependency might be too much churn | |
| 15:23:49 | dansmith | stephenfin: I'm not sure it is a lot of work, as noted above, but I think bauzas was saying that he tried and a bunch of tests fail | |
| 15:24:33 | stephenfin | It's really not a big deal, I agree, but I think it's so trivial and we won't have a chance to fix it until 7.0, which it's reasonable to suggest won't ever happen :) | |
| 15:24:48 | stephenfin | Ah, that would be fair | |
| 15:25:46 | dansmith | stephenfin: it's just not a big enough deal to justify the uncertainty at this point to me | |
| 15:26:01 | gibi | melwitt: I saw it failing today and remembered that there was a recent change in the wsgi_app | |
| 15:26:08 | dansmith | it's just internal developer-only cleanliness and just isn't that important, I think is the assertion the three of us are making above | |
| 15:26:21 | melwitt | gibi: u rock :) | |
| 15:26:26 | gibi | :) | |
| 15:26:32 | dansmith | but it's bauzas' baby, so whatever he wants to do, IMHO | |
| 15:29:43 | bauzas | stephenfin: well, again, I'm not saying it's impossible to do | |
| 15:30:15 | bauzas | stephenfin: but in order to change the param name, we need to also modify the RPC client to provide the right name between the versions | |
| 15:30:25 | bauzas | and if so, we need to write tests | |
| 15:30:42 | bauzas | (and change some tests which would pass the original name directly) | |
| 15:30:58 | stephenfin | If it helps, I count 7 existing tests we'd need to change | |
| 15:31:12 | bauzas | stephenfin: my only concern is the time | |
| 15:31:15 | dansmith | bauzas: yeah, also the new tests, for sure | |
| 15:31:17 | bauzas | RC1 is tomorrow | |
| 15:31:37 | bauzas | and we can't merge later if we want this for Wallaby | |
| 15:33:02 | stephenfin | ack, okay that is super close | |
| 15:33:15 | tosky | melwitt: thanks, will do; I suspect master is not affected (we switched to focal since then, and probably ceph version), but let's see | |
| 15:33:46 | melwitt | ack, sounds cool | |
| 15:36:37 | bauzas | stephenfin: but again, your concern is valid | |
| 15:36:53 | bauzas | what we can do for Xena is at least to change the var name within the methods | |
| 15:37:06 | bauzas | and just keep this ugly param name as it is | |
| 15:37:15 | bauzas | until we cut a new RPC version | |
| 15:37:45 | bauzas | but writing a good fat comment above the method explaining that 7.0 should change this param would also help to not miss the target again | |
| 15:46:30 | gibi | lyarwood: I have a question/suggestion in https://review.opendev.org/c/openstack/nova/+/781138/4/nova/compute/api.py#5495 | |
| 15:54:47 | openstackgerrit | Ghanshyam proposed openstack/nova master: DNM: testing tempest https://review.opendev.org/c/openstack/nova/+/782798 | |
| 15:58:41 | openstackgerrit | Stephen Finucane proposed openstack/nova master: DNM: Rename 'instance_type' parameter https://review.opendev.org/c/openstack/nova/+/782804 | |
| 15:59:08 | stephenfin | bauzas: ^ I can change my vote if you _really_ don't want to do this, but fwict that's all we need ^ | |
| 16:00:02 | stephenfin | I couldn't find an example of a good test to make sure the calls are translated. I could do a simple mock of the v6 method and use 'assert_called_with', but we don't use named kwargs in the v5 proxy so there isn't really anything to check | |
| 16:00:31 | bauzas | stephenfin: first thing, you'd need to rebase your change on top of my RPC bump so we would also need to modify the 5.0 proxy to pass the right arg name | |
| 16:00:40 | stephenfin | did I not do that? | |
| 16:01:01 | stephenfin | yeah, it's on top of your change already | |
| 16:01:06 | bauzas | oh | |
| 16:01:12 | bauzas | sorry, missed this | |
| 16:01:13 | bauzas | but, | |
| 16:01:30 | bauzas | we need to pass the right arg within the 5.0 proxy | |
| 16:01:49 | stephenfin | see above - you're doing that already | |
| 16:02:10 | dansmith | the client needs to pass a different arg for 5.x | |
| 16:02:21 | bauzas | like https://review.opendev.org/c/openstack/nova/+/782804/1/nova/compute/manager.py#10706 | |
| 16:02:22 | dansmith | and we need a test for both modes | |
| 16:02:33 | stephenfin | aha, gotcha. Sec | |
| 16:02:42 | bauzas | stephenfin: we call the 6.0 method within the 5.0 proxy one | |
| 16:02:50 | lyarwood | gibi: yup good point, it's always provided by c-vol but yeah it isn't required in the api so .get() is safer | |
| 16:02:55 | bauzas | so, instance_type should be transformed into flavor | |
| 16:03:00 | bauzas | within the proxty | |
| 16:03:07 | gibi | lyarwood: if it is always provided then we should document it in our API ref | |
| 16:03:19 | bauzas | stephenfin: and then we would need to test the API versioning | |
| 16:03:21 | dansmith | bauzas: and the client needs the same, based on the version pin | |
| 16:03:24 | bauzas | yup | |
| 16:03:51 | bauzas | https://review.opendev.org/c/openstack/nova/+/782804/1/nova/compute/rpcapi.py#963 | |
| 16:04:15 | bauzas | we should say s/t like "if not can_send(6.0): instance_type = flavor | |
| 16:04:25 | dansmith | no, you can't do that, | |
| 16:04:28 | bauzas | and then pass instance_type instead of flavor | |
| 16:04:31 | dansmith | because it'll break if we introduce a 6.1 | |
| 16:04:38 | dansmith | it needs to check the first digit I think | |