/srv/irclogs.ubuntu.com/2012/08/22/#juju-dev.txt

=== negronjl` is now known as negronjl
TheMuemorning07:08
rogpeppeTheMue: morning07:35
TheMuerogpeppe: hiya07:36
=== mthaddon` is now known as mthaddon
TheMuefwereade: heya08:35
fwereadeTheMue, morning08:35
fwereadeTheMue, how's it going?08:35
TheMuefwereade: fine, just waiting for Aram to discuss a mstate idea with him08:36
fwereadeTheMue, cool08:36
TheMuefwereade: and how are you?08:37
fwereadeTheMue, so-so :)08:37
TheMuefwereade: doesn't sound so really cool08:37
fwereadeTheMue, still trying to figure out how niemeyer's upgrade plans will *actually* fit into the uniter08:37
fwereadeTheMue, so finishing it feels far away agin08:37
TheMuefwereade: will be like driving on cobblestone for a while but it will fit08:38
fwereadeTheMue, yeah, we'll get there :)08:38
fwereadeTheMue, cheers :)08:38
TheMuefwereade: the hardest parts will be the semantic changes with lifecycle and caching/refreshing, where state using code has to be changed08:39
fwereadeTheMue, 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 pov08:40
fwereadeTheMue, that's not to say it's easy on you ofc ;)08:40
TheMuefwereade: so far it's not too hard, only effort. but every (m)state user will have to take care that the api is changing08:41
Aramyo.09:06
davecheneysup!09:07
TheMueAram: hi09:09
TheMuedavecheney: oh, hi too ;09:09
TheMueAram: need your help09:10
AramI'll try my best.09:10
TheMueAram: when building mstate I get funnily a "type *mgo.Collection has no field or method UpdateId" even with mgo v2 (with UpdateId) installed09:11
TheMueAram: did you had the same09:12
TheMue?09:12
Aramno.09:12
davecheneyTheMue: go get -u .../mgo09:12
Aramperhaps you should reupdate mgo?09:12
davecheneyerr, go get -u ...mgo09:12
TheMueAram, davecheney: will try09:12
TheMueooops, getting a brz error now during go get09:14
TheMueoh, only temporary, now it updated09:14
TheMueand fixed, thx Aram and davecheney09:15
Aramgreat09:15
TheMueAram: take a look at http://pastebin.ubuntu.com/1160449/ and http://pastebin.ubuntu.com/1160447/. a way to reuse ensureLife()09:16
Aramyeah, the only problem is that machines have integer ids09:16
AramI wish they had string ids09:16
Aramrelations also have integer ids...09:17
AramI wish everything had string ids09:17
Aramwe could use interface{} for as the id function parrameter for ensureLife in the meantime09:19
Arammgo uses reflection and it should work.09:19
TheMueAram: it's worth a try, yes09:19
TheMueAram: i'll give it a try here for units09:20
TheMueAram: 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
TheMueAram: this is no error09:33
TheMueAram: i only think about if a refresh would make sense here09:33
TheMueAram: at least your Dead-check in the bson selector is the most important part09:34
Aramno, it's fine the way it is.09:36
Aramyou should not remove a relation before you are sure it is dead (e.g. you called r.Die() or r.Refresh())09:36
rogpeppeAram: yeah, machine ids being integers is problematic in other places too09:37
Aramthat forces the client to always call r.Die() if he wants to remove a machine, and that's fine.09:37
Aramit can be called any number of times.09:37
TheMueAram: to me it's a bit harsh to panic on a maybe already dead entity09:40
Aramif you want to remove a relation, call r.Die() and you'll never panic :).09:40
TheMueAram: so it's only to discipline the state user09:41
Aramyes.09:41
TheMueAram: *lol*09:42
Aramperhaps we could add a DieAndRemove() function if it's such a common operation.09:43
Aramno09:43
Aramwe should not09:43
Aramyes, I remember now09:43
AramDieAndRemove() is harmful, usually the watcher that receives the dead event calls remove.09:44
Aramand that guy gets a machine with a dead cache and it's fine to call remove.09:44
AramDieAndRemove will mask the dead event, and that may or may not be a bug.09:44
Aramif you really want to remove it independent of any watchers, call r.Die() and r.Remove().09:45
Aramthat makes it very explicit.09:45
TheMueAram: ic, sounds reasonable09:45
AramTheMue: 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:03
Arambecause I had a plan to make tests in mstate_test pure, not to do any trickery outside the public API.10:04
Aramand move the tests that can't be pure into internal tests.10:04
Arambut many tests use this trick.10:04
Aramplus 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
TheMueAram: we have to check them individually. maybe the concrete reason is only in using ZK10:05
TheMueAram: i like the fact that they#ve got an own collection10:06
AramTheMue: yeah, I did a check and the config node trick is only used in 4 tests.10:23
TheMueAram: great, i think we'll find a way to get around10:23
TheMueAram: btw, i like the speed of the mstate tests. there still several missing, but i hope it will stay so fast.10:32
AramI hope that as well.10:32
Aramwatchers will slow it somewhat, but I hope it will still e fast10:32
TheMueAram: i just proposed https://codereview.appspot.com/6476044/10:38
TheMueAram: we could move all lifecycle stuff in an own file and give it an own test suite10:39
Aramwill take a look after lunch10:41
TheMueAram: ok10:43
AramTheMue: you have a review.11:30
TheMueAram: thx11:30
TheMueAram: oh, removed the service check, sh… ;)11:31
TheMueAram: the rest of RemvoveUnit() is like relation and i wanted to keey it common11:32
Aramthat should be changed too.11:33
TheMueAram: then it's ok that i change it back, will do so11:34
Aramthanks11:34
TheMueAram: what problem do you have with "if err := foo(); err != nil { … }"?11:38
TheMueAram: IMHO is a common idiom11:38
AramI find it hard to read, personally, I know it's common.11:39
Aramv, ok := m[something]11:39
Aramthat's fine to me because it makes ok have a very local scope11:40
Aramwhich is great11:40
Arambut for errors I don't like it that much11:40
TheMueAram: ok, even if i like it i will change it here ;)11:42
Aramif you feel strongly about it, keep it, it boils down to personal preference.11:43
TheMueAram: already changed :P11:47
TheMueAram: you've seen, i added one simple test of the state tests11:48
TheMueAram: there it's the final one before all watcher tests11:48
Aramyes, seen it11:48
AramI'm surprised that it works though11:49
TheMueAram: maybe RemoveService() currently doesn't care for lifecycles?11:50
Aramah, it marks associated units as dying11:51
TheMueAram: *lol* ok, works by accident11:52
TheMueAram: do you have a table of which state tests are already ported to mstate?11:54
Aramno.11:54
AramI just picked and chosen as I saw fit11:54
Aramwhen I implemented a feature I searched for tests that use that feature11:54
Aramand ported those11:54
AramTheMue: another review11:57
TheMuethx11:58
TheMueAram: ah, thx for the explanation of the "hard death" :)12:10
TheMueAram: 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 times12:11
Aramyes, 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:13
TheMueAram: shall I do it?12:17
AramTheMue: please do, and also convert relations to the new function.12:17
TheMueAram: ok12:17
niemeyerYo!12:17
Aramhi12:17
TheMueniemeyer: heya12:18
niemeyerAram: Shall be ready for us to poke on it12:18
Aramniemeyer: what, txn?12:18
niemeyerAram: Watches would need a small extension to have mtime on the transaction, but that's trivial.. we can convert meanwhile12:18
niemeyerAram: Yeah12:19
Aramgreat.12:19
Aramshow me what you got, anytime12:19
* TheMue listens too12:20
niemeyerAram: Cool, I'll just move to the main repo, as I've been mistakingly pushing it in a branch in a subdir12:21
niemeyerAram: done12:24
niemeyerAram: It's not tagged, so you'll need to bzr pull it12:24
AramI can't use go.pkgdoc with it? :(12:25
* Aram runs godoc12:26
niemeyerAram: Not yet..12:26
niemeyerAram: I'll announce it when I have a moment12:26
niemeyerAram: and then tag it properly12:26
Aramno issue, godoc is great12:27
* Aram is reading now12:27
niemeyerAram: I'll also document it better, and write a blog post describing details12:28
niemeyerAram: But for the moment it feels wise to just get out of your way :)12:28
Aramok, I read the docs.12:34
Aramnow I shall read the tests to get a feel for it.12:34
niemeyerAram: sim_test.go is where the meat is12:37
niemeyerAram: txn_test.go can see improvements12:38
Aramyes, reading it already12:38
niemeyerAram: The API is almost boring, thankfully12:38
Aramgreat, thanks.12:45
AramI'm going to play with it.12:45
rogpeppeniemeyer: ping12:49
niemeyerrogpeppe: Yo12:51
rogpeppeniemeyer: i was wondering how much the upgrade logic needs to be aware of the dev/release (odd/even) version numbers12:52
rogpeppeniemeyer: i'm thinking it perhaps doesn't, but i'm probably wrong there12:53
niemeyerAram: I think we should start by porting the simple stuff to it12:53
niemeyerAram: Making sure that the changes ("$set", etc) are happening in transactions12:53
niemeyerrogpeppe: IIRC, in our last conversation (in Lisbon?) we agreed it doesn't12:53
Aramyes, I'll do it after I'm done with the things I'm working on ATM.12:54
rogpeppeniemeyer: cool, i'd forgotten about that conversation.12:54
niemeyerrogpeppe: We agreed, IIRC, that all we needed was in the upgrade-juju command itself12:54
niemeyerrogpeppe: If the decision to use it has been made, then why should the agent care12:54
niemeyerrogpeppe: IIRC12:54
rogpeppeniemeyer: ah, sorry, i was talking about the logic in the upgrade-juju command itself12:55
rogpeppeniemeyer: perhaps all it needs to do is make sure it never upgrades from a release version to a dev version.12:56
rogpeppe(and vice versa?)12:57
niemeyerrogpeppe: Unless --dev is used12:58
niemeyerrogpeppe: Vice versa sounds fine12:58
rogpeppeniemeyer: i think i'll add a dev flag to environs.BestTools12:58
niemeyerrogpeppe: Sounds sane12:59
rogpeppeniemeyer: cool13:03
rogpeppeniemeyer: 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
rogpeppeniemeyer: (it's fairly trivial stuff)13:42
niemeyerrogpeppe: Looking13:44
niemeyerrogpeppe: LGTM, with a small side-tracking peeve about the hackish storage test type13:51
rogpeppeniemeyer: 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
niemeyerrogpeppe: Why? It's just a type with methods13:52
niemeyerrogpeppe: What's wrong with just having that?13:52
rogpeppeniemeyer: quite a few methods that need to be changed every time the Environ type changes.13:53
rogpeppeniemeyer: hence i thought best to inherit them all so that piece of code is only sensitive to the methods it cares about.13:53
rogpeppeniemeyer: i agree it feels hackish tho'13:54
rogpeppeniemeyer: hmm, actually i can avoid the *most* hackish bits more easily13:57
niemeyerrogpeppe: Having that in the dummy environment sounds sane to13:59
niemeyertoo13:59
niemeyerrogpeppe: A dummy.Storage we can make use of13:59
niemeyerrogpeppe: So we can do something less awkward once13:59
rogpeppeniemeyer: the dummy Storage already works actually14:00
niemeyerrogpeppe: Well, so do we have a problem? :)14:00
rogpeppeniemeyer: probably not. the FindTools tests do it already. i was doing too much of a naive translation of the tests, i think.14:06
TheMueYeah!14:21
TheMueAram: found a nice way to do the table driven lifecycle test for all entities in life_test.go14:22
Aramhow?14:22
TheMueAram: let me just add unit (relation is already integrated, and i'll show you).14:24
Aramplease14:24
TheMueoops, bracket set too late. ;)14:24
TheMueAram: it's a kind of "for each living do the same as aram has done for relation before".14:25
TheMueAram: Great! Easier than thought. Will propose it now.14:30
niemeyerTheMue: Hm14:32
niemeyerTheMue: I'm not sure if I see what you mean, but FWIW, what Aram has done in relations shouldn't be *copied*14:33
niemeyerTheMue: The idea is to refactor it so that we have a single implementation that is used for all types14:33
TheMueniemeyer: yes, we are working on it. first part has been the only function ensureLife()14:33
TheMueniemeyer: but the goal shall be an embeddable type14:35
niemeyerTheMue: Superb, I imagined you'd already going in that direction, but decided to mention as it's cheaper than fixing later :-)14:35
Arampro 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
TheMueniemeyer: indeed :D14:36
TheMueAram: take a look at https://codereview.appspot.com/6481045/14:40
TheMueniemeyer: btw, /me likes state on mongo. pretty clean (and fast), simply elegant14:44
niemeyerTheMue: That's great to hear!14:44
AramTheMue: you have a review14:48
TheMueAram: thx *click*14:50
TheMueAram: yes, your idea sounds reasonable. will move it.14:52
Aramgreat14:52
TheMueAram: you've seen the idea? when e.g. service is migrated we'll just add it to the slice of envs. ;)14:52
TheMueAram: thanks to your idea with relation it's quite simple14:53
Aramyes, it's great14:54
TheMue4 secs for the whole beast, pretty fast. sadly it will slow down when all watcher tests are added.14:57
niemeyerTheMue: Let's keep an eye on those15:05
niemeyerTheMue: It should be possible to keep the impact pretty low15:05
TheMueniemeyer: right time, i just wanted to ask if there's already a first proposal or impl for watchers in mstate15:07
niemeyerTheMue: No, as we were waiting to see if the txn idea would flop or not15:07
niemeyerTheMue: It seems to work, so we can now start working on it15:08
niemeyerTheMue: (in that direction, rather than the old ones)15:08
TheMueniemeyer: hey, c'mon, it's by you, so it can't flop *lol*15:08
niemeyerTheMue: Oh man, I wish that was true15:08
TheMueniemeyer: i know have to leave the keyboard for a short time. can you msg me the uri to pull it?15:10
AramI just did a bzr pull inside the mgo dir and it was there afterwards15:12
niemeyerTheMue: ^15:14
Aramniemeyer: multiple txn runners can use the same collection, right?15:23
niemeyerAram: Yeah, they must if they are sharing affected documents15:25
Aramok.15:25
niemeyerAram: We only need a single tc15:25
Aramdo we need more than one runner?15:25
Aramniemeyer: ^15:26
niemeyerAram: No, everybody can share the same one15:26
Aramgreat.15:27
Aramthanks15:27
niemeyerAram: np15:27
fwereade_taking a break, back later16:03
Aramniemeyer: how can I do an UpdateAll with your txn?16:07
Aramis it possible?16:08
niemeyerAram: It's not16:08
Aram:(16:08
niemeyerAram: Yeah, this is a change to consider16:08
niemeyerAram: But we can live without it for now I suppose16:08
Aramwell, we do use UpdateAll16:09
Aramto update all units of a service16:09
niemeyerAram: Sure.. where are we using it ATM?16:09
Aramin RemoveService16:09
niemeyerAram: To begin with, we can put them all in a single transaction and run it16:10
niemeyerAram: Won't scale to extremes, but we can address that down the road16:10
Arambut we don't know the ids16:10
niemeyerAram: Hmm.. interesting.. let me think for a moment how we can solve this cheaply16:11
niemeyerAram: What's the change document?16:11
Aramchange = bson.D{{"$set", bson.D{{"life", Dying}}}}16:12
Arammore precisely16:12
Aramsel = bson.D{{"service", svc.doc.Name}}16:12
Aramchange = bson.D{{"$set", bson.D{{"life", Dying}}}}16:12
Aram_, err = s.units.UpdateAll(sel, change)16:12
niemeyerAram: Hmm16:19
niemeyerAram: I think we can make the units themselves responsible for setting their dying state16:21
niemeyerAram: And we can ref-count live units on the service itself16:21
niemeyerAram: So we know when the service can transition to dead16:22
niemeyerAram: The units then have to monitor the service to see when they're supposed to shut down16:22
Aramso each unit watches for its service, and sets itself to dying when the service is set to dying?16:22
niemeyerAram: Yeah16:23
Aramthat's potentially a lot of watches :).16:23
Aramok.16:23
niemeyerAram: In the same transaction that dec's the alive refcounter16:23
AramI've put a // BUG in the code so I remember to do this after I implement watcher16:23
Aram(obviously I can't do it before).16:23
niemeyerAram: Well, not really, sorry16:23
niemeyerAram: Erm.. I'm correcting myself, sorry16:23
niemeyerAram: The ref count should only be dec'd when the unit goes Dead16:24
niemeyerAram: Regarding being a lot of watchers, not really16:24
niemeyerAram: We already have to monitor the service from the unit for other reasons16:24
niemeyerAram: and again, the watching logic should be implemented *client side*16:25
niemeyerAram: There shouldn't be a significant delta in monitoring the service or not16:25
niemeyerAram: Which code?16:29
Aramniemeyer: ?16:29
niemeyerAram: What's the // BUG?16:29
Aramin RemoveService16:30
Aram// BUG(aram): units should monitor their service and set themselves16:30
Aram// to dead. waiting for watchers.16:30
niemeyerAram: Why RemoveService cares?16:30
Aramniemeyer: RemoveService doesn't care, I care so i remember to do it after I implement watchers.16:31
niemeyerAram: Okay, but we can get rid of UpdateAll already, right?16:32
Aramno?16:32
niemeyerAram: Okay, so that's what I'm trying to talk about :)16:32
niemeyerAram: What has to change for us to fix RemoveService in both state and mstate now?16:33
niemeyerAram: Well, or maybe just mstate16:33
Aramfirst we need to implement lifecycle for services.16:34
Aramniemeyer: 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:37
Aram(and if I insert something in that ops).16:38
niemeyerAram: That's right16:39
niemeyerAram: It'll return ErrAborted when the assertion is false16:39
niemeyerAram: (without touching anything)16:39
Aramniemeyer: I think I found a bug in txn.16:39
niemeyerAram: Woot16:39
niemeyerAram: What's up?16:39
Arambasically you can add documents that have the same no-id field identical, and that field is Unique: true16:40
Aramso you can add {"_id": 0, "foo": x} and {"_id": 1, "foo": y} even if "foo" should be unique.16:42
niemeyerAram: Well, I'm not sure I understand the problem.. you certainly can't add the document because MongoDB itself will prevent that from happening16:42
Arammaybe it doesn't end up in the database, but Run doesn't complain.16:42
niemeyerAram: Please read the documentation of Operation in the Insert and Assert fields16:42
AramI did, not sure your point.16:44
Arammy assert is txn.DocMissing whoch is not strong enough.16:45
niemeyerAram: My point is that what you're complaining about is precisely described there16:45
niemeyerAram: 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 not16:46
niemeyerAram: The way to stop the transaction is via Assert16:46
Aramwhat should I write in the Assert in this scenario?16:47
Aramstuff in Assert can only refer to that on object with DocId16:47
Arams/on/one/16:47
niemeyerAram: Right16:47
niemeyerAram: There's basically no way to prevent non-existence of something else that could potentially conflict with the document in any unique field16:48
niemeyerAram: We may somewhat easily workaround it, though16:48
Aramhow?16:48
niemeyerAram: Can you describe the case in more detail?16:49
Aramniemeyer: 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:50
niemeyerAram: By "case" I mean the real problem we have.. I understand the basic issue you're talking about, and we can easily workaround it16:52
niemeyerAram: With relations, we could use key as _id16:52
Aramthat's the real problem we have. we can't fail adding same relation twice.16:53
niemeyerAram: Which sounds realistic16:53
niemeyerAram: And have an auxiliar field for the numeric id16:53
Aramyes, we can use key as _id16:53
Aramthat was the original design :)).16:53
niemeyerAram: Sorry, my time machine had a bug on it and I couldn't use it16:53
niemeyerAram: I'll step out for lunch.. happy to continue in a bit16:54
Aramenjoy16:54
niemeyerCheers16:54
niemeyerFor 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 poke17:52

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