=== max__ is now known as Guest94867 === max__ is now known as Guest28910 [07:11] So the reason some charms end up with '-k8s' in the name, is because in the current charm store, you can't have the same name for different purposes. [07:11] (k8s vs normal iaas charms) [07:12] The intent with the new charm store was to leverage a field like Arch to allow you to 'juju deploy mysql' which if you are in a k8s model is a different charm than if you are in a AWS model. [07:12] So the goal is to get rid of -k8s, but it is a necessary wart today. [07:13] thanks for the update Dmitrii-Sh . You really should sleep earlier. :) [07:14] jam: you're right [07:15] jam: regarding the charms, one thing I noticed is that VIPs make functional testing harder [07:15] there is an expectation that something will be pre-allocated and is not modeled as an abstraction [07:16] Dmitrii-Sh, we talked in Juju about having a way for a charm to say "get me another IP from the provider" which it would then use for its purpose. [07:16] In practice, it was just a testing thing, because all the charms that have VIP end up assigning real meaning to them in the rest of the system [07:16] so an arbitrary value wasn't good enough. [07:17] They *cared* that it was 10.100.20.50 because that was the firewall rule that they set in the some other system. [07:18] I agree, in all cases I've seen so far it was the case. Plus there was a domain name associated with that vip [07:20] jam: w.r.t https://github.com/canonical/operator/issues/193, instead of inheriting in ops/testing.py I could recreate the same type with a different name and __bases__ and __dict__ preserved [07:20] that wouldn't help with mutable class attributes though [07:21] (inheritance doesn't help either so it's not better or worse in a sense) [07:21] Dmitrii-Sh, it sounds like we're doing something bad with SavedState if we can't subclass a type that has it and have it work. [07:21] Dmitrii-Sh, that said, we'd rather not subclass and just fix the attr thing to change attrs on the instance rather than the class [07:41] jam: so far we have .on, .state (or whichever other name a dev picks) and .handle_kind as class attributes. define_event works with types that inherit from EventSetBase so define_event would have to be done for every instance of a type that inherits from EventSetBase. [07:41] Do you have any ideas on where that binding to instances would take place? [07:45] Dmitrii-Sh, I haven't worked out exactly how it would hold together. Maybe EventSetBase. [07:46] We could do something like CharmBase.__init__, but I don't think we want people to have to re implement that for every thing they want to create that emits events. [08:29] * jam lunches [08:29] gooood (belated) morning peeps! [08:30] Gutten Morgen [08:37] Chipaca: https://trello.com/c/mc1YggOd/9-top-10-15-charms-in-op-fmk there is a card to identify top 15 charms to be written in the operator framework. I wrote down the ones that I think would we worthwhile to consider based on the charm store stats. [08:37] Dmitrii-Sh: nice [08:38] Dmitrii-Sh: missing two inputs into that: what field thinks is hawt-and-upcoming, and any special requests :-) [08:39] also, where are my glasses [08:39] The most popular ones in the store are the OpenStack charms but I excluded most of them (except for rabbitmq, mysql and hacluster). [08:39] 👍 [08:39] thank you [08:40] mysql is one of the charms that OpenStack eng recently rewrote in reactive so I am not sure if we need to rewrite it yet again as a first candidate [08:42] :-) no need to start there, indeed [08:42] Chipaca: as for input from the field, I can go and ask them in their channel. I know that Arno was keen on having OSM k8s charms in the new framework. [08:42] I am sure they'd appreciate better landscape charms [08:43] Dmitrii-Sh: ask them for a top 5 (and make it clear it's not a promise) [08:43] ok [08:54] Chipaca, can you see the message above about -k8s ? I can repeat it if you cannot [08:55] jam: not with this account, could you repeat? [08:55] So the reason some charms end up with '-k8s' in the name, is because in the current charm store, you can't have the same name for different purposes. [08:55] (k8s vs normal iaas charms) [08:55] The intent with the new charm store was to leverage a field like Arch to allow you to 'juju deploy mysql' which if you are in a k8s model is a different charm than if you are in a AWS model. [08:55] So the goal is to get rid of -k8s, but it is a necessary wart today. [08:56] jam: yep [08:56] jam: but we're not getting platform for a while yet [08:56] jam: also, also [08:56] jam: i understand it's often the case that the k8s charm and the non-k8s charm is frequently authored by different teams [08:57] jam: in that case, -k8s in the name will continue to be a thing [08:57] Chipaca, indeed. I'm just pointing that -k8s feels like a wart, when you look at "kubectl get all" and see a bunch of foo-k8s-operator objects. [08:57] or as per gustavo's idea yesterday -k8s-operator [08:58] jam: that is fair. There is nothing in the store's roadmap to fix that wart though. [09:00] My idea was -operator, to be fair :) [09:01] Chipaca: Not sure I get the relationship between that wart and the store? How's that a wart in the store? [09:02] niemeyer: the wart can only be fixed by store-side work [09:02] That's what I understood the statement to mean, but I don't get what that's actually referring to [09:03] niemeyer: ownership is by name, you can't have different owners for different platforms under a single name [09:04] Chipaca: We do have delegation in the roadmap, and that seems like an aside.. we have support for independent channels for different architectures, and will have for different platforms as well [09:07] niemeyer: where is delegation in the roadmap? (is this what delegation is? seems a strange name for it) [09:07] niemeyer: i don't understand what you mean by it being an aside [09:08] Chipaca: Delegation of authority [09:08] in on-prem enterprise stores, yes [09:08] that does not sound like this [09:09] Chipaca: It's an aside in the sense that when it becomes an issue we can prioritize the work.. the critical piece of that work is that we need to support orthogonal channels for orthogonal platforms, and that's part of the design right now [09:09] Chipaca: It'll be an excellent problem to have when entirely different authors are fighting to maintain the same charm in different platforms [09:10] Chipaca: I sent an e-mail to their list (CC-ed) and a message to their channel soliciting some feedback. Hopefully we can get some stats out of that. [09:10] Dmitrii-Sh: thank you [09:16] https://github.com/canonical/operator/pull/196 pulls out some of the stuff from my test_model.py patch, as the Harness changes were relevant for interface-pgsql [09:18] jam: did you see i tagged you on issue #193? [09:18] Chipaca, yeah. Dmitrii-Sh and I were chatting about it earlier. [09:18] ah ok [09:19] We can change how we create the class, but it feels like we're doing something wrong if you can subclass something that has state. [09:19] that said, we'd rather not subclass if we can change how the accessors are done. [09:20] How to subclass something that does _not_ have state? Isn't a class without state basically a function? :) [09:21] niemeyer, if you do class MyClass: state = StoredState(); having class NewClass(MyClass): will break. [09:21] at least, that is how I read https://github.com/canonical/operator/issues/193 [09:21] Should we fix that? [09:22] yes. my argument was we should fix that regardless, even if we don't want to create a subclass for testing. [09:22] I'm not suggesting we should encourage subclassing of everything, but it sounds like a slightly awkward hard constraint [09:22] Yeah, same [09:22] will make writing components as mixins hard for example :) [09:23] It just feels like it's an artificial constraint.. we never considered it an intended limitation [09:24] We can discuss when and whether mixins/subclassing/etc make sense, but right now we can't [09:25] It's probably an easy fix as well.. did you look into it jam? [09:26] StoredState is gnarly, undocumented code [09:26] is the attr_name thing it's doing just doing what we do with set_name elsewhere? [09:26] It is indeed.. it exists so that people can not worry about that sort of thing [09:27] I haven't looked deep into it yet. [09:29] I'll see if I can't make the code a bit clearer, unless you're already looking at it [09:29] I'm not right now. looking at interface-pgsql currently [09:31] 👍 [09:40] niemeyer: so the intent is for any class to be able to have a stored state, even if it doesn't inherit from Object? [09:41] Chipaca: Object is not as important as it looks like for the framework.. logic generally check for what they need, not for inheritance [09:42] Chipaca: Object is there for developer convenience [09:42] niemeyer: so it doesn't have to Object, but it does have to look like Object [09:43] Chipaca: If that was the intention then we'd check for inheritance [09:43] Chipaca: The point being that Object may "look like" things that we don't care about [09:44] Chipaca: Makes sense? [09:44] it's duck typing, but without being explicit about what's needed [09:45] no it does not make sense :) [09:45] currently if you just try to use StoredState like this, [09:45] class MyC: stored = StoredState() [09:45] and then do c = MyC(); c.foo = 42 [09:46] it fails, because it tried to call register_type on a 'framework' attribute [09:46] Chipaca: If class C1 depends on attribute A1 and A2, and class C2 offers A1, 2, 3, 4, 5, 6, and 7.. does class C1 depend on how class C2 looks like, or depends on A1 and A2?? [09:46] niemeyer: if class C1 does not document what it expects of the things it's usually used with, and fails in obscure ways as soon as you try to use it outside of that, it might as well depend on every detail of the implementation [09:47] Chipaca: If the water is dirty, do you throw the baby out too? [09:47] Chipaca: Or do you document the baby and clean it up? [09:48] why are you talking to me like this last thing isn't _EXACTLY_ why i started asking the question, dammit [09:48] Chipaca: I'll wait until you relax so we can continue the conversation.. [09:48] * niemeyer steps out and sips some mate.. [11:08] Muy buenos días a todos! [11:15] Dmitrii-Sh, hola! question about gh#193, you say that "having a state attribute on a charm currently raises an exception."... that means that if you have a real charm class, with a class attribute `state = StoredState()`, it will crash? [11:18] Ah, I see now that that conversation continued later [11:18] * facubatista keeps reading [11:23] facubatista: yes, pretty much [11:24] Dmitrii-Sh, sounds to something we should allow to be tested, not something that should be worked around [11:24] Dmitrii-Sh, otherwise we're giving the signal to developers that is a bad idiom of the charm [11:24] jam, ^ [11:25] facubatista, that seems to be the consensus. The specific issue is that testing.Harness uses a subclass, which turns out to break. [11:25] and we don't really want to use a subclass for testing, but we also don't want to break if someone does subclass. [11:26] jam, perfect, thanks [11:31] Sorry for lagging - had a call with David from the OSM team. [11:31] Yes this is definitely something we need to have built-in. All of my current charms use state in some way at the moment, for example. [11:35] Dmitrii-Sh, yeah, the component I'm testing uses state, but isn't a complete charm, so I hadn't run into it yet. [11:36] The 'charm' I'm interacting with is just CharmBase, and then I use that to fire events into my component. [11:38] niemeyer, I tried to build a simple class that uses StoredState... I end up depending on having a Framework... yes, Object is not that important per se, but this "implicit framework needed floating around" is what makes me incomfortable in some situations [11:38] Not saying we should change that, but maybe we could improve something about it [11:38] for example: [11:38] class DBLoggerInterface(Object): [11:38] def __init__(self, charm, name): [11:38] super().__init__(charm, name) [11:39] Having to call Object with the `charm` looks arbitrary [11:40] and that's only because "or the parent is the framework, or the parent has a framework" [11:44] so, it looks like the rule is "there must be a framework floating around for everything" (which is fine, this is the operator framework), but "it's not really floating around, everybody should have it", maybe we need to make that more explicit [11:45] * facubatista is just thinking out loud, doesn't have a concrete proposal [11:57] facubatista: Let's cover that in the standup in a bit.. first thing we need to figure out is why specifically subclassing was breaking down. This is unrelated to what is passed in or whether the object has a framework or not. If the base class worked, the subclass should work too, right? [11:58] niemeyer, absolutely, that's out the question for me; subclassing must work [12:16] The attribute lookup (https://git.io/Jv5uv) is performed in a __dict__ of the type that is supposed to hold the StoredState attribute. When we subclass the type that holds '.state = StoredState()', the new type no longer holds '.state' attribute in its __dict__ (its parent type does). [12:17] what I have in the pdb output in the bug: [12:17] (Pdb++) parent_type.__dict__.items() [12:17] dict_items([('__module__', 'ops.testing'), ('on', .TestEvents object at 0x7f935be611f0>), ('__doc__', None)]) [12:17] (Pdb++) parent_type.__bases__[0].__dict__.items() [12:17] dict_items([('__module__', 'src.charm'), ('state', ), ('HAPROXY_ENV_FILE', PosixPath('/etc/default/haproxy')), ('__init__', ), ('on_install', ), ('on_start', 0x7f935befc1f0>), ('on_stop', ), ('on_config_changed', ), ('on_backends_changed', ), ('__doc__', None)]) [12:25] Dmitrii-Sh, then could we just do getattr(obj) instead of __dict__ ? [12:26] or if we're intentionally avoiding __get tricks, then object.__get_attribute__ ? [12:28] going to try that now [12:32] hmm, self.attr_name can be None [12:37] jam: the first __get__ that will be run is going to have self.attr_name == None [12:38] and then the for loop will be invoked to look up an attribute name by an attribute value [12:38] so we also need to take care of this: `parent_type.__dict__.items()` [12:44] jam: e.g. `for attr_name, attr_value in inspect.getmembers(parent_type): ...` [12:45] Folks, we don't need to *all* be working on the exact same issue [12:53] afaict Dmitrii is the only one actively coding, I'm giving feedback on how I think it could work. [13:03] using getmembers leads to an infinite recursion in some framework test cases so I'll need to explore more [13:10] so for things that do getattr hacks (as we tend to do here), you can often use object.__getattribute__(member) to avoid the classes' __get function (IIRC). [13:12] jam, these methods not implemented in the testing, e.g. [13:12] def storage_list(self, name): [13:12] raise NotImplementedError(self.storage_list) [13:12] jam, should be implemented by whom? where self.storage_list should come from? [13:13] facubatista, they are part of the ModelBackend, and should be implemented in the Harness, just didn't get to the full interface [13:13] sorry, should be implemented in _TestModelBackend, I had NotImplementedError there for us to know what things to add. [13:14] ah, for the future we should have a comment there (ideally, with name and date, like: XXX 2020-03-25 jam: need to implement this in future branches) [13:14] jam, do we have an issue open or a trello card to track this missing work? [13:14] not that I'm aware of. [13:15] jam, we should, otherwise we'll forget about it [13:15] jam, would you please? thanks! [13:16] * facubatista is not really sure if in English that "would you please" is like in Spanish having it implicitly the "take care of", or sounds harsh -- if it sounded bad, was not my intention [13:17] facubatista, no, it is a genuine gentle request. [13:17] thanks [13:17] or at least I interpreted it as such. (probably you could do it otherwise :) [13:20] Created draft PRs for the charms I have at the moment: https://github.com/canonical/cockroachdb-operator/pull/1 https://github.com/canonical/keepalived-vrrp-operator/pull/1 https://github.com/canonical/haproxy-operator/pull/1 [13:20] the repositories under canonical are empty except the license file [14:03] * facubatista brb [14:09] I am rewriting the charmed-osm/mariadb-k8s charm with the operator framework and I am having some trouble providing the mysql relation and relation data. [14:10] Dmitrii-Sh: I tried to implement it by following the gitlab-k8s example but couldn't get it to work. [14:14] karimsye: I am up for a call to give you some guidance if you have time [14:14] * jam eod [14:21] karimsye, can you pastebin what you currently have? thanks! [14:26] Dmitrii-Sh: ok thanks. I have to run an errand, I should be back in about 15 minutes [14:26] karimsye: ok, sgtm [14:26] I'll wait [14:59] Dmitrii-Sh: https://meet.google.com/uck-qvcz-aoq [15:01] karimsye: 2 min, need to send an e-mail [15:01] Dmitrii-Sh: np, I am trying to setup my environment as well [15:06] facubatista: the git repo for my charm (the files in src are not the latest) - https://github.com/SMAK1993/charmed-osm-mariadb-k8s [15:06] facubatista: my latest src/charm.py - https://pastebin.canonical.com/p/R8sKN7FWW3/ [15:07] facubatista: another file being used src/interface_mysql_provides.py - https://pastebin.canonical.com/p/ZCVP43qBcV/ [15:07] interface_mysql_provides.py is my version of https://github.com/johnsca/interface-http/blob/master/interface_http.py [15:08] but trying to implement the mysql interface/relation [15:10] karimsye, reading it [15:10] thanks [15:13] facubatista: in charm.py the class MariaDbProvides at the bottom was my attempt to imitate https://github.com/tvansteenburgh/charm-kine/blob/master/lib/charm.py. The current charm.py isnt using it but when I tried to use it in a similar manner to the charm-kine example, I wasn't able to successfully relate to cs:~charmed-osm/keystone-k8s [15:15] karimsye, let me understand better the behaviour you're NOT getting... if you deploy this code, where is the problem, or when the problem arises? or you didn't get to deploy it yet? [15:17] facubatista: I deployed it and the error I got was: [15:17] https://pastebin.canonical.com/p/TDZD7yhNMq/ [15:18] basically EventBase does not inherit from Object [15:20] * facubatista checks the error [15:26] facubatista: on a call with karimsye and Narinder now [15:26] karimsye, ok, the code looks good AFAIC (by just looking it) for everything related hooking the charm and the interface together and have stuff flying around [15:26] going in [15:26] events are not Objects themselves so there's an issue in using shortcuts [15:30] facubatista was going to look into this.. we should be able to make this work as well [15:32] niemeyer: do you have a moment to go over StoredState? [15:38] I do in about an hour or so [15:38] niemeyer: works for me [15:52] niemeyer, regarding the "having shortcuts in events" I never got a conclusion, got lost in the code, I can hit another round to it [15:53] niemeyer, regarding current problem, was something else, really, funnily we could locate it after applying a patch from this branch https://github.com/canonical/operator/pull/195/files [16:06] facubatista: ping? [16:11] Chipaca, pong [16:11] facubatista: meet plz [16:36] hmm [16:36] niemeyer: not sure if you saw my ping, or if you replied [16:36] Chipaca, nothing from you since the "meet plz" to me and your "hmm" [16:36] something seriously wrong with my thunderbolt ethernet thing [16:36] facubatista: ack, thanks [16:36] niemeyer: ping :) [16:37] nice tracebacks in dmesg [16:42] Chipaca: Just grabbing a cuppa and will be with you [16:43] augh, machine is now all weird [16:43] rebooting, bbiab [16:46] > niemeyer: not sure if you saw my ping, or if you replied [16:46] Chipaca: There was no ping here, FWIW [16:46] niemeyer: yep, facu told me so (so then i pung) [16:47] Chipaca: I'm ready when you are [16:47] now back, without the thunderbolt ethernet [16:47] niemeyer: standup meet work for you? [16:47] Yeah [16:47] ok [17:01] facubatista: alright, I am done with the call. For the purposes of this charm, storing their names is enough. [17:02] Dmitrii-Sh, wonderful! [17:02] Dmitrii-Sh, I wonder about the other code, though, that was storing the apps [17:03] the idea there is that .serve needs to be called once per relation-id [17:03] Cory's original code did it by storing app names [17:04] so if you've seen an app already, you don't need to write relation data to your own unit's data bag multiple times [17:04] it is an idempotent operation which doesn't generate new -changed events for remote units if there are no data changes but this code seems to be avoiding extra relation-set calls [17:18] Dmitrii-Sh, facubatista thanks again! [17:18] karimsye, anytime! [17:29] niemeyer: I need to go make dinner, but thought I'd point you at this before going [17:29] https://github.com/canonical/operator/compare/master...chipaca:heritage#diff-6753f94ec7178cc8798db3ae94d35100R1255 [17:30] need to figure out what's going on, but if I uncomment that line, the test suite hangs [17:30] that's generally considered a bad thing [18:10] Chipaca: Test is creating a framework at the end without closing it [18:10] Chipaca: It must be hanging while opening the next framework, waiting for the former one to go away [19:30] niemeyer: THANK YOU, that was it [20:07] i think we should split the stored state out into its own file; it deserves its own book in the docs :) [20:10] EOD here [20:44] supermarket has changed its timetable, so that was a waste of time. I'll have to go tomorrow morning early. [20:49] jam, there I commented on #196 [20:51] jam, beyond that, we need to talk about _TestingModelBackend... some of its private structures (_relation_ids_map, _relation_names, _relation_list_map, _relation_data) are being used/changed from *outside* the class [20:51] jam, that could potentially lead to incongruences from those structures (like, a relation id being in _relation_ids_map but not in _relation_data) that would lead to super subtle bugs in the future [20:52] jam, it's kind of fine to have several structures to access the same "data corpus", but we need to ensure consistency [20:52] jam, anyway, let's talk on Thursday [20:52] (I'm off tomorrow) [20:53] so! see you all on Thursday! [20:53] * facubatista eods