| SpamapS | hazmat: I'm working on adding ssl cert verification to the EC2 provider if the local txaws supports it... | 00:55 |
|---|---|---|
| hazmat | SpamapS, cool | 01:35 |
| bigjools | fwereade_: hello | 05:27 |
| fwereade_ | bigjools, heyhey | 07:00 |
| bigjools | fwereade_: hey, I was going to ask about doing a constraints thing for maas but I figured it out, seems simple enough | 07:19 |
| fwereade_ | bigjools, cool | 07:20 |
| bigjools | might tap you for a review later :) | 07:20 |
| fwereade_ | bigjools, will maas expect to understand arch/cpu/mem? | 07:20 |
| bigjools | fwereade_: no, only name | 07:21 |
| fwereade_ | bigjools, eek, name might be tricky | 07:21 |
| bigjools | fwereade_: why? | 07:22 |
| fwereade_ | bigjools, I had tried to fight for it back in nov/dec --in orchestra -- on the basis that people will just abuse the classes functionality if we don't provide name | 07:22 |
| fwereade_ | bigjools, but in the end I didn't manage | 07:22 |
| bigjools | don't I just add a maas-name to the registered constraints? | 07:23 |
| bigjools | then the provider will deal with it | 07:23 |
| fwereade_ | bigjools, in fact I *just* dropped orchestra-name from the branch hazmat and I are planning to roll various constraints/env-settings changes into | 07:23 |
| fwereade_ | bigjools, it's not a technical problem, it's a philosophical one... and while I may personally feel that -name constraints are useful enough to warrant a slight loss in, er "purity" | 07:24 |
| fwereade_ | bigjools, ...my opinion did not carry the day :( | 07:25 |
| bigjools | well this is one of the requirements I was handed | 07:25 |
| bigjools | I'm not sure *where* it came from but I suspect the top IYKWIM | 07:25 |
| fwereade_ | bigjools, grar (not to you, just to the communication over the last few days) | 07:25 |
| fwereade_ | bigjools, if there's any way you can backtrace the requirement it would be a great help | 07:26 |
| bigjools | I will check later and let you know | 07:26 |
| fwereade_ | bigjools, awesome, tyvm | 07:27 |
| allenap | fwereade_: Good morning :) I have a couple of branches in the review queue for Juju. What's the normal approach for getting a review here? Do I peg someone, or do you all monitor the review queue? | 08:50 |
| fwereade_ | allenap, heyhey, hassling people is the recommended method | 08:50 |
| fwereade_ | allenap, it's not that we *don't* monitor it, but we can be laxer about it than we should be :( | 08:50 |
| allenap | fwereade_: Can I hassle you then? ;) Of the two, the more important one is https://code.launchpad.net/~allenap/juju/maas-endpoint-url-with-resource-uris/+merge/99353 since it actually makes things work end-to-end. | 08:51 |
| fwereade_ | allenap, a pleasure :) | 08:51 |
| allenap | fwereade_: I am much obliged :) | 08:52 |
| fwereade_ | allenap, generally LGTM; 2 things (that I'm writing up now for the record): (1) would you make .get and .post public so the tests don't hit internals and (2) we have slightly different formatting conventions: import style is different (see rest of codebase) and we don't like closing brackets (of whatever sort) on their own line | 09:00 |
| fwereade_ | allenap, (I'm not sure why we don't like it) | 09:00 |
| allenap | fwereade_: Sure, I'll fix those up. Thanks. | 09:01 |
| fwereade_ | allenap, other one LGTM too, just a couple of minors | 09:11 |
| allenap | fwereade_: Awesome, thank you. | 09:11 |
| fwereade_ | allenap, np, let me know when they're done and I'll hopefully get around to merging them not-too-long after | 09:12 |
| fwereade_ | allenap, and if I make you wait too long, hassle me again ;) | 09:12 |
| allenap | fwereade_: Haha, ta. | 09:12 |
| wrtp | TheMue: morning! | 09:21 |
| TheMue | wrtp: moin, just back from gym | 09:22 |
| TheMue | oh, restart needed, new version | 09:22 |
| TheMue | so, done | 09:23 |
| hazmat | bigjools, we had some of the name discussion yesterday on #cloud-dev, fwiw | 11:03 |
| wrtp | that lovely moment when you discover that the test you thought you'd just broken has actually been broken all along, for a different reason. | 11:58 |
| wrtp | fwereade_: do you think environments.yaml should fail to parse if unknown fields are used in it? | 11:59 |
| fwereade_ | wrtp, kind of, I was amused to discover recently that the schema had "default-ami" while the code (and the users) were using "default-image-id" | 12:00 |
| fwereade_ | wrtp, in go, given that we'll be unpacking into an actual type rather than a databag (right?) I don't think that'd be possible though | 12:01 |
| fwereade_ | wrtp, so in general I think it's quite nice to be able to accept a config file for a later schema so long as it's not directly incompatible | 12:02 |
| wrtp | fwereade_: ok. | 12:02 |
| wrtp | fwereade_: what wouldn't be possible? | 12:02 |
| fwereade_ | wrtp, although how necessary this is does interact interestingly with what we pick for our live-upgrade strategy | 12:02 |
| fwereade_ | wrtp, that class of error shouldn't be possible | 12:02 |
| wrtp | fwereade_: it's possible if the field is optional | 12:03 |
| wrtp | fwereade_: and we allow unrecognised fields | 12:03 |
| fwereade_ | wrtp, but the field has to exist on the type for the data to reach the provider | 12:03 |
| wrtp | fwereade_: no, the provider can see all the fields - it then puts them in its own data type | 12:04 |
| fwereade_ | wrtp, oh, it can see all the fields in the original yaml? | 12:04 |
| wrtp | fwereade_: yes. | 12:04 |
| wrtp | fwereade_: it then chooses which ones to discard. | 12:04 |
| fwereade_ | wrtp, ah, ok, hmm | 12:05 |
| wrtp | fwereade_: funny thing is i had a test to check that extra fields generated an error... but i didn't check the error, and it turned out to be failing for a different reason. | 12:05 |
| wrtp | s/extra/unknown/ | 12:05 |
| fwereade_ | wrtp, ouch | 12:05 |
| fwereade_ | wrtp, that'll learn ya :p | 12:05 |
| wrtp | fwereade_: yeah, am fixing it now. | 12:05 |
| wrtp | fwereade_: but that leaves me with the question: should it actually *be* an error? | 12:06 |
| wrtp | fwereade_: your anecdote above leads me to think that it should. | 12:06 |
| fwereade_ | wrtp, likewise; I just seem to recall niemeyer saying something about the schema stuff that implied it was intended behaviour | 12:07 |
| wrtp | fwereade_: i'll have a word with him later. | 12:08 |
| fwereade_ | wrtp, but it was long enough ago that I can't faithfully recall the motivation | 12:08 |
| fwereade_ | wrtp, cool | 12:08 |
| wrtp | fwereade_: thanks for the feedback | 12:08 |
| fwereade_ | wrtp, a pleasure, as always :) | 12:08 |
| wrtp | :-) | 12:08 |
| fwereade_ | allenap, that's merged, ping me when you've handled the conflicts | 12:20 |
| hazmat | its amazing how much more pleasant reviews are with reitveld | 12:29 |
| hazmat | such a simple thing | 12:29 |
| allenap | fwereade_: All done. | 12:29 |
| fwereade_ | allenap, ack | 12:29 |
| allenap | hazmat: Have you been using reitveld for Juju? The Go work, or for Python too? If the latter I'd like to give it a go if I have another Juju branch. | 12:31 |
| wrtp | hazmat: ain't it true | 12:31 |
| hazmat | allenap, both | 12:31 |
| hazmat | allenap, lbox propose -cr from a branch your ready to submit or resubmit | 12:31 |
| allenap | hazmat: Cool, I'll note that. | 12:31 |
| hazmat | the command is in the go ppa | 12:31 |
| hazmat | bcsaller, the diff for rel-type now seems to include jimbaker's unit-deploy refactor.. which is already on trunk, that seems off | 12:32 |
| allenap | Are you developing with golang-stable, -tip, or -weekly? | 12:35 |
| wrtp | allenap: weekly | 12:35 |
| allenap | Ta. | 12:35 |
| wrtp | allenap: until Go-1 is released. | 12:35 |
| allenap | wrtp: I guess that's not due in time for Precise? | 12:36 |
| wrtp | allenap: i think it probably will happen before precise, but that's only a hunch | 12:36 |
| wrtp | allenap: activity on the golang-dev list has peaked, i think. | 12:36 |
| wrtp | allenap: there aren't many outstanding bugs | 12:37 |
| allenap | wrtp: I ought to get my feet wet then. | 12:37 |
| wrtp | allenap: there's only one Go1=Must issue remaining. woo. | 12:37 |
| wrtp | allenap: and only 5 Priority=Go1 issues | 12:38 |
| wrtp | allenap: down from a few hundred recently. | 12:38 |
| hazmat | bcsaller, merging trunk resolves the diff issues | 12:38 |
| allenap | fwereade_: Oh rats, I completely missed out fixing the imports in get-machines-bug. I'll do that now. Sorry. | 12:46 |
| fwereade_ | allenap, np | 12:46 |
| allenap | fwereade_: Done. | 12:47 |
| wrtp | lunch! | 12:49 |
| fwereade_ | hazmat, how's the environment stuff coming? I've ended up skating close to it and fixing the tests is a bit scary | 13:10 |
| niemeyer | Good morning all! | 13:13 |
| fwereade_ | niemeyer, heyhey | 13:13 |
| niemeyer | fwereade_: Yo | 13:13 |
| fwereade_ | Daviey, ping | 13:14 |
| fwereade_ | niemeyer, I has a concern about constraints | 13:17 |
| fwereade_ | niemeyer, bigjools was saying this morning that as far as he knew maas-name *was* a required constraint | 13:17 |
| fwereade_ | niemeyer, I didn't find out the ultimate source of the requirement, which wuld have been the most helpful thing to do | 13:18 |
| fwereade_ | niemeyer, but there appears to be an expectations problem somewhere and I thought you should probably know ;) | 13:18 |
| niemeyer | fwereade_: I don't know anything about the requirements, but I do a few things about where we're headed too.. it'd be nice to see people coordinating a bit on those matters | 13:20 |
| fwereade_ | allenap, merged | 13:20 |
| allenap | fwereade_: Brilliant, thank you. | 13:20 |
| fwereade_ | niemeyer, wouldn't it just -- in response to your discussion with hazmat yesterday, he confirmed with [someone maas-related] that there would be no maas constraints | 13:21 |
| niemeyer | fwereade_: Oh, ok then.. so I'm not sure about what is being discussed? | 13:22 |
| niemeyer | fwereade_: maas-name *was* a required constraint, but there won't be any? | 13:22 |
| fwereade_ | niemeyer, bigjools seemed to be working on it this morning | 13:23 |
| fwereade_ | niemeyer, <bigjools> well this is one of the requirements I was handed | 13:23 |
| fwereade_ | I'm not sure *where* it came from but I suspect the top IYKWIM | 13:23 |
| niemeyer | fwereade_: Sorry.. I'm lost, there won't be any constraints, but bigjools was handed one this morning? | 13:24 |
| fwereade_ | niemeyer, I am also lost; this is the problem | 13:24 |
| niemeyer | bigjools: ping? | 13:25 |
| fwereade_ | niemeyer, if you feel there's no way to include constraints then that is your call; but I think there's someone somewhere who needs to know about it | 13:25 |
| niemeyer | fwereade_: Sorry, I'm again lost.. when did I say there was no way to include constraints? | 13:26 |
| niemeyer | fwereade_: This isn't in my hands.. | 13:26 |
| fwereade_ | niemeyer, you apeared to be saying that in G+ yesterday; I clearly misunderstood | 13:26 |
| niemeyer | fwereade_: I said I would definitely have ruled out a few things by now | 13:27 |
| niemeyer | fwereade_: Including both constraints and subordinates | 13:27 |
| fwereade_ | niemeyer, ok: then it comes purely down to the issue of "is maas-name philosophically compatible with juju?" | 13:27 |
| hazmat | niemeyer, i pasted you the log from the maas channel yesterday | 13:27 |
| hazmat | which amounted to what fwereade_ just said | 13:27 |
| niemeyer | fwereade_: That's not the same as saying there's no way to do it | 13:27 |
| niemeyer | hazmat: Thanks | 13:28 |
| niemeyer | hazmat: Where was that? | 13:28 |
| hazmat | niemeyer, via priv message | 13:28 |
| niemeyer | hazmat: When? | 13:28 |
| hazmat | http://pastebin.ubuntu.com/900527/ | 13:28 |
| hazmat | hmm. there's a bit more after that | 13:29 |
| hazmat | nope that's basically it | 13:31 |
| wrtp | niemeyer: do you think environments.yaml should fail to parse if unknown fields are used in it? | 13:37 |
| wrtp | niemeyer: (morning, BTW!) | 13:37 |
| niemeyer | wrtp: Probably in some cases.. if unknown fields to well known environments are used, for example | 13:38 |
| wrtp | niemeyer: i thought it did, but my test for it was wrong. so i'm wondering if schema.FieldMap should fail if extra fields are given, which it doesn't currently. or if there should be some way to do that. | 13:40 |
| niemeyer | wrtp: That was by design | 13:40 |
| wrtp | niemeyer: thanks for the review BTW. all improvements, i think. | 13:40 |
| niemeyer | wrtp: But maybe the design was wrong | 13:40 |
| wrtp | niemeyer: yeah, i thought so. | 13:41 |
| wrtp | niemeyer: could work both ways. | 13:41 |
| wrtp | niemeyer: if we decide we want to disallow extra fields in environments.yaml, then i think there should be an easy way to use schema to do that. | 13:41 |
| niemeyer | wrtp: Agreed | 13:56 |
| niemeyer | hazmat, fwereade_: Mail sent | 13:56 |
| fwereade_ | niemeyer, thank you, well put | 13:57 |
| hazmat | niemeyer, sounds good | 14:02 |
| hazmat | doctors appt, bbiab | 14:11 |
| TheMue | wrtp: see https://codereview.appspot.com/5905064/, I followed your approach with the private updater interface. a pretty neat solution. | 14:44 |
| wrtp | TheMue: thanks. having a look now. | 14:46 |
| TheMue | wrtp: thx | 14:47 |
| === hazmat is now known as kapilt | ||
| robbiew | wrtp: call time? | 15:08 |
| wrtp | robbiew: yup | 15:08 |
| wrtp | robbiew: was deep in review mode! | 15:09 |
| robbiew | wrtp: https://talkgadget.google.com/hangouts/extras/canonical.com/the-hulk-hideout | 15:09 |
| wrtp | TheMue: review delivered | 15:10 |
| wrtp | robbiew: i'm there | 15:10 |
| TheMue | wrtp: seen it | 15:10 |
| TheMue | wrtp: thx | 15:10 |
| wrtp | TheMue: np | 15:10 |
| robbiew | wrtp: interesting...so am I | 15:11 |
| * robbiew reloads | 15:11 | |
| * wrtp reloads too | 15:12 | |
| TheMue | wrtp: interesting thoughts, should discuss them after my chat with robbiew later | 15:16 |
| TheMue | wrtp: update() usage and variable renaming sound good, event handling has to be outlined. currently i've got no clue how we could do it. | 15:19 |
| wrtp | TheMue: sorry, was in my chat with robbiew :-) | 15:22 |
| wrtp | TheMue: i think the event handling stuff is near trivial | 15:22 |
| TheMue | wrtp: i've seen, and in a few moments i am so too :D | 15:22 |
| wrtp | TheMue: ping me when you're out | 15:23 |
| robbiew | TheMue: can we delay our call by 30min...at the top of the next hour? | 15:23 |
| wrtp | TheMue: ok, let's have that discussion now :-) | 15:23 |
| TheMue | robbiew: it's ok for me, yes | 15:23 |
| robbiew | TheMue: thnx | 15:24 |
| wrtp | TheMue: what's the difficulty with the event handling? | 15:27 |
| TheMue | wrtp: no concrete difficulty, but have to think about it | 15:31 |
| wrtp | TheMue: i *think* that if you just pass an initial event to waiter.init, and have updater.update take the event value, you should be good. | 15:32 |
| wrtp | TheMue: then the various update methods can just check if the event is what they expect. they might even want to close the event channel and stop when the node is deleted actually. | 15:33 |
| TheMue | wrtp: but we also may need an event mask, to accept multiple event types | 15:34 |
| wrtp | TheMue: i don't think so | 15:34 |
| wrtp | TheMue: there's nothing that's saying "this is the event we're accepting" | 15:34 |
| wrtp | TheMue: just pass all events to the update method | 15:34 |
| wrtp | TheMue: then the update method can decide whether to discard them or not. | 15:35 |
| TheMue | wrtp: ok, so you're thinking about a change to updater and watcher, but the current two concrete implementations won't change (a lot). but other concrete watchers may use the passed event? | 15:36 |
| wrtp | TheMue: yeah | 15:36 |
| TheMue | wrtp: sounds good, indeed | 15:36 |
| wrtp | TheMue: and the current ones may do so too. | 15:36 |
| wrtp | TheMue: e.g. for DELETED | 15:37 |
| TheMue | wrtp: to stop work, yes | 15:38 |
| wrtp | TheMue: yeah | 15:38 |
| TheMue | wrtp: what type of event would you pass to init() in case of the current implementations? just EVENT_CHANGED? | 15:44 |
| wrtp | TheMue: EVENT_CHANGED for the ContentsWatcher; EVENT_CHILD for the ChildWatcher | 15:45 |
| mthaddon | niemeyer: any chance you could take a look at those logs and let me know what URL to use for haproxy check for the store? | 15:46 |
| niemeyer | mthaddon: Sorry for the delay, will do it | 15:47 |
| mthaddon | thx | 15:47 |
| TheMue | wrtp: yeah, pass that as argument to loop() so it can be used as argument for update() | 15:47 |
| wrtp | TheMue: exactly | 15:47 |
| TheMue | wrtp: an elegant solution | 15:48 |
| wrtp | TheMue: i'm glad to get rid of that first "priming" event... it always seemed like a bit of a hack, but the alternative was worse. | 15:48 |
| wrtp | TheMue: (i mean the artificial event channel) | 15:49 |
| TheMue | wrtp: yip, this "first shot" is a hack | 15:49 |
| wrtp | TheMue: yeah, and no longer necessary :-) | 15:49 |
| TheMue | wrtp: great, love it | 15:50 |
| wrtp | TheMue: nice | 15:50 |
| wrtp | TheMue: am glad! | 15:50 |
| TheMue | wrtp: so, w/o stopping after a DELETED it already works | 16:00 |
| TheMue | wrtp: now i'll add that | 16:00 |
| wrtp | TheMue: cool. | 16:00 |
| wrtp | TheMue: oops, realised i never actually reviewed the testing code. | 16:01 |
| TheMue | wrtp: ;) | 16:01 |
| niemeyer | mthaddon: I'll do that right after lunch.. | 16:03 |
| niemeyer | biab | 16:03 |
| mthaddon | thx | 16:03 |
| robbiew | TheMue: ping :) | 16:11 |
| TheMue | robbiew: pong | 16:13 |
| robbiew | TheMue: 1:1? | 16:14 |
| TheMue | yip, i'm ready | 16:15 |
| robbiew | https://talkgadget.google.com/hangouts/extras/canonical.com/the-hulk-hideout | 16:15 |
| TheMue | robbiew: one moment, wrong google login | 16:16 |
| kapilt | fwereade_, ping | 16:22 |
| fwereade_ | kapilt, pong | 16:22 |
| kapilt | fwereade_, was about to merge force-upgrade branch, when i noticed a test that's failing, and i wanted to talk it over with you.. | 16:22 |
| fwereade_ | kapilt, sure | 16:23 |
| fwereade_ | kapilt, one of mine..? | 16:23 |
| kapilt | fwereade_, specifically its test_workflow/test_upgrade_error_before_hook | 16:23 |
| kapilt | fwereade_, yeah.. its the one that tries to see if the bundle has been downloaded and if so force execution of the hook | 16:23 |
| kapilt | for upgrade | 16:23 |
| kapilt | but afaics the upgrade hook was executed, and it failed, and then it was retried without hooks, so it shouldnt' be executed again | 16:24 |
| kapilt | the upgrade.ready stuff will always return true | 16:24 |
| kapilt | unless it raises an error, in which case its stopped anyways | 16:25 |
| kapilt | er. unless prepare raises an error | 16:25 |
| fwereade_ | kapilt, yes, that it true, the client did change | 16:25 |
| fwereade_ | kapilt, I'm still a little surprised that it's failing | 16:26 |
| fwereade_ | kapilt, what's going wrong? | 16:26 |
| kapilt | fwereade_, i removed the bit where it always executes the upgrade hook if upgrade.ready | 16:26 |
| kapilt | since the fire_hooks will take care of it | 16:27 |
| kapilt | and then the test fails because it didn't execute the upgrade-hook on a retry without hooks | 16:27 |
| fwereade_ | kapilt, wait, bundle *can* be none | 16:27 |
| fwereade_ | kapilt, when the unit already has the latest charm and ZK knows about it | 16:28 |
| fwereade_ | kapilt, in that case we *don't* need to run the upgrade again; and we may or may not want to fire_hooks | 16:28 |
| kapilt | fwereade_, the unit charm id is only sent after the succesful completion of upgrade | 16:29 |
| fwereade_ | kapilt, otherwise, we run it again, and we force fire_hooks to True to preserve the guarantee that the first hook to run against a new charm be upgrade-charm | 16:29 |
| kapilt | fwereade_, and prior to the transition there is a check for just that | 16:29 |
| fwereade_ | kapilt, the ZK charm id is set once the code has landed; not after the hook has run | 16:30 |
| fwereade_ | kapilt, if the hook itself fails then --retry-hooks is a valid parameter: we've already tried to run the hook, as guaranteed, and we're in the realm of manual intervention | 16:31 |
| kapilt | fwereade_, ic | 16:31 |
| TheMue | wrtp: seen your comments, the wrapper is curently - as long as we're discussing - for demonstration purposes, like in my case where i'm interested in changed config nodes. | 16:32 |
| TheMue | wrtp: shall be deleted, when it moves into a real usage | 16:32 |
| wrtp | TheMue: in that case, maybe it would be better as an example function? | 16:32 |
| kapilt | fwereade_, the question is why should we execute the upgrade hook on a retry/resolved without specifying hook retry | 16:33 |
| TheMue | wrtp: it is an examle function, just embedded into the test. *lol* | 16:33 |
| wrtp | TheMue: it's confusing that it looks like a test when it's just a demo. | 16:33 |
| TheMue | wrtp: why the delete of line 189? | 16:33 |
| wrtp | TheMue: it's checked later | 16:34 |
| TheMue | wrtp: as i said, only as long as we are discussing. will be removed when the watcher is accepted | 16:34 |
| TheMue | wrtp: oh, yes, just seen it, my fault | 16:34 |
| wrtp | TheMue: as usual, what you're proposing should be the final thing. | 16:34 |
| kapilt | fwereade_, the situation to avoid is execution the upgrade hook twice for the same upgrade.. i kinda of see the reasoning thoug | 16:34 |
| wrtp | TheMue: if you need a demo, you can use a pastebin | 16:35 |
| fwereade_ | kapilt, my understanding is that we're aiming for a guarantee of at least one execution per relevant hook | 16:35 |
| kapilt | fwereade_, this seems simpler if we just set the charm id after the hook exec | 16:35 |
| TheMue | wrtp: pastebin is a bad tool, unordered, no workflow in it, you can't easily see how one paste is related to another one | 16:36 |
| kapilt | fwereade_, definitely, but avoiding extra hook executions where possible is also good | 16:36 |
| kapilt | fwereade_, in this case it does the at least once, but it also does at least twice if the hook failed | 16:36 |
| TheMue | wrtp: a proposal is a proposal, it can be modified, it can be rejected, it can be discussed | 16:36 |
| fwereade_ | kapilt, probably slightly so; only consequence is that we potentially end up overwriting stuff the used "fixed" during his previous manual intervention | 16:36 |
| kapilt | fwereade_, oh.. i guess not | 16:36 |
| wrtp | TheMue: it doesn't really matter. we're reviewing the watcher package, not the code that's using it. | 16:37 |
| fwereade_ | kapilt, I don't *think* it's always twice | 16:37 |
| wrtp | TheMue: i don't think that demo adds anything to the discussion - it's clear that it's possible to use it that way, i think. | 16:37 |
| TheMue | wrtp: back in a few minutes | 16:37 |
| kapilt | fwereade_, your right its not always twice, the logic here makes more sense now, thanks | 16:38 |
| kapilt | fwereade_, its just that on a forced upgrade we don't want to execute the upgrade hook | 16:39 |
| kapilt | so the logic is a bit at odds with that since it internally forces the executiono when it extracts | 16:39 |
| fwereade_ | kapilt, ah-ha... sorry, I totally missed that | 16:39 |
| kapilt | fwereade_, no worries, it was a future use case | 16:41 |
| kapilt | the future is now ;-) | 16:41 |
| fwereade_ | kapilt, ok, actually, I'm confused... why don;t we want to execute the hook? | 16:41 |
| TheMue | wrtp: so, had to fetch the rabbits ;) | 16:42 |
| kapilt | fwereade_, on a forced upgrade we can be in any state | 16:42 |
| fwereade_ | kapilt, bingo | 16:42 |
| kapilt | fwereade_, forced upgrade means just put the charm into place | 16:42 |
| kapilt | so its out of band of the workflow | 16:42 |
| fwereade_ | kapilt, kinda kills the upgrade-charm guarantee, though, and that makes me a touch nervous | 16:43 |
| kapilt | fwereade_, hence the --force param | 16:43 |
| kapilt | fwereade_, its meant as a developer option per the feedback on list from the buildbot/lp yellowteam | 16:43 |
| fwereade_ | kapilt, true, but it would seem appropriate to at least run the hooks for any units that aren't in an error state | 16:44 |
| fwereade_ | kapilt, or maybe not | 16:44 |
| fwereade_ | kapilt, anyway, do what you have to do ;) | 16:45 |
| kapilt | fwereade_, think about from the responsibilities of an upgrade-hook | 16:45 |
| kapilt | fwereade_, if they have to execute in completely broken environments, there's relatively little hope of writing one correctly | 16:45 |
| kapilt | i mean it could be in an install_error.. the upgrade hook wouldn't be able to assume anything about the system | 16:46 |
| fwereade_ | kapilt, but if it's just running happily, should we still not execute the hook? | 16:47 |
| kapilt | fwereade_, not if its being forced, because then it has multiple meanings | 16:47 |
| kapilt | that are contextually dependent on an individual units | 16:47 |
| kapilt | but your operating on the service | 16:48 |
| kapilt | we give users the tools to do this correctly, via resolved | 16:48 |
| kapilt | and inform them of the need when doing a normal upgrade | 16:49 |
| fwereade_ | kapilt, I'm mainly concerned about charms that store data in their own dir and change its representation in an upgrade hook | 16:49 |
| fwereade_ | kapilt, I suppose we could just say "don't do that then" | 16:50 |
| fwereade_ | kapilt, but that feels a bit unhelpful | 16:50 |
| kapilt | fwereade_, we already do effectively when we tell them to resolve non started units | 16:50 |
| kapilt | force is mainly meant for charm authors to more easily iterate on dev | 16:50 |
| fwereade_ | kapilt, I guess the consequence is simply that if you do change representation, you should always be prepared for data to be stored in an old format | 16:51 |
| fwereade_ | kapilt, still feels unhelpful to me tbh, but at least it's easily stated and only really important to devs | 16:52 |
| fwereade_ | kapilt, consider me convinced | 16:52 |
| fwereade_ | kapilt, I have to go in a moment, but would you give me a v quick status update on env settings? | 16:53 |
| kapilt | fwereade_, sure, i'm still doing merges and reviews, i'm hoping to get back to it this evening, i'll at least push what i have | 16:55 |
| fwereade_ | kapilt, I'm worrying that enough bits of the code end up touching env settings that we're going to require them across a vast majority of the tests, and it scares me | 16:56 |
| fwereade_ | kapilt, is this something you've hit? | 16:56 |
| fwereade_ | kapilt, or is it just something I'm hitting in trying to make constraints construction provider-dependent? | 16:56 |
| kapilt | fwereade_, not really outside of the provisioning agent | 16:57 |
| kapilt | and the gssm tests | 16:57 |
| fwereade_ | kapilt, hmm, ok | 16:57 |
| fwereade_ | kapilt, at least I won't have to worry about too many conflicts then | 16:58 |
| fwereade_ | kapilt, gtg, may try to find you later tonight | 16:58 |
| kapilt | fwereade_, cheers | 17:01 |
| TheMue | wrtp: test with DELETED works | 17:01 |
| wrtp | TheMue: cool | 17:01 |
| TheMue | wrtp: next propose will rush in in a few moments | 17:01 |
| wrtp | TheMue: i've got to go shortly anyway | 17:02 |
| TheMue | wrtp: ok | 17:02 |
| kapilt | SpamapS, niemeyer, the mir report throughness is impressive | 17:03 |
| niemeyer | kapilt: Indeed.. high quality review | 17:03 |
| kapilt | one more merge, and i'll work on a reply | 17:04 |
| * kapilt sighs about the lack of time for coding | 17:04 | |
| SpamapS | Always nice to have a fresh set of eyes on things. :) | 17:06 |
| wrtp | gotta go. see y'all tomorrow. | 17:06 |
| TheMue | wrtp: have a nice evening | 17:07 |
| kapilt | niemeyer, incidentally are there any of the specs (jim's or mine) that you didn't feel ready to go in with (minus some polish on each)? | 17:07 |
| TheMue | so, off for now, bye | 17:14 |
| niemeyer | kapilt: Not sure.. I'd have to review them again, but quite a few looked promising | 17:22 |
| kapilt | niemeyer, cool.. also regarding the provider address changing bug discussion from yesterday | 17:23 |
| kapilt | do you have time to delve into that a bit more | 17:23 |
| kapilt | afaics we don't have many options outside of a timer to update it | 17:23 |
| niemeyer | kapilt: I do.. just give me a couple of minutes as I prepare some post-lunch coffee and I'll be fully ready | 17:23 |
| kapilt | niemeyer, sure | 17:23 |
| niemeyer | kapilt: Ready.. hangout? | 17:27 |
| kapilt | niemeyer, sure, invitet out | 17:29 |
| kapilt | jimbaker, ping | 18:27 |
| kapilt | just wanted to see if you wanted to chat about the rel-* reviews | 18:28 |
| jimbaker | kapilt, it | 18:47 |
| jimbaker | is fine, but so far they seem straightforward | 18:47 |
| jimbaker | btw, quick reminder, i will be taking tomorrow off for a vacation day | 18:48 |
| kapilt | jimbaker, cool, i saw it on the calendar | 18:50 |
| jimbaker | kapilt, cool, my kids on are spring break this week, so we will be skiing at vail tomorrow, then they will be in ski school rest of the week | 18:51 |
| jimbaker | or ride school | 18:51 |
| jimbaker | (bringing up snowboards and skis) | 18:52 |
| kapilt | jimbaker, keep in mind, after thursday, there are no more features, only bugfixes. | 18:52 |
| kapilt | jimbaker, so if we want to land rel-id support, it needs to land before then | 18:53 |
| jimbaker | kapilt, that deadline is fine assuming it's compatible with the relation-id work already in review | 18:53 |
| kapilt | jimbaker, yeah.. it should be that was mostly a syntatic change for rel-id.. i'm going through rel-hook-context right now | 18:53 |
| kapilt | you'll have another review shortly | 18:54 |
| jimbaker | ok, and take a look please at relation-hook-commands | 18:54 |
| jimbaker | kapilt, i see why the relation-id-option branch (i was mistakenly calling it relation-hook-commands, i should know) is not off the kanban, it was marked as honolulu | 19:01 |
| jimbaker | not on the kanbad, rather | 19:02 |
| jimbaker | or whatever | 19:02 |
| kapilt | jimbaker, yeah.. lbox has a bug | 19:02 |
| kapilt | jimbaker, with multiple milestones it picks the last one it finds, instead of the oldest open | 19:02 |
| jimbaker | kapilt, got it | 19:02 |
| kapilt | jimbaker, the challenge of a consistent view here is interesting | 19:27 |
| jimbaker | you mean the shared topology aspect? | 19:27 |
| jimbaker | kapilt, ^ | 19:28 |
| kapilt | jimbaker, yes | 19:38 |
| kapilt | jimbaker, doing it that way feels a bit too fragile | 19:38 |
| kapilt | jimbaker, i think its better to just get the relations upfront | 19:39 |
| jimbaker | kapilt, you mean cache all available relation contexts? | 19:40 |
| jimbaker | kapilt, i'm not certain about fragility btw | 19:40 |
| kapilt | jimbaker, effectively yes, its basically one round trip to zk to get them all | 19:40 |
| jimbaker | because this sort of caching is being used in reading self._topology anyway | 19:41 |
| kapilt | jimbaker, just from a code perspective, not in practice.. passing around topologies.. isn't really right, else we should have one everywhere | 19:41 |
| jimbaker | and iirc there's some code duplication of functionality from other state stuff tehre | 19:41 |
| kapilt | jimbaker, we don't have it in state managers | 19:41 |
| jimbaker | kapilt, agreed | 19:41 |
| jimbaker | but it's in the hook code | 19:41 |
| kapilt | and this introducing it there.. again it feels like the wrong solution | 19:41 |
| kapilt | jimbaker, its introducing it to relation state manager as well | 19:42 |
| jimbaker | yes... to avoid code dupe | 19:42 |
| jimbaker | but if we follow the existing hook practice, we would just move that code to hook as well | 19:42 |
| kapilt | well more than that.. to have some semblance of consistency | 19:42 |
| kapilt | else rel-id and would disagree possibly | 19:42 |
| kapilt | jimbaker, what existing practice, you mean user hooks? | 19:43 |
| jimbaker | kapilt, let me find the specific example i have in mind... | 19:43 |
| kapilt | jimbaker, what's wrong with grabbing them up front? | 19:43 |
| kapilt | and removing the topology passing? | 19:43 |
| kapilt | the only time the distinction of what's in the topology is relevant here, is when your fetching relations, if you fetch them up front, your done. | 19:43 |
| jimbaker | kapilt, nothing, except for the fact that there's already precedent for what i did... which might mean, we should fix that part too | 19:44 |
| kapilt | jimbaker, the precendent is on the basis of consistency for a single hook context, not multi-hook contexts | 19:44 |
| kapilt | instead of weaving them together, compose them | 19:44 |
| kapilt | that would be simpler, and remove the passing | 19:45 |
| jimbaker | kapilt, that seems like a very reasonable distinction | 19:45 |
| jimbaker | regardless we will have opportunities for inconsistency, just the nature of how we use ZK | 19:45 |
| kapilt | jimbaker, its go's motto ;-) Don't communicate by sharing memory; share memory by communicating | 19:45 |
| kapilt | jimbaker, the call to get_relations_for_a service is consistent | 19:46 |
| jimbaker | kapilt, i suppose so | 19:46 |
| jimbaker | kapilt, anyway, it's not a very important distinction in my mind - so i'm happy to go with your proposal | 19:47 |
| kapilt | jimbaker, we should talk about the consistency though, it should be clearly convergent in behavior to something consistent | 19:48 |
| kapilt | jimbaker, if a relation departs during execution, the rel hook context still works, and the depart hook is pending | 19:49 |
| jimbaker | kapilt, i would like to point out that the topology is never mutated | 19:49 |
| jimbaker | for the hook contexts | 19:49 |
| kapilt | jimbaker, sure, but i'm not sure that's relevant, its basically trying to pass it around as a read cache, doesn't change that its being woven through the call stack | 19:50 |
| jimbaker | kapilt, again, it does ensure consistency. but again if we want to read the topology several times, possibly with differences, well, that's fine with me | 19:51 |
| kapilt | jimbaker, since we have a well defined point already that we can ensure consistent view, that's a more appropriate place. its a cost for every hook execution.. true.. but its one round trip to zk, and a few objects. we're executing processes here ;-) | 19:52 |
| kapilt | jimbaker, we shouldn't be reading the topology mulitple times | 19:52 |
| jimbaker | kapilt, i don't think cost has anything to do with it | 19:52 |
| kapilt | jimbaker, well not while the hook is executing | 19:52 |
| jimbaker | kapilt, so what's the plan then? | 19:52 |
| kapilt | jimbaker, what reads the topology? | 19:53 |
| kapilt | does a hook context read the topology before your branch? | 19:53 |
| jimbaker | kapilt, i'm pretty sure it does, it's right there in HookContext | 19:53 |
| jimbaker | ;) | 19:53 |
| kapilt | does the invoker? | 19:53 |
| jimbaker | kapilt, no | 19:54 |
| kapilt | jimbaker, right it was doing that for per hook context consistency | 19:54 |
| jimbaker | kapilt, we keep it separate, since HookContext is in juju.state it is privileged to work with topology | 19:54 |
| kapilt | jimbaker, with multiple hook contexts, and one read you end up passing the topology as a required arg | 19:54 |
| jimbaker | kapilt, so you do want to share it then? | 19:55 |
| kapilt | jimbaker, a helper method in the state.hook module | 19:55 |
| jimbaker | then it's fine | 19:55 |
| kapilt | jimbaker, not with optional params | 19:55 |
| kapilt | and not weaving sharing topologies into state managers | 19:55 |
| kapilt | s/shared | 19:55 |
| jimbaker | kapilt, i see little difference, but it's ok with me | 19:55 |
| kapilt | jimbaker, you up for a g+? | 19:56 |
| jimbaker | kapilt, sounds good to me | 19:56 |
| kapilt | jimbaker, invite out | 20:00 |
| andrewsmedina | hazmat ? | 21:17 |
| jimbaker | kapilt, i think i got the instructions wrong on using lbox | 21:55 |
| jimbaker | i see lbox submit actually merges into trunk | 21:56 |
| jimbaker | this is rather unfortunate | 21:56 |
| jimbaker | also i suspect this will now screw up my entire pipeline too once i back out the change | 21:58 |
| jimbaker | anyway, change is now backed out in r502 | 22:08 |
| kapilt | jimbaker, ack | 22:38 |
| kapilt | jimbaker, also sent a bit more on the review via reitveld | 22:39 |
| kapilt | hm.. that's odd | 22:39 |
| === kapilt is now known as hazmat | ||
| jimbaker | kapilt, thanks | 22:49 |
| hazmat | bcsaller, greetings | 22:52 |
| hazmat | bcsaller, i was just looking over your mp, and it looks like the trunk merge went bad | 22:52 |
| hazmat | bcsaller, specifically it reverts the status-changes commit | 22:57 |
| bcsaller | those are merged later, but I can check it again | 22:57 |
| bcsaller | wonder why that happened, I've been refactoring status in the later branch anyway | 22:58 |
| hazmat | bcsaller, yeah.. i'm not really able to make sense of what happened to it, i've tried updating to a non head branch and doing the merge, which worked yesterday.. but seems like bzr always wants to merge to head | 23:01 |
| hazmat | not clear | 23:01 |
| hazmat | andrewsmedina, pong | 23:01 |
| andrewsmedina | hazmat: hi | 23:02 |
| andrewsmedina | hazmat: you made that change in the bootstrap to define configurations of environments? | 23:02 |
| hazmat | andrewsmedina, not yet | 23:03 |
| hazmat | andrewsmedina, although i'm not sure if we're talking about the same thing | 23:03 |
| hazmat | andrewsmedina, you mean the don't sync env every command? and split provider settings out to a different node? | 23:03 |
| hazmat | er. every deploy | 23:04 |
| andrewsmedina | hazmat: yes | 23:08 |
| andrewsmedina | hazmat: I'm talking about your branch ~hazmat/juju/environment-settings | 23:08 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!