[00:57] externalreality: review done, i think we can remove the mock store [00:59] without the mock store the tests won't actually read the yaml files. [01:00] The yaml files are read twice, once for getting the data and once for validating it. [01:01] unfortunately I could work around that [01:02] externalreality: but only one method is called onthe mock store - we create a mock store just to call the one method. the mock store is not used elsewhere after that that i can see [01:03] so can't we call that one method directly? [01:03] Hmmm, sounds right. Will fix. :-D [01:04] ty [02:08] axw: thanks for review. i may have misunderstood your point about relation macaroon. i've replied to your comment. maybe we need to discuss? [02:08] looking [02:10] wallyworld: for the relation macaroon, you're using for the ID: "relation-"+localRel.Tag().Id(). I'm saying that's OK, but you should include the model UUID in the ID as well. otherwise it'll collide with other models (bakery storage is not model-specific) [02:11] axw: ah, right, i missed the bit about bakery storage not being model specific. i thought it was [02:11] wallyworld: alternatively make it blank, but then you'll get a new key for each register call, even for the same relation [02:12] axw: i wanted to give it a name so i could delete it when the relation died [02:12] wallyworld: yep, makes sense [02:12] axw: that was also the reason for naming the offer macaroon. but it can just be cleaned up by the ttl index [02:13] wallyworld: for the offer macaroon, you can have a single one, and then rely on an additional caveat to force the client to prove itself. then use that for revocation [02:13] i.e. refresh [02:14] axw: the idea was that it would be a bearer token rather than have a caveat to discharge [02:14] the controller uses the macaroon, not the user [02:15] wallyworld: I think it's fine one per consume for now, we can revisit that [02:15] yeah [02:20] axw: relation macaroon now includes model uuid [02:21] in the id [02:24] wallyworld: reviewed [02:24] ty [02:24] yeah, i have got more testing [03:53] wallyworld: lxd storage PR, if you have time: https://github.com/juju/juju/pull/7635 [05:13] axw: swap you reviews? https://github.com/juju/juju/pull/7637 [05:14] wallyworld: ok [05:15] wallyworld: what do you think about adding special syntax to "add-storage" for enlisting? like "juju add-storage postgresql/0 pgdata=ebs:vol-123456", where vol-123456 is the provider (EBS) volume ID [05:16] I think that's all the info we need, in order to create a storage instance, volume, and attachments [05:16] axw: that would save an extra command for sure [05:16] axw: but what about adding an ebs volume as detached storage [05:17] wallyworld: it wouldn't be possible, but it is it necessary? you can't add new volumes detached [05:18] axw: i was thinking of the case where someone adds a unit and then later detaches the storage - that storage is now referenced by the model and be attached again elsewhere. i would think we'd want a way to allow an ebs volume to be made known to juju for later attachment [05:19] wallyworld: ah yes, it won't work with --attach-storage [05:20] maybe we could tweak attach-storage [05:23] axw: i'm confused about where "juju-zfs" comes from - it that our (juju's) naming convention? [05:24] wallyworld: it's specified in DefaultPools [05:24] wallyworld: the juju storage pool definition includes an attribute "lxd-pool", which identifies the lxd storage pool to use [05:25] wallyworld: and if the lxd-pool attribute is missing, we use "juju" as the lxd storage pool name [05:26] and zfs.pool_name s passed to lxd === marlinc_ is now known as marlinc [05:33] wallyworld: yes, things other than driver/lxd-pool are passed directly to lxd [05:44] axw: lgtm. with the macaroon storage, other places use json. should we be correct for cmr or consistent? i guess there's no reason we can have cmr use binary [05:44] *can't [05:44] wallyworld: thanks [05:44] wallyworld: not too fussed, your call [05:44] maybe leave for now and take a view [05:45] can see what others think [05:45] wallyworld: CreateStoragePool is not idempotent. we check if it exists first. I could create and hceck on failure, but there's no special error code for "already exists" [05:45] there is an error message we can check, but that could change [05:45] yeah :-( [05:46] wallyworld: I'll just make it do a create, then check if it exists on failure [05:46] ignoring the specifics of the create error [05:46] i think that would be good [05:46] s/good/better [05:49] jam: wtf do you think we need to do with this mongo bug? it seems the pinger is not the root cause? or is it that things get messed up due to the pinger issue and manifest hat way? [05:52] wallyworld: so my feeling on it is that it shouldn't block 2.2.2. I feel like we have had a latent bug, and the pinger bug exacerbated it [05:52] so I think it is a separate bug, but one that we've had [05:53] the lack of clear reproduction situation, and really the need to do live debugging, because the round-trip info is killing us in investigating it [05:53] jam: i have been telling myself that if it is not a regression we could ship, but that if 2.1 didn't have the problem, we can't ship [05:53] missing info, things that you'd pick up on what to do next while you're looking at the exact logs, etc. [05:53] but as you say, it could be in 2.1 [05:54] wallyworld: nobody has reproduced, and getting information out is taking days, IMO that isn't worth delaying a fix of 10 other things that we concretely made better [05:54] exactly [05:54] jam: will eric's PR, we could land as is and follow up with that %q tweak [05:55] wallyworld: with eric's PR, I also don't feel that's a blocker for 2.2.2 being actually released. If we get it in, great, but not a delay [05:56] jam: the issue is a bit shit, we should land it. i can do that and easy to make the subsequent little tweak after that [05:58] wallyworld: sure, but its a little ugly wart that doesn't actually prevent you from doing what you asked to do (hence not a Critical blocker), definitely worthy of a 2.2.X release, I just wouldn't hold anything up on it. [05:59] yeah, ok. i'll land now and it should make it [06:52] jam: did you want to move that bug odd the 2.2.2 milestone? also, eric's PR doesn't need the %q instead of %s because the string comes out of whatever the underlying api call is already quoted. or so my manual testing showed [06:52] *off [06:57] wallyworld: k. I will say it *didn't* get quoted when he was interpreting the error incorrectly, but I did see that one of his tests seemed to say it was quoted [06:57] so I wasn't sure [07:06] np [07:06] i wasn't sure either [07:06] until i tested manually === frankban|afk is now known as frankban === salmankhan1 is now known as salmankhan [09:48] Bug #1704099 opened: Juju state server not starting after reboot <4010>

=== salmankhan1 is now known as salmankhan === akhavr1 is now known as akhavr === tvansteenburgh1 is now known as tvansteenburgh === frankban is now known as frankban|afk [21:31] hml: can you join us in release standup to update on release? https://hangouts.google.com/hangouts/_/canonical.com/juju-release [21:31] wallyworld: on my way [21:34] Bug #1704099 changed: Juju state server not starting after reboot <4010>