Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-11-12
16:48:43 dansmith mriedem: on lyarwood's spec to do stable disk rescue,
16:48:56 dansmith do you have strong feelings about bundling the can-now-rescue-BFV feature in it,
16:49:03 dansmith vs. a relatively tiny follow-on effort?
16:56:01 openstackgerrit sean mooney proposed openstack/nova master: block rebuild when numa topology changed https://review.opendev.org/687957
16:56:02 openstackgerrit sean mooney proposed openstack/nova master: Disable NUMATopologyFilter on rebuild https://review.opendev.org/689861
17:02:01 mriedem dansmith: i haven't read it but what's the difference? you mean making the api capability separate which is what, a microversion and a min compute service version compat check?
17:02:22 dansmith mriedem: not even a min version check I think, a compute capability check
17:02:47 mriedem so the api checking the resource provider traits?
17:03:13 dansmith er, maybe I'm confused, I thought the api could check the compute capabilities, but maybe that's stupid
17:03:25 dansmith so yeah maybe a version check and a cap flag in the compute manager
17:03:52 mriedem i've brought up that idea before about the api checking compute capabilities via traits, e.g. https://review.opendev.org/#/c/666604/
17:03:52 dansmith mriedem: point being, lyarwood had it in his now-abandoned first rev to remove that api check, and his current spec says no API impact, so wondering which way you want to go
17:04:17 mriedem well there is obviously an api impact
17:04:39 dansmith mriedem: ...only if he removes that check in his series again
17:04:59 dansmith mriedem: and, you think that needs a microversion? just removing a check in some cases?
17:05:46 mriedem it's a behavior change that a user opts into, i.e. i can rescue volume-backed servers on cloud A but not cloud B,
17:05:49 dansmith if so, I understand why, but I don't have a good gut feeling for when we do and don't need to signal removing checks
17:05:50 mriedem which is usually a microversion
17:06:02 dansmith they can opt in but still refuse with the same error
17:06:08 dansmith it won't work for all hypervisors,
17:06:18 dansmith and unless the api documents that you can't do that with volume-backed instances,
17:06:19 mriedem same idea for multiattach volume support, same for volume-backed rebuild (if someone finished that up)
17:06:22 dansmith it seems like an obscure thing to me
17:06:37 mriedem i think we've said in the past (and in our docs) that the docs are not a contract, the api behavior is
17:06:55 mriedem and yes i know we fudge these lines all the time, e.g. gibi's changes to support moving servers with qos ports
17:07:13 dansmith absolutely, but I'm not sure this is discoverable as the reason, which is why it doesn't seem clear-cut to me
17:07:16 mriedem and yes i realize not all hypervisors are going to support this so just because a cloud has the microversion that allows the request it doesn't mean it's going to work
17:08:02 dansmith if you view the current error as "You can't rescue that instance for reasons I will not divulge" then all users right now just try it and accept the fate if they can't, without knowing why"
17:08:09 mriedem as i said in lee's original change about microversions, i said i don't know what others would think so it's likely a ML discussion
17:08:12 dansmith in which case allowing it on more things seems fine
17:08:28 mriedem it's not a 409 though right? it's a 400
17:08:53 mriedem yes it's a 400 today
17:08:55 dansmith the distinction doesn't mean anything to me
17:09:07 dansmith 409 is transient or something?
17:09:09 mriedem if it were a 409 that'd be a bit fudgier to saying "you can' do this right now"
17:09:20 dansmith okay, well, that's legit
17:09:31 dansmith probably *should* have been a 409 then, but alas
17:09:54 mriedem i don't think there was a lot of thought put into response code nuance back in the day
17:10:03 dansmith no, clearly not
17:10:53 dansmith does 400 mean it's a fatal error specifically or just not imply retry-ability one way or the other? I would assume the latter as a catch-all
17:11:10 openstackgerrit sean mooney proposed openstack/nova-specs master: Add spec for VM-scoped SR-IOV NUMA affinity https://review.opendev.org/683174
17:11:35 dansmith anyway, if it's a microversion, then it probably makes sense to have that be a separate effort, maybe even before this stable rescue series to open up the possibility, even with no implementors, followed by his patch that actually allows it
17:11:59 sean-k-mooney efried: alex_xu ^ fixed the typos but that is otherwise the same
17:12:44 mriedem 400 means don't try this again
17:14:02 dansmith "The client SHOULD NOT repeat the request without modifications."
17:14:03 sean-k-mooney or at least dont try this again until the resouce is in a different state
17:14:24 dansmith sean-k-mooney: that's not what the w3c says for 400, but does for others, so not sure you're right about that
17:14:41 mriedem sean-k-mooney: you just described 409
17:14:44 sean-k-mooney 400 is bad request
17:14:48 dansmith mriedem: that's an unfortunate implication for our default error code, but fair enough
17:15:11 sean-k-mooney but it may be a bad request for the give state fo the resouce but not in general
17:15:22 dansmith sean-k-mooney: no
17:15:32 sean-k-mooney 409 conflcit is ment for concurrent modifcation
17:15:34 dansmith sean-k-mooney: it says the reason is "malformed syntax" and "do NOT try this again"
17:15:43 dansmith https://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html
17:15:59 dansmith the reason is not "the thing is in the wrong state for the operation you tried"
17:16:08 sean-k-mooney ya fair
17:16:12 sean-k-mooney i normally use https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/400
17:16:29 sean-k-mooney but yes it also highlights "The client should not repeat this request without modification."
17:16:33 dansmith none of those reasons imply retry
17:17:55 sean-k-mooney i think the only time you woudl retry is for 5xx errors
17:18:04 dansmith mriedem: so, spec because api change, and put the api change ahead of lyarwood's patch so we can actually test through it and hold merge until both are ready yeah?
17:18:31 dansmith sean-k-mooney: no, some of the 4xx errors are mentioned as retryable with no modification
17:18:36 dansmith sean-k-mooney: like 408
17:18:57 sean-k-mooney oh ok i normaly think of 503 https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/503 when i think retry
17:19:09 mriedem dansmith: if you're saying single spec, multiple changes (one for compute, one for api), then yes agree
17:19:11 sean-k-mooney oh ya request timeouts
17:19:40 dansmith mriedem: no, I mean another spec for the api change specifically just to avoid mucking up lyarwood's clean understandable spec with the spec to change the api behavior
17:20:10 efried oh sean-k-mooney, you lost your +1s from Vieri and Andriy :(
17:20:44 sean-k-mooney :( your +2 would equal both there +1s right :)
17:21:02 dansmith mriedem: I don't want to do extra work, but it seems like having lyarwood's stable rescue spec be readable in isolation without a bunch of api semantic changes would be beneficial, but maybe the api docs required for just a status change wouldn't be too big
17:21:13 efried sean-k-mooney: re+2, thanks for the update.
17:21:35 sean-k-mooney i still need to fix the typos you noted in the code but thats next on my list
17:22:06 sean-k-mooney then i need to adress dansmith's feedback on another series :)
17:23:26 mriedem dansmith: are you saying the api change would just be changing the status code from 400 to 409 rather than a microversion?
17:23:37 dansmith mriedem: no
17:24:25 mriedem then i don't think a secondary api-only spec is going to be that big of a deal
17:24:43 dansmith cripes
17:24:45 mriedem "depends on blueprint x. in x we make y work on the compute. this spec enables it in the api with a new microversion"
17:25:08 mriedem just do whatever y'all think is best
17:25:13 mriedem i haven't read the current spec
17:25:24 dansmith mriedem: I'm trying to say I think we should separate the api change into its own small spec to keep lyarwood's focused on the disk changes
17:25:42 mriedem and i just said i was ok with that right?
17:25:53 dansmith mriedem: in isolation without his change, we can make the api change to remove the hard block and say "as of microversion X you may or may not be able to rescue BFVs"
17:26:13 dansmith mriedem: it wasn't clear to me
17:26:34 dansmith because that's what I've been suggesting for ten minutes and you seemed to be disagreeing, so I was trying to explain
17:26:39 mriedem so without any compute side changes, we should be able to rescue volume-backed servers today?
17:27:17 mriedem i'm fine with whatever
17:27:27 mriedem let's just stop talking about this
17:27:29 dansmith I'm clearly the problem here, I'll just butt out
17:34:55 artom donnyd, heya, what the difference between your multi-numa and multi-numa-expanded flavors? Or where can I look to find out?
17:35:14 artom (Also, we should talk about enabling hugepages at some point, but that can wait)
17:35:22 sean-k-mooney artom: the expanded one has 16G of ram instead of 8
17:35:31 artom sean-k-mooney, that's it?
17:35:37 sean-k-mooney yep
17:35:51 sean-k-mooney otherwise they both have the same about of disk and cpu
17:35:58 artom And NUMA nodes
17:36:02 sean-k-mooney but have hw:numa_nodes=2
17:36:25 sean-k-mooney and hw:cpu_socket=2 and hw:cpu_thread=2
17:36:43 artom So the non-expanded one is fine

Earlier   Later