[07:08] <TheMue> morning
[07:35] <rogpeppe> TheMue: morning
[07:36] <TheMue> rogpeppe: hiya
[08:35] <TheMue> fwereade: heya
[08:35] <fwereade> TheMue, morning
[08:35] <fwereade> TheMue, how's it going?
[08:36] <TheMue> fwereade: fine, just waiting for Aram to discuss a mstate idea with him
[08:36] <fwereade> TheMue, cool
[08:37] <TheMue> fwereade: and how are you?
[08:37] <fwereade> TheMue, so-so :)
[08:37] <TheMue> fwereade: doesn't sound so really cool
[08:37] <fwereade> TheMue, still trying to figure out how niemeyer's upgrade plans will *actually* fit into the uniter
[08:37] <fwereade> TheMue, so finishing it feels far away agin
[08:38] <TheMue> fwereade: will be like driving on cobblestone for a while but it will fit
[08:38] <fwereade> TheMue, yeah, we'll get there :)
[08:38] <fwereade> TheMue, cheers :)
[08:39] <TheMue> fwereade: the hardest parts will be the semantic changes with lifecycle and caching/refreshing, where state using code has to be changed
[08:40] <fwereade> TheMue, I'm hoping actually the lifecycle will turn out ok, I sketched that out the other day and I *think* its impact is pretty localised from my pov
[08:40] <fwereade> TheMue, that's not to say it's easy on you ofc ;)
[08:41] <TheMue> fwereade: so far it's not too hard, only effort. but every (m)state user will have to take care that the api is changing
[09:06] <Aram> yo.
[09:07] <davecheney> sup!
[09:09] <TheMue> Aram: hi
[09:09] <TheMue> davecheney: oh, hi too ;
[09:10] <TheMue> Aram: need your help
[09:10] <Aram> I'll try my best.
[09:11] <TheMue> Aram: when building mstate I get funnily a "type *mgo.Collection has no field or method UpdateId" even with mgo v2 (with UpdateId) installed
[09:12] <TheMue> Aram: did you had the same
[09:12] <TheMue> ?
[09:12] <Aram> no.
[09:12] <davecheney> TheMue: go get -u .../mgo
[09:12] <Aram> perhaps you should reupdate mgo?
[09:12] <davecheney> err, go get -u ...mgo
[09:12] <TheMue> Aram, davecheney: will try
[09:14] <TheMue> ooops, getting a brz error now during go get
[09:14] <TheMue> oh, only temporary, now it updated
[09:15] <TheMue> and fixed, thx Aram and davecheney
[09:15] <Aram> great
[09:16] <TheMue> Aram: take a look at http://pastebin.ubuntu.com/1160449/ and http://pastebin.ubuntu.com/1160447/. a way to reuse ensureLife()
[09:16] <Aram> yeah, the only problem is that machines have integer ids
[09:16] <Aram> I wish they had string ids
[09:17] <Aram> relations also have integer ids...
[09:17] <Aram> I wish everything had string ids
[09:19] <Aram> we could use interface{} for as the id function parrameter for ensureLife in the meantime
[09:19] <Aram> mgo uses reflection and it should work.
[09:19] <TheMue> Aram: it's worth a try, yes
[09:20] <TheMue> Aram: i'll give it a try here for units
[09:33] <TheMue> Aram: in State.RemoveRelation() you check if the relation is Dead but you don't refresh before. so your life may be Dying (and so the function returns an error) while a concurrent operation already may have marked it Dead.
[09:33] <TheMue> Aram: this is no error
[09:33] <TheMue> Aram: i only think about if a refresh would make sense here
[09:34] <TheMue> Aram: at least your Dead-check in the bson selector is the most important part
[09:36] <Aram> no, it's fine the way it is.
[09:36] <Aram> you should not remove a relation before you are sure it is dead (e.g. you called r.Die() or r.Refresh())
[09:37] <rogpeppe> Aram: yeah, machine ids being integers is problematic in other places too
[09:37] <Aram> that forces the client to always call r.Die() if he wants to remove a machine, and that's fine.
[09:37] <Aram> it can be called any number of times.
[09:40] <TheMue> Aram: to me it's a bit harsh to panic on a maybe already dead entity
[09:40] <Aram> if you want to remove a relation, call r.Die() and you'll never panic :).
[09:41] <TheMue> Aram: so it's only to discipline the state user
[09:41] <Aram> yes.
[09:42] <TheMue> Aram: *lol*
[09:43] <Aram> perhaps we could add a DieAndRemove() function if it's such a common operation.
[09:43] <Aram> no
[09:43] <Aram> we should not
[09:43] <Aram> yes, I remember now
[09:44] <Aram> DieAndRemove() is harmful, usually the watcher that receives the dead event calls remove.
[09:44] <Aram> and that guy gets a machine with a dead cache and it's fine to call remove.
[09:44] <Aram> DieAndRemove will mask the dead event, and that may or may not be a bug.
[09:45] <Aram> if you really want to remove it independent of any watchers, call r.Die() and r.Remove().
[09:45] <Aram> that makes it very explicit.
[09:45] <TheMue> Aram: ic, sounds reasonable
[10:03] <Aram> TheMue: many tests use regular nodes (e.g. machine nodes) as confignodes to inject corrupt data or to inject data without calling the real setters. that's... clever. do we want to preserve this behavior?
[10:04] <Aram> because I had a plan to make tests in mstate_test pure, not to do any trickery outside the public API.
[10:04] <Aram> and move the tests that can't be pure into internal tests.
[10:04] <Aram> but many tests use this trick.
[10:05] <Aram> plus I'd need to modifcy confignodes, as now they are in a different collection, so a machine node can never be a config  node.
[10:05] <TheMue> Aram: we have to check them individually. maybe the concrete reason is only in using ZK
[10:06] <TheMue> Aram: i like the fact that they#ve got an own collection
[10:23] <Aram> TheMue: yeah, I did a check and the config node trick is only used in 4 tests.
[10:23] <TheMue> Aram: great, i think we'll find a way to get around
[10:32] <TheMue> Aram: btw, i like the speed of the mstate tests. there still several missing, but i hope it will stay so fast.
[10:32] <Aram> I hope that as well.
[10:32] <Aram> watchers will slow it somewhat, but I hope it will still e fast
[10:38] <TheMue> Aram: i just proposed https://codereview.appspot.com/6476044/
[10:39] <TheMue> Aram: we could move all lifecycle stuff in an own file and give it an own test suite
[10:41] <Aram> will take a look after lunch
[10:43] <TheMue> Aram: ok
[11:30] <Aram> TheMue: you have a review.
[11:30] <TheMue> Aram: thx
[11:31] <TheMue> Aram: oh, removed the service check, sh… ;)
[11:32] <TheMue> Aram: the rest of RemvoveUnit() is like relation and i wanted to keey it common
[11:33] <Aram> that should be changed too.
[11:34] <TheMue> Aram: then it's ok that i change it back, will do so
[11:34] <Aram> thanks
[11:38] <TheMue> Aram: what problem do you have with "if err := foo(); err != nil { … }"?
[11:38] <TheMue> Aram: IMHO is a common idiom
[11:39] <Aram> I find it hard to read, personally, I know it's common.
[11:39] <Aram> v, ok := m[something]
[11:40] <Aram> that's fine to me because it makes ok have a very local scope
[11:40] <Aram> which is great
[11:40] <Aram> but for errors I don't like it that much
[11:42] <TheMue> Aram: ok, even if i like it i will change it here ;)
[11:43] <Aram> if you feel strongly about it, keep it, it boils down to personal preference.
[11:47] <TheMue> Aram: already changed :P
[11:48] <TheMue> Aram: you've seen, i added one simple test of the state tests
[11:48] <TheMue> Aram: there it's the final one before all watcher tests
[11:48] <Aram> yes, seen it
[11:49] <Aram> I'm surprised that it works though
[11:50] <TheMue> Aram: maybe RemoveService() currently doesn't care for lifecycles?
[11:51] <Aram> ah, it marks associated units as dying
[11:52] <TheMue> Aram: *lol* ok, works by accident
[11:54] <TheMue> Aram: do you have a table of which state tests are already ported to mstate?
[11:54] <Aram> no.
[11:54] <Aram> I just picked and chosen as I saw fit
[11:54] <Aram> when I implemented a feature I searched for tests that use that feature
[11:54] <Aram> and ported those
[11:57] <Aram> TheMue: another review
[11:58] <TheMue> thx
[12:10] <TheMue> Aram: ah, thx for the explanation of the "hard death" :)
[12:11] <TheMue> Aram: i would like to move the table driven test to a lifecycle suite. because if all use the same ensureLife() there's no need for testing it multiple times
[12:13] <Aram> yes, I think it's good if we move the Life type and ensureLife to life.go, and stateChanges and the other tests to life_test.go, in a different suite.
[12:17] <TheMue> Aram: shall I do it?
[12:17] <Aram> TheMue: please do, and also convert relations to the new function.
[12:17] <TheMue> Aram: ok
[12:17] <niemeyer> Yo!
[12:17] <Aram> hi
[12:18] <TheMue> niemeyer: heya
[12:18] <niemeyer> Aram: Shall be ready for us to poke on it
[12:18] <Aram> niemeyer: what, txn?
[12:18] <niemeyer> Aram: Watches would need a small extension to have mtime on the transaction, but that's trivial.. we can convert meanwhile
[12:19] <niemeyer> Aram: Yeah
[12:19] <Aram> great.
[12:19] <Aram> show me what you got, anytime
[12:20]  * TheMue listens too
[12:21] <niemeyer> Aram: Cool, I'll just move to the main repo, as I've been mistakingly pushing it in a branch in a subdir
[12:24] <niemeyer> Aram: done
[12:24] <niemeyer> Aram: It's not tagged, so you'll need to bzr pull it
[12:25] <Aram> I can't use go.pkgdoc with it? :(
[12:26]  * Aram runs godoc
[12:26] <niemeyer> Aram: Not yet..
[12:26] <niemeyer> Aram: I'll announce it when I have a moment
[12:26] <niemeyer> Aram: and then tag it properly
[12:27] <Aram> no issue, godoc is great
[12:27]  * Aram is reading now
[12:28] <niemeyer> Aram: I'll also document it better, and write a blog post describing details
[12:28] <niemeyer> Aram: But for the moment it feels wise to just get out of your way :)
[12:34] <Aram> ok, I read the docs.
[12:34] <Aram> now I shall read the tests to get a feel for it.
[12:37] <niemeyer> Aram: sim_test.go is where the meat is
[12:38] <niemeyer> Aram: txn_test.go can see improvements
[12:38] <Aram> yes, reading it already
[12:38] <niemeyer> Aram: The API is almost boring, thankfully
[12:45] <Aram> great, thanks.
[12:45] <Aram> I'm going to play with it.
[12:49] <rogpeppe> niemeyer: ping
[12:51] <niemeyer> rogpeppe: Yo
[12:52] <rogpeppe> niemeyer: i was wondering how much the upgrade logic needs to be aware of the dev/release (odd/even) version numbers
[12:53] <rogpeppe> niemeyer: i'm thinking it perhaps doesn't, but i'm probably wrong there
[12:53] <niemeyer> Aram: I think we should start by porting the simple stuff to it
[12:53] <niemeyer> Aram: Making sure that the changes ("$set", etc) are happening in transactions
[12:53] <niemeyer> rogpeppe: IIRC, in our last conversation (in Lisbon?) we agreed it doesn't
[12:54] <Aram> yes, I'll do it after I'm done with the things I'm working on ATM.
[12:54] <rogpeppe> niemeyer: cool, i'd forgotten about that conversation.
[12:54] <niemeyer> rogpeppe: We agreed, IIRC, that all we needed was in the upgrade-juju command itself
[12:54] <niemeyer> rogpeppe: If the decision to use it has been made, then why should the agent care
[12:54] <niemeyer> rogpeppe: IIRC
[12:55] <rogpeppe> niemeyer: ah, sorry, i was talking about the logic in the upgrade-juju command itself
[12:56] <rogpeppe> niemeyer: perhaps all it needs to do is make sure it never upgrades from a release version to a dev version.
[12:57] <rogpeppe> (and vice versa?)
[12:58] <niemeyer> rogpeppe: Unless --dev is used
[12:58] <niemeyer> rogpeppe: Vice versa sounds fine
[12:58] <rogpeppe> niemeyer: i think i'll add a dev flag to environs.BestTools
[12:59] <niemeyer> rogpeppe: Sounds sane
[13:03] <rogpeppe> niemeyer: cool
[13:42] <rogpeppe> niemeyer: i'd appreciate a review of this when you have a moment BTW, as i'm stacking stuff on top of it. https://codereview.appspot.com/6485044/
[13:42] <rogpeppe> niemeyer: (it's fairly trivial stuff)
[13:44] <niemeyer> rogpeppe: Looking
[13:51] <niemeyer> rogpeppe: LGTM, with a small side-tracking peeve about the hackish storage test type
[13:52] <rogpeppe> niemeyer: yeah, i thought you might not be keen on that. i wasn't entirely sure of the best way to do it. perhaps pushing into the dummy env might be better.
[13:52] <niemeyer> rogpeppe: Why? It's just a type with methods
[13:52] <niemeyer> rogpeppe: What's wrong with just having that?
[13:53] <rogpeppe> niemeyer: quite a few methods that need to be changed every time the Environ type changes.
[13:53] <rogpeppe> niemeyer: hence i thought best to inherit them all so that piece of code is only sensitive to the methods it cares about.
[13:54] <rogpeppe> niemeyer: i agree it feels hackish tho'
[13:57] <rogpeppe> niemeyer: hmm, actually i can avoid the *most* hackish bits more easily
[13:59] <niemeyer> rogpeppe: Having that in the dummy environment sounds sane to
[13:59] <niemeyer> too
[13:59] <niemeyer> rogpeppe: A dummy.Storage we can make use of
[13:59] <niemeyer> rogpeppe: So we can do something less awkward once
[14:00] <rogpeppe> niemeyer: the dummy Storage already works actually
[14:00] <niemeyer> rogpeppe: Well, so do we have a problem? :)
[14:06] <rogpeppe> niemeyer: probably not. the FindTools tests do it already. i was doing too much of a naive translation of the tests, i think.
[14:21] <TheMue> Yeah!
[14:22] <TheMue> Aram: found a nice way to do the table driven lifecycle test for all entities in life_test.go
[14:22] <Aram> how?
[14:24] <TheMue> Aram: let me just add unit (relation is already integrated, and i'll show you).
[14:24] <Aram> please
[14:24] <TheMue> oops, bracket set too late. ;)
[14:25] <TheMue> Aram: it's a kind of "for each living do the same as aram has done for relation before".
[14:30] <TheMue> Aram: Great! Easier than thought. Will propose it now.
[14:32] <niemeyer> TheMue: Hm
[14:33] <niemeyer> TheMue: I'm not sure if I see what you mean, but FWIW, what Aram has done in relations shouldn't be *copied*
[14:33] <niemeyer> TheMue: The idea is to refactor it so that we have a single implementation that is used for all types
[14:33] <TheMue> niemeyer: yes, we are working on it. first part has been the only function ensureLife()
[14:35] <TheMue> niemeyer: but the goal shall be an embeddable type
[14:35] <niemeyer> TheMue: Superb, I imagined you'd already going in that direction, but decided to mention as it's cheaper than fixing later :-)
[14:36] <Aram> pro tip: if you are using chrome and you have pinned tabs with e-mail etc, never ever open more then one window. you'll close them in wrong order and it will forget your precious saved tabs.
[14:36] <TheMue> niemeyer: indeed :D
[14:40] <TheMue> Aram: take a look at https://codereview.appspot.com/6481045/
[14:44] <TheMue> niemeyer: btw, /me likes state on mongo. pretty clean (and fast), simply elegant
[14:44] <niemeyer> TheMue: That's great to hear!
[14:48] <Aram> TheMue: you have a review
[14:50] <TheMue> Aram: thx *click*
[14:52] <TheMue> Aram: yes, your idea sounds reasonable. will move it.
[14:52] <Aram> great
[14:52] <TheMue> Aram: you've seen the idea? when e.g. service is migrated we'll just add it to the slice of envs. ;)
[14:53] <TheMue> Aram: thanks to your idea with relation it's quite simple
[14:54] <Aram> yes, it's great
[14:57] <TheMue> 4 secs for the whole beast, pretty fast. sadly it will slow down when all watcher tests are added.
[15:05] <niemeyer> TheMue: Let's keep an eye on those
[15:05] <niemeyer> TheMue: It should be possible to keep the impact pretty low
[15:07] <TheMue> niemeyer: right time, i just wanted to ask if there's already a first proposal or impl for watchers in mstate
[15:07] <niemeyer> TheMue: No, as we were waiting to see if the txn idea would flop or not
[15:08] <niemeyer> TheMue: It seems to work, so we can now start working on it
[15:08] <niemeyer> TheMue: (in that direction, rather than the old ones)
[15:08] <TheMue> niemeyer: hey, c'mon, it's by you, so it can't flop *lol*
[15:08] <niemeyer> TheMue: Oh man, I wish that was true
[15:10] <TheMue> niemeyer: i know have to leave the keyboard for a short time. can you msg me the uri to pull it?
[15:12] <Aram> I just did a bzr pull inside the mgo dir and it was there afterwards
[15:14] <niemeyer> TheMue: ^
[15:23] <Aram> niemeyer: multiple txn runners can use the same collection, right?
[15:25] <niemeyer> Aram: Yeah, they must if they are sharing affected documents
[15:25] <Aram> ok.
[15:25] <niemeyer> Aram: We only need a single tc
[15:25] <Aram> do we need more than one runner?
[15:26] <Aram> niemeyer: ^
[15:26] <niemeyer> Aram: No, everybody can share the same one
[15:27] <Aram> great.
[15:27] <Aram> thanks
[15:27] <niemeyer> Aram: np
[16:03] <fwereade_> taking a break, back later
[16:07] <Aram> niemeyer: how can I do an UpdateAll with your txn?
[16:08] <Aram> is it possible?
[16:08] <niemeyer> Aram: It's not
[16:08] <Aram> :(
[16:08] <niemeyer> Aram: Yeah, this is a change to consider
[16:08] <niemeyer> Aram: But we can live without it for now I suppose
[16:09] <Aram> well, we do use UpdateAll
[16:09] <Aram> to update all units of a service
[16:09] <niemeyer> Aram: Sure.. where are we using it ATM?
[16:09] <Aram> in RemoveService
[16:10] <niemeyer> Aram: To begin with, we can put them all in a single transaction and run it
[16:10] <niemeyer> Aram: Won't scale to extremes, but we can address that down the road
[16:10] <Aram> but we don't know the ids
[16:11] <niemeyer> Aram: Hmm.. interesting.. let me think for a moment how we can solve this cheaply
[16:11] <niemeyer> Aram: What's the change document?
[16:12] <Aram> change = bson.D{{"$set", bson.D{{"life", Dying}}}}
[16:12] <Aram> more precisely
[16:12] <Aram> 	sel = bson.D{{"service", svc.doc.Name}}
[16:12] <Aram> 	change = bson.D{{"$set", bson.D{{"life", Dying}}}}
[16:12] <Aram> 	_, err = s.units.UpdateAll(sel, change)
[16:19] <niemeyer> Aram: Hmm
[16:21] <niemeyer> Aram: I think we can make the units themselves responsible for setting their dying state
[16:21] <niemeyer> Aram: And we can ref-count live units on the service itself
[16:22] <niemeyer> Aram: So we know when the service can transition to dead
[16:22] <niemeyer> Aram: The units then have to monitor the service to see when they're supposed to shut down
[16:22] <Aram> so each unit watches for its service, and sets itself to dying when the service is set to dying?
[16:23] <niemeyer> Aram: Yeah
[16:23] <Aram> that's potentially a lot of watches :).
[16:23] <Aram> ok.
[16:23] <niemeyer> Aram: In the same transaction that dec's the alive refcounter
[16:23] <Aram> I've put a // BUG in the code so I remember to do this after I implement watcher
[16:23] <Aram> (obviously I can't do it before).
[16:23] <niemeyer> Aram: Well, not really, sorry
[16:23] <niemeyer> Aram: Erm.. I'm correcting myself, sorry
[16:24] <niemeyer> Aram: The ref count should only be dec'd when the unit goes Dead
[16:24] <niemeyer> Aram: Regarding being a lot of watchers, not really
[16:24] <niemeyer> Aram: We already have to monitor the service from the unit for other reasons
[16:25] <niemeyer> Aram: and again, the watching logic should be implemented *client side*
[16:25] <niemeyer> Aram: There shouldn't be a significant delta in monitoring the service or not
[16:29] <niemeyer> Aram: Which code?
[16:29] <Aram> niemeyer: ?
[16:29] <niemeyer> Aram: What's the // BUG?
[16:30] <Aram> in RemoveService
[16:30] <Aram> 	// BUG(aram): units should monitor their service and set themselves
[16:30] <Aram> 	// to dead. waiting for watchers.
[16:30] <niemeyer> Aram: Why RemoveService cares?
[16:31] <Aram> niemeyer: RemoveService doesn't care, I care so i remember to do it after I implement watchers.
[16:32] <niemeyer> Aram: Okay, but we can get rid of UpdateAll already, right?
[16:32] <Aram> no?
[16:32] <niemeyer> Aram: Okay, so that's what I'm trying to talk about :)
[16:33] <niemeyer> Aram: What has to change for us to fix RemoveService in both state and mstate now?
[16:33] <niemeyer> Aram: Well, or maybe just mstate
[16:34] <Aram> first we need to implement lifecycle for services.
[16:37] <Aram> niemeyer: if I have an Assert:     txn.DocMissing, the Run() will fail if I call it twice, right?
[16:37] <Aram> (with the same ops)
[16:38] <Aram> (and if I insert something in that ops).
[16:39] <niemeyer> Aram: That's right
[16:39] <niemeyer> Aram: It'll return ErrAborted when the assertion is false
[16:39] <niemeyer> Aram: (without touching anything)
[16:39] <Aram> niemeyer: I think I found a bug in txn.
[16:39] <niemeyer> Aram: Woot
[16:39] <niemeyer> Aram: What's up?
[16:40] <Aram> basically you can add documents that have the same no-id field identical, and that field is Unique: true
[16:42] <Aram> so you can add {"_id": 0, "foo": x} and {"_id": 1, "foo": y} even if "foo" should be unique.
[16:42] <niemeyer> Aram: Well, I'm not sure I understand the problem.. you certainly can't add the document because MongoDB itself will prevent that from happening
[16:42] <Aram> maybe it doesn't end up in the database, but Run doesn't complain.
[16:42] <niemeyer> Aram: Please read the documentation of Operation in the Insert and Assert fields
[16:44] <Aram> I did, not sure your point.
[16:45] <Aram> my assert is txn.DocMissing whoch is not strong enough.
[16:45] <niemeyer> Aram: My point is that what you're complaining about is precisely described there
[16:46] <niemeyer> Aram: Of course.. DocMissing means the id isn't there.. it certainly won't dig in the database to see what are all the unique fields that the document may have and which ones are satisfied or not
[16:46] <niemeyer> Aram: The way to stop the transaction is via Assert
[16:47] <Aram> what should I write in the Assert in this scenario?
[16:47] <Aram> stuff in Assert can only refer to that on object with DocId
[16:47] <Aram> s/on/one/
[16:47] <niemeyer> Aram: Right
[16:48] <niemeyer> Aram: There's basically no way to prevent non-existence of something else that could potentially conflict with the document in any unique field
[16:48] <niemeyer> Aram: We may somewhat easily workaround it, though
[16:48] <Aram> how?
[16:49] <niemeyer> Aram: Can you describe the case in more detail?
[16:50] <Aram> niemeyer: this is called many times with a different id: http://paste.ubuntu.com/1161111/ it should fail if the key is unique, but it doesn't. concrete case is in relations, they also have an index on key, not only on _id.
[16:52] <niemeyer> Aram: By "case" I mean the real problem we have.. I understand the basic issue you're talking about, and we can easily workaround it
[16:52] <niemeyer> Aram: With relations, we could use key as _id
[16:53] <Aram> that's the real problem we have. we can't fail adding same relation twice.
[16:53] <niemeyer> Aram: Which sounds realistic
[16:53] <niemeyer> Aram: And have an auxiliar field for the numeric id
[16:53] <Aram> yes, we can use key as _id
[16:53] <Aram> that was the original design :)).
[16:53] <niemeyer> Aram: Sorry, my time machine had a bug on it and I couldn't use it
[16:54] <niemeyer> Aram: I'll step out for lunch.. happy to continue in a bit
[16:54] <Aram> enjoy
[16:54] <niemeyer> Cheers
[17:52] <niemeyer> For the record, I'll take the chance we're low on reviews right now and I'm taking the  the afternoon off to relax a bit after the intense day+night+no weekend of the last few weeks. I'll be around for a moment still to finish the blog post, so feel free to poke