bigjoolsdoes juju assume all machine units have a different IP?03:11
wallyworld_i think so03:20
thumperbigjools: assume in what way?03:39
bigjoolsthumper: I don't know the nature of the assumptions, I just need to know if it does as it affects how we'd implement some Azure stuff03:40
bigjoolsAzure is not a "flat" service03:40
thumperbigjools: I don't think anything is indexed on IP03:40
thumperhowever it does use the address to talk to the machines03:40
bigjoolsyeah I figured03:41
thumperit is what is propagated between units in relations03:41
thumperthere is some work pending which mgz is starting around machine addressability03:41
thumperto remove some of the assumptions03:41
thumperand have clear expectations03:41
thumperbigjools: so touch base with mgz for more info03:41
bigjoolsAzure gives multiple VMs the same IP address and load balances unless you do things differently03:41
bigjoolsthumper: right thanks - Gavin was talking to him already03:42
thumperyou should be fine then :)03:42
bigjoolsIn Azure, you get this sort of structure: Service -> Deployment -> VM / VM / ...03:42
bigjoolseach "service" has its own IP03:43
bigjoolsrather than the VM03:43
jtv2Any reviewers in the house?  I've got a big one waiting: https://codereview.appspot.com/1102704304:00
bigjoolsway to attract reviewers :)04:02
jtv2Any worse than the built-in "please take a look"?  :)04:03
* thumper does a mega move04:31
bigjoolsCan anyone help with this panic in the stdlib please? http://play.golang.org/p/zRZh_cXGy904:42
* davecheney looks04:43
bigjoolsI think I was doing the wrong thing to what I need anyway, but I thought it was interesting that I can panic it :)04:47
davecheneyit's because you're trying to marshall a []string into04:47
davecheney<somethign name="..." />04:47
davecheneywhere ... is whatever []string marshalls too in xml04:48
bigjoolsyes I was expecting <node name="cgf" name="dfgfdg"/>04:48
* davecheney scratches04:48
davecheneyi dunno if that is valid xml04:48
davecheneytwo secs04:48
bigjoolsmight not be04:48
davecheneyi don't think it is04:48
davecheneybut it's a shit poor error message in that case04:48
bigjoolswhat I really wanted was an array of <something name="foo">04:48
bigjoolsthanks for looking04:49
davecheneybigjools: np04:49
* davecheney ponders http://www.w3.org/TR/REC-xml/#attdecls04:49
davecheneyit's been a while since i've had to read this train wreck04:49
bigjoolsXML is the spawn of satan anyway04:50
davecheneybigjools: so youi wanted04:50
davecheney  <something name="foo" />04:50
davecheney   <something name="bar" />04:50
bigjoolsI have that now04:50
bigjoolsby making a sub-struct array04:51
davecheneyyup, that is the solution04:51
davecheneythe impdendence mismatch between XML and Go is pretty high04:51
davecheneyGustavo did some work to make it less crap before 1.0 shipped04:51
bigjoolsit has raised some eyebrows04:51
davecheneybut it's still margian04:51
bigjoolsyou can't use attr with chains, either :(04:52
davecheneychains ?04:53
davecheneythere is a syntax04:53
davecheneyit is certainly non intuative04:53
bigjoolsie `xml:"Node>SubNode,attr"`04:53
davecheneyi think that is a bridge too far04:54
davecheneyi don't go in that package much04:54
* bigjools idly wonders if xpath would have been less of a shit sandwich04:54
davecheneyI believe our man in Brazil might have a solution to that04:55
davecheneynot tried yet04:55
thumperreview incomming $ bzr diff -r branch::parent | wc -l05:03
thumper 59 files changed, 348 insertions(+), 363 deletions(-)05:04
* davecheney shrieks 05:06
thumperthis branch has a high likelyhood of clashing with someone05:06
davecheneyi guess that is how you got your nick05:06
thumperdavecheney: I bit the bullet for jam05:06
thumperdavecheney: this branch kills state.Tools05:07
thumperand makes everything use tools.Tools05:07
thumperand moves agent/tools.go into tools/agent.go05:07
thumperlbox is currently working out the diff05:07
thumperjam: I wanted to mess around with tools a little05:08
thumperjam: in particular, uploading pre-built tools for the local provider05:08
thumperinto the local storage05:08
thumperso I needed to refactor a few methods anyway05:08
thumperseemed stupid to do it in the old place05:08
thumperRietveld: https://codereview.appspot.com/1102804305:08
jamthumper: per the request from fwereade he really likes the idea of an "agent" top level package, rather than a "tools" one.05:08
thumperentirely mechanical05:08
jamSo I was going to pull environs/agent top level and replace tools with it.05:09
jamThough still have agent/Tools instead of state/Tools.05:09
thumperjam: well... someone can change the files later05:09
thumperthis is done05:09
thumperlots of tests fixed05:09
thumperjam: well, the work is half done05:09
thumperbaby steps, no?05:09
thumperI have to admit, the review looks impressive05:10
jamthumper: rather than having to do 2 massive conflicty changes like this, can you just 'bzr mv tools agent' and then use 'agent.Tools' everywhere in your patch, but otherwise leave it the same?05:11
jamWe don't have to move the rest of the agent stuff05:11
jamjust call it agent.Tools so we don't have to have a mass rename again.05:11
jamthumper: or would you rather see tools.Tools *and* and agent/* package?05:11
thumperjam: there are lots of other tools methods too05:12
thumperjam: I'm happy to have agent05:12
thumperjam: as long as it doesn't bring in dependencies on environs or state05:12
jamthumper: I definitely want to nuke state.Tools, I'm just trying to figure out what the best we can replace it with for now, so we can hopefully avoid changing 50+ files again :)05:12
thumperjam: just moving the dir will cause more problems05:12
thumperas there are some places that have environs/agent and tools05:13
jamthumper: right, so don't move the dir. This is more about "replace state.Tools with something better" and agent/ as a top level package probably makes more sense than tools/ (I think)05:13
thumperif you want s/tools/agent/ it will have to wait for tomorrow05:13
jamthumper: np, I'll look at your patch and bring it up with William today, so we can settle it out.05:13
jamI sort of like "tools.Tools"05:13
thumperI think agent.Tools makes more sense IMO05:13
jambut I think there is more than just tools that we want to cluster there.05:13
thumperjam: except heaps of tests has local variables called tools05:14
jamthumper: yeah05:14
jamand Tools is pretty generic05:14
* thumper nods05:14
jamso agent.Tools makes it clear it is the jujud stuff05:14
jamnot just any thing05:14
thumperI have to go put on some veg and rice05:14
thumperlater folks05:14
jamthumper: later05:14
=== tasdomas_afk is now known as tasdomas
rvbajam: Hi John… sorry to bother you with this again… but could you please… update lp:gwacl again :)  (This time I'll send an email to the list as well because we won't update gwacl in the next few days… hopefully).07:19
jamrvba: updating from 166 to 168, does that sound correct?07:21
rvbajam: yep. ta.07:22
jamrvba: for https://code.launchpad.net/~rvb/juju-core/az-destroy/+merge/173657 this is one of those cases where with mocking, if something changes will we notice?07:23
jamrvba: looking at something like "buildDestroyServiceResponse", you are describing what gwacl does under the covers, which certainly feels like a test that should be in gwacl.07:25
jamWould it make more sense to mock the gwacl layer itself, and assert what functions in gwacl are being called?07:26
rvbajam: as I said on the MP, checking which requests have been done is indeed a tradeoff: it forces you to know what gwacl does under the covers to some extend… but it's better than mocking the gwacl layer because if a method changes it's behavior in any significant way, the tests will fail.07:29
jamrvba: but isn't that what the gwacl tests are for? It feels like if you have to fix something in gwacl, causing it to change its behavior, you then have to fix the tests (without actually fixing the juju-core code).07:30
jam(eg, a False Positive test failure)07:31
rvbajam: that's right, it's a bit clumsy in that regard… but I'm not sure mocking the gwacl layer entirely is feasible.07:33
jamrvba: well 'feasible' means it would probably have to be designed from the start as 'interface' things that you then swap out for something different.07:34
jamNot necessarily feasible at this point.07:34
dimiternjam: hey07:34
jamyou can't really monkey patch in go, so you need to build in the abstraction layer07:34
jamhi dimitern07:34
rvbaWe've used that pattern in some cases.07:34
jamrvba: though to be fair, often abstraction layers leave you with really nice decoupled and coherent code :)07:35
dimiternjam: i don't think the changes to deployer would've caused the issues reported upgrading 1.10->dev07:35
dimiternjam: i tested that specifically live on ec207:35
jamdimitern: It was more being hopeful that we had an answer to why upgrade failed rather than thinking it would really fix it.07:38
dimiternjam: hmm.. actually come to think of the tests i did - it was 1.11 -> 1.11.2, so don't know about 1.10 -> 1.11.207:39
rvbajam: the problem with the design you suggest is that, ultimately, it means you are creating a test double in the provider's code.  So it might work nice and clean (compared to the testing strategy we have in place now) at first and then turn into a hairy piece of code that is awkward to maintain, especially in the provider's testing code (as opposed to on gwacl's side).07:39
jamrvba: sure, I'd certainly rather see a hypothetical double exist in gwacl than maintained in juju-core. I'm just trying to get a handle on what the tests add vs failure modes, etc.07:40
jamrvba: if you mock at the wrong level, all you end up with is "the code looks like the code"07:40
jamat the same time, you do want to end up with "things are hooked up in a sane manner".07:41
rvbajam: Don't get me wrong, I think you've got a valid point.  But what we've done here is a tradeoff that stemmed from our experience with the MAAS provider where we decided to create a full test double in the GO MAAS lib.07:41
rvbaThe testing code of the provider was nice an clean.  But the amount of work required to develop and maintain a test double in gomaasapi wasn't worth it.07:42
rvbaAt least that was our conclusion :).07:42
jamrvba: reviewd: https://codereview.appspot.com/11034044/08:00
jammgz: poke08:01
rvbajam: ta!08:01
mgzjam: hey08:46
jammgz: wow, I actually get to see you during a day :)08:46
jamwell, hear from you at least08:46
mgzyeah, trying to get ack on earlier footing08:47
dimiternfwereade: ping08:50
fwereadedimitern, pong08:50
dimiternfwereade: you wanted to talk about the deployer api stuff?08:50
fwereadedimitern, ah yeah08:50
fwereadedimitern, can we do a g+in 10 mins or so please?08:50
dimiternfwereade: sure, just ping me when you're free08:51
jamfwereade: State.Sync() vs State.StartSync() can I get some context as to what they mean?08:52
fwereadejam, both cause the state to get the latest content from the transaction log08:53
jam(what the difference is in practical)08:53
fwereadejam, StartSync returns immediately; Sync doesn't return until all events have been delivered by the watcher package08:53
jamfwereade: so I think I understand that Sync() waits for it to complete before you do your assertions, and StartSync does not.08:53
jamfwereade: but I don't quite understand why worker/uniter/filter_test.go uses both08:54
fwereadejam, note that this does not *necessarily* imply that all consequences have come to pass, though08:54
fwereadejam, we usually StartSync before things we don't expect to happen and Sync before things we do08:54
jamfwereade: is there a way to write a generic fuction that can be passed a  "<- chan Type" ?09:02
jamI know I can write <-chan interface{}09:02
jamBut I don't think you can pass a <-chan Type as that parameter09:02
fwereadejam, afraid not, as far as I'm aware09:03
=== _mup__ is now known as _mup_
fwereadedimitern, hey, I forgot to say I merged api-use-entities; did you have a moment to review api-life-result?10:24
dimiternfwereade: just reviewed10:29
fwereadedimitern, cheers dude10:30
dimiternfwereade: it seemed a bit weird not to have Machine.Refresh() anymore10:30
fwereadedimitern, if it's not used, I don't want it cluttering up the api10:30
fwereadedimitern, cost of reimplementing it one day is dwarfed by the mental load on everybody of keeping it around10:31
dimiternfwereade: it was to be used by the provisioner I think10:31
dimiternfwereade: but yeah, fair point10:31
fwereadedimitern, I *think* the usage profile is different there10:31
fwereadedimitern, and will probably go by another facade regardless10:31
dimiternfwereade: yeah10:32
jamfwereade: provisioner also uses WatchEnvironConfig10:32
jamwhich I know you wanted to change, though *I* don't immediately know how to restrict it.10:33
dimiternjam: but that's bound to change10:33
fwereadejam, all I expect is that we'll use your WatchFor watcher and directly get the EnvironConfig when it's needed10:33
dimiternjam: I think the idea is to have a notifywatcher for that and then call EnvironConfig() to read it when changed10:33
fwereadedimitern, and jam's already implemented that watcher ;)10:34
dimiternfwereade: yep!10:34
jamfwereade: fairy nuff10:35
jamfwereade: I'd like to get your input on: https://codereview.appspot.com/11035043/10:35
jamAs far as having a testing/* object for testing how a channel is operating10:35
jam(akin to state/testing/NotifyWatcherC but for things that aren't Watcher objects.)10:36
jamIn theory NotifyWatcherC could be implemented in terms of the testing/* object, but I didn't go that far)10:36
fwereadejam, I think that's very nice, even if I needed to reread curryResolvedMode when I encountered it10:41
fwereadejam, I'd be +1 on extending it in that direction10:41
jamfwereade: from what I can tell, go1.1 has some more Select/Chan based stuff in reflect, which would let us get around writing currying functions.10:41
jambut I didn't want to dig into that until it was actually useful :)10:42
fwereadejam, quite right too (and it'll be interesting to see how much of a difference that makes... it's nice that reflection exists, but I've never really felt the code that uses it is particularly clean or expressive)10:43
jamfwereade: yeah, reflect seems to mostly gain you "1 function to handle many types" , which interface{} mostly gets you10:43
jamexcept when it doesn't like channels10:44
jambut go 1 didn't have reflected channels :)10:44
jamfwereade: if you can think of a better name than NotifyAsserterC I'm happy to change it, btw.10:45
jtv2Any reviewers available?  I need a second review for https://codereview.appspot.com/11027043/10:45
* fwereade shrugs -- if you think of a better one then go for it :)10:46
fwereadejtv2, ack10:47
dimiternjam: wow, one of those 100+ files +1/-0 lines reviews10:48
jamdimitern: fortunately most of those +1 is a blank line :)10:50
jamdimitern: might be a case where it is easier to review on Launchpad.10:50
jamdimitern: I'm not too concerned about getting them all perfect, just getting them 'good' so people will crib from existing ones.10:51
dimiternjam: i'll take a look on lp10:54
jamthough "j j j j j j j j j j j j " isn't too hard to type :)10:54
jambut does take a lot of page reloads10:54
dimiternjam: so not all gocheck refs are changed to "gc"10:54
jamdimitern: I didn't get to that part, just changing the ordering of imports, and clarified that we want to do "gc"10:55
jamthat is going to touch a lot more content, rather than just imports10:55
dimiternjam: yeah, good point10:55
jamI also didn't get to the other 280 files.10:55
jamfwereade: http://paste.ubuntu.com/5858056/ is what a trivial folding-into-WatcherC looks like10:57
jamthe primary differences:10:57
jama) It will always call c.State.Sync() even for AssertNoChange10:57
jam(when not Lax)10:57
jamb) The naming of AssertNoChange vs AssertNoReceive10:58
jammeans that it proxies the call10:58
jamc) We lose the "watcher did[not do] something" in error messages10:58
jambecause NotifyAsserter talks about channels.10:58
jamworth it?10:58
dimiternjam: do we now have the StringsWatcher we talked about?10:59
jamdimitern: no10:59
jamdimitern: I thought that was on your plate :)11:00
dimiternjam: ah, i see :)11:00
dimiternjam: i was wondering whether to implement the UnitsWatcher in apiserver or do something more generic11:00
dimiternfwereade: what do you think? ^11:01
dimiternin any case it can be done i 2 steps, which is easier for me anyway11:01
jamdimitern: I think for the params adding a StringsWatcher interface, and then implementing UnitsWatcher in terms of that.11:01
jamsince LifeCycleWatchers are going to want it as well, right?11:01
dimiternjam: hmm or implement UW and then extract an iface from it :)11:02
jamdimitern: so the steps I did were: (1) add a state/* interface (NotifyWatcher) (2) add a corresponding api/params NotifyWatcher interface along with NotifyWatcherResult, etc. (3) add a client-side implementation of api/watcher/notifyWatcher, (4) Add a worker/NotifyWorker to consume these things into actions.11:03
jamyou should be able to do something quite similar for StringsWatcher11:03
dimiternjam: yeah, except it's a little foggy what should it have right now11:04
jamwhich might help break it down into steps to complete and get landed. I was generally working on 1-2 branches at a time to make sure that the design was reasonable11:04
jamdimitern: IMO StringsWatcher interface is just NotifyWatcher with a different Changes()11:04
dimiternjam: I should be able to copy most of the the notifywatcher, except for inital event handling and changes11:04
jamdimitern: right11:05
dimiternjam: and then LifecycleWatcher and UnitsWatcher will use StringsWatcher interface11:05
jamdimitern: we *could* go with an ObjectWatcher that had: Changes() <-chan interface{}11:05
dimiternjam: let's not do that just now :)11:06
jambut generic channels are a bit hard (as seen by my recent patch)11:06
fwereadedimitern, jam, yeah, let's make it Strings11:06
jamdimitern: right: func  NewLifecycleWatcher(...) StringsWatcher11:06
dimiternfwereade: Strings?11:07
dimiternfwereade: you mean StringsWatcher or?11:07
fwereadedimitern, StringsWatcher over the API, yeah11:07
jamdimitern: StringsWatcher rather than ObjectWatcher11:08
dimiternfwereade, jam: oh yeah, absolutely11:08
fwereadejtv2, reviewed11:22
jtv2thanks fwereade11:36
fwereadedimitern, jam: if I were to say "relation tags", what would spring to mind?12:09
dimiternfwereade: i see where are you going :)12:09
jamfwereade: I'd be making something up, but a handle for a single relation12:09
jam*or* a relations endpoints defined in terms of tags12:09
fwereadedimitern, jam: the main issue is that relation keys (may) contain spaces, and this doesn't fit well with our existing taggery12:10
dimiternfwereade: relation-wordpress:db-mysql:server ?12:10
dimiternfwereade: should the spaces matter? after the "relation-" prefix anything valid goes12:10
fwereadedimitern, I fear that - is valid in relation name as well as service12:10
fwereadedimitern, fair point, maybe spaces in tags are ok, but if feels a little bit off12:11
jamfwereade: sure, but we still prefix it12:11
dimiternfwereade: so how about @ or $ or # or some other separator?12:11
jamit is true that any-given-string may accidentally match12:11
jambut as long as we only actually pass a real Tag into a Tag field12:11
fwereadedimitern, jam: I was wondering how you guys would feel about relation-%d12:11
jamit must have the "start-" be unique12:11
dimiternfwereade: ah, actually better12:12
dimiternfwereade: not worse than machine-% anyway12:12
jamfwereade: how are relations addressed currently, just by id?12:12
fwereadejam, ha, well, they have an id and a key12:12
dimiternjam: internally; otherwise by name12:12
fwereadejam, the key is derived from the endpoints12:12
jamfwereade: relationDoc struct {Key string `bson:"_id"`}12:13
fwereadejam, stringed in a canonical order, separated by spaces12:13
fwereadejam, yeah, exactly12:13
dimiternfwereade: "derived" is the tricky part12:13
jamI think that means they have a "Key" which is acutalyl an id12:13
jambut I guess they also have an Id12:13
dimiternimplies ordering12:13
fwereadejam, tell me about it12:13
jam fwereade: so does that mean that "Key" as derived from endpoints is actually indexed by Mongo (because it is the _id), but Id as an int is not?12:14
fwereadejam, it is12:14
jamthat would be the only reason I can think of to settle what reference to use in a tag12:14
jamsomething that can be looked up via index12:15
jamso relation-%d is fine, as long as %d is indexed, IMO12:15
fwereadejam, Id is, er, probably not indexed, but should be12:15
dimiternthe id is unique and unambiguous12:15
jamfwereade: I guess that is what our 2k service tests are for, right?12:15
dimiternwhat it's not is "user-friendly"12:15
jamdimitern: I don't think anyone is going to be manually entering them, are they?12:16
fwereadedimitern, yeah, good point, I think that swings it even if it's not the nicest12:16
jamThey will be discovered via api requests.12:16
fwereadejam, dimitern: and relation keys are somewhat unfriendly to enter anyway12:16
dimiternjam: no, but these ids are only internal and never displayed (hmm maybe in hook env vars only)12:16
jamfwereade: relation keys seem far harder to *type* because of the derived nature12:16
fwereadejam, dimitern: what with having to know all names and roles and that12:16
fwereadedam, yeah12:16
fwereadejam, dimitern: hmm, *except*12:17
dimiternfwereade: if there is a way to convert id to name in a tool it doesn't matter really12:17
fwereadejam, dimitern: the LifecycleWatcher uses _id, therefore Key, and it would be nice to be able to easily convert watcher results into tags12:18
dimiternjust relation-name <id> returns derived name and copy/paste that if you need it?12:18
fwereadedimitern, I think the human-readability is a bit of a red herring maybe12:18
fwereadedimitern, in charms there's a different vocabulary (sigh) derived from the id12:18
dimiternfwereade: well, for display purposes we're not using either anyway12:19
jamfwereade: lifecyclewatcher could be taught to return Tags in the api...12:19
dimiternfwereade: we use verbose yaml dump12:19
fwereadejam, yeah, I am wondering about that12:19
fwereadejam, but the same consideration applies to some degree at least12:19
fwereadejam, it should be convenient to convert them12:19
jamfwereade: as in, you should be able to convert them without a db query? Though aren't they used to query the db?12:20
fwereadejam, and with state/watcher we're stuck with _id-based watches at the lowest level12:20
dimiternjam: you don't need a db query - you already have all the services in the relation struct, so RelationTag can be simply implemented12:21
fwereadejam, most of them probably will be, yeah, but having to read the whole object to get an id that we can subsequently use to read it again feels, er, suboptimal12:21
jamfwereade: well the API is intended as a point of Abstraction so we don't have to worry about what is in the DB quite as much.12:21
fwereadejam, dimitern, relation.Tag is easy whatever we choose, Key and Id are both present12:21
fwereadejam, ah, hmm, nice theory, practical concern12:23
fwereadejam, I don't think we can depend on the db docs still existing when we want to convert12:23
dimiternfwereade: why didn't we have relation tags before? there wasn't any need for them through the api?12:23
fwereadejam, so we my be stuck with Key-based ones12:23
fwereadedimitern, yeah, can't auth as *or annotate* a relation12:24
fwereadejam, dimitern: so I think we're stuck with Key12:24
dimiternfwereade: why would we ever need to auth as a relation?12:25
fwereadedimitern, quite12:25
fwereadedimitern, that's why it's not an auther12:25
dimiternnot even an otter :D12:25
fwereadedimitern, but we never annotate them either, which is how this never got considered12:25
dimiternfwereade: hmm.. that might be useful i guess for the gui12:26
fwereadedimitern, jam: what's the worst that can happen if we put a " " in a tag, I wonder12:26
fwereadedimitern, maybe, but they didn't do it because they didn't need it afaik12:26
jamfwereade: the worst? Someone uses it in a shell script and it splits on the extra space.12:27
dimiternfwereade: spaces are scary.. c'mon it's not 70s anymore12:27
jamAnd that causes rm -rf to run on their personal machine12:27
jamand that causes them to go on a killing spree triggering a nuclear war12:27
dimiternany script should assume spaces might be present and use ""12:27
jamfwereade: The only legitimate concern I would have is the charm using shell case12:28
jamI don't know that relation-tag would even be exposed there.12:28
fwereadedimitern, jam: *that* won't happen because the charm doesn't see tags12:28
fwereadedimitern, jam: but the spectres yu raise still bug me12:28
dimiternfwereade: can't we simply s/ /_/ or something?12:29
fwereadedimitern, jam, hmm, maybe12:29
dimiternboth ways12:29
fwereadedimitern, jam, I have a horrible feeling we don't restrict relation names much :/12:29
jamfwereade: it wouldnt' be guaranteed unique if you munge it.12:29
dimiternintelligently ofc - check for existence of the result12:29
jamrelationship_foo and relationship foo12:30
dimiternfwereade: +1 to that12:30
jamfwereade: the transports we currently use for talking about stuff are all space-safe.12:30
jam"JSON" string, etc.12:30
jamshell is the only place I can think of that could be a problem.12:30
dimiternfwereade: we should at least restrict spaces and a few other chars we could use as separators12:30
fwereadejam, dimitern: yeah, I'll try to check that out12:31
fwereadejam, dimitern: it's grown beyond what I should do in this CL anyway12:31
dimiternfwereade: proposal: restrict " ", "@" and/or "#" in relation names?12:31
fwereadejam, dimitern: will surprise us when we hit the uniter facade12:31
fwereadedimitern, I'd much rather restrict it harder, to basically what we have in service names12:32
dimiterneven "/" as we use it already as a separator12:32
dimiternfwereade: ah, good point12:32
fwereadedimitern, weird to have those in rel tags when we sub them out in unit tags12:32
dimiterngive the people too much freedom in name choices and all hell breaks loose ;)12:32
fwereadedimitern, jam: future plans: restrict charm relation names, implement relation tags with s/ /$some_forbidden_char/12:33
fwereadedimitern, today plans: ignore relation tags12:33
dimiternfwereade: sgtm12:33
jamfwereade, dimitern: I'd like a second pair of eyes on something with cmd/jujud/upgrade.go. It has this fancy "delayedTomb()" logic, that AFAICT isn't actually doing anything.13:14
jamit is designed that if delayedtomb->Dying is set, it sleeps for a time and then calls actualtomb->Kill()13:15
jamhowever, the delayed tomb is only a local variable13:15
=== teknico1 is now known as teknico
dimiternjam: where?13:33
jamlook for tomb := delayedTomb()13:33
jamI don't see anything that could try to stop tomb rather than lots of stuff that would stop Upgrader.tomb13:33
jamanyway, I'm done for today. I was just trying to see if I needed to mimic that code, but it doesn't seem to actually be used.13:36
dimiternjam: it looks fishy13:36
jamdimitern: if it set self.tomb or something like that, then I could see it being used13:37
dimiternjam: and it's used in several places as noDelay(), but i'm yet to figure out why13:37
jamdimitern: it might be that I mixed the order around.13:37
dimiternjam: hmm13:37
jamSo if someone calls Kill on Upgrader.Kill() it takes a while to actually call tomb.Kill13:37
jamso the download loop takes 5 min to respond to a Stop request13:38
dimiternjam: i'm not sure but in live tests i observed a lot less than that13:39
jamah, the idea is that Runner will see a task fail (which actually fails because we need an upgrade) so we still allow this task to download new tools.13:39
jamdimitern: upgraderDelay is a var that is probably overridden in tests13:39
dimiternjam: seems likely13:39
dimiternjam: anyway, have a good evening then13:39
fwereadejam, oh, christ, that stuff13:47
fwereadejam, I don't think it's necessary with a sanely configured worker.Runner13:48
fwereadedimitern, jam: you know how any error in any task takes down every task?13:48
fwereadedimitern, jam: the kill delay was a "solution" to that problem13:48
dimiternfwereade: i thought we were changing this with the task runners though?13:49
dimiternfwereade: aahh13:49
fwereadedimitern, jam: so that an upgrade that caused some task to error didn't prevent us from being able to upgrade away from the broken code13:49
fwereadedimitern, jam: now we can implement tasks independently I think it's entirely redundant13:49
fwereadedimitern, jam: that's not to say there aren't failure cases13:50
fwereadedimitern, jam: but there always were anyway13:50
dimiternfwereade: cool, so we can get rid of it once we use the runner13:50
fwereadedimitern, +113:50
fwereadejam, btw, did you see that mail on the lists yesterday? short version is that we somehow installed the wrong mongodb... was wondering if you had any insight into this13:51
fwereadeor mgz for that matter ^^13:52
fwereadedimitern, btw, https://codereview.appspot.com/11041043 is what I was talking about earlier, that I hope implements half the Deployer's methods13:53
dimiternfwereade: cool, will take a look in a bit13:54
* dimitern needs to step out for 20m13:54
=== teknico is now known as teknico-phone
dimiternfwereade: reviewed14:52
fwereadedimitern, cheers14:52
ackkhi, does anyone know what could be the issue here: http://pastebin.ubuntu.com/5858644/ ? It only happens if I deploy a charm from a local repo15:03
dimiternackk: can I see the config.yaml of the charm in question?15:04
ackkdimitern, it's a checkout of lp:~landscape-charmers/charms/precise/landscape-client/trunk , no changes15:06
ackkdimitern, I also tried with a very basic charm, that only logs hooks15:07
ackkdimitern, fwiw, "juju deploy cs:~landscape-charmers/precise/landscape-client" works15:07
dimiternackk: and you're using juju-core tip?15:08
ackkdimitern, yes15:08
dimiternackk: and with the basic charm the same happens?15:08
ackkdimitern, as of 10 minutes ago, at least :)15:08
ackkdimitern, yes15:08
dimiternackk: is ./charms a directory where you're running it?15:09
ackkdimitern, yes, I also tried absolute paths and "." from the directory itself15:10
dimiternackk: can you paste the output of "du -h --all" inside the charms dir?\15:12
ackkdimitern, http://paste.ubuntu.com/5858663/15:13
dimiternackk: and you don't have JUJU_REPOSITORY env var set?15:14
ackkdimitern, nope15:15
dimiternackk: just a sec15:15
ackkdimitern, it seems something strange with that dir, I just tried checking out in a different repo dir and it worked15:16
dimiternackk: ah :)15:16
ackkdimitern, maybe something related to .bzr ?15:16
dimiternackk: shouldn't matter but might be15:16
dimiternackk: can you please test it with .bzr inside and if you repoduce it, please file a bug15:17
=== tasdomas is now known as tasdomas_afk
ackkdimitern, so the .bzr is only inside the landscape-client dir, but I can't reproduce it outside of that dir. Maybe it's just messed up...15:20
ackkdimitern, thanks for the help15:21
dimiternackk: nevertheless, panics like this are really bad - if you manage to backtrack your steps and reproduce it in a bug report will be really awesome!15:21
ackkdimitern, sure, I'll try15:22
dimiternackk: tyvm15:22
fwereadeackk, would you paste the config.yaml please?15:56
fwereadeackk, it's definitely the same in both cases?15:56
ackkfwereade, for landscape-client local and cs: charms? yes, it is. It has something to do with that dir specifically15:57
ackkfwereade, dimitern oh wait I got it, the other charm (not the one I was deploying) had an empty config.yaml, removing it made it work16:02
fwereadeackk, that is technically an invalid config then... but it should be an error, not a panic, I think16:03
fwereade(well obviously it shouldn't be a panic)16:03
fwereade(but part of me is saying that a config with out an "options" key is, well, just a config without options, and should maybe warn but not error... any takers?)16:04
dimiternfwereade, ackk: yeah, that was what I was thinking of - Unmarshl in charm.ReadConfig returns an uninitialized Options map and that panics in the range loop around line 104 in charm/config.go16:08
fwereadedimitern, I think it's an uninitialized *Config, but yeah16:09
dimiternfwereade: yeah in fact16:09
fwereadedimitern, annoying that an empty file unmarshals into a nil pointer without error16:09
dimiternfwereade: I still think a bug report is in order16:09
ackkdimitern, yeah, what confused me is that the issue was actually in a different charm in the same repo, I didn't expect it to be relevant16:09
fwereadedimitern, I'm not sure it's actually a bug, I suspect it's annoying-but-coherent behaviour16:10
dimiternackk: well, at some point when we read the repo I *think* we scan all charms and this might be the issue16:10
ackkdimitern, I see16:10
dimiternfwereade: a panic like this surely deserves a bug and some handling?16:11
fwereadedimitern, definitely16:11
fwereadedimitern, (blast, that was a pastebin link not a bug link)16:11
fwereadedimitern, ackk, I'll do one, thanks16:11
ackkfwereade, dimitern  thank you16:12
dimiternfwereade: we might want to audit all places we Unmarshal stuff like that for similar issues16:12
dimiternackk: thanks for catching this16:12
fwereadedimitern, +1,x216:14
dimiternfwereade: CleanupWatcher seems a candidate for unexporting and migrating to NotifyWatcher16:23
fwereadejam, have a word with jam about it, I think he mentioned that too16:24
dimiternfwereade: ok16:30
TheMueremember - tests don't work if you create temporary directories but don't use them16:32
dimiternfwereade, TheMue: a simple review? https://codereview.appspot.com/11044043/16:34
TheMuedimitern: *click*16:35
* dimitern bbiab16:35
fwereadedimitern, LGTM, one bit to fix16:36
=== tasdomas_afk is now known as tasdomas
=== tasdomas is now known as tasdomas_afk
TheMueah, it passed, yeah16:47
TheMuedimitern: oh, btw, yes, from my side an lgtm too.16:48
dimiternfwereade, TheMue: thanks!16:54
dimiternabentley: hey16:55
abentleydimitern: Hey.16:56
dimiternabentley: I wanted to ask you about the rietveld-enabled versions of lp-propose and lp-submit - where can I find them?16:56
abentleydimitern: It was at https://code.launchpad.net/rvsubmit, but you may not be able to access it because the commercial license has expired.16:58
abentleydimitern: It submits only.  It does not propose.16:59
dimiternabentley: so the idea was to use lbox propose, and rvsubmit later, so you don't have to set LP commit message, set bugs to committed (if linked) and approve the MP?17:03
abentleydimitern: Right, except that it doesn't do the bugs.  I think those get set automatically when the branch is merged, but I could be wrong.17:04
dimiternabentley: well, bugs are no big deal anyway, not so many we're dealing with right now (we will at some point though :) - btw I can access it on LP - so is it a bzr plugin or a separate script?17:05
abentleydimitern: It's a plugin.17:05
dimiternfwereade: I don't really get why srvLifecycleWatcher should be renamed to srvStringsWatcher?17:07
dimiternabentley: i'll try it out, cheers17:08
abentleydimitern: Do you have Tarmac running now, then?17:08
fwereadedimitern, everywhere up from state we should be dropping the distinctions, surely?17:08
dimiternfwereade: do you also mean changing Lifecycle* types to Strings* types - in params, etc.?17:09
dimiternabentley: yep, for some weeks now - and working even :)17:09
fwereadedimitern, probably, yes, although I'd forgotten it had got that far, I'm fine doing it in stages if that's where a sane line seems to you to be17:09
dimiternfwereade: quick g+ ?17:10
fwereadedimitern, sure17:10
dimiternfwereade: https://plus.google.com/hangouts/_/5faf4d98498233fe5228a813ee7bda05c5c2a20c?authuser=0&hl=en17:11
jamdimitern: you mean https://codereview.appspot.com/10876048/ ? (CleanerWorker ?)17:12
dimiternjam: not really - i meant just replace CleanupWatcher with NotifyWatcher in state17:13
jamabentley, dimitern: Tarmac will set bugs to Fix Committed when it merges the code if they are linked on LP to the MP (which happens if you do it directly, via lbox ,or with commit --fixes)17:13
jamdimitern: ah sorry, was thinking the wrong side.17:13
jamI have Workers on the brain17:13
dimiternjam: that last part I didn't get - what's commit --fixes?17:13
jam'bzr commit --fixes lp:1234556'17:14
jamwill set bzr metadata that Launchpad will notice and associate your branch to a bug17:14
dimiternjam: oww - how awesome is that :)17:14
dimiternjam: btw, if you haven't landed that CL yet, consider adding that change to state?17:14
dimiternfwereade: ping?17:14
jamI landed the CleanerWorker already17:14
abentleyjam: There's also lp-link-bug, but that's in the lpreview_body plugin.  I should probably fix that.17:15
dimiternjam: ah, but you have i see17:15
jamdimitern: I think you could just change line 1229, with "func (st *State) WatchCleanups() NotifyWatcher" because *CleanupWatcher is already a NotifyWatcher in spirit17:16
dimiternjam: yep something like that17:21

Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!