[04:36] morning all [08:14] PR operator#319 opened: ops: get version from git [08:17] Issue operator#313 closed: If ops is installed, the test run picks up the wrong one [08:17] PR operator#314 closed: test_main: set up PYTHONPATH so the sub-process picks up this ops [08:18] morning Chipaca [08:18] jam: moin moin :) [08:22] Chipaca, I'd like to chat a bit about serialization of StoredState, we can start on IRC and move from there if you feel it is too involved [08:22] jam: sure [08:23] Chipaca, so I'm pretty sure you're aware that we currently use a pickle into a sqlite database for all StoredState content [08:23] yep [08:24] Juju will accept key=value pairs for 'state-set/get' and can take that 'state-set' as 'key=value' on the CLI or as YAML key:value from --file (which can be '-' for stdin) [08:24] The current spike was done by putting the pickle bytes into base64 and then writing that as the value [08:24] mhmm [08:25] it is a pretty obtuse way to understand what your state is, if you ever wanted to do "juju run --unit u/0 state-get" [08:25] foo: gANLCi4= [08:26] I have some concerns about pickle support, though AIUI since we force it to be simple types with a pass by Marshal [08:26] there shouldn't be too much trouble if you wrote a pickle with python 3.7 and read it with python 3.8 [08:26] (say you did a do-release-upgrade on the machine that was running the charm, and it is now running a different version of python) [08:27] pickle's fine with that, yes [08:27] Chipaca, it doesn't *feel* like a 'stable format to keep your state in over time" [08:27] eh, it's stable enough (you can use 3.8 to load a pickle from 2.2 for example) [08:27] but [08:27] it's not very approachable [08:27] and that might be a bigger issue :) [08:28] jam: given that they're simple types, wouldn't yaml also work for us? [08:28] Chipaca, so the one caveat is that yaml or json as the actual payload loses the type for set vs list [08:29] Also, JSON keys are strictly strings IIRC, so you can't have {0: 'foo'} [08:29] I think YAML supports non-string keys [08:29] are yaml keys also -- ah [08:29] hmm [08:30] jam: and python? [08:30] Python has the nice behavior that json.dumps({0: 'foo'}) => '{"0": "foo"}' [08:30] iow, it silently casts your numeric keys into string keys [08:30] no i mean, make it a python literal [08:31] and use ast.literal_eval to bring it back [08:31] $ python3 -c "import json; print(json.dumps({'0': 'foo', 0: 'bar'}))" [08:31] {"0": "foo", "0": "bar"} [08:32] Chipaca, what is the cleanest way to turn a nested dict into a literal? just repr() ? [08:32] yep [08:32] bah, dicts you can str because repr is str there :) but yes, repr [08:32] Chipaca, and... there isn't much in the way of versioning, so we have a way to change our mind about how we want to store the content. [08:33] s/have/don't have/ ? [08:33] we could add that ourselves, make the first byte of the value be the version? [08:34] does juju care at all what we dump in there? what further processing does it do on it? [08:34] >>> repr({'0': [1,2,3], 0: set(['a', 'b', 'c'])}) [08:34] "{'0': [1, 2, 3], 0: {'b', 'c', 'a'}}" [08:34] >>> ast.literal_eval("{'0': [1, 2, 3], 0: {'b', 'c', 'a'}}") [08:34] {'0': [1, 2, 3], 0: {'b', 'c', 'a'}} [08:34] Chipaca, so Juju *should* just round trip the content we give it. [08:34] yeah, sets have literals too ;) [08:35] Chipaca, yeah, I always use set() because the empty set doesn't have another literal [08:35] ah, yeah [08:36] since {} would be ambiguous. I'm also not a huge fan of "did you notice there were no :" [08:36] and it might fail here :-/ [08:36] because 'set()' isn't a literal [08:36] >>> ast.literal_eval(repr({'0': [1,2,3], 0: set()})) [08:36] Traceback (most recent call last): [08:36] File "", line 1, in [08:36] File "/usr/lib/python3.6/ast.py", line 85, in literal_eval [08:36] return _convert(node_or_string) [08:36] File "/usr/lib/python3.6/ast.py", line 66, in _convert [08:36] in zip(node.keys, node.values)) [08:36] File "/usr/lib/python3.6/ast.py", line 65, in [08:36] return dict((_convert(k), _convert(v)) for k, v [08:36] File "/usr/lib/python3.6/ast.py", line 84, in _convert [08:36] raise ValueError('malformed node or string: ' + repr(node)) [08:36] ValueError: malformed node or string: <_ast.Call object at 0x7f2b34d35160> [08:36] Chipaca, indeed, it does fail :9 [08:36] :( [08:37] so close [08:37] it is also true that we are running pickle.loads() on unknown data [08:38] yes we write it from things that are 'safe', but we are still running the engine on data that we're reading back from 'outside' [08:38] Chipaca, code layering wise, it is also quite hard to not use pickle, because Framework is the part that decides pickle, while Storage decides to write it to the DB [08:39] yeah [08:39] (Framework.save_snapshot is where the marshal.dumps check and pickle.dumps is done) [08:39] i think that could use a refactor, but separately [08:39] Chipaca, though again, we can just move that down into the SQLiteSnapshot layer [08:39] SQLiteStorage [08:40] Chipaca, indeed. snapshot() from the actual objects that people implement is still defined as a dict so we're ok there [08:40] jam: do we care about the spike being forwards-compatible? [08:40] i guess we do but just checking :) [08:40] Chipaca, as in that the current version of the code can read the content written by future versions? [08:41] as in the state written by the spike be readable by future versions [08:41] or if it can't read it, fail gracefully [08:41] Chipaca, we don't care about the current spike, because it was just to show proof-of-concept. no charms that I'm aware of are using it. [08:41] jam: ah so the spike isn't what we'd put in 0.7 [08:42] Chipaca, correct. I'm using it as a starting point, but taking the time to carefully structure things and make sure it is the serialization that we're happy with. [08:42] ok [08:42] I'd really rather use something like json/yaml and have it be inspectable by humans. [08:42] so for the initial spike, pickle is fine then :) [08:42] I think you missed a comment by Stub that people are writing sqlite3 hacks to unpickle the snapshot fields to try to figure out what is going on. [08:43] i've seen that in bugs and etc, yes [08:43] which seems obvious to me -- if i were trying to figure out what was going wrong, i'd want to look at anything i could too [08:43] Chipaca, and we can write a common helper to make it easier to inspect that state [08:44] Chipaca, but ideally it would be less opaque to start with. [08:44] Could we make the statement "sets aren't allowed" and just go with JSON ? [08:44] json also mangles numbers [08:44] ah right [08:44] yeah, I hate that part. [08:44] so it's my least favourite for this :) [08:44] if it *failed* I might be ok with it. [08:44] the fact that it silently corrupts makes it out [08:45] see my above comment where you can json.dumps() and get the integer and string keys colliding with no error. [08:45] we can come up with special handling for empty sets and work with that, also [08:45] I'd actually assumed sets were not allowed by 'simple types only', like json and yaml and toml and everything else people are used to now days. [08:45] Hello! [08:45] ERROR juju.worker.uniter.operation hook "start" (via explicit, bespoke hook script) failed: exit status 1 [08:45] Recommend feeding the CLI tool using the --yaml argument to avoid CLI command length limitations and encoding and locale issues [08:45] I'm getting that error in k8s charms in 2.8.1 [08:46] Chipaca: any ideas? [08:46] davigar15: what operator version? [08:46] latest [08:46] davigar15: can we see the debug logs? [08:46] yes [08:46] stub, yeah, --file is currently the way that I'm interacting with state-set. [08:46] davi [08:47] davigar15, I'd start with "juju model-config logging-config=DEBUG" and "juju debug-log" to get more information about what is going wrong. [08:47] jam: Maybe this time we can use = and . in our keys :) [08:47] we _could_ restrict what you can put in state even further, for this feature [08:47] or, we could special-handle empty sets :) [08:48] (or sets in general!) [08:48] oooh, good trick. Need to debug it :-) [08:49] jam: yaml.safe_load(yaml.dump(set())) [08:49] jam: → set() [08:50] yaml.safe_load(yaml.dump({'0': [1,2,3], 0: set(['a', 'b', 'c'])})) → {0: {'a', 'b', 'c'}, '0': [1, 2, 3]} [08:51] we'd have to check if the !!set annotations require a particular pyyaml version [08:52] application-zookeeper-k8s: 10:52:25 DEBUG unit.zookeeper-k8s/2.start raise TypeError( [08:52] application-zookeeper-k8s: 10:52:25 DEBUG unit.zookeeper-k8s/2.start TypeError: observer methods must now be explicitly provided; please replace observe(self.on.config_changed, self) with e.g. observe(self.on.config_changed, self._on_config_changed) [08:52] There is it [08:53] It has to be explicit. I have self instead [08:53] :-) [08:53] thanks [08:53] jam: but it works in 3.11 (xenial) and 3.12 (bionic) at least [08:53] Chipaca, ^^ that looks like the "catch exceptions and log at higher than DEBUG" :) [08:53] davigar15: :) sorry we broke your charm, glad we were trying to tell you about it at least :) [08:54] jam: yep [08:54] did we have an issue for that? i should do that one next [08:54] we do [08:54] It's okay! nothing in production yet hahaha [08:54] Chipaca, #289 [08:54] Issue #289: hook exception logged at DEBUG [08:54] perfect [08:54] jam: any reason not to go the yaml route (after the refactor to move sqlite decision out of framework) ? [08:55] Chipaca, so I've been told historically that YAML is good if a human is going to edit it, but it has enough edge cases it doesn't make a great machine<=>machine format. [08:55] I think you will need to keep sqlite if you want Juju 2.7 compatibility [08:55] (things like base 60 numbers) [08:55] stub, yes, it isn't removing sqlite [08:56] it is adding an alternative when it is available [08:56] yeah i bungled and meant 'pickle' there [08:56] but what do we want *that* to look like [08:56] framework is pickoing, storage is sqliting [08:56] pickling [08:56] * Chipaca used to be able to type [08:56] Chipaca, I think I'll want to keep the marshal() check, but move the pickling into Storage and out of JujuStorage and try using YAML there. [08:57] Chipaca, it kind of has to happen at the same time, since the interface to the Storage changes from taking a bytes to taking a dict [08:57] but it shouldn't be very invasive. [08:58] jam: so what _is_ a good machine ⇔ machine format? [08:58] With the marshal check, the yaml should always be bog simple except for sets (and maybe other builtin Python types I can't think of right now), and seems fine to me as machine <-> machine [08:58] well, the recommendation was JSON, but for obvious reasons that is out. I don't think pickle/python ast fall into that category. XML is overbroad [08:59] stub, Chipaca certainly YAML is my top pick right now [08:59] base64 of a pickled xml ast dump \o/ [08:59] * Chipaca hides [08:59] eval(repr(blob)) [08:59] exec* [08:59] ]:-) [08:59] wait is exec eval now [09:00] * Chipaca checks [09:00] nope exec is still the right one for maximum pawnage [09:01] anyway, sorry for the silliness :) [09:01] * Chipaca is only a little bit sorry [09:01] hush you [09:01] Chipaca, if only there was some way to get "sudo" in there [09:02] jam: charms run as root ¯\_(ツ)_/¯ [09:06] Chipaca, so one nice property is that refactoring is actually quite localized [09:07] Chipaca, what is the type annotation for "snapshot". Currently we only really ever use 'dict' or None, but it has so far been anything that python can pickle [09:08] is that just "object" ? [09:09] jam: Any ? [09:09] it's not strictly Any, but I don't think there's something better [09:10] OTOH maybe we should enforce it being dict-or-None [09:10] anything else isn't going to be extensible enough for long-term [09:10] Chipaca: Why could my charm be in wating for container, without giving any errors? [09:11] 'waiting for container' sounds like a juju thing to me :) [09:11] ie not yet the charm [09:13] jam: OTOH you *could* spell it all out [09:14] as in, Union[int, float, … ] [09:15] Chipaca, so we do have tests that snapshot() -> str and 'def snapshot(self): return self.value' where value was passed in to .emit(value) [09:15] (where in the test value is only ever a string) [09:18] and it's probably overly opinionated for a framework :) [09:18] i mean the 'always use a dict' thing [09:21] Chipaca, well, dict or None would be acceptable IMO, and we are little if not opinionated :) [09:21] Chipaca, if our review of people's code would always be "you really should use a dict" then we should push that a bit harder, I think. [09:22] PR operator#320 opened: ops/framework.py: Move pickle into Storage [09:24] Chipaca: happening with juju stable version... I'm going to check my charm. [09:26] Can't see anything wrong... https://pastebin.canonical.com/p/BMqcdkT6fN/ [09:29] davigar15: what does the debug log say [09:29] juju model-config logging-config=DEBUG [09:29] ERROR config value expected '=', found "DEBUG" [09:29] haha [09:30] that exact line worked here [09:31] you could also do: juju model-config "logging-config==DEBUG" [09:31] which is the more explicit version [09:31] (that allows you to specify each module separately) [09:31] davigar15, though your particular statement "waiting for container", I believe, is created by Juju [09:32] (if a charm sets a pod spec, and otherwise goes into Active, Juju sets the state to 'waiting for container' until the pod has started) [09:32] davigar15, https://discourse.juju.is/t/how-to-avoid-the-waiting-for-container-message/1369/3 might help? [09:34] specifically https://discourse.juju.is/t/how-to-avoid-the-waiting-for-container-message/1369/6 [09:35] https://pastebin.canonical.com/p/CxPhPygZb4/ [09:35] $ microk8s.kubectl -n zk get pods [09:35] NAME READY STATUS RESTARTS AGE [09:35] modeloperator-658686f765-cfsr6 1/1 Running 0 2m59s [09:35] zookeeper-k8s-operator-0 1/1 Running 0 2m11s [09:35] But the application pods are not even created [09:37] davigar15, so offhand your charm seems to be missing a self.state.set_default(spec=None) as on_config_changed introspects self.state.spec directly [09:38] create Pod zookeeper-k8s-0 in StatefulSet zookeeper-k8s failed error: Pod "zookeeper-k8s-0" is invalid: spec.containers[0].ports[2].name: Invalid value: "leader-election-port": must be no more than 15 characters [09:38] found [09:38] haha [09:38] (I don't think this the problem here, as it means hitting config_changed without start, which will happen with 2.8) [09:39] davigar15, where did you find that? I don't see it in your pastebin [09:55] it's one of those no-rest-for-the-wicked days [09:56] taking a breka now and getting the lunch sorted because then i'll be eating in meetings === facundo__ is now known as facubatista [11:01] Muy buenos días a todos! [11:04] Chipaca: how can I get the number of ensured units with the op framework [11:08] davigar15, is that len(self.get_relation(PEER_RELATION).units) [11:08] or do you need the count before they are actually started? [11:09] good question [11:09] I think I need it before [11:16] https://github.com/canonical/operator/issues/165 is about possibly having the framework expose the count (reading it from goal-state), but that hasn't been implemented, so understanding your use case/adding it to that issue is useful [11:16] davigar15, ^^ [12:05] facubatista: https://github.com/canonical/operator/releases/tag/0.6.0 [12:05] Chipaca, wonderful [13:12] facundo__: note #294 is gtg [13:12] PR #294: Log when a served breakpoint is not really activated because of name mismatch [13:12] Chipaca, ack [13:13] * facundo__ will restart the router to see if all these disconnections stop [13:13] Chipaca, and for issue #21, if it is a script, and we set JUJU_DISPATCH_PATH it will try to reinvoke hooks/install but that will notice the OPERATOR_DISPATCH env and exit early, right? [13:13] PR #21: Persist stored state before framework commit [13:14] jam: indeed [13:14] charmcraft#21, mup m'dear [13:14] Issue charmcraft#21: Event charm.py not defined [13:14] facundo__: want me to fix this ^? i can do that right now [13:15] Chipaca, be my guest [13:19] Chipaca, btw, if you want easier ways to get to source code from a git submodule you can open the '.gitmodules' file which contains the URL that git will use to find the revision [13:19] https://git.launchpad.net/charm-k8s-mattermost/tree/.gitmodules [13:19] jam: 👍 [13:20] github does that for you, eg if you look at this: https://github.com/jameinel/ubuntu-lite/pull/2/files#diff-b1d90847e9ac9d08a6205ad8f09fd5a9L14 it has a direct link to the Operator code [13:20] PR jameinel/ubuntu-lite#2: Operator [13:21] or https://github.com/jameinel/ubuntu-lite/tree/operator/mod if you are browsing the Tree rather than the PR [13:23] Chipaca, while sending the mattermost email, I realized we also really need to review https://github.com/johnsca/resource-oci-image/blob/master/oci_image.py [13:23] since that is as common to be reused as k8s.py [13:24] ah [13:24] jam: ok === facundo__ is now known as facubatista [14:32] closed charmcraft#18, charmcraft#19 and charmcraft#20. I hope I was able to explain why clearly enough for the reporter not to hate us too much :-| [14:32] Issue charmcraft#18: Poetry integration [14:32] Issue charmcraft#19: Pep 518 support for charm metadata [14:32] Issue charmcraft#20: Support pep 517 for building charms [14:51] PR operator#321 opened: have a Code of Conduct [14:54] facubatista, jam, reviews on #23 would mean we could ship it today [14:55] PR #23: Add leadership to model [14:55] charmcraft#23 [14:55] PR charmcraft#23: build: set JUJU_DISPATCH_PATH so we get the right hook [14:55] Chipaca, ack [14:56] Issue operator#220 closed: Log in WARNING when a breakpoint is not served in debug mode [14:56] PR operator#294 closed: Log when a served breakpoint is not really activated because of name mismatch [14:58] Issue operator#262 closed: Harness should support resource-get [14:58] PR operator#296 closed: Oci resources [15:08] Chipaca, would you mind to explain what magic are you doing in charmcraft#23? [15:08] PR charmcraft#23: build: set JUJU_DISPATCH_PATH so we get the right hook [15:09] facubatista: i see no magic :-/ [15:09] facubatista: the {{}} is because I need actual {s in the output [15:10] facubatista: WRT ${parameter:-word} Use Default Values. If parameter is unset or null, the expansion of word is substituted; otherwise, the value of parameter is substituted. [15:11] so ${JUJU_DISPATCH_PATH:-$0} is "either JUJU_DISPATCH_PATH or, if that is unset or null, $0" [15:13] Chipaca, fantastic, thanks!!! [15:14] Chipaca, mind to put a comment saying something like "if Juju don't support the dispatch mechanism, it will execute the hook, and we need sys.argv[0] to be the name of the hook, so we fake JUJU_DISPATCH_PATH to be that value"? [15:14] Chipaca, thanks! [15:15] will do [15:16] Chipaca, approved [15:16] * facubatista -> lunch [15:58] hmmmmmm [15:58] * facubatista is back [15:58] * Chipaca hmms [16:01] dunno if my lxd is stuck, or just my juju [16:01] facubatista: do you have a working thing you could juju ssh into to try something with juju-log? [16:02] Chipaca, yes, but on a meeting right now [16:02] ah [16:02] k [16:18] * facubatista brb [17:02] ok i think i'm off [17:02] * Chipaca EODs [17:03] * facubatista is back [19:44] * facubatista eods