| Posted | Nick | Remark | |
|---|---|---|---|
| #openstack-nova - 2020-04-02 | |||
| 15:59:27 | dansmith | gmann: I'm changing how those checks work, which might mean not | |
| 15:59:47 | dansmith | device_name appears to be nullable at least, in the schema and in the db | |
| 16:00:02 | dansmith | the others are all separate definitions I think | |
| 16:01:06 | gmann | yeah, other does not allow None from schema only device do. | |
| 16:01:53 | openstackgerrit | Huaqiang Wang proposed openstack/nova master: Introduce 'pcpuset' field for InstanceNUMACell https://review.opendev.org/716984 | |
| 16:02:45 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Fix assertEqual param order in Accelerator tests https://review.opendev.org/717069 | |
| 16:02:46 | openstackgerrit | Balazs Gibizer proposed openstack/nova master: Stabilize functional tests https://review.opendev.org/717070 | |
| 16:04:18 | openstackgerrit | Dan Smith proposed openstack/nova master: Allow PUT volume attachments API to modify delete_on_termination https://review.opendev.org/693828 | |
| 16:04:52 | dansmith | gmann: ^ | |
| 16:05:08 | gmann | thanks checking | |
| 16:08:00 | tosky | for stable maintainers: this last review should remove any reference to legacy-tempest-dsvm-full-devstack-plugin-nfs (then we are doing the same in Cinder): https://review.opendev.org/#/c/714958/ | |
| 16:08:14 | gmann | dansmith: do not we nee do check dot with old value vefore change - https://review.opendev.org/#/c/693828/25..26/nova/api/openstack/compute/volumes.py@460 | |
| 16:08:17 | gmann | before | |
| 16:08:54 | dansmith | gmann: why? we sad that if you PUT with no changes it's not an error right? | |
| 16:09:37 | dansmith | technically BDM should not be doing a db save if there are no changes, but not all objects do that properly.. but either way, no reason to just add more to the conditional really | |
| 16:10:12 | gmann | dansmith: yeah but when we add policy in next patch that become difficult to handle on "update request" vs "no change means not update request" | |
| 16:10:29 | dansmith | gmann: I don't follow | |
| 16:11:08 | gmann | in case of "swap + update" - we consider request is for update only when value is changed right. if value is same then we need to consider no update request and it is onyl swap | |
| 16:11:45 | gmann | in that case, how we can check the update policy so that that policy is being checked only when request is update (means value change) | |
| 16:12:02 | dansmith | in 2.85 we should only consider it a swap if volumeId is changed | |
| 16:12:17 | gmann | but we support swap + update together also | |
| 16:12:23 | dansmith | i.e. only check the stricter policy if volumeId is changed, otherwise we check the regular PUT policy | |
| 16:12:24 | gmann | >2.85 | |
| 16:12:55 | dansmith | the swap policy should imply you can also change the other values right? | |
| 16:12:58 | gmann | yeah and checking regular PUT policy when value is same is wrong. | |
| 16:13:11 | dansmith | I disagre | |
| 16:13:32 | gmann | we want to allow update for non-admin | |
| 16:13:36 | dansmith | if volumeId is changed, we check the swap policy, else check PUT policy | |
| 16:13:59 | gmann | let me rephrase | |
| 16:14:23 | dansmith | in this patch it's all one policy, and the next patch splits them into two | |
| 16:16:38 | gmann | ok, we can discuss in next patch but there question was - we should not check the PUT policy if request is swap only not update. | |
| 16:16:56 | openstackgerrit | Huaqiang Wang proposed openstack/nova master: Introduce 'pcpuset' field for InstanceNUMACell https://review.opendev.org/714658 | |
| 16:17:08 | gmann | knowing it is 'not update' is only done by if value is changed | |
| 16:17:25 | dansmith | gmann: and like I said, I think >=2.85, we should only ever consider something a swap if the volumeId changed, otherwise it's a put, even if *nothing* changed | |
| 16:18:29 | gmann | dansmith: that is question actually. if nothing changes should we consider that as update request ? especially when we allow people to replicate the PUT and change only volumeId fir swap | |
| 16:18:51 | gmann | if do so then we endup checking two (PUT and SWAP) policy for swap | |
| 16:19:27 | dansmith | I strongly feel that going forward this should behave like a regular PUT, where you can PUT with no changes all the time, | |
| 16:19:38 | dansmith | and only check the swap policy if you're actually changing the volume | |
| 16:19:45 | dansmith | I don't see how anything else makes sense | |
| 16:21:44 | gmann | dansmith: yeah +1 on 'only check the swap policy if you're actually changing the volume'. but this became hard to implement with no value check of dot. | |
| 16:22:00 | gmann | i think if you rebase the policy patch then it will be clear. | |
| 16:22:28 | dansmith | okay I don't see what the value of dot has to do with which policy gets checked | |
| 16:22:38 | gmann | policy stuff is a challenge here | |
| 16:22:50 | bauzas | gibi: if you don' mind, I would appreciate https://blueprints.launchpad.net/nova/+spec/vgpu-multiple-types to be targeted for ussuri-3 | |
| 16:23:08 | dansmith | gmann: do you not agree that if you have swap permissions, you should also be able to change dot? | |
| 16:23:09 | bauzas | the patches in question are small | |
| 16:23:36 | gmann | dansmith: that is fine but vice versa should not | |
| 16:23:42 | gmann | dansmith: here - https://review.opendev.org/#/c/711194/11..13/nova/api/openstack/compute/volumes.py@468 | |
| 16:23:46 | gibi | bauzas: noted, I will update lauchpad after the meeting | |
| 16:23:56 | gmann | otherwise we end up doing PUT policy check always | |
| 16:24:21 | bauzas | gibi: thanks | |
| 16:24:22 | dansmith | gmann: right but as stephen (and now I) say there, that's the wrong place to check policy | |
| 16:24:58 | dansmith | gmann: we should check at the beginning of update(), either the PUT or PUT-with-swap policy based on whether or not volumeId is changed.. it should be very simple if..else | |
| 16:25:41 | dansmith | gmann: anyway, once the base patch looks like it's +2 I will rebase the policy patch (or help brinzhang_ ) and we can discuss/argue the policy bit on that | |
| 16:27:03 | gmann | dansmith: ok. after rebase it will be more clear. But i think i got your point of swap policy supersede the PUT policy | |
| 16:27:18 | openstackgerrit | Dan Smith proposed openstack/nova master: Allow PUT volume attachments API to modify delete_on_termination https://review.opendev.org/693828 | |
| 16:27:23 | dansmith | gmann: ack | |
| 16:27:30 | dansmith | gmann: ^ that removes one more thing alex asked about that I forgot | |
| 16:27:41 | gmann | ok | |
| 16:28:13 | dansmith | gmann: sorry if it got lost in the policy discussion, but... is there still a need to change the schema for null-ness? | |
| 16:29:41 | gmann | with 'x in attr' check, it is not needed | |
| 16:29:54 | dansmith | gmann: okay cool | |
| 16:31:09 | stephenfin | dansmith: off the top of your head, obj_from_primitive is called whenever we pull an o.vo from the DB, yeah? | |
| 16:31:31 | dansmith | stephenfin: for serialized objects | |
| 16:36:21 | bauzas | artom: FWIW, I left the bare minimum in the etherpad for your OSC concerns, the floor is yours :p | |
| 16:36:28 | bauzas | artom: https://etherpad.openstack.org/p/nova-victoria-ptg | |
| 16:36:31 | artom | bauzas, OK | |
| 16:36:44 | artom | bauzas, I'll proll'y just be lazy and harass mordred | |
| 16:37:01 | bauzas | I think the problem isn't the design as I said | |
| 16:37:06 | artom | About where any new microversion-enabling client code should go | |
| 16:37:10 | bauzas | we discussed it so many times | |
| 16:37:17 | bauzas | I think we just need hands | |
| 16:37:32 | artom | bauzas, well that's the point, right? We already say "if you're adding a new microversion, you must provide the novaclient patch" | |
| 16:37:41 | artom | Not we'll be saying "and you also need to provide the osdk patch" | |
| 16:37:45 | bauzas | artom: it's more complicated than that | |
| 16:37:45 | artom | If osdk is indeed the correct place | |
| 16:37:56 | bauzas | artom: if you're about to propose, say 2.81 | |
| 16:38:10 | bauzas | artom: then you also need to make sure that <2.81 works too | |
| 16:38:29 | bauzas | and then you signed-off for doing all the stuff, yay | |
| 16:38:43 | artom | bauzas, oh, because of the gap | |
| 16:38:45 | artom | *sigh* | |
| 16:38:52 | artom | Yeah ok, I get it | |
| 16:38:54 | mordred | well - once we reach steady-state making sure <2.81 works too should be pretty easy | |
| 16:39:30 | artom | mordred, what does steady-state mean in this context? | |
| 16:39:31 | mordred | that said - if we can just start getting folks to submit an osdk patch wehn new microversions are made so we can start tracking new things, we can use those as opportunities to fill in old stuff - and I'm happy to help do that | |
| 16:39:48 | artom | mordred, I think I'd like that - we have to start somewhere | |
| 16:39:50 | mordred | artom: I mean havig filled in the support for the existing old microversions | |
| 16:40:04 | artom | mordred, ah, that was the thing that mriedem started | |
| 16:40:09 | mordred | most of the time when someone says "I want to support microversion vX" - it turns out ot be quite easy | |
| 16:40:10 | artom | Well, started driving | |
| 16:40:12 | mordred | yeah | |
| 16:40:24 | mordred | but - there's a bunch we need to walk through and make sure we understand :) | |
| 16:40:34 | artom | mordred, in terms of how the Nova API works? | |
| 16:40:49 | mordred | yeah- many of them wind up being no-ops from sdk point of view | |
| 16:41:00 | mordred | but it's good to go look at the MV and say "yup, nothing to do | |
| 16:41:20 | artom | mordred, ah, for example for things like new fields in responses? sdk just shows them with no added logic | |
| 16:41:25 | artom | (Right?) | |
| 16:41:26 | gmann | yeah, like adding the field in response is same list | |
| 16:42:04 | mordred | yeah, well, we need to add new fields when they're added | |
| 16:42:27 | mordred | but - we already are up to date with most fields as of like the mid-2.4x or 2.5x - just not labeled that way | |