environs: remove VarDir global

environs: remove VarDir global

This makes it much easier to make the environs functions
work on hypothetical remote machines, for example.

Also rename VarDir to DataDir throughout.

https://code.launchpad.net/~rogpeppe/juju-core/055-no-global-vardir/+merge/123316

(do not edit description out of merge proposal)

[rog, 2012-09-07 16:40:49]

Please take a look.

[fwereade, 2012-09-10 06:58:29]

Lots to like here, but a few things I'm -1 on.

https://codereview.appspot.com/6501106/diff/1001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6501106/diff/1001/cmd/jujud/agent.go#newcode51
cmd/jujud/agent.go:51: VarDir    string
IMO VarDir is misleading and unhelpful and strictly inferior to JujuDir in every
way. Convince me otherwise?

https://codereview.appspot.com/6501106/diff/1001/container/container.go
File container/container.go (right):

https://codereview.appspot.com/6501106/diff/1001/container/container.go#newco...
container/container.go:38: InitDir string
FWIW, I think the default "/etc/init" in upstart was a mistake, and I'd rather
see "" as an error throughout, but I understand it's out of scope here.

https://codereview.appspot.com/6501106/diff/1001/container/container_test.go
File container/container_test.go (right):

https://codereview.appspot.com/6501106/diff/1001/container/container_test.go#...
container/container_test.go:36: os.Setenv("PATH",
toolsDir+":"+os.Getenv("PATH"))
What's PATH doing in here? I thought there was general agreement that we should
drop it entirely? If we're touching it at all here, we should be making sure
it's empty...

https://codereview.appspot.com/6501106/diff/1001/container/container_test.go#...
container/container_test.go:56: unitName := "juju-agent-dummy-0"
What naming scheme does this conform to?

https://codereview.appspot.com/6501106/diff/1001/container/container_test.go#...
container/container_test.go:67: unitDir := filepath.Join(cont.VarDir, "units",
"dummy-0")
Didn't we agree "agents"?

https://codereview.appspot.com/6501106/diff/1001/environs/cloudinit/cloudinit.go
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/6501106/diff/1001/environs/cloudinit/cloudinit...
environs/cloudinit/cloudinit.go:51: VarDir string
Horrid name again.

https://codereview.appspot.com/6501106/diff/1001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6501106/diff/1001/worker/machiner/machiner.go#...
worker/machiner/machiner.go:28: container container.Container
Wait, what? A machiner can only deploy to one container? Isn't this crack? I
suggest sticking with container.Simple for now rather than baking in what feels
like a really flaky assumption...

https://codereview.appspot.com/6501106/diff/1001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6501106/diff/1001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:209: path := filepath.Join(varDir, "units",
strings.Replace(name, "/", "-", 1))
Again, didn't we agree "agents" and "kind-name"?

[rog, 2012-09-10 19:53:09]

https://codereview.appspot.com/6501106/diff/1001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6501106/diff/1001/cmd/jujud/agent.go#newcode51
cmd/jujud/agent.go:51: VarDir    string
On 2012/09/10 06:58:30, fwereade wrote:
> IMO VarDir is misleading and unhelpful and strictly inferior to JujuDir in
every
> way. Convince me otherwise?

VarDir was a name agreed in a previous discussion. I'd be more than happy to
change to JujuDir throughout however. Gustavo?

https://codereview.appspot.com/6501106/diff/1001/container/container_test.go
File container/container_test.go (right):

https://codereview.appspot.com/6501106/diff/1001/container/container_test.go#...
container/container_test.go:36: os.Setenv("PATH",
toolsDir+":"+os.Getenv("PATH"))
On 2012/09/10 06:58:30, fwereade wrote:
> What's PATH doing in here? I thought there was general agreement that we
should
> drop it entirely? If we're touching it at all here, we should be making sure
> it's empty...

