Earlier  
Later
New Search
Posted Nick Remark
#openstack-nova - 2020-01-09
21:43:00 dansmith okay
21:43:13 efried but still think there's value in smashing them together.
21:43:37 dansmith do you understand that I don't want to mix default-on-init into objects that are representative of row-based data structures on the other side of an rpc pipe?
21:43:55 efried Yes.
21:44:07 efried um, I think I do
21:44:12 efried I don't think I'm suggesting otherwise.
21:44:20 dansmith your latest reply seems to disagree
21:44:55 efried as long as we're talking about object as opposed to class
21:45:47 efried I don't think a class def of a field needs to be tied to every case where an instance of that class is used in a parent object
21:46:23 efried like with nullable, where we're saying a particular instance of that class is tied to a db column that's either nullable or not.
21:46:47 efried (I get that that's not all it says, but that was its impetus)
21:47:02 dansmith um
21:47:30 dansmith maybe you're focusing on objects that are included as fields on another object specifically?
21:47:41 efried yes, that's exactly what I'm doing.
21:48:16 dansmith hang on, ups
21:49:30 dansmith so, I the inner object getting the behavior we want is cool, but not if it breaks the outer objects or the whole system
21:49:58 dansmith and making a single field default-on-init is what I don't want to do
21:50:21 dansmith which is why keying the default-on-init on the default_fn=<callable> isn't okay, IMHO
21:51:56 efried I thought "making a single field default-on-init" is exactly what we *did* want to do. It indicates that we know that field isn't backed by a db column, among other things, and we want the convenience of being able to count on it being set without having to default it explicitly or have it autovivify.
21:53:17 dansmith no, it's precisely what I don't want
21:54:02 dansmith I just finished reading your reply and replied, that we definitely don't need a subclass-per-type like you surmised
21:54:11 efried I'm playing with it now, and I think it's helping me understand the distinction you're making.
21:54:19 efried gimme a couple minutes...
21:56:44 dansmith right, so RequestSpec could become fully ephemeral IMHO
21:57:27 efried okay, so really what the bottom patch does is make __init__(): obj_set_defaults() sanctioned behavior
21:57:35 dansmith which means RequestSpec(NovaObject) becomes RequestSpec(EphemeralObject), and your default request_level_params gets applied at start
21:57:38 dansmith yes
21:57:51 dansmith categorically if inheriting from the mixin, that's the point
21:58:19 dansmith "this object is one of those that doesn't represent database columns and therefore can behave defaultly"
21:58:33 dansmith basically, anything that does not have NovaPersistentObject in its MRO would be okay to make ephemeral I think
21:58:48 efried ...and RequestLevelParams itself would also inherit from EphemeralObject, so that its root_required/root_forbidden fields woul default at that same time?
21:58:55 dansmith that ^ is our mixin for hard database affinity, although we couldn't be quite that sweeping
21:59:00 dansmith yes
22:01:22 efried We could do this just as easily in nova; but we want to do it in ovo to kind of send a message about preferred usage??
22:02:10 dansmith I guess we could do the init defaulting part yeah
22:02:18 efried Could do it in nova as a stopgap until we can suck in the ovo release
22:02:41 dansmith yup
22:02:48 efried and I can see making an infrastructure that poisons use of obj_set_defaults in __init__ unless inheriting from Ephemeral
22:03:05 efried ...to enforce the paradigm
22:03:07 dansmith I thought it was going to involve a little more mucking with obj_set_defaults() than it did, but I was also planning both of these together
22:03:35 dansmith efried: we can test for it in test_objects I think
22:03:44 dansmith efried: instantiate every object (like we do) and make sure that unless it inherits from that mixin, nothing is set after init
22:04:41 efried well, that doesn't prove obj_set_defaults wasn't invoked. I was thinking about making obj_set_defaults set a flag or something, then we can check for that flag in the test
22:05:05 dansmith well, we have (or had)objects in the past that didn't use set_defaults, but just set them explicitly in __init_
22:05:20 efried Is that also a bad thing?
22:05:21 dansmith just "self.foo = None" or something in __init__
22:05:32 efried I would think that would be okay.
22:05:33 dansmith it's the exact same thing, except less obvious, so.. yes?
22:05:42 dansmith it's the *exact* same outcome :)
22:05:42 efried it's just the blanket obj_set_defaults we want to avoid
22:06:08 dansmith you make it sound like it's just an arbitrary rule... the reason for the rule is because of the clobbering of columns
22:07:36 efried which is never ever desirable?
22:08:07 dansmith for a row-based object, never ever
22:08:28 dansmith and without a system in place, people see and copy examples, which is why we have a few in the tree
22:08:49 dansmith any time I or mriedem would catch it in review, we'd get it taken out, but things slip through,
22:09:01 dansmith then people see that example, figure it's okay, etc
22:09:21 efried yeah, I get it. So, an enforcing test.
22:09:35 dansmith so what I'm trying to say is let's make it a little more obvious, you can only do this if you inherit from ephemeral, but you better not be row-based
22:10:02 dansmith all our objects used to be basically 1:1 mappings for the db, but over time we've grown a lot more of these general purpose ones, so it used to be easy to say "you can never ever do this"
22:10:30 efried really, we could do the test without introducing any new semantics. Course, that would mean actually fixing the existing violations, immediately. Which we can put off if we introduce the new semantics.
22:10:39 efried so, I dig.
22:11:12 dansmith you mean we can do the test now and fix the instances we have, right?
22:11:32 dansmith that's true, but I think the point here is, it's legit to want sane defaulting for certain types of objects so let's open the door but in a controlled way
22:11:53 dansmith so anything in tree that does this now either should be inheriting from the new base, or their __init__ behavior needs be to cleaned up
22:12:40 efried If we set up the Ephemeral infrastructure, we can just blindly make those objects inherit from it so we're no worse off than we were before, and fix them "later", but new work will have to be compliant
22:12:40 efried but
22:12:40 efried If we set up the test now, we have to fix the existing objects that violate the rule
22:12:40 efried I mean:
22:13:11 dansmith well, I don't want to blindly do it, I want to examine each case, but there aren't that many that I know of (other than notifications I think)
22:13:19 dansmith but otherwise yes
22:13:29 efried okay, I'm on board.
22:13:31 dansmith no reason to fix them only to open the door to the thing a patch later
22:14:39 dansmith So tomorrow I will copy that into nova, work on the test and see where we are
22:15:44 efried just since it's in my head, would it make sense for the long-term solution to instead use a class variable on VersionedObject, EPHEMERAL=False, which ephemeral subclasses could override to True?
22:15:47 efried instead of a mixin
22:16:33 dansmith doing that means you could have an ephemeral parent and be non-ephemeral yourself, which would be bad
22:16:38 dansmith or the opposite
22:17:24 dansmith you saw my reply that nova things will not need to mixin themselves right?
22:17:37 dansmith there will be a base class in base.py which mixes in and then you'll inherit from that once
22:18:39 dansmith and that means we can always isinstance(thing, NovaEphemeral) to know its true nature
22:19:38 efried as opposed to self.EPHEMERAL. But yeah, the possibility of flipping back and forth in nested subclasses is a reasonable argument against that.
22:20:31 dansmith yeah I just mean you can't really fake your parentage easily
22:20:37 efried though this way you can still do that, but you can only flip *to* ephemeral, and you can only do it once.
22:21:33 efried unless we just don't define the mixin at all. Make NovaEphemeral a subclass of NovaObject.
22:21:36 efried anyway, details.
22:22:05 dansmith yeah, just being a class variable makes it changeable at runtime, vs an essential quality of the thing
22:22:31 dansmith what's the argument for it being a class variable?
22:22:50 dansmith less formality, maybe, but I'm going for formality here
22:22:59 efried "efried doesn't like mixins"?
22:23:36 dansmith oh I don't really either, but I think I was asked to make those qualities mixins because that's how python libraries do things like this without affecting interfaces as much
22:23:48 efried makes it tough to figure out what's happening
22:23:59 efried ye gods, have you seen the openstacksdk project?
22:24:13 dansmith yep, totally agree, which is why the one class that does the mixin in base and then you single-inherit from that, knowing its done right
22:24:20 dansmith mixin heaven?
22:24:32 efried every class has like a dozen mixins, and it's freaking impossible to understand where you go when you invoke a method
22:24:43 dansmith yeah, I'm totally with you on that
22:24:51 dansmith when this was all code in nova there was a lot less mixing going on
22:24:53 efried mixins, and proxy shims, and dynamic construction, and ...
22:25:01 dansmith but it's a library now, so..
#openstack-nova - 2020-01-10
01:16:08 openstackgerrit Merged openstack/nova master: Add missing parameter vdi_uuid in log message https://review.opendev.org/701004

Earlier   Later