Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-01-09
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 :)
17:01:27 efried alistarle: to summarize the reason for needing a microversion, in this context: API consumers need to be able to discover whether they should expect this filter to work or not. And the mechanism for discovering that is by seeing whether a particular microversion is supported.
17:02:34 efried especially in this case where prior to your fix (IIUC) we'll silently ignore the AZ filter, the consumer would have no good way of knowing whether it worked or not.
17:03:06 dansmith other than inspection in the detail case, but yeah
17:03:36 dansmith if they're filtering by az, then O(n) inspection is probably going to be bad if it didn't work :)
17:04:48 efried hence "good"
17:05:07 dansmith yep
17:05:21 alistarle @efrie
17:05:41 alistarle @efried yes I understand better the microversion need now
17:06:10 efried cool
17:08:47 gibi efried: I'm +2 on the vtpm spec https://review.opendev.org/#/c/686804
17:09:27 efried gibi: thanks. Sounds like stephenfin is PTO, hopefully he'll push it when he returns.
17:09:34 efried sean-k-mooney: any idea when he's back?
17:18:13 sean-k-mooney am i belive tuesday so if gibi is +2 and you adresss his nits i think you can proceed without him
17:18:51 sean-k-mooney or bauzas could bug him to take a look they when he show up at his place
17:19:26 sean-k-mooney they are going skieing so if there is an acident we could be down two cores
17:20:38 sean-k-mooney i remember thing in the flight back to hetrown form the first vancouver ptg that there were at least 8 cores and sevel other active comunity member on that flight
18:12:38 gmann dansmith: melwitt efried yeah I agree that if we are changing the filters for admin/non-admin without policy it need microversion otherwise even it is restriction or extensions of allowed filters will cause interop issue.
18:12:51 dansmith ++
18:12:59 efried thanks gmann
18:13:21 gmann but i thought new policy 'os_compute_api:servers:allow_all_filters' solve the filtering issue for non-admin and operator can do that way.
18:14:55 gmann but if changing policy is not preferable then we can reiterate on non-admin filters and may be we may find some filters which we should allow for them. so +1 on spec to iterate on those not just AZ.
18:15:21 openstack bug 1844568 in tempest "[compute] "create_test_server" if networks is undefined and more than one network is present" [Medium,In progress] https://launchpad.net/bugs/1844568 - Assigned to Eric Fried (efried)
18:15:21 openstackgerrit Eric Fried proposed openstack/nova master: DNM: Test granular tempest changes for bug 1844568 https://review.opendev.org/701794
18:15:33 efried ralonsoh: ^
18:15:55 efried gmann: that approach was problematic, see the patches
18:16:03 efried I think tldr it gave too much power
18:16:25 dansmith efried: gibi: I want to work on our marriage instead of just fight all the time: https://review.opendev.org/#/c/701796/
18:16:42 dansmith (see that and below)
18:17:08 efried dansmith: I read that as "f'ing default"
18:18:21 dansmith I think _fn is a fairly common suffix for function, but if you want something else that's fine
18:18:23 dansmith it's short
18:18:33 efried no, it's good, I was just joshing around.
18:18:36 dansmith default_callback or default_function are both long to specify in a class defintion
18:18:38 dansmith okay
18:19:00 dansmith remember the therapist said to be direct in our communication to avoid misunderstandings
18:19:04 dansmith especially in front of the children
18:20:00 dansmith anyway, this'll require an ovo release before we can use either of these, and I don't even know how that happens these days, but I think we'll be able to reparent things like your new request classes,
18:20:07 dansmith and get them the default behavior you want
18:21:29 efried dansmith: I'm not sure I understand how this differs from using the existing syntax, considering the deepcopy we've identified previously. It looks like it might let you do extra things during the initialization that a deepcopy wouldn't account for; but afaict nobody is asking for that?
18:21:53 dansmith efried: which are you talking about?
18:22:02 dansmith the default_fn one?
18:22:43 efried Oh, you said "below", I was waiting for another link in IRC. Looking at the other patch now....
18:22:43 efried default_fn, yes.
18:23:07 dansmith the reason I actually wrote that one is more so you can provide a class than a function
18:23:13 ralonsoh efried, ^^
18:23:13 openstack bug 1844568 in tempest "[compute] "create_test_server" if networks is undefined and more than one network is present" [Medium,In progress] https://launchpad.net/bugs/1844568 - Assigned to Rodolfo Alonso (rodolfo-alonso-hernandez)
18:23:13 openstackgerrit Rodolfo Alonso Hernandez proposed openstack/nova master: DNM: Test global tempest changes for bug 1844568 https://review.opendev.org/701799
18:23:33 efried ralonsoh: ack
18:24:13 dansmith yes, it'll still get deepcopy'd for compat, but not knowing that detail, it's more intuitive for just providing a callable thing like a class and having it instantiated
18:24:36 dansmith you know, I actually might be able to eliminate the deepcopy if you provide a _fn,
18:24:48 dansmith if I make the default wrapper do the deepcopy
18:25:51 efried as long as that wouldn't break anyone already relying on the deepcopy...
18:26:12 dansmith yeah, that's what we need to avoid, so I was just going to punt on it
18:26:31 dansmith but I'm not sure it'd be legit to be relying on that subtle of a detail
18:26:43 sean-k-mooney ralonsoh: is that to deal with the fact that somethins there are multile network and in somecase we race there creation if we change concurancy > 2
18:26:56 ralonsoh sean-k-mooney, yes
18:27:44 efried sean-k-mooney: trying to tackle the "Multiple networks found" CI errors that are making 80% of our CI runs fail.

Earlier   Later