For this CL (which is big enough I think) I was trying to stick to that which is
necessary to remove the global VarDir. container fixes are in the next CL in
line, if that works for you.

https://codereview.appspot.com/6501106/diff/1001/container/container_test.go#...
container/container_test.go:56: unitName := "juju-agent-dummy-0"
On 2012/09/10 06:58:30, fwereade wrote:
> What naming scheme does this conform to?

The one we were using before. container fixes in the next CL.

https://codereview.appspot.com/6501106/diff/1001/container/container_test.go#...
container/container_test.go:67: unitDir := filepath.Join(cont.VarDir, "units",
"dummy-0")
On 2012/09/10 06:58:30, fwereade wrote:
> Didn't we agree "agents"?

as above.

https://codereview.appspot.com/6501106/diff/1001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6501106/diff/1001/worker/machiner/machiner.go#...
worker/machiner/machiner.go:28: container container.Container
On 2012/09/10 06:58:30, fwereade wrote:
> Wait, what? A machiner can only deploy to one container? Isn't this crack? I
> suggest sticking with container.Simple for now rather than baking in what
feels
> like a really flaky assumption...

When LXC is added, we'll probably have "localContainer" and "lxcContainer" here.
how about i rename container to localContainer in anticipation of that?

https://codereview.appspot.com/6501106/diff/1001/worker/uniter/uniter.go
File worker/uniter/uniter.go (right):

https://codereview.appspot.com/6501106/diff/1001/worker/uniter/uniter.go#newc...
worker/uniter/uniter.go:209: path := filepath.Join(varDir, "units",
strings.Replace(name, "/", "-", 1))
On 2012/09/10 06:58:30, fwereade wrote:
> Again, didn't we agree "agents" and "kind-name"?

That will be fixed in an upcoming CL, if that's ok.

[niemeyer, 2012-09-11 18:35:20]

Glad to see the global going away. LGTM considering the following and William's
concerns on it:

https://codereview.appspot.com/6501106/diff/1001/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6501106/diff/1001/cmd/jujud/agent.go#newcode51
cmd/jujud/agent.go:51: VarDir    string
On 2012/09/10 19:53:10, rog wrote:
> On 2012/09/10 06:58:30, fwereade wrote:
> > IMO VarDir is misleading and unhelpful and strictly inferior to JujuDir in
> every
> > way. Convince me otherwise?
> 
> VarDir was a name agreed in a previous discussion. I'd be more than happy to
> change to JujuDir throughout however. Gustavo?

The context we talked about was a slightly different one, but perhaps it makes
sense here as well. The argument was that VarDir (and --var-dir, perhaps) is
better because any directory we rely on, such as ~/.juju, is a "juju dir". The
argument could make sense here too if we think that it is already an argument to
"jujud" informing the path of a directory, so it's more useful to tell what the
directory contains than th e fact it is a "juju" directory. Imagine the follow
ups: --conf-dir, --tmp-dir, whatever. If we go with this, I suggest we go the
extra mile and change --juju-directory to --var-dir.

https://codereview.appspot.com/6501106/diff/1001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6501106/diff/1001/juju/testing/conn.go#newcode22
juju/testing/conn.go:22: // mirroring the intended juju directory structure,
including
As a note, I'm getting increasingly concerned about JujuConnSuite. There's a
tendency in every project to have a jumbo hardness setup in a single place that
puts in place what every single test case may ever want, and it leads towards a
situation where tests slow down at a global pace, and where every single test is
doing a significant dance despite the fact it needs 5% of it only, or perhaps
nothing at all in many cases. Let's watch out to note go down what spiral.

https://codereview.appspot.com/6501106/diff/1001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6501106/diff/1001/worker/machiner/machiner.go#...
worker/machiner/machiner.go:28: container container.Container
On 2012/09/10 19:53:10, rog wrote:
> On 2012/09/10 06:58:30, fwereade wrote:
> > Wait, what? A machiner can only deploy to one container? Isn't this crack? I
> > suggest sticking with container.Simple for now rather than baking in what
> feels
> > like a really flaky assumption...
> 
> When LXC is added, we'll probably have "localContainer" and "lxcContainer"
here.
> how about i rename container to localContainer in anticipation of that?

