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

Issue 28890043: Use single watcher for containr setup (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
6 years ago by wallyworld
Modified:
5 years, 11 months ago
Reviewers:
mp+195711, jameinel
Visibility:
Public.

Description

Use single watcher for containr setup Add a WatchAllContainers API so that a single watcher can be used to start (as needed) each container provisioner, instead of using one eatcher per contsiner type. This branch will need trunk merged and be updated to use the revised SetSupportedContainers API call once my other branch gets approved and landed. https://code.launchpad.net/~wallyworld/juju-core/single-container-watcher/+merge/195711 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 13
Unified diffs Side-by-side diffs Delta from patch set Stats (+202 lines, -57 lines) Patch
A [revision details] View 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/machine.go View 1 chunk +5 lines, -7 lines 2 comments Download
M state/api/provisioner/machine.go View 2 chunks +25 lines, -1 line 3 comments Download
M state/apiserver/provisioner/provisioner.go View 3 chunks +13 lines, -2 lines 4 comments Download
M state/apiserver/provisioner/provisioner_test.go View 1 chunk +37 lines, -0 lines 0 comments Download
M state/state_test.go View 5 chunks +31 lines, -0 lines 0 comments Download
M state/watcher.go View 1 chunk +14 lines, -3 lines 4 comments Download
M worker/provisioner/container_initialisation.go View 4 chunks +71 lines, -40 lines 0 comments Download
M worker/provisioner/container_initialisation_test.go View 3 chunks +4 lines, -4 lines 0 comments Download

Messages

Total messages: 5
wallyworld
Please take a look.
6 years ago (2013-11-19 06:52:12 UTC) #1
jameinel
Some comments, but the rest LGTM https://codereview.appspot.com/28890043/diff/1/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/28890043/diff/1/cmd/jujud/machine.go#newcode241 cmd/jujud/machine.go:241: watcherName := fmt.Sprintf("%s-container-watcher", ...
6 years ago (2013-11-20 12:36:45 UTC) #2
wallyworld
https://codereview.appspot.com/28890043/diff/1/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/28890043/diff/1/cmd/jujud/machine.go#newcode241 cmd/jujud/machine.go:241: watcherName := fmt.Sprintf("%s-container-watcher", machine.Id()) On 2013/11/20 12:36:45, jameinel wrote: ...
5 years, 12 months ago (2013-11-21 12:16:09 UTC) #3
jameinel
https://codereview.appspot.com/28890043/diff/1/state/api/provisioner/machine.go File state/api/provisioner/machine.go (right): https://codereview.appspot.com/28890043/diff/1/state/api/provisioner/machine.go#newcode245 state/api/provisioner/machine.go:245: // to the lifecycles of all containers on the ...
5 years, 12 months ago (2013-11-23 05:08:59 UTC) #4
wallyworld
5 years, 11 months ago (2013-11-29 00:02:45 UTC) #5
https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner.go (right):

https://codereview.appspot.com/28890043/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner.go:149: return p.WatchContainers(args)
On 2013/11/23 05:09:00, jameinel wrote:
> Except the way you've implemented it, WatchContainers("") *does* give you this
> behavior (over the API).
> If you don't actually want that, then we should factor the code into a common
> helper, and say pass in the appropriate watcher.
> 
> That way someone calling WatchContainers wouldn't be allowed to pass in the
> empty string.

I've enhanced the client api for WatchContainers to reject invalid container
types

https://codereview.appspot.com/28890043/diff/1/state/watcher.go
File state/watcher.go (right):

https://codereview.appspot.com/28890043/diff/1/state/watcher.go#newcode204
state/watcher.go:204: isChild := fmt.Sprintf("^%s/[a-z]*/%s$", m.doc.Id,
names.NumberSnippet)
On 2013/11/23 05:09:00, jameinel wrote:
> My concern here is that it fails silently and will cause someone who is adding
a
> new type in the future to scratch their head wondering why it isn't working.
> 
> I could even argue for %s/[^/]+/%s
> 
> Actually, we probably want a + rather than a * anyway, because we don't want
to
> match 0//1 right?
> 
> If you don't see a great place to make the failure more active, fine, but
naming
> it helps increase its visibility a little bit, at least.

Bah, the * instead of + was a typo.

We already have a constant defined in names/machine.go

ContainerSnippet = "(/[a-z]+/" + NumberSnippet + ")"

So I've pulled out the [a-z]+ and made it a new constant:

ContainerTypeSnippet = "[a-z]+"

I think it's reasonable to assume all container type names will be [a-z]+
I could construct a regexp by iterating over all container types and making a
regexp like [lxc|kvm] but I don't thin that adds enough value?
Sign in to reply to this message.

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