#smooth-operator 2020-04-01 <t0mb0> Hey stub, in the old reactive way of doing things if I wanted to execute a piece of code that configures something I'd just set some reactive state and configure a @when handler. Is the right pattern in the operator framework to create a custom EventBase and then register it with framework.observe? Are there examples of creating a custom event that isn't due to a juju hook or relationship state change? <stub> One way is just to call the function directly. <stub> Otherwise, yeah, you define an event, register an observer, then fire the event when you want the observer to run <t0mb0> stub, right but atm I have a chicken and the egg problem where I need to wait for the pod to be ready before I can call the function <stub> I'm just deferring events in that case <t0mb0> stub, yeah that's what I'm doing too :( <stub> 'if not_ready: event.defer(); return' <t0mb0> yeah I'm doing that too but it means I have to pass this event object around <t0mb0> i'd much rather just just set some state then signal a new event <t0mb0> than have to pass the event around and have massive callbacks <t0mb0> er callstack even <stub> Why fire and event to trigger the observer, instead of calling it directly? <stub> oh, I guess you want to defer it there if things are not ready <t0mb0> yeah <t0mb0> I don't want to configure my application in the same function where I spin up the pod <t0mb0> I want to spin up the pod and issue a new event, say "init", then have my init handler just defer loop until the pod is live <t0mb0> I can get it to work, I wondered if there was a cleaner way than passing the even down the stack <stub> https://pastebin.ubuntu.com/p/pgGQKdkFVb/ is roughly how it should go <stub> It gets more complex if you want to add behavior to your events or extra __init__ arguments <stub> https://pastebin.ubuntu.com/p/RHMcTQPJBv/ sorry (typo) <t0mb0> stub, so line 19 is where I am signalling an on.ants_in_my_pants event? <stub> Yup <t0mb0> awesome thanks! <stub> So the emit would happen after the pod_set_spec <Chipaca> good moring! <Chipaca> morning, also * Chipaca decides the typo is ominous and cancels the whole day <jam> at least it wasn't "mourning" <Chipaca> indeed <niemeyer> Good morning * niemeyer reads stub's pastebin and notices facubatista recent change will break people up <niemeyer> Events was renamed to EventSet <Chipaca> niemeyer: which pastebin? <niemeyer> Chipaca: There was a conversation here in the channel shortly before you joined <mthaddon> https://pastebin.ubuntu.com/p/RHMcTQPJBv/ <mthaddon> ^ that's the one <Chipaca> ta <jam> niemeyer, actually it doesn't break that code, as it is using CharmEvents and EventBase which didn't change. <jam> but yes, custom events on types that use EventsBase break unless we do something like "@deprecated\nEventsBase = EventSetBase" <niemeyer> jam: If it didn't change it must change, or we need to revert it.. CharmEvents needs to be CharmEventSet <Chipaca> niemeyer: https://github.com/canonical/operator/pull/191#issuecomment-605875917 <niemeyer> Otherwise we're neither here nor there <niemeyer> Chipaca: Well, yeah, that sounds like an awkward non-agreement <niemeyer> Chipaca: You raised the inconsistency, jam said he liked the old naming better, facundo merged it <t0mb0> I have a requirement to store some secrets, previously we stored them as files inside the leader pod but I'd like to change that so we store these as k8s secrets, does the framework support that yet? I see in this example charm the developer has written his own k8s API https://github.com/majduk/charm-k8s-mongodb/tree/b0e55efd53d38abe26ea9deb271af9922b88b238/src <t0mb0> will the StoredState support this kind of thing? <stub> Personally I'd revert given they are not sets (like set(), frozenset() or collections.abc.Set) <Chipaca> stub: the issue is EventBase vs EventsBase <niemeyer> stub: Yeah, I think we should just revert it.. it sounds like multiple people don't agree the naming is better <niemeyer> t0mb0: What API do you need to access? We definitely want to grow the supported APIs over time <stub> If we have CharmEvents, we can have ObjectEvents <t0mb0> niemeyer, the kubernetes secrets and configmaps <Chipaca> stub: ooo <Chipaca> niemeyer: wdyt? <t0mb0> we're initialising a wordpress deployment with a random password and it'd be nice if we could expose that so operators can retrieve it if the pod dies <niemeyer> Chipaca, stub: Sounds fine to me <Chipaca> PR coming up <Chipaca> niemeyer: another thing: the check for having multiple names for a single instance of a StoredState is bogus, never worked AFAICT, and is untested. Should I drop it, or change it to work and add tests? <Chipaca> this is for class C: foo=StoredState(); bar=foo <jam> Chipaca, did you see my suggestion? <Chipaca> jam: I did. I'll PR that up as well so we can discuss. <jam> (when iterating attributes, just don't exit on first discovery) <Chipaca> ah <Chipaca> no i did not see that one <niemeyer> Chipaca: I see it as preventing an obvious invariant from being broken.. in which case would it be fine to ignore the invariant being wrong? <jam> the real question is whether that actually works. <jam> niemeyer, the problem is that it doesn't actually find the invariant <niemeyer> jam: Invariants don't have to be looked for :) <niemeyer> jam: If the attribute name is wrong, this is all bogus.. <niemeyer> jam: But I agree with your statement, we might look further to detect a bug <Chipaca> so I can change the code to not break out of the loop, and if it finds itself twice it'll catch that <Chipaca> I'll do that, and then run to the shops <stub> Can we have relids being relname:16 format again? I just spent ages tracking down a bug, where I was failing to retrieve data by relid because somewhere thought it should be str(15) instead of int(15) <Chipaca> I'll look at @deprecated after <mthaddon> t0mb0: essentially we're talking about interacting with kubernetes secrets here (being able to store and retrieve them). I think filing an issue about that be the best way to go for now <niemeyer> stub: The relation ID is really the number.. the name is there to aid people when they are in a hook context. But in Python we have both the name and the scottashley@example.org. <niemeyer> mthaddon, t0mb0: +1 <niemeyer> And also +1 in supporting the API internally somehow * mthaddon nods - sounds good <stub> yeah, and and now I need to ensure nothing casts it to a string or it becomes a different key according to dictionary lookups. Just stuffing in a pile of asserts to see what fed me a string instead of an int. <niemeyer> stub: Good question.. please let me know when you find out. If we have other APIs manipulating these IDs independently, we definitely need to look into it. <t0mb0> stub, do you know if there is a maximum number of times an event will be reemitted? * stub shrugs <jam> t0mb0, under what circumstance? as in if you keep defer() then will it even not reemit ? <niemeyer> stub: This should only be an issue if you're running juju CLI commands next to the framework, in which case the answer would be to use the APIs in the framework <niemeyer> We'll soon start talking directly to the agent, and allow iterating over multiple hook events if desired.. this will not work if one is interacting with the agent externally <t0mb0> jam, so i'm basically defer looping until self.model.get_binding('website').network.ingress_addresses[0] returns something <t0mb0> I'd expect juju debug-log to be spamming my logger statements but it doesn't <jam> t0mb0, the only thing that causes deferred events to reemit is another hook invocation. if you are deferring and then exiting the hook, you are waiting for the next hook to tell you to check. <jam> that hook might be 'config-changed' or 'update-status', but nothing that would 'spam the logs' as update-status is on a 5-min timer. <t0mb0> oh <stub> t0mb0: (and you won't see log.debug() level messages atm if they are missing entirely) <jam> t0mb0, generally state that you read from Juju won't change during the lifetime of a hook anyway (otherwise hooks have to check at the end of the hook if something is different than at the beginning of the hook) <jam> juju generally fires another hook to let you know if stuff that you responded to changed (eg, you get 'config-changed' when config is different than the last time config-changed ran) <t0mb0> jam, so if I'm waiting for my pod to be active before resuming the rest of the config-changed stuff what should I be doing? <jam> (also, I think set-pod-spec doesn't take effect until the hook exits, either, but I know there was some question there) <jam> t0mb0, so I thought there was supposed to be an event if your IP address changes, which might match what you're hoping to do? <jam> Juju doesn't currently feed the charm operator the full k8s events, IIRC <stub> Sounds like check if things are ready in a config-changed hook, and if not just wait for the next config-changed hook <t0mb0> jam, https://github.com/johnsca/charm-gitlab-k8s/blob/master/src/charm.py this charm makes use of a custom HTTPInterface is that what you're talking about? <t0mb0> stub, but there won't be another config-changed hook right? because you're returning from the on_config_changed function? <t0mb0> unless you do a self.on.config_changed.emit() or something similar? <t0mb0> if the state isn't set <stub> There will be another config-changed hook when something changes that Juju wants to tell you about. So if it is sending one when the pod IP addresses change, you can wait until that config-changed hook is fired. Might not be the first, won't be the last. <niemeyer> t0mb0: Also, this is normal code.. you don't have to (and most likely should not) have all of your logic living _inside_ the on_config_changed method <niemeyer> Instead, ideally code should be organized in methods and types that properly reflect what they are doing, and then you simply call them by their name whenever an appropriate situation shows up <t0mb0> niemeyer, that's how I roughly have it now, I'm checking to see if various variables on my state object are set and calling methods when they are and returning an event.defer() if they're not <niemeyer> t0mb0: Yeah, if I get what you're saying that's a pattern I've seen repeatedly in charms, but it doesn't feel like a very nice way to organize code in general.. what I've seen looks like: <niemeyer> 1) Ensure everything in the state and in the machine about everything the charm could possibly want to do is as it should be <niemeyer> 2) Do everything the charm possibly would like to do based on other checks previously done <niemeyer> The problem with this approach is that those several checks are detached from the code they actually "protect", and local encapsulation is broken down <niemeyer> A much better approach is to do things in smaller pieces, with local information.. for example, we want small components that are responsible for their own knowledge, and emit high-level events that are meaningful for what they do <t0mb0> niemeyer, so in that case I'd need k8s to tell Juju the pod is alive and ready so I can observe that event right? <niemeyer> t0mb0: Yes, that definitely sounds like something we want to have more comfortable access to <t0mb0> niemeyer, i'm guessing that doesn't exist yet and I should raise an issue? <niemeyer> t0mb0: Yes please.. but also, meanwhile let's find a nice way for you to move forward with your needs without being blocked on that issue <t0mb0> I feel like for initial setups of wordpress we don't mind waiting 5 minutes or so for another hook to run <niemeyer> t0mb0: Also note that what I described above says more about the overall organization of the charm itself than about external needs <niemeyer> t0mb0: In other words, improving things in that way doesn't need external events <t0mb0> niemeyer, yeah it makes sense. that's why i asked (s)tub about writing custom events earlier <niemeyer> t0mb0: Right, we made events very easy to create and use precisely so we can reuse that form of encapsulation into our own custom components as well <niemeyer> t0mb0: As a random example, it would make sense for example to have a MySQLServer endpoint handler that would consume the relation events, and emit higher-level on_database_available, etc <niemeyer> I'm somewhat surprised that the k8s support in juju doesn't inform the charm of the pod being ready.. I wonder what kind of pattern people have been using so far <niemeyer> Do people just fork off a daemon that keeps trying to do things with it? <niemeyer> I will be digging further into k8s in the coming days <t0mb0> niemeyer, it seems like the existing charms shy away from doing application initialisation and it's left to the user. I imagine you could bake initialisation logic into the docker image and then have some kind of lock to stop non leader pods from racing <stub> So.... using the relation id as a dictionary key isn't great if you are going to convert it to json, cause it will be cast into a string and won't round trip <niemeyer> stub: Where's the json conversion taking place <niemeyer> ? <stub> my code <stub> stuffing a data structure into leadership settings in this case <niemeyer> I see <stub> >>> assert json.loads(json.dumps({1: True})) == {"1": True} <stub> solution... stick to yaml :) <niemeyer> stub: Yeah, I'm surprised the json package doesn't complain about it <niemeyer> "Oh, sure.. here is your data completely different now sir." <niemeyer> t0mb0: I'd like to significantly improve the control charms have over their workload containers <niemeyer> t0mb0: That's one of my key goals over the next couple of cycles <t0mb0> niemeyer, I mean something like an "pod-ready" event/hook would be nice <t0mb0> once the readinessProbe is green <niemeyer> I need to get my hands dirty to get a better view of how we'll accomplish that <niemeyer> t0mb0: Yeah, something like that as well <Chipaca> ok, now yes, to the shops * Chipaca really really needs to go <stub> git+ssh://git.launchpad.net/~stub/interface-pgsql/+git/operator seems to be working now if anyone wants a look. Unit tests next week. <stub> And the 'operator' branch of git+ssh://git.launchpad.net/~stub/plinth/+git/plinth-charm is a (private) non-k8s operator framework charm using it, still needing its actions wired up and a tidy * stub wanders off into the long weekend <niemeyer> stub: Thanks for the branches, and have fun! <niemeyer> stub: Or have a good isolation I guess :) <stub> I'm shopping for everyone tomorrow... got to keep my ancestors fed. <niemeyer> stub++ <Chipaca> jam: was @deprecated a suggestion to use https://pypi.org/project/deprecation/, or was it something else? <jam> Chipaca, I think I looked at 2 or 3 different ways of doing deprecation. It feels a bit overdone to do deprecation before we hit a 1.0 but certainly I do think we want a *plan* around deprecation. <jam> If we feel we need it from now, then we can <jam> Chipaca, given the simplicity of deprecation, it would be nice to not need an external lib <Dmitrii-Sh> Added unit tests for a peer relation to one of my charm PRs: https://github.com/canonical/cockroachdb-operator/pull/1/commits/95c61ebf40eaf812eb46e4b90055f47f8545704e (they need PR #196 to be merged to pass because of the issue #202) <Dmitrii-Sh> https://git.io/JvdO4 - TestCharmClass here is something worth discussing I think. The peer relation interface code expects a parent charm class to provide an event so that it can expose cluster id details on a relation when it happens. However, using the concrete CockroachDBCharm type in interface-specific unit tests is problematic because <Dmitrii-Sh> interface component events can trigger CockroachDBCharm handlers that do real work (such as writing config). Instead of using mocking, I am using CharmBase here but with an event set for CockroachDBCharm. <Dmitrii-Sh> This provides me with a way to still trigger CockroachDBCharm-specific events for the interface code to react to but without invoking the charm logic I don't need. <jam> Dmitrii-Sh, so why isn't the code that depends on an event/generates an event a separate class? it doesn't have to be, but it might be interesting to think about keeping the Charm as only generating Juju events, and having something else that generates higher level events. <Dmitrii-Sh> jam: *thinking about how to rework it*. The first impression is that I would still have to avoid triggering handlers but only on a different type. <Chipaca> so many reviews to do <Chipaca> so little coffee <Chipaca> also, PEP 9999 <jam> Chipaca, so we should use foo,bar,baz,quux as animal-friendly alternatives? <Chipaca> jam: I say we go anti-pep-9999 and make all our variables refer to animals being mutilated in 'orrible ways <jam> deadbeef we have <jam> slaughterhouse <Chipaca> 'foo' can become 'gib' <jam> Dmitrii-Sh, so I would think you'd want abstractions for places that your code interacts with "other things" like the filesystem, and then in tests you would substitute an alternate implementation of those that doesn't actually interact with the system. <Dmitrii-Sh> jam: ty, I see what you mean. The charm itself would only poke an implementation of this abstraction which I would replace in the test in some way. <jam> right <jam> certainly you don't want your actual charm rewriting /etc/postgresql.conf while you are testing. <niemeyer> Is that PEP-9999: https://www.blackcomb-shop.eu/en-PT/skirt-craft-1904867pep-9999black/ <niemeyer> It's the first hit on scottashley@example.org.. <niemeyer> It's also no longer available, so we don't need to worry <Chipaca> niemeyer: https://github.com/gerritholl/peps/blob/animal-friendly/pep-9999.rst <jam> http://www.montypython.net/scripts/fruit.php "Self Defense Against Fruit" I don't know that one nearly as well. <Chipaca> niemeyer: aw, but bugs with side effects are my favourite! <Chipaca> they're the gift that carry on giving! <niemeyer> I know.. I also like how colorful they are :) <Chipaca> niemeyer: got a couple of questions about debug-hook, after talking with rick h and reading through the discussion on the spec <Chipaca> niemeyer: if you have 15 minutes to a half hour that'd be good <Chipaca> (non-overlapping holidays + feature freeze means I'm trying to get this sorted myself, rather than wait for facubatista to be back) <niemeyer> Chipaca: I'm just off a call.. if you have time in ~5 minutes, I'll prepare a mate and we can dive into it <Chipaca> niemeyer: sgtm <niemeyer> Alright <niemeyer> Chipaca: Standup? <Chipaca> niemeyer: omw <Chipaca> niemeyer: i'll ping you for a meet as soon as i get out of my next one <jam> Chipaca, oth <jam> Chipaca, anything I can hepl with? <Chipaca> jam: if you're around, you're welcome to join <Chipaca> jam: but it is rather late for you <jam> true enough, though I wouldn't worry terribly about having the Charmcraft code finished for Juju feature freeze. <Chipaca> jam: so I hear :) <Chipaca> jam, niemeyer, standup meet if you could <Chipaca> jam: very optional for you <jam> I can give it a few mins <niemeyer> I need a couple of minutes, but will be there <Chipaca> crazy day today <Chipaca> i got to do 0 of Dmitrii-Sh's reviews <Chipaca> i also got to run 0 <Chipaca> i'm going to go run because daylight is nearly over, then make dinner, then review some things <Chipaca> ttfn! <niemeyer> Enjoy! <Dmitrii-Sh> Would be good to get more eyes on this one - https://github.com/canonical/operator/issues/206. It seems to ask for `goal-state`-like functionality but I suggested a different idea. <Dmitrii-Sh> this seems to be a common problem when it comes to setting up stateful applications - you need to know whether to have one or multiple units at the beginning of a unit lifetime to initialize a cluster state from the initial cluster unit. In some cases this is merely about setting an initial replication factor to 1 vs 3 (or some other value). <jam> Dmitrii-Sh, I'm of the mind that we should let them know about it. But the other option is to have 'we've told you what we're going to tell you for now' events that let you make progress. <jam> eg, "start" will be called after all the relation-created <jam> so having some form of "we're done telling you about relation-joined for now" <jam> but relation-joined is blocked on the units actually coming up, and doesn't let you know what the admin's *intent* of deployment is <Dmitrii-Sh> jam: yes, it feels like a unit need reasonable checkpoints with enough data about the overall model to make progress. Most importantly, at the beginning of a lifetime where inital state is created. <Dmitrii-Sh> one other use-case that comes to mind besides clustering is setting up TLS <Dmitrii-Sh> at the beginning it is important not to have any exchanges without TLS between units if the end goal is to have TLS enabled <Dmitrii-Sh> you might later decide to remove a relation with a particular CA charm that generates certs/keys and choose another but this is unlikely to change quickly <niemeyer> There are different ideas intermixed here <niemeyer> We already discussed this (jam and myself) a while ago and we both seem to agree that having a number of units as a hint, and a created relation hook, sorts out a good part of the desire <niemeyer> We should avoid having something like goal state, though, as it creates a mixed and confusing reality for the unit <niemeyer> Another part of what's described above sounds like just configuration #smooth-operator 2020-04-02 <jam> mroning all <Chipaca> good morning! * Chipaca stayed up too late and now needs more coffee <Chipaca> is there a good reason for _is_our_unit and _is_our_app to be called like that, as opposed to something more unified like _is_ours (or _is_self)? <niemeyer> Morning Chipaca <niemeyer> Chipaca: Reviews mainly.. it wasn't easy to get to those names either :) <jam> morning Chipaca and niemeyer <niemeyer> "Is ours what?" ""self is something else" and it goes on.. <niemeyer> Morning jam <jam> Chipaca, niemeyer : if the parameter of 'juju debug-code' is now '--at', should the env var still be JUJU_BREAKPOINT? <jam> 'JUJU_DEBUG_AT' or something along those lines feels like it matches better. <jam> The doc is also now significantly out of date, and I'm not sure I was around for all the conversations. <jam> I pretty much just understood changing "juju debug-hook --breakpoint" to "juju debug-code --at" but the rest seem to stay the same. <niemeyer> Sounds reasonable and generic enough to be useful in multiple ways.. can't think of better alternatives right away <Chipaca> niemeyer: are you talking about AT or about BREAKPOINT there <Chipaca> can be read both ways <niemeyer> The suggestion <niemeyer> (AT) <Dmitrii-Sh> Chipaca: added a link to the spec doc here https://trello.com/c/vcZTwoOI <Dmitrii-Sh> I am going to update it with more details <niemeyer> We had an agreement to have 160 as the line length <niemeyer> I also asked to have "sandbox" into the git ignore list <niemeyer> Looks like both have been undone by 907672c0901168903bf17c94165b034881bd35bd <niemeyer> Looking at the diff here https://github.com/canonical/operator/pull/162/files <niemeyer> The new lines on the right are consistently less readable <niemeyer> https://usercontent.irccloud-cdn.com/file/5g77t1LD/image.png <niemeyer> Do you really prefer reading the one on the right? <niemeyer> There are several of those... <niemeyer> Can we please revert this PR? <Chipaca> niemeyer: 160 is much too wide <niemeyer> Chipaca: I don't care about the number as much as I care about the result <Chipaca> niemeyer: yes, i prefer the one on the right <niemeyer> We can make it 1000 is if it looks better <niemeyer> Chipaca: Really!? Are you seriously saying you read that completely broken up text and code better than what you have on the left side? <Chipaca> niemeyer: and sanbox is still in git ignore <Chipaca> niemeyer: it's unclear what sandbox is in the gitignore for, but it's still there <Chipaca> without a comment or anything it's a prime candidate for cleanup <niemeyer> Chipaca: Both the line length and sandbox have been discussed before you were in the team <niemeyer> Chipaca: The line length is 160 precisely so that it reads better.. we got several examples and decided that in practice it ends up looking nicer with longer lines than with arbitrarily broken down indentations <niemeyer> Chipaca: The sandbox is a directory I personally use to put things that should not be in the tree <jam> So for my personal use, I prefer narrower than 160 for line lengths, as I tend to have 2 or 3 files open concurrently (one for the code, one for the tests of the code, sometimes another as a reference for the code I'm implementing) <jam> https://www.python.org/dev/peps/pep-0008/#maximum-line-length itself recommends up to 99 chars, <jam> I don't prefer the 'indent to where the (' opens, as it tends to waste a lot of horizontal space <niemeyer> jam: How do broken down lines look like in your terminal? Does it fit more than 99 chars, or is it broken down further? <jam> so I tend to do the return immediately after the (which causes it to indent just a single extra indent level) <niemeyer> jam: Because reviews look terrible.. it's neither 99 nor 160 <niemeyer> jam: Which means we get the manual line breaking, and then the enforced breaking on top of that either way <niemeyer> jam: (as we had discussed many months ago) <jam> I believe the actual line length discussion predates me as well. <jam> In my editor, it tends to horizontal scroll, in a terminal, it wraps the line <niemeyer> jam: Or maybe it just wasn't interesting enough to be talking in the channel.. ;) <niemeyer> jam: Okay, so 99 will *also* wrap and horizontal scroll.. <niemeyer> Yeah, just barely.. conversation happened on Oct 3rd, you joined on Oct 2nd <jam> fair enough. I remember it being 160, but certainly didn't have a strong rationale for defending it staying there. <niemeyer> It's okay.. the rationale is not super insightful other than we looked at code and it looked worse when indenting too much <niemeyer> Reading the delta seems to confirm that still.. <niemeyer> Sometimes it's like this: <niemeyer> https://www.irccloud.com/pastebin/LDdXKI2x/ <niemeyer> Sometimes like that: <niemeyer> https://www.irccloud.com/pastebin/nIyHc8G3/ <niemeyer> And sometimes this: <niemeyer> https://www.irccloud.com/pastebin/1ex3TAbJ/ <niemeyer> Versus stuff like this: <niemeyer> - raise RuntimeError("StoredState shared by {}.{} and {}.{}".format(parent_tname, self.attr_name, parent_tname, attr_name)) <niemeyer> https://www.irccloud.com/pastebin/cy32xfyL/ <niemeyer> I easily prefer the latter, maybe because I'm unusual in that I don't appreciate jumping my eyes around with no semantic involved and do appreciate its tightness. We can discuss further in the standup. <jam> so personally, while I don't prefer arbitrarily wrapped differently each time, I do prefer the wrapped form. I think it is because it decreases the linear distance <jam> I think there was also an issue with github rendering, at least IIRC that is what Chipaca put the PR up for. <niemeyer> The screenshot I first posted above was from GH <facubatista> Muy buenos dÃ­as a todos! <facubatista> So, you decided to go -vvvvvv mode the day I was off? :p * facubatista has a lot of stuff to read <Chipaca> facubatista: it's _because_ you were off <Chipaca> facubatista: it's you that's been inhibiting us * Chipaca just out of a meeting and needs to read up <facubatista> :) <jam> I run a 27" monitor, and while I can go wider, I tend not to maximize browser windows because they are too wide. <jam> In my normal width: https://imgur.com/cSVJFWU the old one wraps but the new one doesn't. <niemeyer> jam: To be clear, my point was never that the monitor was wide enough to fit 160 chars <jam> If I max the window, neither wraps https://imgur.com/kfjK3IR <jam> but I actually find it hard to review at that size, because the two sides are too far apart <jam> it is hard visually to follow from left to right for me <niemeyer> jam: The point is rather that this is not a great way to force the formatting of code, because in practice it ends up looking worse in multiple places <jam> and if I make it narrow https://imgur.com/1YEpDMK it looks awful in the new ay <niemeyer> jam: Extremely long lines can usually be reorganized to not be extremely long by using proper coding practices <niemeyer> jam: And we can always *choose* to break lines down where really appropriate <niemeyer> jam: What the limit does is to force people to break them down whatever it takes.. and we end up with the stuff mentioned <niemeyer> That's subjective, though.. so let's just raise that up in the next standup and see where the team stands <niemeyer> jam: This "narrow" view is actually what I get naturally over here, and I'm on a high-resolution (4k) 14 inch monitor <jam> for whatever reason my 'natural' window size is about 1200x1200 pix. it fits most content cleanly (bugs on launchpad, gogole docs, etc) <niemeyer> My resolution is 2560x1440 (2k actually) <niemeyer> I use a tiling WM, but for that sort of content the window is always full screen <jam> Chipaca, facubatista : I tried to encapsulate the discussions that happened yesterday around "Debugging live charms" and brought up a couple more questions <jam> https://docs.google.com/document/d/1zKwzo26bNVFgohaRB_kdUCsFPY3OcjfcpQklGpkOHpc/edit# <jam> I also have 'juju debug-code' sketched up, testing it now. <facubatista> jam, will go there when I finish all the reading here, 5' <facubatista> I don't like this breaking lines style: <facubatista> some_call(foobar, <facubatista> other, <facubatista> not only because the totally wasted space there, but also because it's indented to (75% chance) a weird column <facubatista> and pathologically leads to code like this: https://pastebin.canonical.com/p/fb7XW8yJWx/ <facubatista> niemeyer, for me code at the left here is totally broken: https://usercontent.irccloud-cdn.com/file/5g77t1LD/image.png <niemeyer> facubatista: Broken in which sense? You're probably not using that word in ways I normally read it :) <niemeyer> Dmitrii-Sh: https://github.com/canonical/cockroachdb-operator/pull/1 has an initial review <facubatista> the idea is also to not see it as the right, never, that's why a sensible value for column limit like 99 is thought to have it not too wide to have those line breaks <facubatista> niemeyer, I mean, my eyes can not parse that; the moment I have a line that wraps and gets too long, I lose the identation <niemeyer> facubatista: Agreed.. and that's my main concern, to be clear. 160 is an arbitrary number to prevent such formatting.. I don't have anything locally that would make 160 special. <niemeyer> facubatista: But, that formatting in the right is what I consistently get.. <niemeyer> I probably need a wider monitor, which is ironic given that I'm the one advocating for wider lines <Dmitrii-Sh> thanks, also submitted the tls-certificates interface for review https://github.com/canonical/operator/pull/210 <niemeyer> ANyway, lunch time here <niemeyer> And we have a meeting in just a few <niemeyer> See you soon <Dmitrii-Sh> need to figure out what to do about python-cryptography for pull/210 <facubatista> aghhh, canonical's pastebin messes with the identation <facubatista> niemeyer, btw, what you see on the right there is also because it's indented where the parenthesis break! that should be like this: http://linkode.org/#A3zKfjmPofCP2jgBR5eo5 <facubatista> (or like the second version there) <jam> facubatista, I also prefer that wrapping <facubatista> niemeyer, stub, regarding the surprising json key: https://bugs.python.org/issue34972 <jam> facubatista, "its how the format works", in that it doesn't support ints for keys, not that it treats ints as strings... <jam> IMO <facubatista> jam, yes, it was surprising for me too! I actually found the issue when going to report the issue, a couple of months ago :) * facubatista had it fresh because of that <facubatista> jam, thanks for the massive editing in the Debugger doc <jam> facubatista, It ended a bit ugly, but I was hoping to show the changes as discussed, and make sure it was clear what was changing. <facubatista> jam, yes, it's way better for me to see that, instead of having to read everything and diff current text to what was in my memory <narinderguptamac> Hello all <facubatista> hello narinderguptamac <narinderguptamac> facubatista, was taking to Dmitri about peer relation and got it cleared that any peer event will fire after start event <facubatista> narinderguptamac, nice! * facubatista -> lunch <crodriguez> hello hello <crodriguez> anyone tried to attach secrets with this new feature from juju 2.8? https://discourse.juju.is/t/k8s-spec-v3-changes/2698. It doesn't work for me and I'm unsure if it points to juju or the framework itself <crodriguez> also, is there an example somewhere with the import of the k8s yaml manifest in the charm.py, instead of having to write the spec in a yaml format ourselves? I believe calvin did it for his cdk-cats charm, but I don't think it was fully functional <Chipaca> crodriguez: o/! <Chipaca> crodriguez: we haven't done the work to support the new secrets thing AFAIK <Chipaca> crodriguez: we will shortly <crodriguez> Chipaca: aah ok then I'll wait before integrating that. Thanks! Is there a newsletter / discourse / etc. where updates, new features, etc are published? <jam> Chipaca, crodriguez : so any pod spec that they give us we pass on through, I think you're crossing 2 different things. <jam> crodriguez, the first thing that stands out from the thread is making sure your pod spec is version 3, beyond that *I* don't know what the values of the fields should be. <jam> crodriguez, the actual bug you have is in your code, though <jam> application-mssql: 10:15:33 DEBUG unit.mssql/0.config-changed if self.state.spec != new_spec: <jam> that sais you are looking at 'self.state.spec' but you never initialized the field <jam> crodriguez, you'll want to have something like "self.state.set_default(spec=None)" in your charm's __init__.py <jam> sorry def __init__(): <jam> crodriguez, I'm actually past EOD here, but if you can put your actual code somewhere we can give you feedback on it. <crodriguez> thank you jam! I'll look into the init section of my charm... it might be missing things related to updates have been made to the operator framework. My code is here https://github.com/camille-rodriguez/charm-mssql/tree/add-secret , feel free to look at it tomorr <crodriguez> tomorrow*. I'll check the state of the spec like you mentionned <Chipaca> crodriguez: what is _write_state in that code? <jam> crodriguez, yeah, I think you used to have initialization in _write_state that was called by on_install, but when you got rid of that, you removed your initialization <crodriguez> mhh cause on_install wasn't working until recently. Most of that code was written with the operator framework in late december/early jan, and I'm getting back to it now. I need to clean the on_start if on_install is the right way to go now <jam> crodriguez, so in __init__ having it do self.state.set_default() is the recommended way <jam> that way it is guaranteed to happen even after an upgrade/etc. <crodriguez> okay I'll add that <jam> but also won't ever overwrite the values that you've actually set. <Chipaca> jam: you go have a good evening now :) <jam> Chipaca, well, I have to hang around to get my PR reviewed. Might as well offer some tech support. <Chipaca> aw <crodriguez> mmh "AttributeError: attribute 'set_default' is not stored" <niemeyer> defaultS? <niemeyer> Hi Camille :) <jam> niemeyer, crodriguez : its 'set_default' but it sounds like you have an out-of-date copy of ops <niemeyer> I'm not 100% sure as it's been a few weeks, but that's what came to mind when you mentioned the error <jam> if you're using submodules you'll need to 'git pull' in the subdir. <niemeyer> Ah, and jam just proved me wrong.. efficiency! :) <crodriguez> in the operator folder? it doesn't have info on the upstream repo <crodriguez> I thought it was a command like update submodules or something similar.. <Dmitrii-Sh> crodriguez: if you have submodules in your repository, you can use this to update all of them: git submodule update --recursive --remote <Dmitrii-Sh> but a prerequisite to that is actually adding it <Dmitrii-Sh> git submodule add https://github.com/canonical/operator mod/operator <jam> Dmitrii-Sh, ah, the --remote is the trick that I didn't realize so always just did 'git pull' which worked for me. <crodriguez> okay that recursive update command worked, thanks. I moved on to another bug with the framework haha <crodriguez> well with my code <Dmitrii-Sh> jam: yes it's not very intuitive which reminds me of https://imgs.xkcd.com/comics/git_2x.png <Dmitrii-Sh> crodriguez: what is it? Can I help? <jam> Dmitrii-Sh, git submodule update --help isn't particularly... helpful <Dmitrii-Sh> jam: heh, it's rather "--quiet" <crodriguez> Are we supposed to use the on_install or the on_start hook now? or both? install being for the first deploy, and start for if you restart the nodes? <crodriguez> so if I change my pod_spec version to 3, the 'rules' and 'global' keywords are not recognised anymore. Is that from a change in the framework or in juju? <crodriguez> lol sorry I'm bombing y'all with questions <Dmitrii-Sh> https://bugs.launchpad.net/juju/+bug/1854635 It's still "start" for now while we use 2.7. This fix was committed to 2.8 for k8s charms to get the install event <Dmitrii-Sh> but then "dispatch" will also be in 2.8 https://github.com/juju/juju/pull/11257/files#diff-95b793849a6a72c6122da04f68ecb586 <jam> crodriguez, so the framework makes use of on_install to do some initialization stuff, but I wouldn't think you have the information you need to start a pod until after config-changed, and probably around start. <jam> Dmitrii-Sh, I actually thought that landed in 2.7.4, doesn't look like the bug was updated. <jam> "bake and land in 2.7.3" is pretty far away from 2.7.4/5 so not sure where its at. <Dmitrii-Sh> jam: hmm, I looked at #5 https://bugs.launchpad.net/juju/+bug/1854635/comments/5 for reference, haven't looked at the actual commits <Dmitrii-Sh> crodriguez: regarding keywords: you probably mean "roles". There's an example of a spec here in "QA steps" https://github.com/juju/juju/pull/11293 - how different is it to what you have? <crodriguez> spec = { <crodriguez> 'version': 2, <crodriguez> 'serviceAccount': { <crodriguez> 'global': True, <crodriguez> 'rules': [ <crodriguez> { <crodriguez> 'apiGroups': ['apps'], <crodriguez> 'resources': ['statefulsets', 'deployments'], <crodriguez> 'verbs': ['*'], <crodriguez> }, <crodriguez> { <crodriguez> oops <crodriguez> so in this spec, if I change version to 3, I get this : <crodriguez> https://www.irccloud.com/pastebin/2qFlNy81/ <crodriguez> same for unknown field 'rules' <jam> crodriguez, offhand I don't think you're allowed to set global service accounts in a charm. They effect people that aren't in your namespace <jam> I'm not the one who has the definitive answer to that, though. <jam> I know there were discussions <Dmitrii-Sh> the error seems to be coming from the framework though <Dmitrii-Sh> b'ERROR json: unknown field "global"\n' <Dmitrii-Sh> hmm, actually that doesn't look like something we'd emit <Dmitrii-Sh> I just tried using that spec as an input in a unit test and it didn't error out (because we just write to a file) <crodriguez> Hmm. Okay, I'll look more into that. I need to step out for an hour, will circle back about this. Thanks everyone! <Dmitrii-Sh> version: 3 <Dmitrii-Sh> serviceAccount: <Dmitrii-Sh> automountServiceAccountToken: true <Dmitrii-Sh> roles: <Dmitrii-Sh> - global: true <Dmitrii-Sh> rules: <Dmitrii-Sh> - apiGroups: [""] <Dmitrii-Sh> resources: ["pods"] <Dmitrii-Sh> verbs: ["get", "watch", "list"] <Dmitrii-Sh> - nonResourceURLs: ["*"] <Dmitrii-Sh> verbs: ["*"] <Dmitrii-Sh> crodriguez: it seems like you need to have "global: true" under roles <Dmitrii-Sh> and then rules under roles as well <jam> Dmitrii-Sh, ModelError(e.stderr) means it is coming from Juju <jam> Dmitrii-Sh, hm, seems v2 is quite a bit different than v3. https://discourse.juju.is/t/updated-podspec-yaml-new-features/2124 <jam> vs https://github.com/juju/juju/pull/11293 <Dmitrii-Sh> jam: I think charms need to specify min-juju-version: 2.8.0 and use a v3 spec if they support v3 only * facubatista is back <Dmitrii-Sh> but this is slightly unfriendly to a user <jam> Dmitrii-Sh, so AIUI, you can stick with V2, but the goal is to get to something we want to recommend quickly vs a lot of polish on migration <Dmitrii-Sh> jam: agreed, there is nothing restricting the usage of v3 via pod.spec_set AFAIKS <jam> Dmitrii-Sh, we need to add 'k8s-spec-set' to the list of 2.8-isms. btw <jam> Chipaca, ^^ juju is recommended a different name for setting the definition of your pod in 2.8 <Dmitrii-Sh> jam: right, the command got renamed <facubatista> is there a reason we're not sending DEBUG logs from the framework/charms to juju <facubatista> ? <niemeyer> I don't know what behavior you are observing, but the goal is to make debug logs visible if the user asks to see debug logs <Dmitrii-Sh> I originally wanted to use DEBUG: https://github.com/canonical/operator/pull/133/commits/2c778fc3f128b088790229bc3cc55bf7f7daedd6#diff-a76991ba26c9c9b312be489400d3f481L21 <Dmitrii-Sh> IIRC the reason not to use it was to encourage people not to use "debug" for everything. It would be nice to retrieve the debug level from the level set for a given model but there's no way to do that yet https://bugs.launchpad.net/juju/+bug/1861987 <niemeyer> What we don't want is for the norm to be seeing debug logs at all times, otherwise what we have is really info logs <niemeyer> I see debug logs as a fire hose, where even the smallest detail is fine <niemeyer> That's not the kind of thing that should be on by default.. if it is on, we'll learn not to log things at all to not overload people with boring details, and we no longer have debug logs <facubatista> niemeyer, agree with you, that's why I wrote in https://github.com/canonical/operator/issues/198 that the logger needs to be in INFO by default (the user can easily change that) -- the problem is that the *handler* is in INFO, and we should be in DEBUG so if the logger is set to debug, those messages will reach Juju <facubatista> *it* should be in DEBUG... <niemeyer> Again, I'm not claiming it's correct.. just that this is what I hope we get <Dmitrii-Sh> I think we should change the JujuLogHandler level to DEBUG. Otherwise even if one overrides the root logger level in the charm, the messages are still filtered at the handler level <Dmitrii-Sh> that would be changing the JujuLogHandler constructor to be like this: def __init__(self, model_backend, level=logging.DEBUG): <facubatista> Dmitrii-Sh, yeap, that's what issue #198 talks about <niemeyer> Tentatively calling it a day.. o/ <tvansteenburgh1> Hi everyone. The Canonical k8s team will soon publish (as a preview release) our first k8s charm based on the operator framework. Source code is at https://github.com/charmed-kubernetes/charm-multus. Feedback welcome. <Chipaca> tvansteenburgh1: ack <Dmitrii-Sh> crodriguez: the charm tvansteenburgh1 has a v3 spec example ^ https://github.com/charmed-kubernetes/charm-multus/blob/master/src/charm.py#L87-L125 <Dmitrii-Sh> seems to confirm the theory I had <crodriguez> Dmitrii-Sh: thanks for your help. I will compare and adjust to use the v3 * facubatista eods <Dmitrii-Sh> https://github.com/canonical/operator/issues/211- found during testing <Dmitrii-Sh> https://github.com/canonical/operator/pull/212 - would be helpful for progressing #210 #smooth-operator 2020-04-03 <Dmitrii-Sh> o/ <jam> Dmitrii-Sh, morning. (I'm not really here, but there were comments left by someone on https://github.com/canonical/operator/pull/212) <Dmitrii-Sh> jam: thanks, just responded there <Dmitrii-Sh> I think extending add_relation by allowing it to accept initial data would be a good way to move some checks away from update_relation_data and make it less overloaded than it is right now <jam> Dmitrii-Sh, maybe we have both options written out and can compare the tests that result and what we feel is easier to understand <Dmitrii-Sh> jam: ok, I'll make a branch locally so that I have the current implementation preserved <Dmitrii-Sh> and see what I can come up with <jam> I had initially done the initial_app_data form, and then was trying to streamline and saw that you could use update for self. but I wasn't convinced which way would be better. <Dmitrii-Sh> jam: also, if I look at this signature `def add_relation(self, app, *, initial_unit_data=None, initial_app_data=None, remote_app_data=None):`, it gets confusing on which argument is for which purpose for peer relations <jam> app is redundant for peer but the rest isn't <jam> I would say for peers we just make one of them illegal to set <Dmitrii-Sh> hmm, there is a case where our unit comes app and there's already something in peer app relation data <Dmitrii-Sh> just thinking about a later addition of `relation-created` events - add_relation should trigger "relation-created" for our unit but during that there may already be some app relation data present for that relation-id and a "remote app" (which may be local on a peer rel) <Dmitrii-Sh> I think it should be illegal to use the `remote_app_data` argument for peer relations - I am going to raise an exception if that happens and see how it looks like <t0mb0> Hi I've been struggling with some issues with actions. It seems every time I tried to retrieve some data from StorageState with my action I keep getting the initial value of my variable, however this variable gets populated with some data as part of the configuration process and I can confirm the variable is set. It seems that when I call my action it is referencing a different state object?? https://pastebin.canonical.com/p/jWT6bDcRVR/ <t0mb0> am I missing something ^^ or should I raise a bug <jam> t0mb0, can you link the code? <t0mb0> jam, https://pastebin.canonical.com/p/7BnhqRxDfG/ <jam> t0mb0, so #1 i would suggest moving from an on_start setting all the attributes to a 'set_default()' call in __init__ <jam> t0mb0, the on_start doing apt-get stuff is fine <t0mb0> jam, I had done a set_default for in __init__ for the data i'm trying to retrieve <jam> t0mb0, that isn't the code you linked <jam> hard to debug code that isn't what I'm looking at :) <t0mb0> jam, yeah I know, I'm just saying it's something I've already tested <t0mb0> I've been trying all number of things to try and isolate the issue :P <t0mb0> I'll move it back to __init__ though <jam> t0mb0, ah, you're on k8s, right? Charms fire hooks in the Operator pod, but fire actions in the Workload pod. <jam> (currently) <jam> I believe there is a flag <t0mb0> :O <t0mb0> I am in k8s yup <jam> t0mb0, sigh, the flag is only in dev mode. So if you set "export JUJU_DEV_FEATURE_FLAG=juju-v3" then "juju run-action" is renamed to "juju run" and has a "juju run --operator" to run the action in the operator pod. <jam> t0mb0, I'd bring that up in #juju about not being able to supply --operator to 'juju run-action' <t0mb0> jam, does this work with microk8s juju? <t0mb0> I don't seem to have that flag when I export the variable there <jam> t0mb0, it might be a 2.8 feature, not sure. <jam> t0mb0, with any k8s <t0mb0> hmm <jam> t0mb0, submitted https://bugs.launchpad.net/juju/+bug/1870487 for you. <t0mb0> thanks! <t0mb0> jam, I do have juju run --operator <t0mb0> but I'm not trying to run a shell cmd, I'm trying to run a charm action <t0mb0> ah - on another read of that bug report you're saying there is no reason not to also include --operator to "run-action" <jam> t0mb0, ah, it was juju run that grew --operator, not run-action <jam> t0mb0, bug update <jam> updated <t0mb0> jam, shouldn't state be the same whether it's running in the operator pod or on the workload? <jam> t0mb0, state is read from the local disk atm, no way to read it from the other pod <t0mb0> jam, so if the operator pod was to die then you'd lose that state? <jam> t0mb0, operator pods are currently stateful stets <jam> sets <jam> t0mb0, so they ask k8s for a disk mount that will float with a restart of the pod <jam> t0mb0, in 2.8 there are some hook tools that will let us save state back in the controller, and work that would allow operator pods to become stateless <jam> the operator framework doesn't support that yet. its on our 'todo next' list <t0mb0> jam, I also noticed that if I initialised admin_password in on_start, then my action was able to read it, it was just when that value was overwritten in the configure hook <jam> t0mb0, once you declare a pod spec, the Juju code copies the charm in an init container to the application pod <jam> t0mb0, currently the charm state is stored in the state dir, causing it to get copied <jam> but that is 'state as it exists at one point in time' <jam> which is naughty of how it is all done and definitely shouldn't be relied upon <Dmitrii-Sh> that also has an effect of not being able to update StoredState from actions such that regular hooks can see those updates <Dmitrii-Sh> as jam says, it should work when we have controller-side state storage used in the framework with juju 2.8 <t0mb0> am I correct in understanding that by emitting an event like this https://github.com/dshcherb/cockroachdb-operator/blob/master/src/charm.py#L192 emit() will block or should it be asynchronous? <Chipaca> mo'in <niemeyer> t0mb0: That call dispatches the event to handlers <niemeyer> Morning <t0mb0> niemeyer, so if I was to do something like this https://pastebin.canonical.com/p/hpzGSrTSGh/ it shouldn't cause a stack overflow or hit maximum recursion limits? <niemeyer> If you emit an event inside the handler of that event, yes bad things can happen if that takes place forever <niemeyer> It's also not super nice to read and maintain despite anything else <niemeyer> It's awkward to say "Hey, it's now initialized!" inside the thing that handles "Oh, is it initialized? Cool, let me handle that.".. makes sense? <t0mb0> niemeyer, it shouldn't take place forever in that eventually the pod would have loaded (this is an interim solution until juju notifies us when the pod is alive). <niemeyer> It's better that it's known to be going away, but it doesn't make they nice.. I'd still look for a different solution <niemeyer> *that <niemeyer> t0mb0: You might just defer the event, for example <t0mb0> niemeyer, yeah that's my preferred solution atm however it's kind of annoying having to wait around for an update-status hook to fire for that event to be processed again <niemeyer> t0mb0: Okay, let me think about this and I will try to suggest something <t0mb0> niemeyer, it's not super critical, I think we're happy to just wait for an update-status for the time being until we have more k8s hooks to work off <t0mb0> I was just wondering if the emit() loop was viable <niemeyer> t0mb0: Viable it is.. it's just not a very nice way to organize things.. it's a very complex way
{'channel': 'smooth-operator', 'month': 4}