This is true, but the decision is made considering settings, which means that
the API won't look like this (won't receive it through constructor), so I agree
with William that this seems premature. It's also a bit unrelated to the CL
topic.

[niemeyer, 2012-09-11 20:13:01]

Putting it back to WIP, pending mainly the changes from William's review, and
consensus around --var-dir or not.

[rog, 2012-09-12 13:54:06]

https://codereview.appspot.com/6501106/diff/1001/juju/testing/conn.go
File juju/testing/conn.go (right):

https://codereview.appspot.com/6501106/diff/1001/juju/testing/conn.go#newcode22
juju/testing/conn.go:22: // mirroring the intended juju directory structure,
including
On 2012/09/11 18:35:21, niemeyer wrote:
> As a note, I'm getting increasingly concerned about JujuConnSuite. There's a
> tendency in every project to have a jumbo hardness setup in a single place
that
> puts in place what every single test case may ever want, and it leads towards
a
> situation where tests slow down at a global pace, and where every single test
is
> doing a significant dance despite the fact it needs 5% of it only, or perhaps
> nothing at all in many cases. Let's watch out to note go down what spiral.

I am aware of that possibility, and I will keep a watchful eye in that
direction, but I don't think it's a problem with JujuConnSuite yet. All it does
besides connecting to the state (its principal function) is create a few tiny
files and directories, with minimal cost. This new documentation merely
documents existing behaviour.

[rog, 2012-09-12 13:54:35]

Please take a look.

[fwereade, 2012-09-12 13:58:20]

On 2012/09/11 20:13:01, niemeyer wrote:
> Putting it back to WIP, pending mainly the changes from William's review, and
> consensus around --var-dir or not.

FWIW, I don't really like VarDir because it sounds uncomfortably vague... and,
well, our log dir is also a var dir. How would you feel about JujuRoot?

I'd be a lot happier with VarDir if it were /var itself, and we made everything
(including the log dir) relative to that. Crack?

[niemeyer, 2012-09-12 14:11:50]

On 2012/09/12 13:58:20, fwereade wrote:
> On 2012/09/11 20:13:01, niemeyer wrote:
> > Putting it back to WIP, pending mainly the changes from William's review,
and
> > consensus around --var-dir or not.
> 
> FWIW, I don't really like VarDir because it sounds uncomfortably vague... and,
> well, our log dir is also a var dir. How would you feel about JujuRoot?

It is vague because it is truly vague. The only thing it means is "juju's
directory under var", because under it we have a bunch of different things with
more precise naming (bundles, unit containers, etc). No single precise FooDir
name will encompass it all.

> I'd be a lot happier with VarDir if it were /var itself, and we made
everything
> (including the log dir) relative to that. Crack?

Yeah, not great. LogDir should be under /var/log as usual for applications in
Linux.

[niemeyer, 2012-09-12 14:25:56]

For the record, consensus was reached on DataDir over IRC.

[fwereade, 2012-09-12 14:35:28]

On 2012/09/12 14:11:50, niemeyer wrote:
> On 2012/09/12 13:58:20, fwereade wrote:
> > I'd be a lot happier with VarDir if it were /var itself, and we made
> everything
> > (including the log dir) relative to that. Crack?
> 
> Yeah, not great. LogDir should be under /var/log as usual for applications in
> Linux.

When did I suggest it shouldn't be? /var/log/juju *is* under /var

[fwereade, 2012-09-12 15:17:26]

https://codereview.appspot.com/6501106/diff/5004/container/container.go
File container/container.go (right):

