| bigjools | does juju assume all machine units have a different IP? | 03:11 |
|---|---|---|
| wallyworld_ | i think so | 03:20 |
| thumper | bigjools: assume in what way? | 03:39 |
| bigjools | thumper: 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 stuff | 03:40 |
| bigjools | Azure is not a "flat" service | 03:40 |
| thumper | bigjools: I don't think anything is indexed on IP | 03:40 |
| thumper | however it does use the address to talk to the machines | 03:40 |
| bigjools | yeah I figured | 03:41 |
| thumper | it is what is propagated between units in relations | 03:41 |
| thumper | there is some work pending which mgz is starting around machine addressability | 03:41 |
| thumper | to remove some of the assumptions | 03:41 |
| thumper | and have clear expectations | 03:41 |
| thumper | bigjools: so touch base with mgz for more info | 03:41 |
| bigjools | Azure gives multiple VMs the same IP address and load balances unless you do things differently | 03:41 |
| thumper | ?! | 03:42 |
| bigjools | thumper: right thanks - Gavin was talking to him already | 03:42 |
| thumper | you should be fine then :) | 03:42 |
| bigjools | In Azure, you get this sort of structure: Service -> Deployment -> VM / VM / ... | 03:42 |
| bigjools | each "service" has its own IP | 03:43 |
| bigjools | rather than the VM | 03:43 |
| jtv2 | Any reviewers in the house? I've got a big one waiting: https://codereview.appspot.com/11027043 | 04:00 |
| bigjools | way to attract reviewers :) | 04:02 |
| jtv2 | Any worse than the built-in "please take a look"? :) | 04:03 |
| * thumper does a mega move | 04:31 | |
| bigjools | Can anyone help with this panic in the stdlib please? http://play.golang.org/p/zRZh_cXGy9 | 04:42 |
| * davecheney looks | 04:43 | |
| bigjools | I 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 |
| davecheney | it's because you're trying to marshall a []string into | 04:47 |
| davecheney | <somethign name="..." /> | 04:47 |
| davecheney | where ... is whatever []string marshalls too in xml | 04:48 |
| bigjools | yes I was expecting <node name="cgf" name="dfgfdg"/> | 04:48 |
| * davecheney scratches | 04:48 | |
| davecheney | head | 04:48 |
| davecheney | i dunno if that is valid xml | 04:48 |
| davecheney | two secs | 04:48 |
| bigjools | might not be | 04:48 |
| davecheney | i don't think it is | 04:48 |
| davecheney | but it's a shit poor error message in that case | 04:48 |
| bigjools | what I really wanted was an array of <something name="foo"> | 04:48 |
| bigjools | thanks for looking | 04:49 |
| davecheney | bigjools: np | 04:49 |
| * davecheney ponders http://www.w3.org/TR/REC-xml/#attdecls | 04:49 | |
| davecheney | it's been a while since i've had to read this train wreck | 04:49 |
| bigjools | XML is the spawn of satan anyway | 04:50 |
| davecheney | bigjools: so youi wanted | 04:50 |
| davecheney | <div> | 04:50 |
| davecheney | <something name="foo" /> | 04:50 |
| davecheney | <something name="bar" /> | 04:50 |
| bigjools | yes | 04:50 |
| davecheney | </div> | 04:50 |
| bigjools | I have that now | 04:50 |
| davecheney | ok | 04:51 |
| bigjools | by making a sub-struct array | 04:51 |
| davecheney | yup, that is the solution | 04:51 |
| davecheney | the impdendence mismatch between XML and Go is pretty high | 04:51 |
| davecheney | Gustavo did some work to make it less crap before 1.0 shipped | 04:51 |
| bigjools | it has raised some eyebrows | 04:51 |
| davecheney | but it's still margian | 04:51 |
| davecheney | marginal | 04:51 |
| bigjools | you can't use attr with chains, either :( | 04:52 |
| davecheney | chains ? | 04:53 |
| bigjools | xml:Node1>Node2 | 04:53 |
| davecheney | there is a syntax | 04:53 |
| davecheney | it is certainly non intuative | 04:53 |
| bigjools | ie `xml:"Node>SubNode,attr"` | 04:53 |
| davecheney | i think that is a bridge too far | 04:54 |
| davecheney | i don't go in that package much | 04:54 |
| * bigjools idly wonders if xpath would have been less of a shit sandwich | 04:54 | |
| davecheney | I believe our man in Brazil might have a solution to that | 04:55 |
| davecheney | http://blog.labix.org/2013/06/07/efficient-xpath-for-go | 04:55 |
| davecheney | not tried yet | 04:55 |
| thumper | review incomming $ bzr diff -r branch::parent | wc -l | 05:03 |
| thumper | 2315 | 05:03 |
| thumper | 59 files changed, 348 insertions(+), 363 deletions(-) | 05:04 |
| * davecheney shrieks | 05:06 | |
| thumper | this branch has a high likelyhood of clashing with someone | 05:06 |
| davecheney | i guess that is how you got your nick | 05:06 |
| thumper | davecheney: I bit the bullet for jam | 05:06 |
| thumper | davecheney: this branch kills state.Tools | 05:07 |
| jam | ; | 05:07 |
| thumper | and makes everything use tools.Tools | 05:07 |
| thumper | and moves agent/tools.go into tools/agent.go | 05:07 |
| thumper | lbox is currently working out the diff | 05:07 |
| thumper | jam: I wanted to mess around with tools a little | 05:08 |
| thumper | jam: in particular, uploading pre-built tools for the local provider | 05:08 |
| thumper | into the local storage | 05:08 |
| thumper | so I needed to refactor a few methods anyway | 05:08 |
| thumper | seemed stupid to do it in the old place | 05:08 |
| thumper | Rietveld: https://codereview.appspot.com/11028043 | 05:08 |
| jam | thumper: per the request from fwereade he really likes the idea of an "agent" top level package, rather than a "tools" one. | 05:08 |
| thumper | entirely mechanical | 05:08 |
| jam | So I was going to pull environs/agent top level and replace tools with it. | 05:09 |
| jam | Though still have agent/Tools instead of state/Tools. | 05:09 |
| thumper | jam: well... someone can change the files later | 05:09 |
| thumper | this is done | 05:09 |
| thumper | lots of tests fixed | 05:09 |
| thumper | jam: well, the work is half done | 05:09 |
| thumper | baby steps, no? | 05:09 |
| thumper | I have to admit, the review looks impressive | 05:10 |
| jam | thumper: 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 |
| jam | We don't have to move the rest of the agent stuff | 05:11 |
| jam | just call it agent.Tools so we don't have to have a mass rename again. | 05:11 |
| jam | thumper: or would you rather see tools.Tools *and* and agent/* package? | 05:11 |
| thumper | jam: there are lots of other tools methods too | 05:12 |
| thumper | jam: I'm happy to have agent | 05:12 |
| thumper | jam: as long as it doesn't bring in dependencies on environs or state | 05:12 |
| jam | thumper: 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 |
| thumper | jam: just moving the dir will cause more problems | 05:12 |
| thumper | as there are some places that have environs/agent and tools | 05:13 |
| jam | thumper: 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 |
| thumper | if you want s/tools/agent/ it will have to wait for tomorrow | 05:13 |
| jam | thumper: np, I'll look at your patch and bring it up with William today, so we can settle it out. | 05:13 |
| jam | I sort of like "tools.Tools" | 05:13 |
| thumper | I think agent.Tools makes more sense IMO | 05:13 |
| jam | but I think there is more than just tools that we want to cluster there. | 05:13 |
| thumper | jam: except heaps of tests has local variables called tools | 05:14 |
| jam | thumper: yeah | 05:14 |
| jam | and Tools is pretty generic | 05:14 |
| * thumper nods | 05:14 | |
| jam | so agent.Tools makes it clear it is the jujud stuff | 05:14 |
| jam | not just any thing | 05:14 |
| thumper | I have to go put on some veg and rice | 05:14 |
| thumper | later folks | 05:14 |
| jam | thumper: later | 05:14 |
| === tasdomas_afk is now known as tasdomas | ||
| rvba | jam: 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 |
| jam | rvba: updating from 166 to 168, does that sound correct? | 07:21 |
| rvba | jam: yep. ta. | 07:22 |
| jam | rvba: 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 |
| jam | rvba: 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 |
| jam | Would it make more sense to mock the gwacl layer itself, and assert what functions in gwacl are being called? | 07:26 |
| rvba | jam: 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 |
| jam | rvba: 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 |
| rvba | jam: 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 |
| jam | rvba: 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 |
| jam | Not necessarily feasible at this point. | 07:34 |
| dimitern | jam: hey | 07:34 |
| jam | you can't really monkey patch in go, so you need to build in the abstraction layer | 07:34 |
| jam | hi dimitern | 07:34 |
| rvba | Right. | 07:34 |
| rvba | We've used that pattern in some cases. | 07:34 |
| jam | rvba: though to be fair, often abstraction layers leave you with really nice decoupled and coherent code :) | 07:35 |
| dimitern | jam: i don't think the changes to deployer would've caused the issues reported upgrading 1.10->dev | 07:35 |
| dimitern | jam: i tested that specifically live on ec2 | 07:35 |
| jam | dimitern: It was more being hopeful that we had an answer to why upgrade failed rather than thinking it would really fix it. | 07:38 |
| dimitern | jam: 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.2 | 07:39 |
| rvba | jam: 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 |
| jam | rvba: 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 |
| jam | r | 07:40 |
| jam | rvba: if you mock at the wrong level, all you end up with is "the code looks like the code" | 07:40 |
| jam | at the same time, you do want to end up with "things are hooked up in a sane manner". | 07:41 |
| rvba | jam: 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 |
| rvba | The 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 |
| rvba | At least that was our conclusion :). | 07:42 |
| jam | rvba: reviewd: https://codereview.appspot.com/11034044/ | 08:00 |
| jam | mgz: poke | 08:01 |
| rvba | jam: ta! | 08:01 |
| mgz | jam: hey | 08:46 |
| jam | mgz: wow, I actually get to see you during a day :) | 08:46 |
| jam | well, hear from you at least | 08:46 |
| mgz | yeah, trying to get ack on earlier footing | 08:47 |
| mgz | *back | 08:47 |
| dimitern | fwereade: ping | 08:50 |
| fwereade | dimitern, pong | 08:50 |
| dimitern | fwereade: you wanted to talk about the deployer api stuff? | 08:50 |
| fwereade | dimitern, ah yeah | 08:50 |
| fwereade | dimitern, can we do a g+in 10 mins or so please? | 08:50 |
| dimitern | fwereade: sure, just ping me when you're free | 08:51 |
| jam | fwereade: State.Sync() vs State.StartSync() can I get some context as to what they mean? | 08:52 |
| fwereade | jam, both cause the state to get the latest content from the transaction log | 08:53 |
| jam | (what the difference is in practical) | 08:53 |
| fwereade | jam, StartSync returns immediately; Sync doesn't return until all events have been delivered by the watcher package | 08:53 |
| jam | fwereade: so I think I understand that Sync() waits for it to complete before you do your assertions, and StartSync does not. | 08:53 |
| jam | fwereade: but I don't quite understand why worker/uniter/filter_test.go uses both | 08:54 |
| fwereade | jam, note that this does not *necessarily* imply that all consequences have come to pass, though | 08:54 |
| fwereade | jam, we usually StartSync before things we don't expect to happen and Sync before things we do | 08:54 |
| jam | fwereade: is there a way to write a generic fuction that can be passed a "<- chan Type" ? | 09:02 |
| jam | I know I can write <-chan interface{} | 09:02 |
| jam | But I don't think you can pass a <-chan Type as that parameter | 09:02 |
| jam | right? | 09:02 |
| fwereade | jam, afraid not, as far as I'm aware | 09:03 |
| === _mup__ is now known as _mup_ | ||
| fwereade | dimitern, hey, I forgot to say I merged api-use-entities; did you have a moment to review api-life-result? | 10:24 |
| dimitern | fwereade: just reviewed | 10:29 |
| fwereade | dimitern, cheers dude | 10:30 |
| dimitern | fwereade: it seemed a bit weird not to have Machine.Refresh() anymore | 10:30 |
| fwereade | dimitern, if it's not used, I don't want it cluttering up the api | 10:30 |
| fwereade | dimitern, cost of reimplementing it one day is dwarfed by the mental load on everybody of keeping it around | 10:31 |
| dimitern | fwereade: it was to be used by the provisioner I think | 10:31 |
| dimitern | fwereade: but yeah, fair point | 10:31 |
| fwereade | dimitern, I *think* the usage profile is different there | 10:31 |
| fwereade | dimitern, and will probably go by another facade regardless | 10:31 |
| dimitern | fwereade: yeah | 10:32 |
| jam | fwereade: provisioner also uses WatchEnvironConfig | 10:32 |
| jam | which I know you wanted to change, though *I* don't immediately know how to restrict it. | 10:33 |
| dimitern | jam: but that's bound to change | 10:33 |
| fwereade | jam, all I expect is that we'll use your WatchFor watcher and directly get the EnvironConfig when it's needed | 10:33 |
| dimitern | jam: I think the idea is to have a notifywatcher for that and then call EnvironConfig() to read it when changed | 10:33 |
| fwereade | dimitern, and jam's already implemented that watcher ;) | 10:34 |
| dimitern | fwereade: yep! | 10:34 |
| jam | fwereade: fairy nuff | 10:35 |
| jam | fwereade: I'd like to get your input on: https://codereview.appspot.com/11035043/ | 10:35 |
| jam | As far as having a testing/* object for testing how a channel is operating | 10:35 |
| jam | (akin to state/testing/NotifyWatcherC but for things that aren't Watcher objects.) | 10:36 |
| jam | In theory NotifyWatcherC could be implemented in terms of the testing/* object, but I didn't go that far) | 10:36 |
| fwereade | jam, I think that's very nice, even if I needed to reread curryResolvedMode when I encountered it | 10:41 |
| fwereade | jam, I'd be +1 on extending it in that direction | 10:41 |
| jam | fwereade: 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 |
| jam | but I didn't want to dig into that until it was actually useful :) | 10:42 |
| fwereade | jam, 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 |
| jam | fwereade: yeah, reflect seems to mostly gain you "1 function to handle many types" , which interface{} mostly gets you | 10:43 |
| jam | except when it doesn't like channels | 10:44 |
| jam | but go 1 didn't have reflected channels :) | 10:44 |
| jam | fwereade: if you can think of a better name than NotifyAsserterC I'm happy to change it, btw. | 10:45 |
| jtv2 | Any 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 | |
| fwereade | jtv2, ack | 10:47 |
| jtv2 | thx | 10:48 |
| dimitern | jam: wow, one of those 100+ files +1/-0 lines reviews | 10:48 |
| jam | dimitern: fortunately most of those +1 is a blank line :) | 10:50 |
| jam | dimitern: might be a case where it is easier to review on Launchpad. | 10:50 |
| jam | dimitern: I'm not too concerned about getting them all perfect, just getting them 'good' so people will crib from existing ones. | 10:51 |
| dimitern | jam: i'll take a look on lp | 10:54 |
| jam | though "j j j j j j j j j j j j " isn't too hard to type :) | 10:54 |
| jam | but does take a lot of page reloads | 10:54 |
| dimitern | jam: so not all gocheck refs are changed to "gc" | 10:54 |
| jam | dimitern: I didn't get to that part, just changing the ordering of imports, and clarified that we want to do "gc" | 10:55 |
| jam | that is going to touch a lot more content, rather than just imports | 10:55 |
| dimitern | jam: yeah, good point | 10:55 |
| jam | I also didn't get to the other 280 files. | 10:55 |
| jam | fwereade: http://paste.ubuntu.com/5858056/ is what a trivial folding-into-WatcherC looks like | 10:57 |
| jam | the primary differences: | 10:57 |
| jam | a) It will always call c.State.Sync() even for AssertNoChange | 10:57 |
| jam | (when not Lax) | 10:57 |
| jam | b) The naming of AssertNoChange vs AssertNoReceive | 10:58 |
| jam | means that it proxies the call | 10:58 |
| jam | c) We lose the "watcher did[not do] something" in error messages | 10:58 |
| jam | because NotifyAsserter talks about channels. | 10:58 |
| jam | worth it? | 10:58 |
| dimitern | jam: do we now have the StringsWatcher we talked about? | 10:59 |
| jam | dimitern: no | 10:59 |
| jam | dimitern: I thought that was on your plate :) | 11:00 |
| dimitern | jam: ah, i see :) | 11:00 |
| dimitern | jam: i was wondering whether to implement the UnitsWatcher in apiserver or do something more generic | 11:00 |
| dimitern | fwereade: what do you think? ^ | 11:01 |
| dimitern | in any case it can be done i 2 steps, which is easier for me anyway | 11:01 |
| jam | dimitern: I think for the params adding a StringsWatcher interface, and then implementing UnitsWatcher in terms of that. | 11:01 |
| jam | since LifeCycleWatchers are going to want it as well, right? | 11:01 |
| dimitern | jam: hmm or implement UW and then extract an iface from it :) | 11:02 |
| jam | dimitern: 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 |
| jam | you should be able to do something quite similar for StringsWatcher | 11:03 |
| dimitern | jam: yeah, except it's a little foggy what should it have right now | 11:04 |
| jam | which 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 reasonable | 11:04 |
| jam | dimitern: IMO StringsWatcher interface is just NotifyWatcher with a different Changes() | 11:04 |
| dimitern | jam: I should be able to copy most of the the notifywatcher, except for inital event handling and changes | 11:04 |
| jam | dimitern: right | 11:05 |
| dimitern | jam: and then LifecycleWatcher and UnitsWatcher will use StringsWatcher interface | 11:05 |
| jam | dimitern: we *could* go with an ObjectWatcher that had: Changes() <-chan interface{} | 11:05 |
| dimitern | jam: let's not do that just now :) | 11:06 |
| jam | but generic channels are a bit hard (as seen by my recent patch) | 11:06 |
| fwereade | dimitern, jam, yeah, let's make it Strings | 11:06 |
| jam | dimitern: right: func NewLifecycleWatcher(...) StringsWatcher | 11:06 |
| jam | IMO | 11:06 |
| dimitern | fwereade: Strings? | 11:07 |
| dimitern | fwereade: you mean StringsWatcher or? | 11:07 |
| fwereade | dimitern, StringsWatcher over the API, yeah | 11:07 |
| jam | dimitern: StringsWatcher rather than ObjectWatcher | 11:08 |
| dimitern | fwereade, jam: oh yeah, absolutely | 11:08 |
| fwereade | jtv2, reviewed | 11:22 |
| jtv2 | thanks fwereade | 11:36 |
| fwereade | dimitern, jam: if I were to say "relation tags", what would spring to mind? | 12:09 |
| dimitern | fwereade: i see where are you going :) | 12:09 |
| jam | fwereade: I'd be making something up, but a handle for a single relation | 12:09 |
| jam | *or* a relations endpoints defined in terms of tags | 12:09 |
| fwereade | dimitern, jam: the main issue is that relation keys (may) contain spaces, and this doesn't fit well with our existing taggery | 12:10 |
| dimitern | fwereade: relation-wordpress:db-mysql:server ? | 12:10 |
| dimitern | fwereade: should the spaces matter? after the "relation-" prefix anything valid goes | 12:10 |
| fwereade | dimitern, I fear that - is valid in relation name as well as service | 12:10 |
| fwereade | dimitern, fair point, maybe spaces in tags are ok, but if feels a little bit off | 12:11 |
| jam | fwereade: sure, but we still prefix it | 12:11 |
| dimitern | fwereade: so how about @ or $ or # or some other separator? | 12:11 |
| jam | it is true that any-given-string may accidentally match | 12:11 |
| jam | but as long as we only actually pass a real Tag into a Tag field | 12:11 |
| fwereade | dimitern, jam: I was wondering how you guys would feel about relation-%d | 12:11 |
| jam | it must have the "start-" be unique | 12:11 |
| dimitern | fwereade: ah, actually better | 12:12 |
| dimitern | fwereade: not worse than machine-% anyway | 12:12 |
| jam | fwereade: how are relations addressed currently, just by id? | 12:12 |
| fwereade | jam, ha, well, they have an id and a key | 12:12 |
| dimitern | jam: internally; otherwise by name | 12:12 |
| fwereade | jam, the key is derived from the endpoints | 12:12 |
| jam | fwereade: relationDoc struct {Key string `bson:"_id"`} | 12:13 |
| fwereade | jam, stringed in a canonical order, separated by spaces | 12:13 |
| fwereade | jam, yeah, exactly | 12:13 |
| dimitern | fwereade: "derived" is the tricky part | 12:13 |
| jam | I think that means they have a "Key" which is acutalyl an id | 12:13 |
| jam | but I guess they also have an Id | 12:13 |
| jam | ugh | 12:13 |
| dimitern | implies ordering | 12:13 |
| fwereade | jam, tell me about it | 12: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 |
| fwereade | jam, it is | 12:14 |
| jam | that would be the only reason I can think of to settle what reference to use in a tag | 12:14 |
| jam | something that can be looked up via index | 12:15 |
| jam | so relation-%d is fine, as long as %d is indexed, IMO | 12:15 |
| fwereade | jam, Id is, er, probably not indexed, but should be | 12:15 |
| dimitern | the id is unique and unambiguous | 12:15 |
| jam | fwereade: I guess that is what our 2k service tests are for, right? | 12:15 |
| dimitern | what it's not is "user-friendly" | 12:15 |
| jam | dimitern: I don't think anyone is going to be manually entering them, are they? | 12:16 |
| fwereade | dimitern, yeah, good point, I think that swings it even if it's not the nicest | 12:16 |
| jam | They will be discovered via api requests. | 12:16 |
| fwereade | jam, dimitern: and relation keys are somewhat unfriendly to enter anyway | 12:16 |
| dimitern | jam: no, but these ids are only internal and never displayed (hmm maybe in hook env vars only) | 12:16 |
| jam | fwereade: relation keys seem far harder to *type* because of the derived nature | 12:16 |
| fwereade | jam, dimitern: what with having to know all names and roles and that | 12:16 |
| fwereade | dam, yeah | 12:16 |
| fwereade | jam, dimitern: hmm, *except* | 12:17 |
| dimitern | fwereade: if there is a way to convert id to name in a tool it doesn't matter really | 12:17 |
| fwereade | jam, dimitern: the LifecycleWatcher uses _id, therefore Key, and it would be nice to be able to easily convert watcher results into tags | 12:18 |
| dimitern | just relation-name <id> returns derived name and copy/paste that if you need it? | 12:18 |
| fwereade | dimitern, I think the human-readability is a bit of a red herring maybe | 12:18 |
| fwereade | dimitern, in charms there's a different vocabulary (sigh) derived from the id | 12:18 |
| dimitern | fwereade: well, for display purposes we're not using either anyway | 12:19 |
| jam | fwereade: lifecyclewatcher could be taught to return Tags in the api... | 12:19 |
| dimitern | fwereade: we use verbose yaml dump | 12:19 |
| fwereade | jam, yeah, I am wondering about that | 12:19 |
| fwereade | jam, but the same consideration applies to some degree at least | 12:19 |
| fwereade | jam, it should be convenient to convert them | 12:19 |
| jam | fwereade: as in, you should be able to convert them without a db query? Though aren't they used to query the db? | 12:20 |
| fwereade | jam, and with state/watcher we're stuck with _id-based watches at the lowest level | 12:20 |
| dimitern | jam: you don't need a db query - you already have all the services in the relation struct, so RelationTag can be simply implemented | 12:21 |
| fwereade | jam, 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, suboptimal | 12:21 |
| jam | fwereade: 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 |
| fwereade | jam, dimitern, relation.Tag is easy whatever we choose, Key and Id are both present | 12:21 |
| fwereade | jam, ah, hmm, nice theory, practical concern | 12:23 |
| fwereade | jam, I don't think we can depend on the db docs still existing when we want to convert | 12:23 |
| dimitern | fwereade: why didn't we have relation tags before? there wasn't any need for them through the api? | 12:23 |
| fwereade | jam, so we my be stuck with Key-based ones | 12:23 |
| fwereade | dimitern, yeah, can't auth as *or annotate* a relation | 12:24 |
| fwereade | jam, dimitern: so I think we're stuck with Key | 12:24 |
| dimitern | fwereade: why would we ever need to auth as a relation? | 12:25 |
| fwereade | dimitern, quite | 12:25 |
| fwereade | dimitern, that's why it's not an auther | 12:25 |
| dimitern | not even an otter :D | 12:25 |
| fwereade | dimitern, but we never annotate them either, which is how this never got considered | 12:25 |
| fwereade | ;p | 12:25 |
| dimitern | fwereade: hmm.. that might be useful i guess for the gui | 12:26 |
| fwereade | dimitern, jam: what's the worst that can happen if we put a " " in a tag, I wonder | 12:26 |
| fwereade | dimitern, maybe, but they didn't do it because they didn't need it afaik | 12:26 |
| jam | fwereade: the worst? Someone uses it in a shell script and it splits on the extra space. | 12:27 |
| dimitern | fwereade: spaces are scary.. c'mon it's not 70s anymore | 12:27 |
| jam | And that causes rm -rf to run on their personal machine | 12:27 |
| jam | and that causes them to go on a killing spree triggering a nuclear war | 12:27 |
| dimitern | any script should assume spaces might be present and use "" | 12:27 |
| jam | fwereade: The only legitimate concern I would have is the charm using shell case | 12:28 |
| jam | I don't know that relation-tag would even be exposed there. | 12:28 |
| fwereade | dimitern, jam: *that* won't happen because the charm doesn't see tags | 12:28 |
| fwereade | dimitern, jam: but the spectres yu raise still bug me | 12:28 |
| dimitern | fwereade: can't we simply s/ /_/ or something? | 12:29 |
| fwereade | dimitern, jam, hmm, maybe | 12:29 |
| dimitern | both ways | 12:29 |
| fwereade | dimitern, jam, I have a horrible feeling we don't restrict relation names much :/ | 12:29 |
| jam | fwereade: it wouldnt' be guaranteed unique if you munge it. | 12:29 |
| dimitern | intelligently ofc - check for existence of the result | 12:29 |
| jam | relationship_foo and relationship foo | 12:30 |
| dimitern | fwereade: +1 to that | 12:30 |
| jam | fwereade: the transports we currently use for talking about stuff are all space-safe. | 12:30 |
| jam | "JSON" string, etc. | 12:30 |
| jam | shell is the only place I can think of that could be a problem. | 12:30 |
| dimitern | fwereade: we should at least restrict spaces and a few other chars we could use as separators | 12:30 |
| fwereade | jam, dimitern: yeah, I'll try to check that out | 12:31 |
| fwereade | jam, dimitern: it's grown beyond what I should do in this CL anyway | 12:31 |
| dimitern | fwereade: proposal: restrict " ", "@" and/or "#" in relation names? | 12:31 |
| fwereade | jam, dimitern: will surprise us when we hit the uniter facade | 12:31 |
| fwereade | dimitern, I'd much rather restrict it harder, to basically what we have in service names | 12:32 |
| dimitern | even "/" as we use it already as a separator | 12:32 |
| dimitern | fwereade: ah, good point | 12:32 |
| fwereade | dimitern, weird to have those in rel tags when we sub them out in unit tags | 12:32 |
| dimitern | give the people too much freedom in name choices and all hell breaks loose ;) | 12:32 |
| fwereade | dimitern, jam: future plans: restrict charm relation names, implement relation tags with s/ /$some_forbidden_char/ | 12:33 |
| fwereade | dimitern, today plans: ignore relation tags | 12:33 |
| dimitern | fwereade: sgtm | 12:33 |
| jam | fwereade, 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 |
| jam | it is designed that if delayedtomb->Dying is set, it sleeps for a time and then calls actualtomb->Kill() | 13:15 |
| jam | however, the delayed tomb is only a local variable | 13:15 |
| === teknico1 is now known as teknico | ||
| dimitern | jam: where? | 13:33 |
| jam | cmd/jujud/upgrade.go | 13:33 |
| jam | look for tomb := delayedTomb() | 13:33 |
| dimitern | ok | 13:33 |
| jam | I don't see anything that could try to stop tomb rather than lots of stuff that would stop Upgrader.tomb | 13:33 |
| jam | anyway, 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 |
| dimitern | jam: it looks fishy | 13:36 |
| jam | dimitern: if it set self.tomb or something like that, then I could see it being used | 13:37 |
| dimitern | jam: and it's used in several places as noDelay(), but i'm yet to figure out why | 13:37 |
| jam | dimitern: it might be that I mixed the order around. | 13:37 |
| dimitern | jam: hmm | 13:37 |
| jam | So if someone calls Kill on Upgrader.Kill() it takes a while to actually call tomb.Kill | 13:37 |
| jam | so the download loop takes 5 min to respond to a Stop request | 13:38 |
| dimitern | jam: i'm not sure but in live tests i observed a lot less than that | 13:39 |
| jam | ah, 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 |
| jam | dimitern: upgraderDelay is a var that is probably overridden in tests | 13:39 |
| dimitern | jam: seems likely | 13:39 |
| dimitern | jam: anyway, have a good evening then | 13:39 |
| fwereade | jam, oh, christ, that stuff | 13:47 |
| fwereade | jam, I don't think it's necessary with a sanely configured worker.Runner | 13:48 |
| fwereade | dimitern, jam: you know how any error in any task takes down every task? | 13:48 |
| fwereade | dimitern, jam: the kill delay was a "solution" to that problem | 13:48 |
| dimitern | fwereade: i thought we were changing this with the task runners though? | 13:49 |
| dimitern | fwereade: aahh | 13:49 |
| fwereade | dimitern, jam: so that an upgrade that caused some task to error didn't prevent us from being able to upgrade away from the broken code | 13:49 |
| fwereade | dimitern, jam: now we can implement tasks independently I think it's entirely redundant | 13:49 |
| fwereade | dimitern, jam: that's not to say there aren't failure cases | 13:50 |
| fwereade | dimitern, jam: but there always were anyway | 13:50 |
| dimitern | fwereade: cool, so we can get rid of it once we use the runner | 13:50 |
| fwereade | dimitern, +1 | 13:50 |
| fwereade | jam, 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 this | 13:51 |
| fwereade | or mgz for that matter ^^ | 13:52 |
| fwereade | dimitern, btw, https://codereview.appspot.com/11041043 is what I was talking about earlier, that I hope implements half the Deployer's methods | 13:53 |
| dimitern | fwereade: cool, will take a look in a bit | 13:54 |
| * dimitern needs to step out for 20m | 13:54 | |
| === teknico is now known as teknico-phone | ||
| dimitern | fwereade: reviewed | 14:52 |
| fwereade | dimitern, cheers | 14:52 |
| ackk | hi, 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 repo | 15:03 |
| dimitern | ackk: can I see the config.yaml of the charm in question? | 15:04 |
| ackk | dimitern, it's a checkout of lp:~landscape-charmers/charms/precise/landscape-client/trunk , no changes | 15:06 |
| ackk | dimitern, I also tried with a very basic charm, that only logs hooks | 15:07 |
| ackk | dimitern, fwiw, "juju deploy cs:~landscape-charmers/precise/landscape-client" works | 15:07 |
| dimitern | ackk: and you're using juju-core tip? | 15:08 |
| ackk | dimitern, yes | 15:08 |
| dimitern | ackk: and with the basic charm the same happens? | 15:08 |
| ackk | dimitern, as of 10 minutes ago, at least :) | 15:08 |
| ackk | dimitern, yes | 15:08 |
| dimitern | ackk: is ./charms a directory where you're running it? | 15:09 |
| ackk | dimitern, yes, I also tried absolute paths and "." from the directory itself | 15:10 |
| dimitern | ackk: can you paste the output of "du -h --all" inside the charms dir?\ | 15:12 |
| ackk | dimitern, http://paste.ubuntu.com/5858663/ | 15:13 |
| dimitern | ackk: and you don't have JUJU_REPOSITORY env var set? | 15:14 |
| ackk | dimitern, nope | 15:15 |
| dimitern | ackk: just a sec | 15:15 |
| ackk | dimitern, it seems something strange with that dir, I just tried checking out in a different repo dir and it worked | 15:16 |
| dimitern | ackk: ah :) | 15:16 |
| ackk | dimitern, maybe something related to .bzr ? | 15:16 |
| dimitern | ackk: shouldn't matter but might be | 15:16 |
| dimitern | ackk: can you please test it with .bzr inside and if you repoduce it, please file a bug | 15:17 |
| === tasdomas is now known as tasdomas_afk | ||
| ackk | dimitern, 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 |
| ackk | dimitern, thanks for the help | 15:21 |
| dimitern | ackk: 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 |
| ackk | dimitern, sure, I'll try | 15:22 |
| dimitern | ackk: tyvm | 15:22 |
| fwereade | ackk, would you paste the config.yaml please? | 15:56 |
| fwereade | ackk, it's definitely the same in both cases? | 15:56 |
| ackk | fwereade, for landscape-client local and cs: charms? yes, it is. It has something to do with that dir specifically | 15:57 |
| ackk | fwereade, dimitern oh wait I got it, the other charm (not the one I was deploying) had an empty config.yaml, removing it made it work | 16:02 |
| fwereade | ackk, that is technically an invalid config then... but it should be an error, not a panic, I think | 16: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 |
| dimitern | fwereade, 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.go | 16:08 |
| fwereade | dimitern, I think it's an uninitialized *Config, but yeah | 16:09 |
| dimitern | fwereade: yeah in fact | 16:09 |
| fwereade | dimitern, annoying that an empty file unmarshals into a nil pointer without error | 16:09 |
| dimitern | fwereade: I still think a bug report is in order | 16:09 |
| ackk | dimitern, 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 relevant | 16:09 |
| fwereade | dimitern, I'm not sure it's actually a bug, I suspect it's annoying-but-coherent behaviour | 16:10 |
| dimitern | ackk: well, at some point when we read the repo I *think* we scan all charms and this might be the issue | 16:10 |
| ackk | dimitern, I see | 16:10 |
| dimitern | fwereade: a panic like this surely deserves a bug and some handling? | 16:11 |
| fwereade | dimitern, definitely | 16:11 |
| fwereade | dimitern, (blast, that was a pastebin link not a bug link) | 16:11 |
| fwereade | dimitern, ackk, I'll do one, thanks | 16:11 |
| ackk | fwereade, dimitern thank you | 16:12 |
| dimitern | fwereade: we might want to audit all places we Unmarshal stuff like that for similar issues | 16:12 |
| dimitern | ackk: thanks for catching this | 16:12 |
| fwereade | dimitern, +1,x2 | 16:14 |
| dimitern | fwereade: CleanupWatcher seems a candidate for unexporting and migrating to NotifyWatcher | 16:23 |
| fwereade | jam, have a word with jam about it, I think he mentioned that too | 16:24 |
| dimitern | fwereade: ok | 16:30 |
| TheMue | gnah! | 16:31 |
| TheMue | remember - tests don't work if you create temporary directories but don't use them | 16:32 |
| dimitern | fwereade, TheMue: a simple review? https://codereview.appspot.com/11044043/ | 16:34 |
| TheMue | dimitern: *click* | 16:35 |
| * dimitern bbiab | 16:35 | |
| fwereade | dimitern, LGTM, one bit to fix | 16:36 |
| === tasdomas_afk is now known as tasdomas | ||
| === tasdomas is now known as tasdomas_afk | ||
| TheMue | ah, it passed, yeah | 16:47 |
| TheMue | dimitern: oh, btw, yes, from my side an lgtm too. | 16:48 |
| dimitern | fwereade, TheMue: thanks! | 16:54 |
| dimitern | abentley: hey | 16:55 |
| abentley | dimitern: Hey. | 16:56 |
| dimitern | abentley: I wanted to ask you about the rietveld-enabled versions of lp-propose and lp-submit - where can I find them? | 16:56 |
| abentley | dimitern: 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 |
| abentley | dimitern: It submits only. It does not propose. | 16:59 |
| dimitern | abentley: 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 |
| abentley | dimitern: 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 |
| dimitern | abentley: 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 |
| abentley | dimitern: It's a plugin. | 17:05 |
| dimitern | fwereade: I don't really get why srvLifecycleWatcher should be renamed to srvStringsWatcher? | 17:07 |
| dimitern | abentley: i'll try it out, cheers | 17:08 |
| abentley | dimitern: Do you have Tarmac running now, then? | 17:08 |
| fwereade | dimitern, everywhere up from state we should be dropping the distinctions, surely? | 17:08 |
| dimitern | fwereade: do you also mean changing Lifecycle* types to Strings* types - in params, etc.? | 17:09 |
| dimitern | abentley: yep, for some weeks now - and working even :) | 17:09 |
| fwereade | dimitern, 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 be | 17:09 |
| dimitern | fwereade: quick g+ ? | 17:10 |
| fwereade | dimitern, sure | 17:10 |
| dimitern | fwereade: https://plus.google.com/hangouts/_/5faf4d98498233fe5228a813ee7bda05c5c2a20c?authuser=0&hl=en | 17:11 |
| jam | dimitern: you mean https://codereview.appspot.com/10876048/ ? (CleanerWorker ?) | 17:12 |
| dimitern | jam: not really - i meant just replace CleanupWatcher with NotifyWatcher in state | 17:13 |
| jam | abentley, 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 |
| jam | dimitern: ah sorry, was thinking the wrong side. | 17:13 |
| jam | I have Workers on the brain | 17:13 |
| dimitern | jam: that last part I didn't get - what's commit --fixes? | 17:13 |
| jam | 'bzr commit --fixes lp:1234556' | 17:14 |
| jam | will set bzr metadata that Launchpad will notice and associate your branch to a bug | 17:14 |
| dimitern | jam: oww - how awesome is that :) | 17:14 |
| dimitern | jam: btw, if you haven't landed that CL yet, consider adding that change to state? | 17:14 |
| dimitern | fwereade: ping? | 17:14 |
| jam | I landed the CleanerWorker already | 17:14 |
| abentley | jam: There's also lp-link-bug, but that's in the lpreview_body plugin. I should probably fix that. | 17:15 |
| dimitern | jam: ah, but you have i see | 17:15 |
| jam | dimitern: I think you could just change line 1229, with "func (st *State) WatchCleanups() NotifyWatcher" because *CleanupWatcher is already a NotifyWatcher in spirit | 17:16 |
| dimitern | jam: yep something like that | 17:21 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!