Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(2693)

Issue 25040043: Refactor container provisioner (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years ago by wallyworld
Modified:
6 years ago
Reviewers:
william.reade, mp+194795, fwereade, jameinel
Visibility:
Public.

Description

This branch changes how containers are provisioned. Before, the container provisioner task is started with the machine agent. Now the start up of that task is delayed until a container is actually requested. The basic concept is: - set up container listener - when container of type is requested: * stop container listener * ensure dependencies of that container type are installed * start container provisioning task https://code.launchpad.net/~wallyworld/juju-core/lazy-container-provisioners/+merge/194795 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 5

Patch Set 2 : - #

Patch Set 3 : - #

Patch Set 4 : - #

Total comments: 8

Patch Set 5 : - #

Patch Set 6 : - #

Patch Set 7 : - #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+492 lines, -67 lines) Patch
A [revision details] View 1 2 3 4 5 6 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/agent.go View 1 2 3 chunks +2 lines, -3 lines 0 comments Download
M cmd/jujud/machine.go View 1 2 3 4 10 chunks +65 lines, -32 lines 1 comment Download
M cmd/jujud/unit.go View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M state/api/params/internal.go View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download
M state/api/provisioner/machine.go View 1 2 3 4 1 chunk +22 lines, -0 lines 1 comment Download
M state/api/provisioner/provisioner_test.go View 1 2 3 4 1 chunk +13 lines, -0 lines 0 comments Download
M state/apiserver/provisioner/provisioner.go View 1 2 3 4 1 chunk +25 lines, -0 lines 1 comment Download
M state/apiserver/provisioner/provisioner_test.go View 1 2 3 4 1 chunk +63 lines, -0 lines 0 comments Download
M state/machine.go View 1 2 chunks +2 lines, -4 lines 0 comments Download
A worker/provisioner/container_initialisation.go View 1 2 3 4 5 6 1 chunk +120 lines, -0 lines 2 comments Download
A worker/provisioner/container_initialisation_test.go View 1 2 3 4 5 6 1 chunk +114 lines, -0 lines 0 comments Download
A worker/provisioner/package_test.go View 1 2 3 1 chunk +14 lines, -0 lines 0 comments Download
M worker/provisioner/provisioner.go View 1 chunk +2 lines, -0 lines 0 comments Download
M worker/provisioner/provisioner_test.go View 1 2 3 4 4 chunks +13 lines, -15 lines 0 comments Download
M worker/runner.go View 1 2 8 chunks +19 lines, -10 lines 0 comments Download

Messages

Total messages: 6
fwereade
These may be redundant now, I'll look at the rest again. https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go File state/api/params/internal.go (right): ...
6 years ago (2013-11-13 09:31:00 UTC) #1
jameinel
https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go File state/api/params/internal.go (right): https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go#newcode31 state/api/params/internal.go:31: Errors []*Error On 2013/11/13 09:31:01, fwereade wrote: > Do ...
6 years ago (2013-11-13 09:37:57 UTC) #2
fwereade
A few thoughts -- generally looking solid though. https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/container_initialisation.go File worker/provisioner/container_initialisation.go (right): https://codereview.appspot.com/25040043/diff/60001/worker/provisioner/container_initialisation.go#newcode61 worker/provisioner/container_initialisation.go:61: // ...
6 years ago (2013-11-13 10:26:10 UTC) #3
wallyworld
Please take a look. https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go File state/api/params/internal.go (right): https://codereview.appspot.com/25040043/diff/1/state/api/params/internal.go#newcode31 state/api/params/internal.go:31: Errors []*Error On 2013/11/13 09:37:57, ...
6 years ago (2013-11-14 05:25:08 UTC) #4
william.reade
I don't really love the proliferation of watchers here -- isn't it simpler to have ...
6 years ago (2013-11-18 15:30:58 UTC) #5
william.reade
6 years ago (2013-11-18 23:22:22 UTC) #6
OK, after much discussion, this LGTM given a SetSupportedContainers API rather
than AddSupportedContainers. I think there's still room to improve further, but
demanding perfection at the expense of progress is sucky and I won't do that.
Any more. Today...
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b