| menn0 | thumper, waigani: I just did a "juju destroy-environment local --yes --force" to 2 env JES both of which had new LXC containers coming up and the containers were left behind | 01:25 |
|---|---|---|
| menn0 | known limitation until the new env descruction stuff happens? | 01:26 |
| waigani | hmmm... | 01:26 |
| thumper | menn0: known... | 01:26 |
| waigani | menn0: thanks for the heads up, I'll add it to my things to manually test | 01:26 |
| thumper | menn0: I can explain... | 01:26 |
| thumper | not surprising | 01:26 |
| thumper | which is why we want to tell people to use system destroy | 01:27 |
| thumper | and environment destroy | 01:27 |
| thumper | not destroy-environment | 01:27 |
| menn0 | cool | 01:27 |
| menn0 | just making sure it was a known thing | 01:27 |
| thumper | I'll explain when I'm not shoveling food in my mouth | 01:28 |
| menn0 | I can pretty much guess where the problem is so no rush | 01:29 |
| menn0 | thumper: i thought I just finished the state pool stuff but it seems i've made the state server panicky | 01:42 |
| menn0 | thumper: I think I know why though | 01:42 |
| thumper | menn0: did you want to hop on a call to hear why destroy-environment --force is evil? | 01:49 |
| menn0 | thumper: sure | 01:50 |
| thumper | menn0, waigani: https://plus.google.com/hangouts/_/canonical.com/evil | 01:50 |
| waigani | thumper: http://reviews.vapour.ws/r/2250/, cheers | 02:11 |
| sinzui | thumper: "destroy-environment --force" is eveil because it circumvents blocks, leaves resources behind, and users report bugs that juju did exactly as they told it to do? | 02:15 |
| thumper | for JES it is evil because it doesn't contact the api server, and just destroys the system environment, leaving all hosted environments running but headless | 02:15 |
| * thumper goes to make a coffee urgently | 02:16 | |
| sinzui | thumper: ouch, but --force also circumvents the api server, which is why my blocks can be ignored | 02:16 |
| thumper | sinzui: yes, it doesn't even try | 02:23 |
| thumper | sinzui: the new system destroy and environment destroy commands do try | 02:23 |
| sinzui | thumper: hurray for sanity | 02:24 |
| menn0 | thumper: if you have time: http://reviews.vapour.ws/r/2445/ | 03:13 |
| menn0 | thumper: i'm really happy with how this worked out | 03:13 |
| * thumper looks | 03:13 | |
| thumper | menn0: mostly good, just concerned about reordering the defer calls | 03:30 |
| menn0 | thumper: hmmm I wasn't intending on reordering them | 03:31 |
| * menn0 checks | 03:31 | |
| thumper | you moved them all into a single function that is deferred | 03:32 |
| thumper | but then still call defer on each of the internals | 03:32 |
| thumper | and you changed the order | 03:32 |
| thumper | effectively reorering | 03:32 |
| menn0 | but defers run inside out | 03:32 |
| thumper | either reorder and don't defer | 03:32 |
| thumper | or defer | 03:32 |
| thumper | yes | 03:33 |
| * menn0 does a quick check on play | 03:33 | |
| thumper | happy to talk through | 03:33 |
| menn0 | thumper: I think I did the right thing: http://play.golang.org/p/fzFmAHa86q | 03:34 |
| menn0 | sorry | 03:34 |
| menn0 | hang on | 03:34 |
| menn0 | thumper: this one: http://play.golang.org/p/S4ehts8JxY | 03:35 |
| thumper | menn0: but you did version three in http://play.golang.org/p/2d9vMgldxc | 03:36 |
| menn0 | thumper: I see what I screwed up | 03:36 |
| menn0 | all the defers in the new func shouldn't be there | 03:36 |
| menn0 | i'm an idiot | 03:36 |
| thumper | right | 03:36 |
| menn0 | thumper: my eyes were not seeing the inner defers | 03:36 |
| menn0 | man I was trying to make that code clearer and unintentionally made it much worse | 03:37 |
| thumper | :) | 03:37 |
| menn0 | thumper: i saw the bit about env.Life ! state.Alive too | 03:39 |
| menn0 | i'll fix that in a followup PR | 03:39 |
| menn0 | it's not really related to this change | 03:39 |
| menn0 | I think the condition should still be there, but as: else if env.Life() == state.Dead | 03:40 |
| thumper | hmmm... | 03:41 |
| thumper | nope | 03:41 |
| thumper | because we may want to get logs from a dead environment | 03:42 |
| thumper | prior to everything being deleted some time later | 03:42 |
| menn0 | thumper: hmm ok | 03:42 |
| thumper | the dying / dead aspects should stop us adding things to the environment | 03:46 |
| thumper | but we need to be able to view / introspect it while it is dying | 03:46 |
| thumper | and after it is dead | 03:46 |
| * thumper looks in the morgue | 03:46 | |
| menn0 | thumper: but remmeber we've removed most of the assertions on env life | 03:46 |
| menn0 | thumper: I guess the critical places, like adding a machine still check | 03:47 |
| * menn0 wishes you could edit past IRC comments :) | 03:47 | |
| * menn0 pushes the StatePool PR | 03:49 | |
| thumper | right | 03:50 |
| thumper | dead / dying environments can't get new machines or services | 03:50 |
| thumper | everything else needs a machine or service | 03:50 |
| menn0 | can't get storage either i'm pretty sure | 03:50 |
| thumper | although in the coming future we should also check environment level network spaces and storage | 03:50 |
| * thumper nods | 03:50 | |
| * thumper has to head into town briefly | 03:51 | |
| thumper | bbl to review waigani's branch | 03:51 |
| * menn0 has to swap his UK driver's license for a NZ one | 03:51 | |
| thumper | gee menn0 | 03:53 |
| thumper | jumping the merge gun | 03:53 |
| thumper | :) | 03:53 |
| * thumper does a post hoc shipit | 03:53 | |
| menn0 | thumper: sorry... missed there wasn't one there already | 03:53 |
| menn0 | thumper: if you have time: http://reviews.vapour.ws/r/2446/ | 05:18 |
| dimitern | dooferlad, TheMue, morning | 08:52 |
| dooferlad | dimitern: hello! | 08:52 |
| dimitern | dooferlad, TheMue, can you have a look at http://reviews.vapour.ws/r/2448/ please? | 08:52 |
| dooferlad | dimitern: on it | 08:53 |
| dimitern | dooferlad, cheers | 08:53 |
| dooferlad | dimitern: we really need to get better at splitting cosmetic changes from functional changes :-| | 08:54 |
| dooferlad | dimitern: or get a better diff viewer! | 08:54 |
| dimitern | dooferlad, yeah, when there's no deadline to catch :) | 08:55 |
| dooferlad | dimitern: :-) | 08:55 |
| dimitern | dooferlad, also, if you can give it a spin on EC2 - bootstrap, space list, space create, subnet add, subnet list (with juju subnet -- debug .. if needed), subnet add with CIDR and/or ProviderId | 08:59 |
| dimitern | dooferlad, --constraints spaces= does not cause errors in my tests, but obviously doesn't yet respect them | 09:00 |
| dooferlad | dimitern, TheMue: hangout | 09:01 |
| dimitern | dooferlad, omw, getting coffee | 09:02 |
| TheMue | dooferlad: omw, got coffee | 09:04 |
| dooferlad | dimitern: do you have a moment for EC2 testing? | 09:52 |
| dimitern | dooferlad, sure | 09:52 |
| dooferlad | dimitern: https://ide.c9.io/dooferlad/juju | 09:53 |
| dooferlad | TheMue: ^^ if you want to join in | 09:53 |
| TheMue | dooferlad: having phone, come in a few moments | 09:53 |
| dimitern | dooferlad, omw | 09:54 |
| TheMue | dimitern: btw, would you like a feature flag for the spaces support? like for address allocation? that way we could turn it on and off in tests with dummy. | 10:47 |
| dimitern | TheMue, nope, we need another way to turn it on and off - e.g. having a "broken" method is one way | 10:48 |
| TheMue | dimitern: ok | 10:49 |
| dimitern | TheMue, or a flag settable by calling dummy.SetSupportsSpaces(bool) | 10:49 |
| TheMue | dimitern: a direct method needs that you have direct access to the provider. often it is hidden deeply in our embedded tests. so I prefer the "broken" approach | 10:52 |
| dimitern | TheMue, I'm fine with either - it's the same effort I think | 10:54 |
| TheMue | dimitern: in first implementation yes, in tests I really liked the indirect approach - here with feature flag, but broken is fine too - during the tests | 10:55 |
| mattyw | axw, https://github.com/juju/juju/pull/3072/files#diff-aa7f5128113461cdc1ee5c2335a09d6cR52 | 12:54 |
| frankban | ericsnow: I replied to your requests at http://reviews.vapour.ws/r/2416/ could you please take another look? | 13:26 |
| sinzui | dimitern: master wasn't merged into net-cli. your branch failed. Master now has singnigicant divergence and we don't know if it is good. | 13:36 |
| sinzui | dimitern: I am not sure what to recommend. test master and hope for a pass, then test the updated net-cli to get a pass? | 13:37 |
| natefinch | finally got my code working... now to remove 100 printfs | 13:39 |
| dimitern | sinzui, we merged master this morning | 13:47 |
| dimitern | sinzui, and I have a PR which hopefully solves pingerSuite.TestAgentConnectionDelaysShutdownWithPing and MachineSuite.TestManageEnvironRunsInstancePoller | 13:48 |
| sinzui | dimitern: excellent. master just started testing. I will make net-cli the most important branch to test next. I am setting CI to wait 8 hours if there is a failure to give people time to investigate and retry failures cause by subtrates | 13:48 |
| dimitern | sinzui, awesome | 13:49 |
| dimitern | dooferlad, TheMue, please have a look http://reviews.vapour.ws/r/2454/ | 13:56 |
| TheMue | dimitern: yep | 13:57 |
| dooferlad | dimitern: on it | 13:57 |
| dimitern | cheers guys! | 13:57 |
| wwitzel3 | ericsnow, natefinch, katco: hangout working for you guys? | 13:59 |
| katco | was in one half an hour ago | 13:59 |
| ericsnow | wwitzel3: yep | 13:59 |
| katco | ericsnow: moonstone working fine | 14:00 |
| natefinch | wwitzel3: make sure it's having you join with your canonical account | 14:00 |
| TheMue | dimitern: reviewed | 14:02 |
| dimitern | TheMue, ta! | 14:08 |
| dooferlad | dimitern: done | 14:09 |
| ericsnow | cmars: is this what I think it is? http://reviews.vapour.ws/r/2453 | 14:09 |
| cmars | ericsnow, yep | 14:10 |
| ericsnow | cmars: cool! | 14:10 |
| cmars | :) | 14:10 |
| dimitern | dooferlad, thanks! | 14:11 |
| ericsnow | cmars: at least it got reviewed <wink> | 14:14 |
| cmars | ericsnow, we're going to re-review it in greater detail next week. important to get it landed for now to get CI testing, but it won't be glossed over. all other PRs have been reviewed carefully | 14:20 |
| ericsnow | cmars: oh, I don't doubt that :) | 14:20 |
| ericsnow | cmars: thanks for getting it done! | 14:21 |
| cmars | ericsnow, wasn't me, really. axw & wallyworld are the heroes | 14:21 |
| ericsnow | cmars: ah, right; commit history FTW :) | 14:23 |
| dimitern | TheMue, I'm not sure what's your last comment about? adding a comment in the patched function checking the result of newWorker? | 14:25 |
| TheMue | dimitern: only adding comments I missed when creating this test. that FinishedWorker means the Addresser doesn't run while not FinishedWorker means it's the Addresser (sadly not direct testable) | 14:26 |
| TheMue | dimitern: but it's only maintenance cosmetic, for later | 14:27 |
| TheMue | dimitern: maybe we should add a worker.HasFinished(w worker.Worker) bool for it soon, when those tests get more. will keep it in mind for the Firewaller | 14:28 |
| dimitern | TheMue, I see, ok I'll add comments about the meaning of a FinishedWorker as a result | 14:30 |
| dimitern | dooferlad, about your comment | 14:30 |
| dimitern | dooferlad, I don't believe the issue is related to clock time | 14:30 |
| dooferlad | dimitern: oh, I thought you were increasing the timeout | 14:31 |
| dimitern | dooferlad, yeah, but your comment caused me to think of a different approach | 14:34 |
| dimitern | dooferlad, using attempt loops we don't have to sleep explicitly | 14:34 |
| ericsnow | frankban: I'm reading through your responses to my review right now :) | 14:52 |
| frankban | ericsnow: ty! | 14:52 |
| niemeyer_ | Hey all | 15:35 |
| natefinch | howdy niemeyer_ | 15:36 |
| niemeyer_ | Do we have any envs with MongoDB deployed on IPv6? | 15:36 |
| natefinch | there was talk about that on the mailing list. I think the answer is that it needs work, i.e. not really working currently, though I don't know the details. | 15:37 |
| mgz | cmars: hey, are you around | 15:54 |
| ericsnow | frankban: thanks for the thoughtful responses to my review comments | 15:55 |
| ericsnow | frankban: I see where you're coming from now | 15:56 |
| frankban | ericsnow: cool, thank you, reading your review | 15:58 |
| ericsnow | frankban: BTW, good call on making the JSON field tag "yaml" :) | 15:59 |
| ericsnow | frankban: my only remaining concern is about adding the "guiserver" methods to the "public" API facade | 16:03 |
| frankban | ericsnow: yeah I was looking at that | 16:05 |
| frankban | ericsnow: the GUI is really just a user client, and I am not sure about having a separate facade for the GUI because the GUI basically uses all the Client methods already (including the mega-watcher) | 16:05 |
| frankban | ericsnow: actually I think the GUI is the most used juju API client after juju itself ;-) | 16:06 |
| ericsnow | frankban: :) | 16:06 |
| ericsnow | frankban: I think it would be good to at least have methods like GetBundleChanges on a separate facade | 16:07 |
| frankban | ericsnow: so the GUI is JS connecting the ws, it needs the user to authenticate and all the endpoint it uses are in the client facade (excpet for the initial admin.login one) | 16:07 |
| ericsnow | frankban: ah | 16:07 |
| ericsnow | frankban: that's fine | 16:08 |
| frankban | ericsnow: so basically like this the change is transparent, the GUI connects and sends requests over the ws, and GetBundleChanges is one request like many others (ServiceDeploy etc.) | 16:08 |
| ericsnow | frankban: so one goal is to minimize changes to the GUI client code? | 16:09 |
| frankban | ericsnow: yes, and we can adjust things later if the amount of GUI specific endpoints increases, but I don;t believe so | 16:10 |
| ericsnow | frankban: using multiple facades on the same WS should be fine, no? | 16:10 |
| ericsnow | frankban: k | 16:10 |
| frankban | ericsnow: thanks | 16:10 |
| ericsnow | frankban: I still think there should be a bit more discussion (on juju-dev) about where methods like GetBundleChanges should live in the API | 16:12 |
| ericsnow | frankban: but moving the code to a more appropriate place later (if needed) wouldn't be hard | 16:13 |
| frankban | ericsnow: I agree, but consider this is merging into a feature branch, I think we will be able to introduce these changes gradually, and rick_h_ is already working on this | 16:13 |
| ericsnow | frankban: how urgent is it that GetBundleChanges land? Could it wait a few days? | 16:13 |
| ericsnow | frankban: oh, duh | 16:13 |
| frankban | ericsnow: this is not landing in trunk | 16:13 |
| ericsnow | frankban: feature branch :) | 16:14 |
| ericsnow | frankban: I'll give you a ship-it but encourage you to at least ask jam and fwereade about it :) | 16:14 |
| ericsnow | frankban: oh, and there are some missing tests | 16:15 |
| frankban | ericsnow: thanks! yes this will be a long running branch and with rick_h_ we are already working on getting people aware of our goals and the changes to be made for the embedded gui story | 16:15 |
| frankban | ericsnow: which ones? back to reading the review | 16:15 |
| rick_h_ | ericsnow: there's a spec with this work out to wallyworld and jam atm | 16:15 |
| rick_h_ | ericsnow: we're waiting on feedback but with holidays and such we're moving forward with the feature branch with the caveat that things will be tweaked based on feedback | 16:16 |
| ericsnow | rick_h_: cool! | 16:16 |
| rick_h_ | ericsnow: as you mention, moving the endpoint isn't really going to make/break anything here. | 16:16 |
| rick_h_ | ericsnow: https://docs.google.com/document/d/1zZY0a-dby-VnrBXaoFARIN8Mj6pdXLeZzTXmH0mRUik/edit in case it helps at all or you're curious | 16:16 |
| ericsnow | rick_h_: yep :) | 16:16 |
| rick_h_ | ericsnow: but we're definitely eager for core folks to know/udnersteand what's up and where things are headed as we want to make sure we're doing thigns by the book, that your folks feel comfy with fixes that might be required later/etc. | 16:17 |
| rick_h_ | ericsnow: so please feel free to push back on anything you would to another core team | 16:18 |
| ericsnow | rick_h_: sounds good; I'll be sure to read through the spec | 16:18 |
| rick_h_ | ericsnow: ty, any feedback is welcome. I ran thumper through it last night | 16:18 |
| ericsnow | rick_h_: you can count on me to speak up :) | 16:18 |
| rick_h_ | ericsnow: and we look forward to the other when they're back on duty | 16:18 |
| rick_h_ | :) | 16:18 |
| frankban | ericsnow: GetBundleChanges is tested in apiserver/client/bundles_test.go | 16:18 |
| ericsnow | frankban: hmm, we're trying to move away from full-stack tests like that | 16:20 |
| * ericsnow looks at tests for other API methods (on server and client) | 16:20 | |
| frankban | ericsnow: thanks | 16:20 |
| ericsnow | frankban: blech, our existing tests aren't very good examples :/ | 16:23 |
| frankban | :-/ | 16:23 |
| ericsnow | frankban: for an example of unit-ish tests see apiserver/backups and api/backups | 16:24 |
| ericsnow | frankban: "feature" tests (effectively integration tests) live in the top-level featuretests package | 16:25 |
| ericsnow | frankban: we're making an effort to improve our approach to testing in core which is why I'm bringing all this up | 16:26 |
| frankban | ericsnow: sure, I made a note and I'll take a look on Monday | 16:27 |
| ericsnow | frankban: coolio | 16:27 |
| frankban | ericsnow: thanks and have a nice weekend | 16:27 |
| ericsnow | frankban: you too and thanks for working on this! | 16:28 |
| natefinch | ericsnow: this id/name thing is killing me | 19:15 |
| ericsnow | natefinch: how so? | 19:15 |
| natefinch | ericsnow: just the fact that sometimes we munge the name/id and sometimes we don't.... and so I have to know exactly when to munge the name/id to match what's stored in whichever map | 19:17 |
| ericsnow | natefinch: we should always be identifying them by the full ID | 19:17 |
| ericsnow | natefinch: where in particular are we not? | 19:18 |
| natefinch | ericsnow: but what is the full ID and where do I get it from? This is the problem I have with untrack. So the current problem is getting the test infrastructure to do the right thing when I tell it to untrack.... which is actually kind of a problem in itself that we have logic in the test infrastructure | 19:19 |
| natefinch | ericsnow: like stubAPIClient | 19:20 |
| natefinch | ericsnow: https://github.com/natefinch/juju/blob/untrack-cmd/process/context/base_test.go#L287 | 19:21 |
| ericsnow | natefinch: the user should have passed in the full ID (or the name, by which we look up the full ID) | 19:21 |
| natefinch | ericsnow: the user doesn't know what the full ID is, since that's a composed thing which we create, isn't it? They know the name... I don't think they can possibly be expected to know anything else | 19:22 |
| ericsnow | natefinch: that's fine; we just have to do the lookup for them | 19:23 |
| natefinch | ericsnow: my point is... the fact that we're doing all this logic stuff in stubAPIClient is bad. We're duplicating production code logic, and relying on this test harness to have the same implementation. | 19:24 |
| ericsnow | natefinch: the API client shouldn't be doing any of that lookup | 19:25 |
| ericsnow | natefinch: I thought we agreed that we would hold off on that part | 19:25 |
| ericsnow | natefinch: i.e. the API client should always expect full IDs | 19:25 |
| natefinch | ericsnow: I'm talking about this - https://github.com/natefinch/juju/blob/untrack-cmd/process/context/base_test.go#L201 | 19:26 |
| natefinch | ericsnow: that's logic that is duplicating what the real apiclient would be doing | 19:26 |
| ericsnow | natefinch: setNew is just a convenience function to inject values into the API | 19:29 |
| ericsnow | natefinch: it's not part of the stubbed API implementation | 19:30 |
| natefinch | ericsnow: Ahh, I was missing a flush in there... I didn't expect that to make a difference, but it does. Sorry. | 19:33 |
| ericsnow | natefinch: np :) | 19:34 |
| natefinch | ok, everything works... now to merge wwitzel3's rename commit :/ | 20:37 |
| natefinch | oh this is gonna suck | 20:44 |
| natefinch | evidently git is not smart enough to merge <file renamed> with <file modified> .... how is that not something it can just do automatically? | 20:45 |
| natefinch | "oh hey, the old file just got moved, so I'll merge the changes into the new location" :/ | 20:45 |
| ericsnow | natefinch: yeah, I just went through that pain yesterday | 20:46 |
| natefinch | google/stack overflow to the rescue.... git rebase feature-proc-mgmt -X rename-threshold=5% | 20:48 |
| natefinch | ....mostly | 20:49 |
| katco | can i get a rubber-stamp on http://reviews.vapour.ws/r/2455/? this exact patch has already been landed in 1.24 | 21:09 |
| perrito666 | katco: done | 21:16 |
| katco | perrito666: ty sir | 21:16 |
| perrito666 | since I reviewed the other one I believe you :) | 21:16 |
| katco | haha | 21:17 |
| perrito666 | I must say, buying an electric toaster is a life changing decision, I no longer forget and burn my toasts | 21:23 |
| perrito666 | (which mostly happens while reading this channel or reading code of juju) | 21:24 |
Generated by irclog2html.py 2.7 by Marius Gedminas - find it at mg.pov.lt!