User1: User2 review done, i think we can remove the mock store
User2: without the mock store the tests won't actually read the yaml files.
User2: The yaml files are read twice, once for getting the data and once for validating it.
User2: unfortunately I could work around that
User1: User2 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
User1: so can't we call that one method directly?
User2: Hmmm, sounds right. Will fix. :-D
User1: ty
User1: User3 thanks for review. i may have misunderstood your point about relation macaroon. i've replied to your comment. maybe we need to discuss?
User3: looking
User3: User1 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)
User1: User3 ah, right, i missed the bit about bakery storage not being model specific. i thought it was
User3: User1 alternatively make it blank, but then you'll get a new key for each register call, even for the same relation
User1: User3 i wanted to give it a name so i could delete it when the relation died
User3: User1 yep, makes sense
User1: User3 that was also the reason for naming the offer macaroon. but it can just be cleaned up by the ttl index
User3: User1 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
User3: i.e. refresh
User1: User3 the idea was that it would be a bearer token rather than have a caveat to discharge
User1: the controller uses the macaroon, not the user
User3: User1 I think it's fine one per consume for now, we can revisit that
User1: yeah
User1: User3 relation macaroon now includes model uuid
User1: in the id
User3: User1 reviewed
User1: ty
User1: yeah, i have got more testing
User3: User1 lxd storage PR, if you have time: https://github.com/juju/juju/pull/7635
User1: User3 swap you reviews? https://github.com/juju/juju/pull/7637
User3: User1 ok
User3: User1 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
User3: I think that's all the info we need, in order to create a storage instance, volume, and attachments
User1: User3 that would save an extra command for sure
User1: User3 but what about adding an ebs volume as detached storage
User3: User1 it wouldn't be possible, but it is it necessary? you can't add new volumes detached
User1: User3 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
User3: User1 ah yes, it won't work with --attach-storage
User1: maybe we could tweak attach-storage
User1: User3 i'm confused about where "juju-zfs" comes from - it that our (juju's) naming convention?
User3: User1 it's specified in DefaultPools
User3: User1 the juju storage pool definition includes an attribute "lxd-pool", which identifies the lxd storage pool to use
User3: User1 and if the lxd-pool attribute is missing, we use "juju" as the lxd storage pool name
User1: and zfs.pool_name s passed to lxd
User3: User1 yes, things other than driver/lxd-pool are passed directly to lxd
User1: User3 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
User1: *can't
User3: User1 thanks
User3: User1 not too fussed, your call
User1: maybe leave for now and take a view
User1: can see what others think
User3: User1 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"
User3: there is an error message we can check, but that could change
User1: yeah :-(
User3: User1 I'll just make it do a create, then check if it exists on failure
User3: ignoring the specifics of the create error
User1: i think that would be good
User1: s/good/better
User1: User4 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?
User4: User1 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
User4: so I think it is a separate bug, but one that we've had
User4: 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
User1: User4 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
User4: missing info, things that you'd pick up on what to do next while you're looking at the exact logs, etc.
User1: but as you say, it could be in 2.1
User4: User1 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
User1: exactly
User1: User4 will eric's PR, we could land as is and follow up with that %q tweak
User4: User1 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
User1: User4 the issue is a bit shit, we should land it. i can do that and easy to make the subsequent little tweak after that
User4: User1 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.
User1: yeah, ok. i'll land now and it should make it
User1: User4 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
User1: *off
User4: User1 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
User4: so I wasn't sure
User1: np
User1: i wasn't sure either
User1: until i tested manually
User5: Bug #1704099 opened: Juju state server not starting after reboot <4010> <juju-core:New> <juju-release-tools:New> <https://launchpad.net/bugs/1704099>
User1: User6 can you join us in release standup to update on release? https://hangouts.google.com/hangouts/_/canonical.com/juju-release
User6: User1 on my way
User5: Bug #1704099 changed: Juju state server not starting after reboot <4010> <juju:New> <https://launchpad.net/bugs/1704099>
juju-dev
Ubuntu IRC