[01:11] <axw> wallyworld: was doing dropoff (same goes for every tuesday), hence not at standup. yesterday I worked on taking instance AZ from volume attachments
[01:12] <wallyworld> axw: no worries, figured that's where you were
[01:12] <axw> I think I've got it working with AWS
[01:12] <wallyworld> awesome
[01:12] <wallyworld> axw: we also need to look at bug 1692729
[01:12] <mup> Bug #1692729: juju add-storage doesn't always grab ebs volumes on aws <juju:New> <https://launchpad.net/bugs/1692729>
[01:13] <axw> weird
[01:13] <axw> ok
[01:18] <hml> wallyworld: can you take a look at the updates to: https://github.com/juju/juju/pull/7354
[01:19] <wallyworld> hml: yep
[01:19] <hml> wallyworld: ty
[01:26] <axw> wallyworld: is https://bugs.launchpad.net/juju/+bug/1692729 a release blocker?
[01:26] <mup> Bug #1692729: juju add-storage doesn't always grab ebs volumes on aws <juju:Triaged by axwalk> <https://launchpad.net/bugs/1692729>
[01:27] <wallyworld> axw: i think so, given a stakeholder found it and it affects conjure-up
[01:27] <axw> ok
[01:27] <axw> I'll finish up what I'm doing and look at that next
[01:37] <wallyworld> hml: LGTM, but with some tweaks to the test structure
[01:38] <hml> wallyworld: looking
[01:44] <thumper> babbageclunk: I need a rubberduck... or teddybear
[01:44] <thumper> babbageclunk: can I use you?
[01:44] <babbageclunk> thumper: yay, of course!
[01:45] <thumper> babbageclunk: stuff it is
[03:02] <wallyworld> babbageclunk: here's a PR with an upstream patch if you have a moment later https://github.com/juju/juju/pull/7375
[03:28] <babbageclunk> wallyworld: lgtm's
[03:28] <wallyworld> ty
[03:28] <babbageclunk> oops - lgtm'd
[03:29] <wallyworld> babbageclunk: let's hope it fixes one of the source of races
[03:38] <babbageclunk> <fingers crossed emoji>
[03:53]  * thumper gets on filing bugs
[03:53] <thumper> jam: got 5 minutes for an update
[03:53] <thumper> ?
[03:53] <jam> thumper: sure
[03:53] <thumper> jam: 1:1
[03:54] <jam> joining
[04:20] <mup> Bug #1629113 changed: Juju deploy wordpress fails with MaaS <juju-core:Expired> <https://launchpad.net/bugs/1629113>
[04:40] <wallyworld> axw: when you get a moment, here's a small PR to fix some CI test races https://github.com/juju/juju/pull/7376
[04:46] <axw> wallyworld: left some comments
[04:48] <wallyworld> ok, ta
[04:54] <wallyworld> axw: i replied, see what you think
[04:55] <axw> wallyworld: "The race is because a test patches the Period value. This is done outside the watcher and is normally ok, except when race test are run."  <- if it's outside the watcher, how is there a race?
[04:56] <wallyworld> axw: because other tests use the watcher
[04:56] <wallyworld> so one test patches the global var
[04:56] <wallyworld> and another test is using a watcher which tries to access that var
[04:57] <wallyworld> that's my theory. i could repro easily. the PR fixes it
[04:59] <axw> wallyworld: the tests within a package are run sequentially. it sounds an awful lot like the test is doing dodgy things, and we should fix that too. the code change is fine though.
[04:59] <wallyworld> even with race?
[04:59] <wallyworld> it only shows up with race testing
[05:00] <axw> wallyworld: it won't ever do anything particularly bad, just maybe affect the timing of unrelated tests
[05:01] <wallyworld> the test doesn't do anything too bad on the surface. the Test() itself runs s.PatchValue(&watcher.Period, 200*time.Millisecond)
[05:01] <wallyworld> and then it starts up stuff
[05:01] <axw> which test is it?
[05:01] <wallyworld> TestDowngradeOnMasterWhenOtherControllerDoesntStartUpgrade
[05:03] <wallyworld> core issue looks like a problem with clean up
[05:03] <wallyworld> maybe watcher isn't being stopped
[05:03] <wallyworld> or stops when the cleanup runs to reset the patched value
[05:03] <axw> wallyworld: that test is totally doing the wrong thing. once the test has started, State (thus the watcher) will already have been created
[05:04] <axw> wallyworld: I think the PatchValue should be moved to SetUpSuite
[05:04] <wallyworld> hmmm, i think that's a good point. so it is patching the value after the watcher starts and tries to rely on that
[05:05] <wallyworld> yeah, moving it would be better
[05:05] <axw> wallyworld: changing the code to pass the period in is a step in the right direction though IMO
[05:05] <wallyworld> yeah, agreed
[05:05] <axw> wallyworld: i.e. away from using globals
[05:05] <wallyworld> yup
[05:05] <wallyworld> i'll move the patch code also
[05:05] <axw> wallyworld: thanks. just wanted to make sure the test wasn't doing anything too heinous
[05:06] <wallyworld> np, thanks for being thorough
[05:24] <wallyworld> thumper: can you attach relevant log snippets to the txnlog bug?
[05:27] <thumper> sure
[05:48] <babbageclunk> wallyworld: Can you take another look at https://github.com/juju/juju/pull/7369? It's changed a bit...
[05:48] <wallyworld> sure
[05:49] <babbageclunk> thanks
[06:24] <wallyworld> babbageclunk: reviewed, thanks. i'm still quibbling about a set and a slice in the watcher. i like the uniter facade rework. oops, one thing i forgot - we should really catch and handle any CodeNotImplemented errors in the api uniter client. there's not much that can be done except to log a nice message and error out so the uniter restarts with the expectaction that the server will eventually start running the right version (I think)
[06:24] <wallyworld> or maybe it's not an issue since the controller always gets upgraded first
[07:32] <wpk> a one-liner: https://github.com/juju/juju/pull/7377
[11:29] <babbageclunk> wallyworld: yeah - I think you're right about the slice+set. Doesn't make sense on the scale of relations we're talking about.
[11:41] <mup> Bug #1692866 opened: /snap/bin not in path for juju run/juju ssh <juju-core:New> <https://launchpad.net/bugs/1692866>
[12:09] <babbageclunk> wpk: around? I've changed the Uniter API, and needed to change some of the versioning there. https://github.com/juju/juju/pull/7369
[12:09] <babbageclunk> wpk: Could you take a look and confirm that I've handled your NetworkConfig/NetworkInfo change right?
[12:10] <babbageclunk> wpk: I've got NetworkConfig only on the v4 API and NetworkInfo only on v5. Make sense?
[12:18] <wpk> Is that the right PR? I don't see any Network{Info,Config} related things there
[12:19] <wpk> Ah, OK, NOw I see
[12:19] <babbageclunk> wpk: Well, none of it's about those, but if you look at the changes to apiserver/uniter/uniter.go
[12:19] <babbageclunk> wpk :)
[12:20] <wpk> Ah, the diff wasn't loaded
[12:21] <wpk> IMHO it's cleaner to have a 'clean' object on top (like UniterAPI) with everything implemented and make all the workarounds in 'older' stuff, so that if we want to get rid of old one we can simply, well, get rid of it
[12:22] <wpk> so UniterAPI is complete V5, and UniterAPIv{somethingolder} has the newer functions e.g. masked out
[12:22] <wpk> But that's a political decision :)
[12:23] <wpk> (we can even have the old functions in separate file, for cleaniness sake)
[12:25] <babbageclunk> wpk: Well, there's no way to mask out functions properly if you embed the pristine one, so the only way to do that would be not to embed but to explicitly delegate all the ones we want on each version.
[12:25] <wpk> Why there's no way?
[12:25] <wpk> From API standpoint if you create same-named function with 'wrong' signature it won't get published
[12:26] <babbageclunk> wpk: ? I'm not sure that's right.
[12:26] <wpk> func (u *UniterAPIV3) NetworkInfo(_, _ struct{}) {}
[12:26] <wpk> this one
[12:26] <wpk> it masks NetworkInfo from UniterAPIV3
[12:27] <wpk> since we don't publish functions with 2 arguments
[12:27] <babbageclunk> wpk: Hmm, didn't know about the 2-argument thing.
[12:28] <wpk> the rules are in rpc/rpcreflect/type.go, newMethod
[12:29] <babbageclunk> wpk: Ok, I'll check that out in the morning. Thanks.
[12:29] <wpk> function can have 0 or 1 argument, and 0 or 1 return values + optional error
[12:29] <wpk> (so (), (error), (x), (x, error), but not (x,y) )
[12:32] <babbageclunk> wpk: Yeah, makes sense. Ok, I'll do that, thanks!
[12:33] <wpk> Good night :)
[12:35] <babbageclunk> wpk: 'night!
[14:21] <mup> Bug #1692866 changed: /snap/bin not in path for juju run/juju ssh <juju:Incomplete> <snapd (Ubuntu):New> <https://launchpad.net/bugs/1692866>
[23:23] <thumper> I'm at that phase of bug diagnostics where I'm wondering: how can this possibly be happening
[23:29] <babbageclunk> thumper: oh I love that bit
[23:30] <thumper> babbageclunk: I'm hitting the same code path from two different directions, one succeeds and one fails
[23:30] <thumper> WTF
[23:37] <thumper> babbageclunk: got 5 minutes to be a teddy bear?
[23:38] <babbageclunk> thumper: sure!