[04:30] mroning all === max__ is now known as Guest85769 [08:18] good morning! [08:21] * Chipaca stayed up too late and now needs more coffee [08:40] 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)? [08:46] Morning Chipaca [08:47] Chipaca: Reviews mainly.. it wasn't easy to get to those names either :) [08:48] morning Chipaca and niemeyer [08:48] "Is ours what?" ""self is something else" and it goes on.. [08:48] Morning jam [08:53] Chipaca, niemeyer : if the parameter of 'juju debug-code' is now '--at', should the env var still be JUJU_BREAKPOINT? [08:53] 'JUJU_DEBUG_AT' or something along those lines feels like it matches better. [08:54] The doc is also now significantly out of date, and I'm not sure I was around for all the conversations. [08:54] I pretty much just understood changing "juju debug-hook --breakpoint" to "juju debug-code --at" but the rest seem to stay the same. [08:56] Sounds reasonable and generic enough to be useful in multiple ways.. can't think of better alternatives right away [08:56] niemeyer: are you talking about AT or about BREAKPOINT there [08:56] can be read both ways [08:57] The suggestion [08:57] (AT) [10:27] Chipaca: added a link to the spec doc here https://trello.com/c/vcZTwoOI [10:27] I am going to update it with more details [10:31] We had an agreement to have 160 as the line length [10:32] I also asked to have "sandbox" into the git ignore list [10:32] Looks like both have been undone by 907672c0901168903bf17c94165b034881bd35bd [10:34] Looking at the diff here https://github.com/canonical/operator/pull/162/files [10:34] The new lines on the right are consistently less readable [10:35] https://usercontent.irccloud-cdn.com/file/5g77t1LD/image.png [10:35] Do you really prefer reading the one on the right? [10:35] There are several of those... [10:35] Can we please revert this PR? [10:35] niemeyer: 160 is much too wide [10:36] Chipaca: I don't care about the number as much as I care about the result [10:36] niemeyer: yes, i prefer the one on the right [10:36] We can make it 1000 is if it looks better [10:36] Chipaca: Really!? Are you seriously saying you read that completely broken up text and code better than what you have on the left side? [10:36] niemeyer: and sanbox is still in git ignore [10:39] niemeyer: it's unclear what sandbox is in the gitignore for, but it's still there [10:39] without a comment or anything it's a prime candidate for cleanup [10:39] Chipaca: Both the line length and sandbox have been discussed before you were in the team [10:40] 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 [10:40] Chipaca: The sandbox is a directory I personally use to put things that should not be in the tree [10:46] 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) [10:47] https://www.python.org/dev/peps/pep-0008/#maximum-line-length itself recommends up to 99 chars, [10:47] I don't prefer the 'indent to where the (' opens, as it tends to waste a lot of horizontal space [10:48] jam: How do broken down lines look like in your terminal? Does it fit more than 99 chars, or is it broken down further? [10:48] so I tend to do the return immediately after the (which causes it to indent just a single extra indent level) [10:48] jam: Because reviews look terrible.. it's neither 99 nor 160 [10:48] jam: Which means we get the manual line breaking, and then the enforced breaking on top of that either way [10:49] jam: (as we had discussed many months ago) [10:49] I believe the actual line length discussion predates me as well. [10:50] In my editor, it tends to horizontal scroll, in a terminal, it wraps the line [10:50] jam: Or maybe it just wasn't interesting enough to be talking in the channel.. ;) [10:52] jam: Okay, so 99 will *also* wrap and horizontal scroll.. [10:56] Yeah, just barely.. conversation happened on Oct 3rd, you joined on Oct 2nd [11:01] fair enough. I remember it being 160, but certainly didn't have a strong rationale for defending it staying there. [11:03] It's okay.. the rationale is not super insightful other than we looked at code and it looked worse when indenting too much [11:03] Reading the delta seems to confirm that still.. [11:04] Sometimes it's like this: [11:04] https://www.irccloud.com/pastebin/LDdXKI2x/ [11:04] Sometimes like that: [11:05] https://www.irccloud.com/pastebin/nIyHc8G3/ [11:05] And sometimes this: [11:05] https://www.irccloud.com/pastebin/1ex3TAbJ/ [11:06] Versus stuff like this: [11:06] - raise RuntimeError("StoredState shared by {}.{} and {}.{}".format(parent_tname, self.attr_name, parent_tname, attr_name)) [11:07] https://www.irccloud.com/pastebin/cy32xfyL/ [11:08] 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. [11:08] 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 [11:09] I think there was also an issue with github rendering, at least IIRC that is what Chipaca put the PR up for. [11:09] The screenshot I first posted above was from GH [11:15] Muy buenos días a todos! [11:16] So, you decided to go -vvvvvv mode the day I was off? :p [11:16] * facubatista has a lot of stuff to read [11:16] facubatista: it's _because_ you were off [11:16] facubatista: it's you that's been inhibiting us [11:17] * Chipaca just out of a meeting and needs to read up [11:17] :) [11:17] I run a 27" monitor, and while I can go wider, I tend not to maximize browser windows because they are too wide. [11:18] In my normal width: https://imgur.com/cSVJFWU the old one wraps but the new one doesn't. [11:18] jam: To be clear, my point was never that the monitor was wide enough to fit 160 chars [11:18] If I max the window, neither wraps https://imgur.com/kfjK3IR [11:18] but I actually find it hard to review at that size, because the two sides are too far apart [11:18] it is hard visually to follow from left to right for me [11:18] 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 [11:19] and if I make it narrow https://imgur.com/1YEpDMK it looks awful in the new ay [11:19] jam: Extremely long lines can usually be reorganized to not be extremely long by using proper coding practices [11:19] jam: And we can always *choose* to break lines down where really appropriate [11:19] jam: What the limit does is to force people to break them down whatever it takes.. and we end up with the stuff mentioned [11:21] That's subjective, though.. so let's just raise that up in the next standup and see where the team stands [11:22] jam: This "narrow" view is actually what I get naturally over here, and I'm on a high-resolution (4k) 14 inch monitor [11:24] for whatever reason my 'natural' window size is about 1200x1200 pix. it fits most content cleanly (bugs on launchpad, gogole docs, etc) [11:26] My resolution is 2560x1440 (2k actually) [11:26] I use a tiling WM, but for that sort of content the window is always full screen [11:32] Chipaca, facubatista : I tried to encapsulate the discussions that happened yesterday around "Debugging live charms" and brought up a couple more questions [11:32] https://docs.google.com/document/d/1zKwzo26bNVFgohaRB_kdUCsFPY3OcjfcpQklGpkOHpc/edit# [11:32] I also have 'juju debug-code' sketched up, testing it now. [11:33] jam, will go there when I finish all the reading here, 5' [11:46] I don't like this breaking lines style: [11:46] some_call(foobar, [11:46] other, [11:47] not only because the totally wasted space there, but also because it's indented to (75% chance) a weird column [11:47] and pathologically leads to code like this: https://pastebin.canonical.com/p/fb7XW8yJWx/ [11:48] niemeyer, for me code at the left here is totally broken: https://usercontent.irccloud-cdn.com/file/5g77t1LD/image.png [11:49] facubatista: Broken in which sense? You're probably not using that word in ways I normally read it :) [11:50] Dmitrii-Sh: https://github.com/canonical/cockroachdb-operator/pull/1 has an initial review [11:50] 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 [11:51] 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 [11:51] 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. [11:51] facubatista: But, that formatting in the right is what I consistently get.. [11:52] I probably need a wider monitor, which is ironic given that I'm the one advocating for wider lines [11:52] thanks, also submitted the tls-certificates interface for review https://github.com/canonical/operator/pull/210 [11:52] ANyway, lunch time here [11:52] And we have a meeting in just a few [11:52] See you soon [11:52] need to figure out what to do about python-cryptography for pull/210 [11:54] aghhh, canonical's pastebin messes with the identation [11:55] 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 [11:55] (or like the second version there) [11:58] facubatista, I also prefer that wrapping [12:20] niemeyer, stub, regarding the surprising json key: https://bugs.python.org/issue34972 [12:23] facubatista, "its how the format works", in that it doesn't support ints for keys, not that it treats ints as strings... [12:23] IMO [12:24] jam, yes, it was surprising for me too! I actually found the issue when going to report the issue, a couple of months ago :) [12:24] * facubatista had it fresh because of that [12:28] jam, thanks for the massive editing in the Debugger doc [12:29] 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. [12:29] 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 [14:38] Hello all [14:39] hello narinderguptamac [14:41] facubatista, was taking to Dmitri about peer relation and got it cleared that any peer event will fire after start event [14:44] narinderguptamac, nice! [15:14] * facubatista -> lunch [15:24] hello hello [15:25] 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 [15:28] 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 [15:37] crodriguez: o/! [15:38] crodriguez: we haven't done the work to support the new secrets thing AFAIK [15:38] crodriguez: we will shortly [15:39] Chipaca: aah ok then I'll wait before integrating that. Thanks! Is there a newsletter / discourse / etc. where updates, new features, etc are published? [15:41] Chipaca, crodriguez : so any pod spec that they give us we pass on through, I think you're crossing 2 different things. [15:41] 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. [15:42] crodriguez, the actual bug you have is in your code, though [15:42] application-mssql: 10:15:33 DEBUG unit.mssql/0.config-changed if self.state.spec != new_spec: [15:42] that sais you are looking at 'self.state.spec' but you never initialized the field [15:42] crodriguez, you'll want to have something like "self.state.set_default(spec=None)" in your charm's __init__.py [15:42] sorry def __init__(): [15:43] crodriguez, I'm actually past EOD here, but if you can put your actual code somewhere we can give you feedback on it. [15:45] 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 [15:46] tomorrow*. I'll check the state of the spec like you mentionned [15:48] crodriguez: what is _write_state in that code? [15:50] 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 [15:51] 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 [15:52] crodriguez, so in __init__ having it do self.state.set_default() is the recommended way [15:52] that way it is guaranteed to happen even after an upgrade/etc. [15:52] okay I'll add that [15:52] but also won't ever overwrite the values that you've actually set. [15:54] jam: you go have a good evening now :) [15:54] Chipaca, well, I have to hang around to get my PR reviewed. Might as well offer some tech support. [15:55] aw [15:55] mmh "AttributeError: attribute 'set_default' is not stored" [15:56] defaultS? [15:56] Hi Camille :) [15:56] niemeyer, crodriguez : its 'set_default' but it sounds like you have an out-of-date copy of ops [15:57] I'm not 100% sure as it's been a few weeks, but that's what came to mind when you mentioned the error [15:57] if you're using submodules you'll need to 'git pull' in the subdir. [15:57] Ah, and jam just proved me wrong.. efficiency! :) [15:59] in the operator folder? it doesn't have info on the upstream repo [15:59] I thought it was a command like update submodules or something similar.. [16:03] crodriguez: if you have submodules in your repository, you can use this to update all of them: git submodule update --recursive --remote [16:03] but a prerequisite to that is actually adding it [16:03] git submodule add https://github.com/canonical/operator mod/operator [16:04] Dmitrii-Sh, ah, the --remote is the trick that I didn't realize so always just did 'git pull' which worked for me. [16:04] okay that recursive update command worked, thanks. I moved on to another bug with the framework haha [16:05] well with my code [16:05] jam: yes it's not very intuitive which reminds me of https://imgs.xkcd.com/comics/git_2x.png [16:06] crodriguez: what is it? Can I help? [16:06] Dmitrii-Sh, git submodule update --help isn't particularly... helpful [16:07] jam: heh, it's rather "--quiet" [16:07] 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? [16:11] 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? [16:11] lol sorry I'm bombing y'all with questions [16:12] 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 [16:12] but then "dispatch" will also be in 2.8 https://github.com/juju/juju/pull/11257/files#diff-95b793849a6a72c6122da04f68ecb586 [16:12] 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. [16:13] Dmitrii-Sh, I actually thought that landed in 2.7.4, doesn't look like the bug was updated. [16:13] "bake and land in 2.7.3" is pretty far away from 2.7.4/5 so not sure where its at. [16:13] jam: hmm, I looked at #5 https://bugs.launchpad.net/juju/+bug/1854635/comments/5 for reference, haven't looked at the actual commits [16:16] 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? [16:18] spec = { [16:18] 'version': 2, [16:18] 'serviceAccount': { [16:18] 'global': True, [16:18] 'rules': [ [16:18] { [16:18] 'apiGroups': ['apps'], [16:18] 'resources': ['statefulsets', 'deployments'], [16:18] 'verbs': ['*'], [16:18] }, [16:18] { [16:19] oops [16:19] so in this spec, if I change version to 3, I get this : [16:19] https://www.irccloud.com/pastebin/2qFlNy81/ [16:19] same for unknown field 'rules' [16:26] 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 [16:27] I'm not the one who has the definitive answer to that, though. [16:27] I know there were discussions [16:27] the error seems to be coming from the framework though [16:28] b'ERROR json: unknown field "global"\n' [16:28] hmm, actually that doesn't look like something we'd emit [16:29] 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) [16:30] Hmm. Okay, I'll look more into that. I need to step out for an hour, will circle back about this. Thanks everyone! [16:31] version: 3 [16:31] serviceAccount: [16:31] automountServiceAccountToken: true [16:31] roles: [16:31] - global: true [16:31] rules: [16:31] - apiGroups: [""] [16:31] resources: ["pods"] [16:31] verbs: ["get", "watch", "list"] [16:31] - nonResourceURLs: ["*"] [16:31] verbs: ["*"] [16:33] crodriguez: it seems like you need to have "global: true" under roles [16:33] and then rules under roles as well [16:34] Dmitrii-Sh, ModelError(e.stderr) means it is coming from Juju [16:37] Dmitrii-Sh, hm, seems v2 is quite a bit different than v3. https://discourse.juju.is/t/updated-podspec-yaml-new-features/2124 [16:37] vs https://github.com/juju/juju/pull/11293 [16:39] jam: I think charms need to specify min-juju-version: 2.8.0 and use a v3 spec if they support v3 only [16:39] * facubatista is back [16:39] but this is slightly unfriendly to a user [16:45] 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 [16:46] jam: agreed, there is nothing restricting the usage of v3 via pod.spec_set AFAIKS [16:46] Dmitrii-Sh, we need to add 'k8s-spec-set' to the list of 2.8-isms. btw [16:47] Chipaca, ^^ juju is recommended a different name for setting the definition of your pod in 2.8 [16:48] jam: right, the command got renamed [16:51] is there a reason we're not sending DEBUG logs from the framework/charms to juju [16:51] ? [16:55] 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 [16:56] I originally wanted to use DEBUG: https://github.com/canonical/operator/pull/133/commits/2c778fc3f128b088790229bc3cc55bf7f7daedd6#diff-a76991ba26c9c9b312be489400d3f481L21 [16:56] 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 [16:56] 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 [16:57] I see debug logs as a fire hose, where even the smallest detail is fine [16:58] 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 [16:58] 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 [16:59] *it* should be in DEBUG... [16:59] Again, I'm not claiming it's correct.. just that this is what I hope we get [17:12] 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 [17:13] that would be changing the JujuLogHandler constructor to be like this: def __init__(self, model_backend, level=logging.DEBUG): [17:19] Dmitrii-Sh, yeap, that's what issue #198 talks about [17:23] Tentatively calling it a day.. o/ [17:26] 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. [17:26] tvansteenburgh1: ack [17:28] crodriguez: the charm tvansteenburgh1 has a v3 spec example ^ https://github.com/charmed-kubernetes/charm-multus/blob/master/src/charm.py#L87-L125 [17:28] seems to confirm the theory I had === tvansteenburgh1 is now known as tvansteenburgh [17:38] Dmitrii-Sh: thanks for your help. I will compare and adjust to use the v3 [20:25] * facubatista eods [20:38] https://github.com/canonical/operator/issues/211- found during testing [22:40] https://github.com/canonical/operator/pull/212 - would be helpful for progressing #210