https://codereview.appspot.com/6501106/diff/5004/container/container.go#newco...
container/container.go:19: // be used.
Apropos of not-very-much, how would you feel about (separately, ofc) dropping
the default /etc/init from upstart, and demanding that it be specified here?

https://codereview.appspot.com/6501106/diff/5004/container/container.go#newco...
container/container.go:43: // TODO choose an LXC container when the unit
requires isolation.
s/choose/create/

[rog, 2012-09-12 15:22:30]

Please take a look.

[fwereade, 2012-09-12 15:30:42]

LGTM, thanks

https://codereview.appspot.com/6501106/diff/9009/container/container_test.go
File container/container_test.go (right):

https://codereview.appspot.com/6501106/diff/9009/container/container_test.go#...
container/container_test.go:29: toolsDir := environs.AgentToolsDir(dataDir,
"unit-0")
Can we make this unit-dummy-0 or something just for future consistency's sake?
Unless it's fixed in the followup and I just don;t remember, that is :).

[niemeyer, 2012-09-12 15:41:29]

https://codereview.appspot.com/6501106/diff/9009/cmd/jujud/agent.go
File cmd/jujud/agent.go (right):

https://codereview.appspot.com/6501106/diff/9009/cmd/jujud/agent.go#newcode57
cmd/jujud/agent.go:57: f.StringVar(&c.DataDir, "data-directory",
"/var/lib/juju", "directory for juju data")
Since we're changing it anyway, can we have it as --data-dir please?

https://codereview.appspot.com/6501106/diff/9009/cmd/jujud/agent.go#newcode64
cmd/jujud/agent.go:64: return requiredError("data-directory")
Ditto.

https://codereview.appspot.com/6501106/diff/9009/container/container.go
File container/container.go (right):

https://codereview.appspot.com/6501106/diff/9009/container/container.go#newco...
container/container.go:42: func Deploy(cfg Config, unit *state.Unit) (err error)
{
The change in the interface of this package is unrelated to turning VarDir into
a local variable, and the change seems to be worse than the original one.

The original interface can be used as originally planned:

    simple := container.Simple(DataDir, InitDir)

and everything else can be preserved.

LXC will have different needs, and will work equally well.

https://codereview.appspot.com/6501106/diff/9009/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6501106/diff/9009/worker/machiner/machiner.go#...
worker/machiner/machiner.go:12: var destroy = container.Destroy
Can we avoid the mocking here?  We're just replacing a global with a local..
it's not clear why we need this now.

[rog, 2012-09-12 16:46:17]

Please take a look.

https://codereview.appspot.com/6501106/diff/1001/worker/machiner/machiner.go
File worker/machiner/machiner.go (right):

https://codereview.appspot.com/6501106/diff/1001/worker/machiner/machiner.go#...
worker/machiner/machiner.go:28: container container.Container
On 2012/09/11 18:35:21, niemeyer wrote:
> On 2012/09/10 19:53:10, rog wrote:
> > On 2012/09/10 06:58:30, fwereade wrote:
> > > Wait, what? A machiner can only deploy to one container? Isn't this crack?
I
> > > suggest sticking with container.Simple for now rather than baking in what
> > feels
> > > like a really flaky assumption...
> > 
> > When LXC is added, we'll probably have "localContainer" and "lxcContainer"
> here.
> > how about i rename container to localContainer in anticipation of that?
> 
> This is true, but the decision is made considering settings, which means that
> the API won't look like this (won't receive it through constructor), so I
agree
> with William that this seems premature. It's also a bit unrelated to the CL
> topic.

reverted after discussion on line.

[niemeyer, 2012-09-12 16:51:55]

LGTM with the suggested use of --data-dir that was uncovered in the previous
review.

[rog, 2012-09-12 17:35:36]

*** Submitted:

environs: remove VarDir global

This makes it much easier to make the environs functions
work on hypothetical remote machines, for example.

Also rename VarDir to DataDir throughout.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6501106