Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-01-09
15:57:00 openstack bug 1852610 in OpenStack Compute (nova) rocky "API allows source compute service/node deletion while instances are pending a resize confirm/revert" [Medium,In progress] https://launchpad.net/bugs/1852610 - Assigned to Matt Riedemann (mriedem)
15:57:00 openstackgerrit Merged openstack/nova stable/rocky: Add functional recreate test for bug 1852610 https://review.opendev.org/698108
15:57:05 openstackgerrit Merged openstack/nova stable/rocky: Add functional recreate revert resize test for bug 1852610 https://review.opendev.org/698110
15:57:34 dansmith sean-k-mooney: libvirt handles it in both cases eventually, but that's my point about all of this
15:57:55 dansmith sean-k-mooney: we should have the user ask for what they want, not what they want how they think it's plumbed today
15:58:07 dansmith because if we move vgpu handling completely to cyborg in the future,
15:58:22 dansmith the syntax shouldn't have to change, the users shouldn't have to know how the cloud is doing that behind the scenes, etc
15:58:32 dansmith we're supposed to be an abstraction, not just an API
15:58:53 sean-k-mooney yes i agree with that in principal. and yes libvirt will provide it in either case where it is invetoried by the livbrit driver or cyborg
15:59:48 efried Currently we have ways to use the placement syntax mostly-not-awkwardly, which is where we're conflicting.
15:59:48 efried So when it comes to choosing between using placement syntax in an awkward way, or inventing a new fit-for-purpose custom syntax, I vote for the latter.
15:59:48 efried So we're going to need the snowflake syntax no matter what.
15:59:48 efried IF we could reasonably use placement syntax for everything, that would be okay, but that is not going to be possible.
15:59:48 efried Yes, we had to invent syntax for cyborg, and we'll have to extend existing syntax for numa. IMO that's not a bad thing.
15:59:50 sean-k-mooney part of the proably however was if i wanted 2 fpga with differet bit stream i might need to select different devices to be compatible
15:59:59 efried I feel like we're repeating at this point.
16:00:05 efried I need to run to a meeting. BBIAB
16:00:55 sean-k-mooney that 2 devices with different requirements lead to the need for use to model each request as a seperate request group with different traits
16:00:56 dansmith sean-k-mooney: right, today that will add a third permutation of the cyborg syntax to my point above
16:01:18 sean-k-mooney so it would force the grouping syntax if we jsut use the placeemtn stuff directly
16:01:39 sean-k-mooney well not on the nova side it woudl jsut eb 2 device profiles
16:01:48 sean-k-mooney but yes i could
16:02:03 dansmith sean-k-mooney: I'm saying today there is only one flavor key which is "_the_ device profile" in cyborg
16:02:16 sean-k-mooney yes
16:02:22 dansmith if you need to add more in the future, like we will for hot-attach, then we'll be adding something _else_
16:02:34 sean-k-mooney ya
16:04:15 sean-k-mooney i know its not a sexy thing but i kind of do feel like maybe next cycle it would be good to try and rationalise this and write down how we envision this evolveing
16:05:14 sean-k-mooney e.g. taking all the feature we have like this and trying to express them in with placement for resouces and hw:* for tweeks ectra and see what that looks like
16:06:13 sean-k-mooney at lest when it comes to numa i think we need to do that for cpus and memory before doing numa in placmenet
16:08:36 dansmith there goes sean-k-mooney promising to do stuff "next cycle" :)
16:08:50 dansmith but yes, I thought we were a lot more unified on the plan obviously
16:09:06 dansmith and after this conversation, it feels more like "eff it, just invent new stuff each time"
16:13:29 openstackgerrit Victor Coutellier proposed openstack/nova-specs master: Non-admin user can filter their instances by AZs https://review.opendev.org/701763
16:13:59 alistarle Hi, I was waiting for the #open-discussion part of the meeting but seems there was not today
16:14:09 alistarle I have reported a blueprint about the ability for non admin to filter server by availability zone, I was very surprised that it is an admin only one : https://blueprints.launchpad.net/nova/+spec/non-admin-filter-instance-by-az
16:15:10 alistarle @efried review my code and suggest me to talk about it in the meeting today, in case there will need a spec for that, but waiting for the open discussion I have written the spec :)
16:18:00 dansmith alistarle: if it's an api change, then it needs to be a spec
16:18:46 alistarle It is not really an api change, as everything are already available, it is just about allowing non-admin user to do something restricted to admin
16:18:48 dansmith alistarle: az is a thing that is exposed to users in general, so filtering on it is probably okay, but some thought will need to be given because of things like cells
16:19:33 dansmith oh I see
16:19:33 alistarle But thanks, at least I have not wrote the specs for nothing ;)
16:20:46 dansmith I'm still not sure it doesn't require a microversion bump, fwiw
16:20:49 alistarle That's why in the code, @efried was not sure about requiring a new microversion or not, as it does not really change something in the API
16:21:24 dansmith gmann: ^
16:22:07 alistarle I agree with you, in my opinion it is not required
16:22:13 melwitt hm, I'd have thought that would just be to add a new policy item, not a need a new microversion
16:22:28 dansmith melwitt: well, looking at the code around the change, I dunno
16:22:45 alistarle There is already a policy item available, as explained in the alternative in the specs, for it does not really fit the needs
16:22:48 dansmith the way non-admin search params are handled, everything is very specific
16:23:21 alistarle Or it fit too much the need I would say ^^
16:23:21 dansmith melwitt: also, unless the user gets a 401 when they try to use it now, a microversion is the only signaling method that they can expect it to work
16:24:37 dansmith alistarle: your assertion that policy changes during upgrade are hard is no longer valid, I think,
16:24:47 dansmith since the policy file is just what you want to change, not the full list
16:24:53 dansmith and if it is, your vendor is doing it wrong
16:25:15 alistarle Hmm ok fine, but what about giving too much right to customer ?
16:25:48 dansmith I'm not sure what that means
16:26:09 melwitt oh, hm. when os_compute_api:servers:allow_all_filters was added it wasn't a new microversion https://github.com/openstack/nova/commit/7c56588647be64a2248b1f37d40369765bc6b977 and I thought you couldn't otherwise know to expect it to work
16:26:09 alistarle They can filter on everything then, not only AZ
16:26:30 alistarle And I really think some filter must stay admin only by default
16:27:30 dansmith melwitt: hmm, is that a boolean just "you can use all filters or not" ?
16:28:02 melwitt so based on that, I thought adding something like os_compute_api:servers:allow_az_filter and default it to admin-only would be in the same vein
16:28:17 dansmith melwitt: the reason I'm not sure is because of all of these filters are added specifically by microversion: https://review.opendev.org/#/c/701609/3/nova/api/openstack/compute/servers.py
16:28:36 dansmith melwitt: that's not what is being proposed, fwiw
16:28:40 alistarle Hmm yes, but why not just allowing user to do it by default ? As AZ are a public exposed thing
16:28:45 melwitt dansmith: yeah it's boolean. looking at your link now
16:29:13 dansmith melwitt: ack, okay so that's not precise enough to just add the az filter for people, but I get your point on the microversion thing
16:29:20 melwitt dansmith: ohhhh, I see now
16:29:54 dansmith melwitt: enabling that would appear to enable a bunch of new filters for non-admins without a microversion, but those are still covered by the microversion in which they were addd
16:30:03 sean-k-mooney dansmith: :) on internal call but i would like to look at this before next cycle but want to look at porting the legacy jobs and finish the image metadata prefiler before stratign anything new
16:30:20 dansmith melwitt: it's just that the way the code is laid out, it seems like what filters a user can expect is very tied to microversion, hence my punt to gmann
16:31:08 dansmith melwitt: especially since that method clearly says "this is just the non-admin search filters"
16:31:25 melwitt alistarle: I had been thinking so as not to cause an undetectable change in api behavior upon upgrade but yeah I guess if az's are public across the board you're saying why not just change it. in that case I think yeah you'd need a microversion to signal the switch?
16:31:46 dansmith melwitt: it could be that admins can filter on anything and that gets passed straight to the db, which is how that used to work, so just enabling it by name for the non-admins would be adding a new documented public filter, which would be a microversion
16:33:02 melwitt dansmith: yeah, I think I see your point now
16:34:24 alistarle I think that was efried point too here in the code : https://review.opendev.org/#/c/701609/
16:37:07 efried more or less, yes
16:41:17 alistarle Ok so should I wait for review in the specs or you think It's is not required and work directly on the code ?
16:42:05 efried alistarle: You can work on the code if you want, but obviously we won't consider merging it until/unless the spec is approved.
16:42:49 efried I think this makes sense, though, and should be very simple even with the microversion change. I wouldn't expect it to be controversial.
16:43:51 dansmith yup
16:44:11 alistarle Great news :) So I will look at your comment about the tests I can add waiting for your reviews then ;)
16:45:34 sean-k-mooney efried: oh by the way stephen is on PTO which is why he did not get time to review that last night
16:45:48 efried ahh, thanks for the info sean-k-mooney
16:46:00 efried alistarle: do you know how to make a microversion?
16:48:11 alistarle Hmmm, not at all :/
16:48:48 alistarle Is there a documentation somewhere ?
16:49:47 efried yes
16:49:49 efried stand by
16:52:20 efried alistarle: https://docs.openstack.org/nova/latest/contributor/microversions.html
16:52:37 efried Also, if you like to learn/work by example, you could look at previous changes that introduce new microversions.
16:54:12 efried That does more than you need, but it includes a microversion bump.
16:54:12 efried you'll find that L1280-1 were added via I46edd595e7417c584106487123774a73c6dbe65e / https://review.opendev.org/#/c/648662/
16:54:12 efried https://review.opendev.org/#/c/701609/3/nova/api/openstack/compute/servers.py
16:54:12 efried alistarle: For example, if you git blame the file you're messing with
16:55:04 alistarle Ok good, thanks for the resources !
16:55:25 efried btw, if you find incorrect or outdated information in the doc while you're working on this, please also propose corrections to the docs :)
16:56:06 alistarle I will work on that then, seems to be far more change than the original change
16:59:03 dansmith alistarle: the microversion bump will dwarf your actual change, for sure
16:59:31 dansmith but that's how our api versioning scheme works, so the size tradeoff isn't an argument to avoid doing it
17:00:39 alistarle No problem, I totally agree with that :)

Earlier   Later