Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2019-02-27
19:34:01 mriedem "None of those things are capabilities that a flavor or image would require."
19:34:33 efried that conversation sounds familiar
19:34:43 efried I was apparently convinced enough to +2 the thing.
19:34:52 mriedem "Eric Fried Mar 29, 2018 Patch Set 1: Code-Review+1Okay, I'm convinced we don't need the missing capabilities as traits. Thanks for your patience, all."
19:35:01 efried artom signed off on it too.
19:35:42 mriedem i think i would agree that if a compute driver capability is going to be reported as a trait, it should probably be a standard trait
19:35:42 efried so, fine, we decided we didn't need them to be standard, and the reason was because we didn't think they would need to be scheduled to. So the question remains: do we need to put them onto the compute node as traits? (Given ^ they would be CUSTOM_ fo sho)
19:35:49 artom efried, yeah, but that's like saying a kid ate some candy ;)
19:36:07 artom And in my defense, 'twas me who started this whole kerfuffle we're in now
19:36:27 efried yup, so it's only fair that the blame ended up back atcha.
19:36:54 artom But... was it a *useful* kerfuffle?
19:36:56 mriedem especially because if we added a new driver capability, like supports_trusted_certs, and before it's standard we report CUSTOM_SUPPORTS_TRUSTED_CERTS and then we have a standard trait and it becomes COMPUTE_TRUSTED_CERTS, that's going to be a weird situation for anyone building flavors requiring the former
19:37:07 artom Or did we just re-hash a past discussion to arrive at the same conclusion?
19:37:22 efried artom: No, this is a slightly new discussion and a decision that needs to be made.
19:37:29 efried but we did have to rehash the old discussion as a prerequisite :)
19:37:34 artom \o/ I'm useful!
19:37:38 mriedem i kind of also wish we weren't discussing the design of this ~1 week from feature freeze
19:37:49 artom Sorta
19:37:51 efried mriedem: Yes, there should be no reason we need to introduce a standard trait but wait to use it in nova.
19:37:54 mriedem because i'm going to say "sure whatever you want idk anymore"
19:38:02 mriedem and in a year we'll be saying "wtf did we do this?"
19:39:17 efried jaypipes: Do you agree with mriedem? To paraphrase: If we're going to tag the compute RP with a driver capability trait, it should be a standard trait; and conversely we should not tag the compute RP with a driver capability trait that is not standard; and therefore we should never tag the compute RP with a CUSTOM_* capability trait.
19:39:36 jaypipes reading...
19:40:06 efried (note use of 'capability' throughout - obviously the compute RP can get CUSTOM_* traits from elsewhere - admin, update_provider_tree, etc.)
19:42:10 jaypipes efried, mriedem: I feel the same as I did back then. If the thing is a capability of the compute node that an end user could feasibly want to influence scheduling, it should be a standardized os-trait. if it's just an internal implementation of the virt driver that isn't something an end-user would ever care about but is important for the virt driver infrastructure, just make it a simple attribute of the virt driver class.
19:43:01 efried okay, I'll mark up the review for aspiers
19:43:28 mriedem jaypipes: yeah i agree
19:43:45 mriedem as for why the patch was written how it was when i did a year ago, i can only guess (and already did)
19:43:49 jaypipes in the case of trusted certs, if this is something an end user will say "get me on a host that supports trusted certs", then it should be in os-traits as COMPUTE_TRUSTED_CERTS or whatever.
19:44:00 mriedem i believe it is
19:44:10 jaypipes and it sounds like end users do want that (via a flavor of course) so yeah.
19:44:20 mriedem https://github.com/openstack/os-traits/commit/3e0a6ea4f5045e0bdfe5a2e8097a184ba2c93c5a#diff-98123dc3728cad793e21fa497cf05723
19:44:23 mriedem it's not really the end user
19:44:28 mriedem it's not something on a flavor or image
19:44:43 mriedem it's that the user says 'i want you to honor my request' and we send you to some compute that has no f'ing clue what that thing is
19:45:09 mriedem i.e. placement request filters
19:45:16 mriedem same for multiattach volumes and device tags
19:45:21 jaypipes that's scheduling though. :)
19:45:31 jaypipes but yes.
19:46:02 mriedem my point was just trusted certs are not a flavor/image thing
19:46:11 mriedem you can't build aggregates around them
19:47:13 jaypipes ack
19:48:41 mriedem efried: aspiers: ok documented https://review.openstack.org/#/c/538498/19/nova/virt/driver.py@1034
19:49:23 efried mriedem: nice, race condition, correctly assumed "furiously"
19:52:33 melwitt the middle of being scheduled (and something older than rocky should be still in the middle of being scheduled)
19:52:33 melwitt mriedem, dansmith: thanks for the comments on the InstanceMapping.user_id patch. I just replied to mriedem. dansmith, build request doesn't have a user_id field, and request spec has had the user_id field only since rocky. do you think that's ok to rely on? I had been thinking we have to account for request specs older than rocky, but maybe that doesn't make sense because we're trying to account for old instance mappings that are in
19:52:57 melwitt *shouldn't
19:53:22 dansmith melwitt: right, it seems only worse to rely on something that has only existed since stein, but I thought it was on reqspec from well before that
19:53:23 dansmith either way,
19:53:37 melwitt it was added to reqspec in rocky
19:53:38 dansmith *all* IMs are missing it now, only some reqspecs are
19:53:40 dansmith okay
19:53:57 melwitt https://github.com/openstack/nova/commit/6e49019fae80586c4bbb8a7281600cf6140c176a
19:54:03 dansmith just seems like a joined query would be plenty fast, and then we're not just duplicating this again and again
19:54:10 dansmith once per instance per db should be enough :D
19:54:30 dansmith that's two years ago.. is that rocky?
19:54:54 melwitt yeah, I thought that would be older but I notice the included in version shows 18.0.0
19:55:13 dansmith should be queens right?
19:55:25 dansmith er, actually
19:55:28 dansmith maybe before that?
19:55:30 melwitt must have taken a long time to merge. queens is 17.0.0
19:55:34 dansmith queens was early '18
19:55:50 melwitt it merged may 2018
19:55:59 melwitt https://review.openstack.org/565340
19:56:06 dansmith ohh, I see, the 2017 date is the first git date I guess
19:56:11 melwitt yeah
19:56:27 dansmith well, anyway, same argument I think
19:56:44 melwitt I'm totally cool with not caring about request specs older than rocky if I got that reassurance
19:57:10 dansmith regardless, you could drop the first two patches and just change how you query for it whenever you do that
19:58:08 dansmith we could fix up old reqspecs on server GET for that matter
19:58:33 dansmith change our IM query to be IM joined with reqspec.user_id, and if null, set it to the instance.user_id we get from the cell before returning to the user
19:58:44 dansmith that would be pretty low overhead I think,
19:59:18 dansmith or just tell people if you want this to be accurate, make sure your migrations are done, just like would have to be the case if we added a new column to IM
20:00:09 melwitt migration for what, request spec? the addition of user_id didn't come with a migration
20:02:32 melwitt dansmith ^
20:02:55 dansmith melwitt: but you'd need one for your column, right? and I'm not sure why we don't have one for the reqspec one
20:03:06 melwitt oh, you mean me add one
20:03:10 melwitt yeah, ok
20:03:37 melwitt man, all that work I did to do all the various user_id populations
20:03:44 dansmith I'm saying just add one migration, for the field we have, which would be enough for all cases, instead of adding it to another place that has to also get patched up
20:04:02 melwitt I think I'm following now. thanks
20:04:24 melwitt back to the salt mines
20:06:12 dansmith melwitt: why do we not already have a migration for user_id on reqspec?
20:06:22 dansmith did that just get missed maybe?
20:06:42 melwitt dansmith: I dunno. probably wasn't considered important for already existing records
20:06:54 dansmith any idea why?
20:07:56 melwitt no, that's my guess
20:08:14 melwitt mriedem: do you happen to know why we didn't do nova-manage data migration for RequestSpec.user_id? https://review.openstack.org/565340
20:08:33 dansmith seems really weird to me
20:08:46 melwitt aye
20:09:04 dansmith I think because it's for "out of tree filters" and thus wasn't really done completely
20:09:35 dansmith just another reason not to further proliferate "I need this duplicate info over here so I'll add it again" kinda stuff
20:10:08 melwitt yeah. I was thinking in the box
20:11:38 mriedem melwitt: was never added - as dansmith said, it was added for an out of tree filter (mgagne) that relied on the user_id and therefore a full-fledged data migration wasn't added for existing request specs because we hack them prior to scheduling during moves
20:11:48 mriedem but any persistence of that request_spec.user_id update is purely accidental
20:12:53 dansmith yeah, that's unfortunate
20:12:57 mriedem https://github.com/openstack/nova/commit/6e49019fae80586c4bbb8a7281600cf6140c176a is queens btw
20:12:57 melwitt request spec, what's persisted? what isn't? it's a surprise!
20:13:26 dansmith melwitt: right, let's migrate to make it consistent and remove this point of ambiguity for the future :)
20:13:32 mriedem oh shit i guess that was rocky

Earlier   Later