| davecheney | Path conflict: cmd/juju/ssh.go.THIS / cmd/juju/ssh.go | 00:39 |
|---|---|---|
| davecheney | why does this keep happening | 00:39 |
| davecheney | this file isn't even in this branch | 00:39 |
| rogpeppe | fwereade_: mornin' | 06:11 |
| rogpeppe | TheMue: hiya | 06:15 |
| TheMue | rogpeppe: morning | 06:16 |
| TheMue | rogpeppe: up so early in the morning? | 06:16 |
| rogpeppe | TheMue: yeah, lying awake, so got up | 06:16 |
| TheMue | rogpeppe: makes sense | 06:17 |
| rogpeppe | fwereade_: ping | 06:17 |
| rogpeppe | fwereade_: ping | 07:01 |
| fwereade_ | rogpeppe, pong | 07:12 |
| rogpeppe | fwereade_: i've done a fix for the internal traffic thing, but need to write a test for it, and that requires deploying a charm live, and i'm not quite sure of the best approach | 07:13 |
| rogpeppe | fwereade_: the approach i'm thinking of is to use testing/charm and assume that the charms in there are portable | 07:14 |
| fwereade_ | rogpeppe, sound like something we have little option but to leave to the FTs if that's so -- presumably you can at least check the security groups though? | 07:15 |
| rogpeppe | fwereade_: i.e. copy one of the charms into a local directory under the current series | 07:15 |
| fwereade_ | rogpeppe, sorry, what *precisely* are you trying to write a test for? | 07:15 |
| rogpeppe | fwereade_: tbh i want to write the test anyway. i don't see why we shouldn't have one. | 07:15 |
| rogpeppe | fwereade_: i want to check that a new machine comes up and manages to connect to the state. | 07:16 |
| rogpeppe | fwereade_: actually, i *can* do that without deploying a charm. | 07:16 |
| fwereade_ | rogpeppe, well, indeed, +1 to testing everything, but -1 to actually touching outside state in unit tests | 07:16 |
| rogpeppe | fwereade_: what do you mean by "touching outside state"? we already do, in the ec2 live tests. | 07:17 |
| rogpeppe | fwereade_: i think it would be good to have a test that deploys a charm in jujutest/livetests | 07:18 |
| fwereade_ | rogpeppe, does that run every time? | 07:18 |
| rogpeppe | fwereade_: no|! | 07:18 |
| fwereade_ | rogpeppe, +1 to that | 07:18 |
| fwereade_ | rogpeppe, sorry, miscommunications clearly | 07:18 |
| rogpeppe | fwereade_: you'd know if it did - it takes about 12 minutes to run | 07:18 |
| rogpeppe | fwereade_: so... | 07:18 |
| fwereade_ | rogpeppe, yeah, I thought so | 07:18 |
| fwereade_ | rogpeppe, which is why I don't consider it to be amongst the "unit tests" | 07:19 |
| rogpeppe | fwereade_: that's why i didn't call it a "unit test" :-) | 07:19 |
| fwereade_ | rogpeppe, although I am aware that the various categories of test are somewhat fuzzily defined | 07:19 |
| fwereade_ | rogpeppe, "but -1 to actually touching outside state in unit tests" | 07:19 |
| rogpeppe | fwereade_: ah yes | 07:20 |
| fwereade_ | rogpeppe, aaanyway, good morning :) | 07:20 |
| rogpeppe | fwereade_: top of the day to yourself too, sir :-) | 07:20 |
| TheMue | fwereade_: good morning | 07:21 |
| fwereade_ | TheMue, heyhey | 07:21 |
| rogpeppe | fwereade_: so, do you think we should have a repository of charms, one per series, with charms that are actually intended to run; or do you think we can assume that the charms that we use in testing are portable across series? | 07:21 |
| fwereade_ | rogpeppe, I feel like that should be a pretty safe assumption, and pretty easy to rectify if we balls it up | 07:21 |
| fwereade_ | rogpeppe, I *would* like to lean up the testing repo though | 07:22 |
| rogpeppe | fwereade_: perhaps we could add a "series" argument Repo.ClonedURL | 07:23 |
| rogpeppe | s/Repo/to Repo/ | 07:23 |
| fwereade | rogpeppe, sorry, I suspect I missed stuff; last I saw was "...t the charms that we use in testing are portable across series?" | 07:24 |
| rogpeppe | fwereade: yes, but we need a URL with the correct series in | 07:24 |
| fwereade | rogpeppe, sure, but the series is almost entirely irrelevant to the testing charms -- its structure happens to be a Repo but that's basically coincidental | 07:25 |
| rogpeppe | fwereade: (funny really how the "URL" in charm.URL is anything but "universal" - it only finds charms :-]) | 07:25 |
| fwereade | rogpeppe, heh, indeed | 07:25 |
| rogpeppe | fwereade: yes, but when we put the charm into the state, it needs to be put in with the correct series (i think!) so that the remote machine will use it correctly. | 07:26 |
| fwereade | rogpeppe, the series of the charm determines the series of the machine | 07:26 |
| fwereade | rogpeppe, pick the series you want, construct a URL, and you're done... right? | 07:27 |
| rogpeppe | fwereade: the URL has to actually refer to something | 07:27 |
| fwereade | rogpeppe, why? | 07:27 |
| fwereade | rogpeppe, you can make up whatever crap you like | 07:27 |
| rogpeppe | fwereade: because it gets pushed to the state | 07:27 |
| rogpeppe | fwereade: so you'd need to copy the directory too. it's not hard of course, but perhaps worth a function in testing, no? | 07:28 |
| fwereade | rogpeppe, I do that sort of thing in a couple of places around the tests for Uniter and BundlesDir... perhaps there's something relevant there | 07:29 |
| rogpeppe | fwereade: i'd like to use this as an excuse to live-test our standard deploy mechanism. | 07:29 |
| fwereade | rogpeppe, then I don;t see what needs to be put in state that isn't already handled by deploy | 07:29 |
| rogpeppe | fwereade: we need to give a charm to deploy, no? | 07:30 |
| fwereade | rogpeppe, right | 07:30 |
| rogpeppe | fwereade: which needs to be in a directory named after a valid series | 07:30 |
| fwereade | rogpeppe, I *see* | 07:30 |
| fwereade | rogpeppe, (or just using a faked-up charm store if I want to be pedantic, but yes, that is the best way) | 07:31 |
| rogpeppe | fwereade: yeah, we could do that do | 07:31 |
| rogpeppe | to | 07:31 |
| rogpeppe | too :-) | 07:31 |
| fwereade | rogpeppe, anyway, sorry, my perspective has been skewed looking at this problem from the UA perspective, when I can force the right context on myself everything you say makes sense | 07:33 |
| rogpeppe | fwereade: np! | 07:33 |
| fwereade | rogpeppe, and I think it's just fine to assume that testing charms are valid on every series | 07:33 |
| fwereade | rogpeppe, I'm not sure any of them *actually* do anything | 07:33 |
| rogpeppe | fwereade: well, we can rectify that in the future... | 07:33 |
| fwereade | rogpeppe, but that shouldn't be a problem for this test, right? | 07:34 |
| rogpeppe | fwereade: exactly | 07:34 |
| fwereade | rogpeppe, cool | 07:34 |
| rogpeppe | fwereade: i wonder about some prefix for names of charms that are intended for live testing | 07:34 |
| rogpeppe | fwereade: "live-basic" for a starter charm, perhaps, that does nothing other than install | 07:35 |
| rogpeppe | fwereade: then it's obvious which charms need to be looked at from the point-of-view of "will this actually run?" | 07:35 |
| fwereade | rogpeppe, hmm, maybe... part of me is saying we should put live test charms in the charm store ;p | 07:36 |
| rogpeppe | fwereade: that's not a bad idea actually | 07:36 |
| fwereade | rogpeppe, it has aspects of a good idea and aspects of a bad idea ;) | 07:36 |
| rogpeppe | fwereade: except that i can't do that now | 07:37 |
| fwereade | rogpeppe, quite so -- for now I would just slap some oneiric, precise, quantal symlinks into the testing repo and call it a day | 07:37 |
| rogpeppe | fwereade: ah, now yer talkin'! | 07:37 |
| fwereade | :D | 07:38 |
| === TheMue_ is now known as TheMue | ||
| rogpeppe | fwereade: maybe we should give explicit "series" arguments to all the functions in testing.Repo | 07:39 |
| rogpeppe | fwereade: that way we can easily asked for a cloned charm in a particular series for example | 07:40 |
| rogpeppe | fwereade: | 07:45 |
| rogpeppe | // ClonedURL makes a copy of the charm directory named name | 07:46 |
| rogpeppe | // into the destination directory (creating it if necessary), | 07:46 |
| rogpeppe | // and returns a URL for it. The URL will have the given series. | 07:46 |
| rogpeppe | func (r *Repo) ClonedURL(dst, series, name string) *charm.URL { | 07:46 |
| rogpeppe | fwereade: i think that's all we need. | 07:46 |
| fwereade | rogpeppe, feels like overkill, surely? can't you use a local repo pointing to the dir with the symlinks and leave it at that? | 07:47 |
| fwereade | rogpeppe, I worry I have skewed context again | 07:48 |
| rogpeppe | fwereade: i don't think so. all the methods in testing.Repo hard-code the series name "series" | 07:48 |
| fwereade | rogpeppe, yeah, testing.Repo *might* even be meant to be internal really | 07:48 |
| rogpeppe | fwereade: yeah, i wondered about that | 07:48 |
| fwereade | rogpeppe, have a vague feeling something used it once, maybe | 07:49 |
| rogpeppe | fwereade: it's quite nice as a namespace for all those methods | 07:49 |
| fwereade | rogpeppe, true | 07:49 |
| fwereade | rogpeppe, but its path is also a perfectly good path for JUJU_REPOSITORY, I think | 07:50 |
| fwereade | rogpeppe, does it have a clone-the-whole-thing method? | 07:50 |
| rogpeppe | fwereade: that's actually insufficient, i think, as cloned charms can't be under the same path | 07:50 |
| rogpeppe | fwereade: it should do. | 07:50 |
| rogpeppe | fwereade: or at any rate, any requested charm should be cloned. | 07:52 |
| rogpeppe | fwereade: what was the last thing you saw? | 07:52 |
| rogpeppe | fwereade: i saw: | 07:52 |
| rogpeppe | [08:50:35] <fwereade> rogpeppe, does it have a clone-the-whole-thing method? | 07:52 |
| fwereade | <fwereade> rogpeppe, or alternatively just clone a charm to where you want it for one specific test, which may well be in a newly-hacked-up repo with the perfect series alredy | 07:53 |
| fwereade | rogpeppe, I saw fwereade: it should do. | 07:53 |
| rogpeppe | [08:52:19] <rogpeppe> fwereade: or at any rate, any requested charm should be cloned. | 07:53 |
| fwereade | rogpeppe, I think it's more just that any dir we tell juju about to should be outside source control | 07:54 |
| fwereade | s/to // | 07:54 |
| rogpeppe | fwereade: indeed | 07:54 |
| rogpeppe | fwereade: maybe it should be a fixture | 07:55 |
| rogpeppe | CharmSuite | 07:55 |
| rogpeppe | fwereade: then we can have JujuConnSuite use it and most other things get it for free | 07:56 |
| fwereade | rogpeppe, I'm not sure that's justified -- lots of tests already get by just cloning the one or two they need, and that's a lot of cloning that might otherwise be unnecessary | 07:56 |
| rogpeppe | fwereade: i'd make it so that it only clones on request | 07:56 |
| rogpeppe | fwereade: if you ask for a charm, it ensures it's cloned, then gives you a reference to it | 07:57 |
| fwereade | rogpeppe, fair enough then | 07:57 |
| rogpeppe | fwereade: the advantage of making it a fixture is that it's clear who has responsibility for cleaning up the clones | 07:57 |
| rogpeppe | fwereade: as it is, clients have that responsibility, and that means you can't use a single global JUJU_REPOSITORY | 07:58 |
| fwereade | rogpeppe, yeah, sounds sensible | 07:58 |
| rogpeppe | fwereade: actually, i don't think you *can* use a single global JUJU_REPOSITORY | 07:59 |
| rogpeppe | fwereade: because you might have more than one active CharmSuite | 08:00 |
| fwereade | rogpeppe, ha, yes | 08:00 |
| rogpeppe | fwereade: but i don't wanna use the env var anyway; we'll just have CharmSuite.Path or something | 08:00 |
| rogpeppe | RepoPath probably | 08:00 |
| fwereade | rogpeppe, yep, sgtm | 08:00 |
| rogpeppe | fwereade: ok, seems like a plan | 08:01 |
| rogpeppe | fwereade: funny how fixing something fairly trivial (internal traffic enabling) leads on to a seemingly totally unrelated code change. | 08:01 |
| fwereade | rogpeppe, yeah, one takes some strange paths on occasion :) | 08:02 |
| rogpeppe | fwereade: oh, that's *much* nicer now | 08:24 |
| fwereade | rogpeppe, cool :D | 08:24 |
| rogpeppe | fwereade: 16 lines less and more general | 08:26 |
| fwereade | rogpeppe, awesome | 08:27 |
| Aram | moin. | 08:30 |
| fwereade | Aram, heyhey | 08:30 |
| TheMue | Aram: moin moin | 08:32 |
| rogpeppe | Aram: hiya | 08:44 |
| fwereade | evening davecheney | 09:17 |
| fwereade | rogpeppe, you recall that jujud-not-on-PATH thing I pasted last night? | 11:16 |
| rogpeppe | fwereade: yes | 11:16 |
| rogpeppe | fwereade: i have a feeling that our cloud-init doesn't set up PATH, though i haven't checked | 11:17 |
| fwereade | rogpeppe, ah ok -- isn't the path the the executable predictable based on tools, though? | 11:17 |
| fwereade | rogpeppe, maybe we should be using an absolute path | 11:17 |
| rogpeppe | hmm. the path should be based on agent name | 11:18 |
| rogpeppe | fwereade: /var/lib/juju/tools/$agent/jujud i think | 11:18 |
| fwereade | rogpeppe, and we know what agent we're trying to run :) | 11:18 |
| fwereade | rogpeppe, and that's all somewhere in container? | 11:18 |
| rogpeppe | fwereade: yeah, although perhaps it might seem better for the agent Main to set up the PATH | 11:19 |
| rogpeppe | fwereade: no, not in container | 11:19 |
| rogpeppe | fwereade: container just does a path search | 11:19 |
| fwereade | <fwereade> rogpeppe, ah, I thought that was what actually installed the unit | 11:20 |
| rogpeppe | fwereade: last thing i saw: | 11:20 |
| rogpeppe | [12:18:59] <fwereade> rogpeppe, and that's all somewhere in container? | 11:20 |
| rogpeppe | [12:19:23] <rogpeppe> fwereade: yeah, although perhaps it might seem better for the agent Main to set up the PATH | 11:21 |
| rogpeppe | [12:19:38] <rogpeppe> fwereade: no, not in container | 11:21 |
| rogpeppe | [12:19:48] <rogpeppe> fwereade: container just does a path search | 11:21 |
| rogpeppe | [ | 11:21 |
| fwereade | rogpeppe, you are probably right that they should just be on PATH | 11:22 |
| fwereade | rogpeppe, but well hmm | 11:22 |
| fwereade | rogpeppe, multiple versions | 11:22 |
| rogpeppe | fwereade: that's fine | 11:22 |
| fwereade | rogpeppe, how so? | 11:22 |
| rogpeppe | fwereade: the agent dir is a symlink to the right version | 11:22 |
| rogpeppe | fwereade: that's how we upgrade | 11:23 |
| fwereade | rogpeppe, ok, sorry, so what exactly goes on PATH? | 11:23 |
| rogpeppe | fwereade: i'll just check | 11:23 |
| fwereade | rogpeppe, ahhhh sorry the agent Main | 11:23 |
| davechen1y | fwereade: i think you should always try to call the binary with an abosolute path | 11:23 |
| rogpeppe | fwereade: yeah | 11:23 |
| rogpeppe | davechen1y: why so? | 11:23 |
| davechen1y | the cloudinit or initd environment is very sparce | 11:23 |
| fwereade | davechen1y, +1 | 11:24 |
| fwereade | rogpeppe, yeah, I maybe misunderstood again | 11:24 |
| rogpeppe | davechen1y: we do always call the binary with an absolute path | 11:24 |
| fwereade | rogpeppe, which agent main sets up what path? | 11:24 |
| davechen1y | rogpeppe: i thought so, I remember that from cloudinit | 11:24 |
| rogpeppe | davechen1y: but we want the agents to set up PATH so that anything doing FindPath("jujud") will get the right thing | 11:25 |
| davechen1y | rogpeppe: i would try to avoid that | 11:25 |
| davechen1y | PATH is a shell thing | 11:25 |
| davechen1y | and we're not an interactive process | 11:25 |
| davechen1y | I think you'll find it more reliable if you manage that without PATH | 11:26 |
| fwereade | rogpeppe, I'm inclined to just make sure AgentToolsDir is on the PATH when we're calling hooks | 11:26 |
| * Aram agrees | 11:26 | |
| Aram | with davechen1y that is | 11:26 |
| rogpeppe | fwereade: definitely | 11:26 |
| rogpeppe | davechen1y: the PATH isn't just for our internal consumption | 11:26 |
| rogpeppe | davechen1y: it's also for commands running inside hooks | 11:26 |
| davechen1y | rogpeppe: that is an important, but seperate issue | 11:27 |
| fwereade | rogpeppe, but that's unit-agent-specific at the moment | 11:27 |
| rogpeppe | davechen1y: if we don't use PATH, then we have to have a separate place to store the current agent name | 11:27 |
| rogpeppe | davechen1y: because the path to the executable depends on the currently running agent | 11:27 |
| rogpeppe | fwereade: hmm, which piece of code was failing? | 11:28 |
| rogpeppe | davechen1y: have you changed the ssh code in state recently? | 11:28 |
| rogpeppe | davechen1y: i just saw a transient error in TestSSHConnect | 11:29 |
| rogpeppe | davechen1y: http://paste.ubuntu.com/1187088/ | 11:29 |
| davechen1y | rogpeppe: never seen that before | 11:30 |
| davechen1y | maybe you are racing with the mock sshd starting | 11:30 |
| rogpeppe | davechen1y: neither me. let's hope it never happens again. | 11:30 |
| fwereade | rogpeppe, I am wondering how we're going to manage juju-run though, given that there could be multiple unit agents in a container | 11:30 |
| rogpeppe | fwereade: that's fine | 11:30 |
| rogpeppe | fwereade: each agent has its own name | 11:30 |
| rogpeppe | fwereade: the unit agent name should contain the unit name | 11:31 |
| fwereade | rogpeppe, so juju-run picks the right jujuc to call based on unit name... I guess | 11:31 |
| rogpeppe | fwereade: interesting | 11:31 |
| rogpeppe | fwereade: i think it should look at the context. | 11:32 |
| rogpeppe | fwereade: which might contain the unit name, yeah | 11:32 |
| fwereade | rogpeppe, what happens when we want to upgrade juju-run, I wonder ;p | 11:32 |
| fwereade | rogpeppe, I don;t think there's any way to know that | 11:32 |
| rogpeppe | fwereade: what's juju-run again? | 11:32 |
| fwereade | rogpeppe, the idea of juju-run is it's called from anywhere | 11:32 |
| fwereade | rogpeppe, run this script as if it were a hook please | 11:32 |
| fwereade | rogpeppe, I think the plans are pretty firm, just not urgent | 11:33 |
| rogpeppe | fwereade: wouldn't it be reasonable to say that it must be run by a process that descends from a hook? | 11:33 |
| fwereade | rogpeppe, totally unreasonable, wouldn't it? | 11:33 |
| rogpeppe | fwereade: really? | 11:33 |
| fwereade | rogpeppe, calling juju-run from within a hook will deadlock, surely? | 11:33 |
| rogpeppe | fwereade: yes. but calling juju-run from within a background process started by a hook should be fine | 11:34 |
| fwereade | rogpeppe, and how do you make a cron job run in a process descended from a hook? | 11:34 |
| rogpeppe | fwereade: i was wondering about that | 11:34 |
| rogpeppe | fwereade: how does that juju-run know how to talk to the right jujud anyway? | 11:35 |
| fwereade | rogpeppe, I think juju-run has to take a unit name... or *maybe* infer it from context where possible... but I think I'd rather be explicit | 11:35 |
| rogpeppe | fwereade: i'm not sure i'm happy with executables being called with no control at all. need to think on it. | 11:37 |
| fwereade | rogpeppe, sorry, which executables? | 11:37 |
| rogpeppe | fwereade: juju-run in particular. but also the executables that are called by the script invoked by juju-run | 11:38 |
| fwereade | rogpeppe, the idea of juju-run is that it will wait for no other hook to be running and then execute the supplied script within a normal context | 11:38 |
| rogpeppe | fwereade: it scuppers my idea of how to do major-version upgrades, i think | 11:39 |
| fwereade | rogpeppe, just like any other hook, neatly serialized with all the other hook executions | 11:39 |
| fwereade | rogpeppe, heh :( | 11:39 |
| fwereade | rogpeppe, anyway, we can worry about this later and figure out some way to make it work with the upgrades that will already be in place | 11:42 |
| * fwereade pops a couple of levels | 11:42 | |
| fwereade | rogpeppe, now, the stuff that was failing was: | 11:43 |
| fwereade | 2012/09/04 15:49:43 JUJU deploying unit etherpad-lite/0 | 11:43 |
| fwereade | gah | 11:43 |
| rogpeppe | fwereade: ah, of course. that's fine. | 11:43 |
| rogpeppe | fwereade: that would be fixed by setting up $PATH | 11:43 |
| fwereade | 2012/09/04 15:49:43 JUJU cannot deploy unit etherpad-lite/0: cannot find executable: exec: "jujud": executable file not found in $PATH | 11:43 |
| rogpeppe | fwereade: which, despite mr cheney's objections, i think is probably the right approach | 11:44 |
| fwereade | rogpeppe, I guess I need to read the code some more, I'm not clear on what exactly is happening here | 11:45 |
| rogpeppe | fwereade: it's quite simple | 11:45 |
| fwereade | rogpeppe, I think I'm convinced that PATH is not something we should be depending on, though | 11:45 |
| rogpeppe | fwereade: container does a path search for the jujud exe | 11:45 |
| rogpeppe | fwereade: then puts that in the cloudinit executable | 11:45 |
| fwereade | rogpeppe, I don't think it's any of our business what PATH upstart happens to run with | 11:45 |
| rogpeppe | fwereade: we don't use any PATH stuff inside the upstart script | 11:46 |
| fwereade | rogpeppe, I'll shut up, you were saying useful things | 11:46 |
| rogpeppe | fwereade: the container package itself does the path search | 11:46 |
| rogpeppe | fwereade: if we don't have PATH, we need some global place to store the executable dir | 11:46 |
| rogpeppe | fwereade: i.e. some global place to store the current agent name | 11:47 |
| rogpeppe | fwereade: i think that PATH is actually a reasonable way to encode that. it's all we care about. and everything just falls out from that imo | 11:47 |
| fwereade | rogpeppe, sorry, what are we using the executable dir for? | 11:48 |
| rogpeppe | fwereade: all it takes is that each agent does os.Setenv("PATH", environs.AgentToolsDir(agentName) + ":" + os.Getenv("PATH")) | 11:48 |
| rogpeppe | fwereade: the executable dir is agent-specific | 11:48 |
| rogpeppe | fwereade: which means each agent can upgrade in its own sweet time | 11:49 |
| fwereade | rogpeppe, I understand that bit | 11:49 |
| rogpeppe | fwereade: ha! i've just realised that the cloudinit heuristic is wrong, wrong, wrong! | 11:50 |
| fwereade | rogpeppe, well, I shall feel pleased with myself in a non-sepcific and slightly bewildered way :) | 11:50 |
| rogpeppe | fwereade: we don't need the path at all | 11:51 |
| fwereade | rogpeppe, I was wondering what it was for | 11:51 |
| rogpeppe | fwereade: because the cloudinit script should not run the jujud from the agent that's creating the script | 11:51 |
| rogpeppe | fwereade: so there needs to be a little more logic in container to create the agent tools dir symlink | 11:51 |
| rogpeppe | fwereade: for the agent it's about to start | 11:52 |
| rogpeppe | fwereade: it might be good if it set PATH in the cloudinit script too | 11:52 |
| rogpeppe | fwereade: that way jujud doesn't have to muck about with PATH at all | 11:52 |
| fwereade | rogpeppe, excellent, our thoughts are in alignment then... but maybe not on that last bit | 11:52 |
| rogpeppe | fwereade: well, that's only a half-baked thought tbh | 11:53 |
| fwereade | rogpeppe, I feel that creating PATH for the hooks is very specific to hook-running, and not something that should be dealt with by all agents just for this one case | 11:53 |
| fwereade | rogpeppe, anyway, I might take a look at that briefly, unless you are underemployed | 11:54 |
| rogpeppe | fwereade: please do | 11:55 |
| fwereade | rogpeppe, although if I get unblocked on uniter implementation that will take precedence ;) | 11:55 |
| fwereade | rogpeppe, cheers | 11:55 |
| fwereade | rogpeppe, that might be an after-lunch thing though :) | 11:55 |
| niemeyer | Hullah! | 12:33 |
| niemeyer | I'll just announce a mgo release before kicking fully in | 12:34 |
| fwereade | niemeyer, heyhey | 12:38 |
| rogpeppe | niemeyer: yo! | 12:41 |
| niemeyer | fwereade, rogpeppe: Heya! | 12:43 |
| fwereade | niemeyer, I have a hideous pile of reviews for you today, but it should be better than one hideous review :) | 12:44 |
| niemeyer | fwereade: LOL | 12:46 |
| niemeyer | fwereade: Thanks a lot for splitting them up | 12:46 |
| * rogpeppe wishes he saw all the launchpad emails. | 12:46 | |
| fwereade | niemeyer, np, it turned out to be very helpful to do so, the code is better thanks to it | 12:46 |
| fwereade | niemeyer, there's still one big one but an awful lot of the lines are moves/deletes/trivials, so hopefully it won;t hurt too bad | 12:46 |
| niemeyer | fwereade: Sweet | 12:49 |
| niemeyer | Okaaay | 13:01 |
| * niemeyer jumps onto a pleasant review queue | 13:01 | |
| rogpeppe | fwereade: here's the upshot of our earlier conversation. i *think* it's nicer, but i'm biased. what do you think? https://codereview.appspot.com/6495086/ | 13:15 |
| * fwereade looks | 13:17 | |
| niemeyer | rogpeppe: What's the motivation behind the change? | 13:18 |
| rogpeppe | niemeyer: the initial motivation was that i wanted to deploy a charm in jujutest.LiveTests | 13:18 |
| rogpeppe | niemeyer: that meant that i needed to create my own charm repository and manually populate it with a charm of the right series. | 13:19 |
| niemeyer | rogpeppe: The first changed file already shows something that is slightly less ideal | 13:19 |
| niemeyer | rogpeppe: It's now bundling a new charm for every test, rather than for the suite | 13:19 |
| rogpeppe | niemeyer: the time impact is minimal | 13:19 |
| niemeyer | rogpeppe: We can't care both about having a fast suite and then disregard that kind of change | 13:20 |
| rogpeppe | niemeyer: 0.05seconds of penalty on that package | 13:21 |
| rogpeppe | niemeyer: i don't think we mind too much about 50ms | 13:21 |
| niemeyer | rogpeppe: Okay, that is irrelevant indeed | 13:21 |
| rogpeppe | niemeyer: though i would indeed like it if we could choose whether to use test fixtures for the duration of a test or a suite | 13:22 |
| niemeyer | rogpeppe: That doesn't make sense to me.. the test fixture can do whatever it pleases, and we can organize the tests in whatever way we like | 13:23 |
| niemeyer | rogpeppe: If you want a suite for the duration of a test, have a suite with a test (!) | 13:23 |
| rogpeppe | niemeyer: what if you want a suite which sets up everything for each test, but you want it to work for a whole suite? | 13:24 |
| fwereade | rogpeppe, +1 to that, +-0 to the change itself purely from an API point of view... much of the time "series" is irrelevant, and it seems tedious to type it out each time | 13:24 |
| niemeyer | rogpeppe: That's exactly what suites do? | 13:24 |
| rogpeppe | niemeyer: it seems wrong to call SetUpTest inside a SetUpSuite | 13:24 |
| niemeyer | rogpeppe: Sorry, that makes even less sense | 13:25 |
| niemeyer | rogpeppe: SetUpTest runs for each test.. SetUpSuite runs for each suite.. we can do whatever with those | 13:25 |
| rogpeppe | niemeyer: yes, but that means the fixture itself decides whether the context it sets up is appropriate for a test or a suite | 13:25 |
| rogpeppe | niemeyer: but i *think* that it doesn't matter - what matters is the context that it sets up, not where it's used | 13:26 |
| niemeyer | rogpeppe: The fixture itself should only be used when it makes sense | 13:26 |
| niemeyer | rogpeppe: If it's not appropriate to have a test in a fixture, don't put it | 13:26 |
| niemeyer | rogpeppe: We can have as many suites as we please | 13:27 |
| niemeyer | rogpeppe: With whatever organization we do | 13:27 |
| rogpeppe | niemeyer: i'm not sure what you mean by "in" a fixture. AFAICS a test suite *uses* a fixture. and perhaps that's the difficulty i'm having. | 13:27 |
| niemeyer | rogpeppe: Nope, a test suite *is* the fixture | 13:28 |
| rogpeppe | niemeyer: i think that's a hangover from inheritance days | 13:28 |
| rogpeppe | niemeyer: it doesn't seem a good fit for the go way of composing things. | 13:28 |
| niemeyer | rogpeppe: Well, we can debate about that, but that's a separate problem from not being able to do things | 13:29 |
| rogpeppe | niemeyer: sure. anyway, i suppose that your concern about doing the bundle in every test comes out of this issue. if this fixture was *solely* concerned with setting and tearing down a charm repo (in whatever context), then we could call SetUp inside SetUpSuite as easily as SetUpTest | 13:30 |
| niemeyer | rogpeppe: There's absolutely nothing preventing a type from doing *solely* whatever it wants to do and being called *solely* wherever we want to | 13:32 |
| rogpeppe | niemeyer: i've idly thought quite a few times that all the current fixture "suites" could be factored into types that conform to this interface: http://paste.ubuntu.com/1187225/ | 13:32 |
| fwereade | niemeyer, rogpeppe: do we have any easy way to get a state.Info out of a state.State? | 13:33 |
| fwereade | niemeyer, rogpeppe: feels kinda evil so I imagine not | 13:33 |
| rogpeppe | fwereade: i don't think so. | 13:33 |
| rogpeppe | fwereade: why? | 13:33 |
| fwereade | rogpeppe, I need it to fill in --zookeeper-servers for the unit agent | 13:34 |
| rogpeppe | fwereade: haven't you got an Environ? | 13:34 |
| niemeyer | rogpeppe: Yeah, we may eventually have something like that supported by stock gocheck | 13:34 |
| niemeyer | rogpeppe: Meanwhile, there's nothing preventing it from existing, except for the convenience of having a suite embedded onto another type and having it just work | 13:35 |
| fwereade | rogpeppe, I'm in simpleContainer.Deploy; should I? | 13:35 |
| fwereade | rogpeppe, and I'm called by Machiner, which just has a *state.Machine | 13:35 |
| fwereade | rogpeppe, I'll also want it for Uniter when that comes to deploy subordinates | 13:36 |
| rogpeppe | niemeyer: actually, you can do that too: http://paste.ubuntu.com/1187233/ | 13:36 |
| rogpeppe | niemeyer: then embed gocheck.TestFixture(something) or gocheck.SuiteFixture(something) and it'll all be done for you | 13:36 |
| * rogpeppe thinks that's quite neat actually | 13:37 | |
| niemeyer | rogpeppe: I think it's more code without a clear win | 13:37 |
| rogpeppe | niemeyer: you could even have fixture composition operators | 13:37 |
| niemeyer | rogpeppe: The point was to not embed.. if I have to embed and manage it all by hand, there's no reason to do it | 13:38 |
| rogpeppe | niemeyer: if i include 2 fixtures currently, i have to write about 20 lines of code that are entirely mechanical. | 13:39 |
| niemeyer | rogpeppe: You're not improving that situation by having a different mechanism to create a type that does the same thing we have today | 13:39 |
| niemeyer | rogpeppe: You are adding a step to *create a suite*. Everything involved in *using a suite* is contained in *create suite* + *use suite* | 13:40 |
| rogpeppe | niemeyer: maybe so. would you get rid of the SetUpTest etc methods entirely? | 13:40 |
| niemeyer | rogpeppe: I'd make it so that they are not necessary | 13:41 |
| rogpeppe | niemeyer: interesting. i don't quite see how it would work though. | 13:41 |
| niemeyer | rogpeppe: I'll get there eventually | 13:43 |
| fwereade | niemeyer, rogpeppe: ISTM that we are going to end up with noticeable duplication across Uniter and Machiner, if both are to be responsible for deploying units | 13:43 |
| fwereade | niemeyer, rogpeppe: it feels like we're missing some abstraction somewhere | 13:44 |
| fwereade | niemeyer, rogpeppe: does this resonate with either of you? | 13:44 |
| rogpeppe | niemeyer: ah, you could do it by having a test methods which took a fixture as an arg | 13:44 |
| rogpeppe | fwereade: container is supposed to be the shared abstraction | 13:44 |
| niemeyer | fwereade: Not really | 13:44 |
| rogpeppe | fwereade: what else will be duplicated? | 13:45 |
| niemeyer | fwereade: Deploying another unit from within a set up container should be as complex as putting a upstart script in place and running it, right? | 13:45 |
| fwereade | niemeyer, true | 13:45 |
| fwereade | niemeyer, I guess it's just a matter of a... UnitSubordinateWatcher? | 13:46 |
| fwereade | niemeyer, I guess the client code does end up reasonably different | 13:48 |
| fwereade | TheMue, we don't have any way to watch a unit's subordinates, do we? | 13:49 |
| TheMue | fwereade: today? afaik no. | 13:49 |
| fwereade | TheMue, np, we'll do it when we need it :) | 13:50 |
| rogpeppe | fwereade: you can watch units assigned to the current machine and ignore all but the subordinates | 13:55 |
| fwereade | rogpeppe, yeah, easy enough, cheers | 13:55 |
| niemeyer | rogpeppe: Reviewed it | 14:06 |
| niemeyer | fwereade: How do you mean (different)? | 14:07 |
| fwereade | niemeyer, not in any major way -- just stuff like what containers they actually deploy to -- ie it doesn't feel like it will really be so unbearably similar as to demand a single implementation | 14:08 |
| rogpeppe | niemeyer: thanks. it was WIP though, and i'm currently thinking this is a better direction, based on the above discussion and fwereade's reaction: http://paste.ubuntu.com/1187296/ | 14:08 |
| niemeyer | rogpeppe: Heh | 14:08 |
| niemeyer | rogpeppe: I think it was mostly ok, despite a few issues | 14:08 |
| niemeyer | rogpeppe: I'll wait until you *actually* want a review, though | 14:09 |
| rogpeppe | niemeyer: the essence is the same except that the original Repo type name is retained and everyone operates on that. | 14:10 |
| rogpeppe | niemeyer: rather than creating a fixture. | 14:10 |
| rogpeppe | niemeyer: very sorry to have wasted your time. | 14:12 |
| rogpeppe | niemeyer: most of your comments are still accurate BTW | 14:14 |
| niemeyer | rogpeppe: No problem, will be glad to review whatever you think makes sense as an improvement | 14:15 |
| fwereade | niemeyer, rogpeppe, TheMue, Aram, mramm2: could be better, but -- and forgive the spam: | 14:21 |
| fwereade | william@diz:~/code/go/src/launchpad.net/juju-core$ juju status | 14:21 |
| fwereade | machines: | 14:21 |
| fwereade | 0: | 14:21 |
| fwereade | dns-name: ec2-50-16-72-153.compute-1.amazonaws.com | 14:21 |
| fwereade | instance-id: i-1478c96e | 14:21 |
| fwereade | proposed-agent-version: 0.0.0 | 14:21 |
| fwereade | agent-version: 0.0.1 | 14:21 |
| fwereade | 1: | 14:21 |
| fwereade | agent-version: 0.0.1 | 14:21 |
| fwereade | dns-name: ec2-23-20-216-107.compute-1.amazonaws.com | 14:21 |
| fwereade | instance-id: i-f66adb8c | 14:21 |
| fwereade | proposed-agent-version: 0.0.0 | 14:21 |
| fwereade | services: | 14:21 |
| fwereade | etherpad-lite: | 14:21 |
| fwereade | charm: etherpad-lite | 14:21 |
| fwereade | units: | 14:21 |
| fwereade | etherpad-lite/0: | 14:22 |
| fwereade | proposed-agent-version: 0.0.0 | 14:22 |
| fwereade | status: error | 14:22 |
| fwereade | status-info: 'hook failed: "install"' | 14:22 |
| fwereade | agent-version: 0.0.0 | 14:22 |
| fwereade | machine: 1 | 14:22 |
| fwereade | exposed: true | 14:22 |
| rogpeppe | fwereade: yay! | 14:22 |
| niemeyer | fwereade: Holy crap!! | 14:24 |
| niemeyer | HOLY CRAP! | 14:24 |
| TheMue | Yiiiieha! | 14:24 |
| * niemeyer does the funky chicken around the chair | 14:25 | |
| fwereade | :DD | 14:25 |
| TheMue | fwereade: grats, guy, this looks damn good | 14:25 |
| TheMue | fwereade: and is definitely no spam, it good beef | 14:26 |
| fwereade | haha | 14:26 |
| TheMue | s/it/it's/ | 14:26 |
| mramm2 | fwereade: So very awesome! | 14:59 |
| niemeyer | mramm2: Well, s/fwereade/team/, really.. | 15:03 |
| niemeyer | This is the peak of several months of everybody's hard work.. remarkable really | 15:04 |
| TheMue | dinnertime (instead of lunch today) | 15:06 |
| niemeyer | TheMue: :-) | 15:09 |
| niemeyer | TheMue: Enjoy | 15:09 |
| rogpeppe | niemeyer, fwereade: i think this is nicer (and it's ready for review :-]) https://codereview.appspot.com/6495086 | 15:12 |
| niemeyer | rogpeppe: You've got a review on https://codereview.appspot.com/6498085/ | 15:13 |
| rogpeppe | niemeyer: yeah, i've occasionally wondered about version.IsValid. will do. | 15:14 |
| rogpeppe | niemeyer: thanks a lot for the review BTW! | 15:14 |
| rogpeppe | niemeyer: we need to have a chat about those tests in that earlier CL | 15:15 |
| rogpeppe | niemeyer: i wondered what you'd think of my version bumping scheme :-) | 15:16 |
| rogpeppe | niemeyer: IMO *any* version bumping screws up our versioning. | 15:17 |
| rogpeppe | niemeyer: so we might was well screw it up in an obvious way | 15:17 |
| niemeyer | rogpeppe: Not really, there are ways we can fix this | 15:18 |
| * rogpeppe listens | 15:18 | |
| niemeyer | rogpeppe: Well, let's do it in parts.. | 15:19 |
| rogpeppe | niemeyer: sure, i'll remove it for the time being | 15:19 |
| niemeyer | rogpeppe: For that branch, can we please drop the bumping? | 15:19 |
| niemeyer | rogpeppe: It's mostly ready otherwise, except for those trivials | 15:19 |
| rogpeppe | niemeyer: still gotta get the prereqs through... but thanks, i'm happy | 15:19 |
| niemeyer | rogpeppe: Super, me too, thanks a lot for the functionality | 15:20 |
| niemeyer | rogpeppe: So, the tests.. | 15:20 |
| niemeyer | rogpeppe: Which one do you wanna talk about first? | 15:20 |
| rogpeppe | niemeyer: the "does nothing" tests | 15:20 |
| niemeyer | rogpeppe: Okay, | 15:20 |
| niemeyer | rogpeppe: This is indeed boring to test, no doubts | 15:21 |
| rogpeppe | niemeyer: yeah. i'm interested to hear your thoughts. | 15:21 |
| niemeyer | rogpeppe: Let me explain first a vague thought I had, which depends on functionality that is coming in mstate | 15:22 |
| * rogpeppe is listening | 15:23 | |
| niemeyer | rogpeppe: What we're trying to test, vaguely speaking, is that a change in the version was observed | 15:23 |
| rogpeppe | niemeyer: yeah | 15:23 |
| niemeyer | rogpeppe: So that we can be reassured that is is being ignored, rather than updating | 15:23 |
| rogpeppe | niemeyer: yup | 15:23 |
| niemeyer | rogpeppe: The difficulty we have is that we don't have a good mechanism to define "steady state" | 15:24 |
| niemeyer | rogpeppe: So I'm trying to imagine of ways we could fix that | 15:24 |
| niemeyer | rogpeppe: I'm not even sure if mstate can help there, to be honest | 15:25 |
| rogpeppe | niemeyer: ah, are you thinking we can rely on the synchronous delivery of watcher events to all watchers? | 15:25 |
| niemeyer | rogpeppe: Yeah, but that doesn't quite work I think | 15:25 |
| rogpeppe | niemeyer: no, different connections | 15:25 |
| niemeyer | rogpeppe: Because we're precisely trying to make it so that events are not really delivered if the consumer didn't bother to ask, right? That was yesterday's conversation | 15:26 |
| rogpeppe | niemeyer: i await the watcher API with eager anticipation :-) | 15:26 |
| niemeyer | rogpeppe: Well, the API is ready.. we need the implementation :) | 15:27 |
| rogpeppe | :-) | 15:27 |
| niemeyer | rogpeppe: So, let's see.. | 15:27 |
| niemeyer | rogpeppe: If we had a mechanism to force all events to at least be made available to consumers if they wish, | 15:28 |
| niemeyer | rogpeppe: it might be simpler to define the concept of "steady state" for a loop | 15:28 |
| niemeyer | rogpeppe: So we could say, for example, upgrader.Sync() | 15:29 |
| rogpeppe | niemeyer: i'm not sure what that means | 15:29 |
| niemeyer | rogpeppe: and know it has dealt with every other event that was available till then, and found nothing else to do | 15:29 |
| niemeyer | rogpeppe: If we do that, we can realize changes in the state, Sync, and test that nothing was done | 15:30 |
| rogpeppe | niemeyer: isn't that almost equivalent to the current approach - just send a value into the inner loop rather than receiving one? | 15:31 |
| niemeyer | rogpeppe: Without assuming things about specific code paths | 15:31 |
| niemeyer | rogpeppe: It's completely unrelated to the current approach, IMO | 15:31 |
| niemeyer | rogpeppe: They just have the same goal | 15:31 |
| niemeyer | rogpeppe: Which, well, is precisely what we're trying to preserve | 15:31 |
| rogpeppe | niemeyer: i'm not sure what Sync means though. we don't have events, we have state. | 15:31 |
| niemeyer | rogpeppe: We have both state and events that are emitted as a consequence of changing state | 15:32 |
| rogpeppe | niemeyer: ah, do you mean State.Sync ? | 15:32 |
| niemeyer | rogpeppe: No, I meant upgrader.Sync() indeed | 15:33 |
| niemeyer | rogpeppe: Because we must verify that the *upgrader* is done processing events | 15:33 |
| rogpeppe | niemeyer: but the events are being delivered by the state, right? | 15:33 |
| niemeyer | rogpeppe: Right | 15:33 |
| niemeyer | rogpeppe: But you're on the right track | 15:34 |
| rogpeppe | niemeyer: so how do we know that the Sync call has arrived before the event we're expecting from the state? | 15:34 |
| niemeyer | rogpeppe: I'm planning to make available state.ForceWatcherRefresh() | 15:34 |
| niemeyer | rogpeppe: That will at least make all events ready for consumption locally | 15:35 |
| rogpeppe | niemeyer: ok. but does it guarantee they've been delivered? | 15:35 |
| niemeyer | rogpeppe: No, because there's an intermediate loop that processes watches | 15:35 |
| rogpeppe | niemeyer: so i still don't see how upgrader.Sync can work reliably | 15:36 |
| niemeyer | rogpeppe: The thing that actually implements the e.g. foo.WatchWhatever() loop | 15:36 |
| niemeyer | rogpeppe: Right, I'm still thinking through to see if there's a better way, but no matter what in the short term there's no fantastic way to assert the all events have been made available | 15:36 |
| niemeyer | rogpeppe: So, | 15:36 |
| niemeyer | rogpeppe: To avoid blocking you further, | 15:36 |
| rogpeppe | niemeyer: AFAICS the only way of telling if an event has been delivered is to ask the thing it's been delivered to | 15:36 |
| niemeyer | rogpeppe: Sure, but that sucks for various reasons | 15:37 |
| rogpeppe | niemeyer: yeah, i see your point. but... the alternative sucks too :-) | 15:37 |
| niemeyer | rogpeppe: Not the least one, it means hardcoding logic for every tiny test we need to do | 15:37 |
| niemeyer | rogpeppe: My proposal is for us to implement a mechanism that is good enough for the moment | 15:37 |
| niemeyer | rogpeppe: and avoids the hackery | 15:38 |
| * rogpeppe is all ears | 15:38 | |
| niemeyer | rogpeppe: By defining a test mode in those cases that enables the Sync method to work | 15:38 |
| niemeyer | rogpeppe: When we call Sync(), the loop initializes a channel that was previously nil to time.After(100 * time.Millisecond) | 15:39 |
| rogpeppe | niemeyer: i still don't see how Sync can ever work properly | 15:39 |
| niemeyer | rogpeppe: and puts it in the main loop | 15:39 |
| niemeyer | rogpeppe: When Sync is acting, the value is reset every time the loop begins | 15:40 |
| niemeyer | rogpeppe: When the timeout happens, Sync returns | 15:40 |
| niemeyer | rogpeppe: With mstate's flush logic I described, this will become quite reliable, and we can probably reduce the timeout to half of this or less | 15:41 |
| rogpeppe | niemeyer: if we're going to do that, we may as well just have a timeout in the test rather than cluttering the code for a feature that may never be possible to implement properly | 15:41 |
| niemeyer | rogpeppe: I believe that eventually we can actually have a Sync() method on all watchers, and turn the mechanism into a deterministic system, and avoid issues completely | 15:41 |
| rogpeppe | niemeyer: even *with* the flush logic, we'll still need a timeout | 15:42 |
| niemeyer | rogpeppe: Not necessarily, no, as I just mentioned | 15:42 |
| rogpeppe | niemeyer: i'm not convinced that's possible, i'm afraid | 15:42 |
| niemeyer | rogpeppe: Wouldn't be the first time. :) | 15:42 |
| rogpeppe | niemeyer: how can any watcher know if it has received all the events that might happen as a result of a change to the db? | 15:43 |
| niemeyer | rogpeppe: It's fine, though.. if you want to put a short timeout in the test for the moment, that works for me.. the test should ensure that nothing happens though, rather than just assuming nothing happens. | 15:43 |
| niemeyer | rogpeppe: I'm not going to solve that problem today | 15:44 |
| rogpeppe | niemeyer: ok, i think i'll go for that as the simple approach for the time being, pending some future mechanism. i'm not very happy about it though. we can easily check that the event gets delivered... and even if we have Sync, we'd be checking the same thing, i think. anyway, i'll go for the timeout. | 15:46 |
| niemeyer | rogpeppe: Agreed. I'm not very happy either. But I'm even less happy with what's there now. | 15:47 |
| niemeyer | rogpeppe: I'm really concerned about having that as a normal practice, and dirtying up all code with that kind of "address breakpoint" that not only doesn't pay for itself, but makes for brittle tests that are hard to debug and dirty up the logic. | 15:48 |
| niemeyer | rogpeppe: I promise I'll keep your core concern in mind, though, and try to have a more reasonable proposal eventually | 15:48 |
| niemeyer | I'll step out for lunch.. biab | 15:49 |
| rogpeppe | niemeyer: BTW about ChangeEnviroConfig... | 15:49 |
| rogpeppe | niemeyer: ok, enjoy | 15:49 |
| niemeyer | rogpeppe: Oh, ok | 16:39 |
| niemeyer | rogpeppe: Wanna conitnue? | 16:40 |
| niemeyer | continue | 16:40 |
| rogpeppe | niemeyer: it was only a brief observation: my intention wasn't to do a delta, but to atomically write the whole thing. the delta is defined by the function itself. but maybe that's not possible with mstate actually. | 16:41 |
| rogpeppe | niemeyer: i've fixed the tests to do a timeout BTW; i think the branch may be LGTY now. | 16:42 |
| niemeyer | rogpeppe: There's no way to not do a delta with the sample you provided. | 16:44 |
| niemeyer | rogpeppe: You did attrs := AllAttrs; attrs["foo"] = "bar | 16:44 |
| niemeyer | rogpeppe: So attrs is a whole document, which has the issues I described | 16:45 |
| rogpeppe | niemeyer: and then returned all the attributes, which are bundled into a Config and written in their entirety. | 16:45 |
| niemeyer | rogpeppe: Exactly.. you said you're trying to fix the problem of applying deltas, but that isn't | 16:45 |
| rogpeppe | niemeyer: or can't you write a whole Config at once | 16:45 |
| niemeyer | rogpeppe: We can't, and that's the interface we have today | 16:45 |
| niemeyer | Erm | 16:45 |
| niemeyer | rogpeppe: We can, and that's the interface we have today | 16:45 |
| niemeyer | rogpeppe: With the known issues.. | 16:46 |
| niemeyer | rogpeppe: If we're not solving the issues, we can just use it | 16:46 |
| rogpeppe | niemeyer: can't we do the equivalent of zk's RetryChange? | 16:46 |
| niemeyer | rogpeppe: I've covered that in the review too | 16:46 |
| niemeyer | rogpeppe: Still presents issues | 16:46 |
| rogpeppe | niemeyer: we can't make an invalid config because we make the Config before writing it. i'm still not seeing it. but anyway, i've removed the function. | 16:47 |
| fwereade | niemeyer, I have a feeling I'm going to feel very stupid in a moment, but: http://paste.ubuntu.com/1187523/ | 16:49 |
| fwereade | brb | 16:49 |
| niemeyer | rogpeppe: A Config is not necessarily a valid environment configuration.. | 16:49 |
| rogpeppe | niemeyer: true | 16:51 |
| niemeyer | fwereade: No perceived stupidity.. it's indeed not immediately obvious | 16:52 |
| niemeyer | fwereade: I've argued quite a bit when the jujucharms stuff was created to avoid that kind of issue, but lost the argument in a miserable way | 16:54 |
| fwereade | niemeyer, can you explain what is going on in words suitable for the hard of thinking? | 16:54 |
| niemeyer | fwereade: It's actually trivial: the charm store revisions charms monotonically.. anything that is looking at the content through alternative means may report a different reality | 16:55 |
| fwereade | niemeyer, I thought that ended up meaning we picked the lowest possible rev that was higher than any other possible rev | 16:57 |
| niemeyer | fwereade: and to be honest, I don't see anything out of sync in jujucharms | 16:57 |
| niemeyer | fwereade: It's not reporting the wrong revision, at least in this page | 16:57 |
| niemeyer | fwereade: branch revision != charm revision | 16:58 |
| niemeyer | fwereade: revision numbers in Bazaar (or any DVCS) are wildly unreliable | 16:58 |
| fwereade | niemeyer, yeah, I've been thinking about the content of revision and/or the legacy revision in metadata.yaml | 16:59 |
| niemeyer | fwereade: Charms obtained from the store should have the proper number there | 16:59 |
| fwereade | niemeyer, it appears not to | 16:59 |
| niemeyer | fwereade: Example? | 16:59 |
| fwereade | niemeyer, well, pasted -- the store thinks that 2 is the most recent revision, but there is nothing I can see on jujucharms with rev 2 at all | 17:00 |
| niemeyer | fwereade: Sorry, I don't understand | 17:00 |
| fwereade | niemeyer, according to the rev files on jujucharms, it should be 22, AFAICT | 17:01 |
| niemeyer | fwereade: Which rev files, and why does it matter? | 17:01 |
| niemeyer | fwereade: jujucharms is not the store | 17:01 |
| fwereade | niemeyer, well, yes, this is true, but I was expecting them to have roughly similar content | 17:01 |
| niemeyer | fwereade: They have roughly similar content | 17:01 |
| fwereade | niemeyer, to the point where at least I could figure out some correspondence from one to the other | 17:02 |
| niemeyer | fwereade: This is actually possible | 17:02 |
| niemeyer | fwereade: Not sure if it's exposed, but there's a digest in the store | 17:02 |
| niemeyer | fwereade: Which is the revision digest | 17:02 |
| fwereade | niemeyer, but, I'm sorry, I have to go eat supper... I'll reread some stuff this evening and see if I can see the light | 17:02 |
| niemeyer | fwereade: The branch revision number is not the way to do that, though.. | 17:03 |
| niemeyer | fwereade: Because revision numbers are highly unreliable in Bazaar and in pretty much any DVCS | 17:03 |
| fwereade | niemeyer, yeah, I only reported the branch revision for the sake of completeness | 17:03 |
| fwereade | niemeyer, anyway, sorry, must go | 17:03 |
| niemeyer | fwereade: Well, not really | 17:03 |
| niemeyer | fwereade: You were basing your analysis on it I think | 17:03 |
| niemeyer | fwereade: Enjoy | 17:03 |
| === mramm2 is now known as mramm | ||
| niemeyer | rogpeppe: I don't get the "// If a test failed, make sure we see any error from the upgrader.".. if a test failed, we already have a failure.. why are we deferring a second check? | 17:44 |
| rogpeppe | niemeyer: i had this problem when development - if an assert fails because some event was not received, it's quite possible the upgrader has died with an error which we want to see | 17:45 |
| niemeyer | rogpeppe: Let's drop the assert then, and use a check + wait? | 17:46 |
| niemeyer | rogpeppe: It feels like we're fighting our own test in a way | 17:46 |
| rogpeppe | niemeyer: waitDeath, yes? | 17:46 |
| niemeyer | rogpeppe: Yeah, it looks nice, thanks | 17:47 |
| rogpeppe | niemeyer: so you're saying use Check rather than Assert? | 17:47 |
| rogpeppe | niemeyer: sure. they're equivalent in a defer anyway | 17:47 |
| niemeyer | rogpeppe: I'm just wondering about that defer which seems to fight the test itself | 17:47 |
| rogpeppe | niemeyer: if we don't use a defer, we can't use Assert anywhere else | 17:47 |
| rogpeppe | niemeyer: there are lots of tests elsewhere that do defer func() {err := x.Close(); c.Assert(err, IsNil)} | 17:49 |
| rogpeppe | niemeyer: this is the moral equivalent | 17:49 |
| rogpeppe | niemeyer: i've gotta go. very close to reproposing the upgrade-juju branch, but just remembered Version.IsValid which is more than i can do in 2 minutes... | 17:51 |
| niemeyer | rogpeppe: Where is that test running Stop? | 17:51 |
| rogpeppe | niemeyer: it isn't. but it always waits for the upgrader to die, so that should be ok, i think. | 17:52 |
| rogpeppe | niemeyer: (no need to call Stop if Wait has returned, right?) | 17:52 |
| niemeyer | rogpeppe: If it always waits for the upgrader to die, why do we have a defer? | 17:52 |
| niemeyer | rogpeppe: No, that's not true | 17:53 |
| niemeyer | rogpeppe: If the test fails, we have an upgrader running in background | 17:53 |
| rogpeppe | niemeyer: that's true. we should call stop anyway, right? in a defer. | 17:53 |
| niemeyer | rogpeppe: Wait is merely saying "Uh.. didn't see it stop!" | 17:53 |
| niemeyer | rogpeppe: Yeah, c.Assert(Stop(), IsNil) | 17:53 |
| rogpeppe | niemeyer: ok, i'll call Stop | 17:53 |
| niemeyer | rogpeppe: That'd be fine | 17:53 |
| rogpeppe | niemeyer: in a defer? | 17:54 |
| niemeyer | rogpeppe: Yeah, as we generally do for the stop stuff | 17:54 |
| niemeyer | rogpeppe: Sorry for not being clear before.. it had something smelly and I couldn't quite put my finger on it | 17:54 |
| rogpeppe | niemeyer: that's fine. Stop is much better there. | 17:56 |
| rogpeppe | niemeyer: https://codereview.appspot.com/6490067/ is ready for final review i think | 17:58 |
| niemeyer | rogpeppe: I'm reproducing the counter bug now, btw.. I'll have a deeper look and try to fix it once and for all | 17:58 |
| rogpeppe | niemeyer: great. i do see it a *lot* | 17:58 |
| niemeyer | rogpeppe: Sorry about that | 17:59 |
| rogpeppe | niemeyer: like at that exact moment! | 17:59 |
| niemeyer | :-) | 17:59 |
| rogpeppe | niemeyer: gotta go. have fun. | 17:59 |
| niemeyer | rogpeppe: Have fun, and thanks! | 17:59 |
| rogpeppe | TheMue, fwereade, Aram, niemeyer: night all | 18:00 |
| fwereade | gn rg | 18:00 |
| fwereade | gn rogpeppe | 18:00 |
| niemeyer | fwereade: Btw, https://bugs.launchpad.net/charmworld/+bug/1046444 | 18:01 |
| niemeyer | fwereade: I'll also add the digest information to the returned info | 18:01 |
| fwereade | niemeyer, ah, cool, tyvm | 18:01 |
| niemeyer | fwereade: np, and thanks for raising the issue | 18:01 |
| fwereade | niemeyer, always a pleasure to blunder constructively into things :) | 18:02 |
| niemeyer | fwereade: Yeah, appreciated. It is a slighly unclear subject, unfortunately | 18:03 |
| TheMue | rogpeppe: gn | 18:04 |
| niemeyer | Hah | 18:06 |
| niemeyer | I think the status counter stuff is an actual bug | 18:06 |
| niemeyer | mthaddon: ping | 18:06 |
| niemeyer | And so it is.. | 18:14 |
| fwereade-on-juju | niemeyer: would you believe how I'm posting this? | 18:34 |
| fwereade-on-juju | niemeyer: http://ec2-50-19-8-166.compute-1.amazonaws.com:3000/ | 18:34 |
| niemeyer | fwereade-on-juju: OMG | 18:34 |
| TheMue | niemeyer: i have to leave. do you see any chance to review the two agent alive branches till tomorrow? tomorrow my vacancies start, but i can do some stuff till friday in the evening hours. | 18:35 |
| OMG-IT-WORKS | WOOHOO | 18:35 |
| TheMue | fwereade-on-juju: FANTASTIC | 18:36 |
| OMG-IT-WORKS | TheMue: Definitely, I'll make sure to have it ready by your morning | 18:36 |
| fwereade-on-juju | and the unit agent log looks sane as well, barring presence spam that I've absent-mindedly fixed in about 3 branches that never made it to trunk | 18:36 |
| fwereade-on-juju | http://paste.ubuntu.com/1187643/ | 18:36 |
| OMG-IT-WORKS | fwereade-on-juju: Oops, sorry about that | 18:36 |
| TheMue | OMG-IT-WORKS: great, thx. i want to have it in befor we leave. | 18:36 |
| * fwereade-on-juju dances, whoops, backflips | 18:37 | |
| fwereade-on-juju | OMG-IT-WORKS: nah, I suspect they've always been working branches that get taken apart and proposed piece by piece | 18:37 |
| TheMue | fwereade-on-juju: hehe, my claps his hands to your dance | 18:37 |
| OMG-IT-WORKS | TheMue: Thanks a lot, that's appreciated, although I feel a bit bad that you're looking at it over your holidays | 18:38 |
| TheMue | OMG-IT-WORKS: don't worry, our current progress is too fantastic to not participate :D | 18:39 |
| OMG-IT-WORKS | What *doesn't* seem to work so well is Subway, though :-) | 18:39 |
| OMG-IT-WORKS | Some messages just don't show up | 18:40 |
| fwereade | OMG-IT-WORKS, yeah, seems so | 18:40 |
| fwereade | OMG-IT-WORKS, ah well | 18:40 |
| OMG-IT-WORKS | NOT OUR PROBLEM | 18:41 |
| OMG-IT-WORKS | -) | 18:41 |
| fwereade | :D | 18:41 |
| OMG-IT-WORKS | (for once!) | 18:41 |
| fwereade | haha | 18:41 |
| fwereade | right, I think I really am done for the night, unless la famille falls asleep early again | 18:44 |
| fwereade | gn all, take care, and wave a tearful goodbye to the first ever working go-juju service deployment :) | 18:45 |
| fwereade_ | rogpeppe, btw, is there something specific that demands that unit agent names be "unit-foo-123" rather than just "foo-123"? | 20:04 |
| fwereade_ | rogpeppe, I'm feeling weird tacking "unit-" in front of everything | 20:05 |
| niemeyer | fwereade_: +1 | 20:06 |
| niemeyer | fwereade_: Although I don't know the context | 20:06 |
| fwereade_ | niemeyer, I don't think it introduces any ambiguities | 20:06 |
| fwereade_ | niemeyer, just for AgentToolsDir | 20:07 |
| niemeyer | fwereade_: Ah, the reasoning is that we'll have other agents there I think | 20:18 |
| niemeyer | fwereade_: machine-N, provisioning-N, etc | 20:19 |
| niemeyer | fwereade_: So unit-NAME is just making it even | 20:19 |
| fwereade_ | niemeyer, I'd be just as happy with prefixes everywhere | 20:19 |
| niemeyer | fwereade_: I think we do have them, don't we? | 20:19 |
| fwereade_ | niemeyer, at the moment I think htere's an unstated assumption that there will be <=1 of each of those, so they;re not N'd | 20:19 |
| niemeyer | fwereade_: Hmm | 20:20 |
| niemeyer | fwereade_: I see.. we'll probably not have more than a single one, but I do think we should have the suffix | 20:20 |
| fwereade_ | niemeyer, yeah... only question is where the provisioner ID comes from for now :) | 20:21 |
| fwereade_ | niemeyer, just copy the machine ID? | 20:21 |
| niemeyer | fwereade_: I'd be fine with that.. rogpeppe will be fixing it soon anyway, as we'll need a real entity for the provisioner as part of the upgrading work | 20:22 |
| niemeyer | (which sounds like a sensible thing, no matter what) | 20:22 |
| fwereade_ | niemeyer, +1 | 20:22 |
| fwereade_ | niemeyer, I guess it becomes AgentToolsDir(kind, name string) | 20:23 |
| niemeyer | fwereade_: I wonder if we might have something more straightforward | 20:24 |
| fwereade_ | niemeyer, I wouldn't say no to that -- you have a suggestion? | 20:24 |
| niemeyer | fwereade_: func AgentToolsDir(agent interface{}) { switch agent.(type) { ... } ) | 20:25 |
| niemeyer | fwereade_: Where do we use this function ATM? | 20:25 |
| * niemeyer looks | 20:25 | |
| fwereade_ | niemeyer, not clear what agent is actually expected to be | 20:25 |
| niemeyer | fwereade_: Okay, we use it in a number of different places.. so, better idea: | 20:27 |
| niemeyer | func (u *Unit) PathKey() string { return "unit-" + u.Name() } | 20:27 |
| niemeyer | func AgentToolsDir(agent interface{ PathKey() string }) { return filepath.Join(prefix, agent.PathKey()) } | 20:28 |
| niemeyer | EOM :) | 20:29 |
| fwereade_ | niemeyer, with /-replacement, ok; similar for machine; and just hack something up for provisioner? | 20:29 |
| fwereade_ | niemeyer, (man, I really want to just auto-deploy a juju-provisioner charm :)) | 20:30 |
| niemeyer | fwereade_: Yeah, for the moment we can have a locally-built value for provisioner | 20:30 |
| niemeyer | fwereade_: That's feeling unlikely at the moment.. it's actually less convenient due ot upgrading/versionoing/etc | 20:30 |
| niemeyer | fwereade_: This PathKey() sounds useful for other places too, btw | 20:31 |
| niemeyer | fwereade_: (like, building the /var/lib dir for the unit) | 20:31 |
| fwereade_ | niemeyer, yes indeed | 20:32 |
| fwereade_ | niemeyer, (but I bet we could still do a juju-provisioner charm that works with upgrades... not now though :)) | 20:34 |
| niemeyer | fwereade_: Hmm.. that'd be interesting, but I think we'd have to bridge two different worlds for that (charm upgrades and agent upgrades) | 20:34 |
| fwereade_ | niemeyer, I *think* they're independent... I don't see the charm as needing many upgrades, and all we'd need is to bounce the upstart job whenever we saw a config-changed | 20:36 |
| fwereade_ | niemeyer, so long as the upstart job is written in terms of the unit's AgentToolsDir, anyway :) | 20:37 |
| fwereade_ | niemeyer, it *is* a bit hackish, I agree :) | 20:37 |
| niemeyer | fwereade_: Yeah :) | 20:50 |
| niemeyer | Bug fixed! | 20:50 |
| fwereade_ | niemeyer, yay! (what bug?) | 20:51 |
| niemeyer | LOL | 20:51 |
| niemeyer | fwereade_: The one in juju-core/store you likely see every other time you run "go test" (/me hides) | 20:51 |
| fwereade_ | niemeyer, YAY! | 20:51 |
| niemeyer | fwereade_: and interestingly, it was a *real* bug | 20:52 |
| fwereade_ | niemeyer, ha, hiding in plain sight | 20:52 |
| niemeyer | Yeah | 20:52 |
| niemeyer | Will need to ping mthaddon to see if the counter data in production needs fixing | 20:52 |
| niemeyer | and it's up: https://codereview.appspot.com/6490083 | 20:59 |
| niemeyer | fwereade_: and this one has the digest support: https://codereview.appspot.com/6500083 | 21:10 |
| * niemeyer opens a bug on charmworld to use it | 21:10 | |
| niemeyer | fwereade_: ping by any chance? | 22:03 |
| niemeyer | Okay, that was a long and nice day | 22:53 |
| niemeyer | davecheney: Morning! | 22:53 |
| niemeyer | I'm stepping out for dinner+rest | 22:53 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!