| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2021-03-24 | |||
| 11:27:36 | gibi | I just wanted to note that the anchor generation works differently if the label starts with a number | |
| 11:27:52 | gibi | so we should prefer labels starting with a nonnumber for stable achors | |
| 11:28:02 | gibi | but this one will work for the case you added it | |
| 11:28:10 | gibi | as the anchor and the ref is in the same doc | |
| 11:28:17 | gibi | and sphinx magic helps | |
| 11:28:43 | lyarwood | it's in a different doc? | |
| 11:31:08 | gibi | it is generated by the same single sphinx run so the anchor is resolved properly | |
| 11:31:44 | gibi | but externally (ie, from the python-novaclient docs) you would need to refer to the numeric id of the ancor 'id80' but that is not stable | |
| 11:32:15 | gibi | sphinx generates proper labelled achnors if the label starts with a nonnumber | |
| 11:32:21 | lyarwood | kk | |
| 11:33:49 | openstackgerrit | Lee Yarwood proposed openstack/nova master: doc: Use a non-numerical anchor when referencing microversions https://review.opendev.org/c/openstack/nova/+/782696 | |
| 12:10:34 | openstackgerrit | Rico Lin proposed openstack/nova master: [Arm64][libvirt] Fix read bytes from json files https://review.opendev.org/c/openstack/nova/+/782634 | |
| 12:13:10 | hrw | can someone point me to doc about how nova/libvirt create cgroupv2 dirs? | |
| 12:26:55 | openstackgerrit | Lee Yarwood proposed openstack/nova master: doc: Use a non-numerical anchor when referencing microversions https://review.opendev.org/c/openstack/nova/+/782696 | |
| 12:35:14 | lyarwood | https://review.opendev.org/q/topic:bug/1917619 & https://review.opendev.org/q/topic:bug/1919487 could use reviews if anyone has bandwidth this week btw | |
| 12:36:29 | lyarwood | gibi / stephenfin ; https://review.opendev.org/c/openstack/nova/+/768466 also this that gets us closer to finally closing out our migration to native zuulv3 jobs | |
| 12:36:44 | lyarwood | it has taken ages to land all the required stuff in devstack and the ceph plugin | |
| 12:37:13 | stephenfin | lyarwood: ack | |
| 12:37:42 | stephenfin | bauzas: Looking at the RPC change now. Can we rename parameters in this change? | |
| 12:51:01 | openstackgerrit | Stephen Finucane proposed openstack/nova master: docs: Clarify value for 'hw:cpu_realtime' extra spec https://review.opendev.org/c/openstack/nova/+/782708 | |
| 13:23:28 | openstackgerrit | Merged openstack/nova master: docs: Add version{added,changed} notes to the hw_machine_type guide https://review.opendev.org/c/openstack/nova/+/779446 | |
| 13:24:31 | openstackgerrit | Merged openstack/nova master: docs: Add note about rescuing bfv instances with the 2.87 microversion https://review.opendev.org/c/openstack/nova/+/779479 | |
| 13:26:07 | openstackgerrit | Merged openstack/nova master: Reset global wsgi app state in unit test https://review.opendev.org/c/openstack/nova/+/782691 | |
| 13:27:12 | openstackgerrit | Merged openstack/nova master: docs: Add a resource limits guide https://review.opendev.org/c/openstack/nova/+/781433 | |
| 14:18:19 | bauzas | stephenfin: whoops, just saw your question | |
| 14:19:16 | bauzas | stephenfin: please, don't ask to change the instance_type for my change | |
| 14:19:43 | bauzas | I know about this fact, but changing the parameter for this change is not related to the needed behaviour ? | |
| 14:19:45 | bauzas | right? | |
| 14:30:56 | bauzas | stephenfin: actually, I do understand your concern | |
| 14:31:17 | bauzas | problem is, changing this param name would also need to modify the rpcapi module | |
| 14:31:25 | bauzas | and then the tests... | |
| 14:32:09 | bauzas | something saying : if ver==6.0: <use flavor for the param name>; <use instance_type> | |
| 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 | bauzas | deprecating the old field and adding a new one | |
| 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: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 | |