environs/ec2: make format strings govet friendly

environs/ec2: make format strings govet friendly

This proposal was a follow on request from

https://code.launchpad.net/~dave-cheney/juju-core/go-environs-ec2-bootstrap-govet/+merge/115527

(do not edit description out of merge proposal)

[dave_cheney.net, 2012-07-16 01:48:20]

Please take a look.

[rog, 2012-07-16 11:29:39]

LGTM modulo comment below.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:142: conf := &upstart.Conf{
i think it's worth factoring out this code, especially as we've got the unit
agent still to come.

here's what i did in my branch:


func (cfg *machineConfig) jujuTools() string {
	return "/var/lib/juju/tools/" + versionDir(cfg.toolsURL)
}

func newCloudInit(cfg *machineConfig) (*cloudinit.Config, error) {
	if err := verifyConfig(cfg); err != nil {
		return nil, err
	}
	c := cloudinit.New()

	c.AddSSHAuthorizedKeys(cfg.authorizedKeys)
	if cfg.zookeeper {
		pkgs := []string{
			"default-jre-headless",
			"zookeeper",
			"zookeeperd",
			"libzookeeper-mt2",
		}
		for _, pkg := range pkgs {
			c.AddPackage(pkg)
		}
	}

	addScripts(c,
		"sudo mkdir -p /var/lib/juju",
		"sudo mkdir -p /var/log/juju")

	// Make a directory for the tools to live in, then fetch the
	// tools and unarchive them into it.
	addScripts(c,
		"bin="+shquote(cfg.jujuTools()),
		"mkdir -p $bin",
		fmt.Sprintf("wget -O - %s | tar xz -C $bin", shquote(cfg.toolsURL)),
	)

	addScripts(c,
		"JUJU_ZOOKEEPER="+shquote(cfg.zookeeperHostAddrs()),
		fmt.Sprintf("JUJU_MACHINE_ID=%d", cfg.machineId),
	)

	debugFlag := ""
	if log.Debug {
		debugFlag = " --debug"
	}

	// zookeeper scripts
	if cfg.zookeeper {
		addScripts(c,
			cfg.jujuTools()+"/jujud initzk"+
				" --instance-id "+cfg.instanceIdAccessor+
				" --env-type "+shquote(cfg.providerType)+
				" --zookeeper-servers localhost"+zkPortSuffix+
				debugFlag,
		)
	}

	if err := addAgentScript(c, cfg, "machine", debugFlag); err != nil {
		return nil, err
	}
	if cfg.provisioner {
		if err := addAgentScript(c, cfg, "provisioning", debugFlag); err != nil {
			return nil, err
		}
	}

	// general options
	c.SetAptUpgrade(true)
	c.SetAptUpdate(true)
	c.SetOutput(cloudinit.OutAll, "| tee -a /var/log/cloud-init-output.log", "")
	return c, nil
}

func addAgentScript(c *cloudinit.Config, cfg *machineConfig, name, args string)
error {
	svc := upstart.NewService("jujud-" + name)
	// TODO(rogerpeppe) change upstart.Conf.Cmd to []string so that
	// we don't have to second-guess upstart's quoting rules.
	conf := &upstart.Conf{
		Service: *svc,
		Desc:    "juju " + name + " agent",
		Cmd: cfg.jujuTools() + "/jujud " + name +
			" --zookeeper-servers " + fmt.Sprintf("'%s'", cfg.zookeeperHostAddrs()) +
			" --log-file /var/log/juju/provision-agent.log",
	}
	if args != "" {
		conf.Cmd += " " + args
	}
	cmds, err := conf.InstallCommands()
	if err != nil {
		return fmt.Errorf("cannot make cloudinit %s agent upstart script: %v", name,
err)
	}
	addScripts(c, cmds...)
	return nil
}

[fwereade, 2012-07-16 23:07:36]

LGTM modulo below.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (left):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ol...
environs/ec2/cloudinit.go:116: // TODO start machine agent
Total bikeshedding: the MA feels to me more fundamental than the PA, and should
therefore be started first.

Digression/expansion:

Hm, is there any particular reason the MA isn't itself responsible for starting
a PA itself, just as it starts UAs? This might actually be cleaner overall:
rather than the is-machine-0 hacks that have started to insinuate their way into
the go code, it would *surely* be better to have a runs-provisioner field in
machine state: we could use that to determine machine-specialness in a far
neater way. And, then, the MA could start the PA itself, just as it starts UAs;
this feels like the sort of thing that "should" be the MA's job.

In fact, double hmm: I'm not proposing this seriously right now, but it doesn't
even feel like it would be especially hard to implement the PA as a charm and
run it as a unit anyway.

Further digression: as authorized-keys makes its way usefully into /environment
(rather than just being a hacked-up one-time setup thing) we really ought to
just make the machine agent responsible for watching and replacing the machine's
authorized-keys as it changes; if we did that, we might plausibly be able to
drop it entirely from cloud-init time setup. (Although maybe we shouldn't,
because a machine agent that didn't run would leave the instance
inaccessible...)

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:21: machiner bool
I think this field is redundant: we should always run a machine agent.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:141: // we don't have to second-guess upstart's
quoting rules.
Not really relevant to this CL, but don't we have to guess upstart's quoting
rules regardless?

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit_test.go
File environs/ec2/cloudinit_test.go (right):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:72: t.checkScripts(c, "--machine-id [0-9]+")
Shouldn't we be checking --machine-id is part of the same script that launches
the machine agent? Or are you doing something smart that I've missed?

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:75: t.checkScripts(c, "--machine-id [0-9]+")
Ditto

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/ec2.go#newcode267
environs/ec2/ec2.go:267: machiner:           true,
Do we *ever* want machiner to be false?

[dave_cheney.net, 2012-07-17 00:53:22]

Please take a look.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (left):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ol...
environs/ec2/cloudinit.go:116: // TODO start machine agent
On 2012/07/16 23:07:36, fwereade wrote:
> Total bikeshedding: the MA feels to me more fundamental than the PA, and
should
> therefore be started first.

I thought so too, but when I raised the question at last weeks video conference
the prevailing view was using the MA to spawn a UA to run a PA as a service was
off the cards for the moment. Please correct me if I misinterpreted that.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ol...
environs/ec2/cloudinit.go:116: // TODO start machine agent
> Digression/expansion:
> 
> Hm, is there any particular reason the MA isn't itself responsible for
starting
> a PA itself, just as it starts UAs? This might actually be cleaner overall:
> rather than the is-machine-0 hacks that have started to insinuate their way
into
> the go code, it would *surely* be better to have a runs-provisioner field in
> machine state: we could use that to determine machine-specialness in a far
> neater way. And, then, the MA could start the PA itself, just as it starts
UAs;
> this feels like the sort of thing that "should" be the MA's job.
> 
> In fact, double hmm: I'm not proposing this seriously right now, but it
doesn't
> even feel like it would be especially hard to implement the PA as a charm and
> run it as a unit anyway.
> 
> Further digression: as authorized-keys makes its way usefully into
/environment
> (rather than just being a hacked-up one-time setup thing) we really ought to
> just make the machine agent responsible for watching and replacing the
machine's
> authorized-keys as it changes; if we did that, we might plausibly be able to
> drop it entirely from cloud-init time setup. (Although maybe we shouldn't,
> because a machine agent that didn't run would leave the instance
> inaccessible...)

I think this was discussed as far back as UDS-Q. Please raise a bug to have this
implemented, you've got a presumptive +1 from me :)

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:21: machiner bool
On 2012/07/16 23:07:36, fwereade wrote:
> I think this field is redundant: we should always run a machine agent.

SGTM. We can always add it back if we find a host that doesn't need an MA.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:141: // we don't have to second-guess upstart's
quoting rules.
On 2012/07/16 23:07:36, fwereade wrote:
> Not really relevant to this CL, but don't we have to guess upstart's quoting
> rules regardless?

Rog ? Should I just drop these two TODO's, or open a bug for them ?

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:142: conf := &upstart.Conf{
On 2012/07/16 11:29:39, rog wrote:
> i think it's worth factoring out this code, especially as we've got the unit
> agent still to come.
> 
> here's what i did in my branch:
> 
> 
> func (cfg *machineConfig) jujuTools() string {
> 	return "/var/lib/juju/tools/" + versionDir(cfg.toolsURL)
> }
> 
> func newCloudInit(cfg *machineConfig) (*cloudinit.Config, error) {
> 	if err := verifyConfig(cfg); err != nil {
> 		return nil, err
> 	}
> 	c := cloudinit.New()
> 
> 	c.AddSSHAuthorizedKeys(cfg.authorizedKeys)
> 	if cfg.zookeeper {
> 		pkgs := []string{
> 			"default-jre-headless",
> 			"zookeeper",
> 			"zookeeperd",
> 			"libzookeeper-mt2",
> 		}
> 		for _, pkg := range pkgs {
> 			c.AddPackage(pkg)
> 		}
> 	}
> 
> 	addScripts(c,
> 		"sudo mkdir -p /var/lib/juju",
> 		"sudo mkdir -p /var/log/juju")
> 
> 	// Make a directory for the tools to live in, then fetch the
> 	// tools and unarchive them into it.
> 	addScripts(c,
> 		"bin="+shquote(cfg.jujuTools()),
> 		"mkdir -p $bin",
> 		fmt.Sprintf("wget -O - %s | tar xz -C $bin", shquote(cfg.toolsURL)),
> 	)
> 
> 	addScripts(c,
> 		"JUJU_ZOOKEEPER="+shquote(cfg.zookeeperHostAddrs()),
> 		fmt.Sprintf("JUJU_MACHINE_ID=%d", cfg.machineId),
> 	)
> 
> 	debugFlag := ""
> 	if log.Debug {
> 		debugFlag = " --debug"
> 	}
> 
> 	// zookeeper scripts
> 	if cfg.zookeeper {
> 		addScripts(c,
> 			cfg.jujuTools()+"/jujud initzk"+
> 				" --instance-id "+cfg.instanceIdAccessor+
> 				" --env-type "+shquote(cfg.providerType)+
> 				" --zookeeper-servers localhost"+zkPortSuffix+
> 				debugFlag,
> 		)
> 	}
> 
> 	if err := addAgentScript(c, cfg, "machine", debugFlag); err != nil {
> 		return nil, err
> 	}
> 	if cfg.provisioner {
> 		if err := addAgentScript(c, cfg, "provisioning", debugFlag); err != nil {
> 			return nil, err
> 		}
> 	}
> 
> 	// general options
> 	c.SetAptUpgrade(true)
> 	c.SetAptUpdate(true)
> 	c.SetOutput(cloudinit.OutAll, "| tee -a /var/log/cloud-init-output.log", "")
> 	return c, nil
> }
> 
> func addAgentScript(c *cloudinit.Config, cfg *machineConfig, name, args
string)
> error {
> 	svc := upstart.NewService("jujud-" + name)
> 	// TODO(rogerpeppe) change upstart.Conf.Cmd to []string so that
> 	// we don't have to second-guess upstart's quoting rules.
> 	conf := &upstart.Conf{
> 		Service: *svc,
> 		Desc:    "juju " + name + " agent",
> 		Cmd: cfg.jujuTools() + "/jujud " + name +
> 			" --zookeeper-servers " + fmt.Sprintf("'%s'", cfg.zookeeperHostAddrs()) +
> 			" --log-file /var/log/juju/provision-agent.log",
> 	}
> 	if args != "" {
> 		conf.Cmd += " " + args
> 	}
> 	cmds, err := conf.InstallCommands()
> 	if err != nil {
> 		return fmt.Errorf("cannot make cloudinit %s agent upstart script: %v", name,
> err)
> 	}
> 	addScripts(c, cmds...)
> 	return nil
> }

Done.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit_test.go
File environs/ec2/cloudinit_test.go (right):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:72: t.checkScripts(c, "--machine-id [0-9]+")
On 2012/07/16 23:07:36, fwereade wrote:
> Shouldn't we be checking --machine-id is part of the same script that launches
> the machine agent? Or are you doing something smart that I've missed?

No, nothing that smart. I'll check the command starts with jujud machine.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/cloudinit_test....
environs/ec2/cloudinit_test.go:75: t.checkScripts(c, "--machine-id [0-9]+")
On 2012/07/16 23:07:36, fwereade wrote:
> Ditto

Done.

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6405046/diff/2001/environs/ec2/ec2.go#newcode267
environs/ec2/ec2.go:267: machiner:           true,
On 2012/07/16 23:07:36, fwereade wrote:
> Do we *ever* want machiner to be false?

Done.

[dave_cheney.net, 2012-07-17 23:21:23]

Please take a look.

[niemeyer, 2012-07-18 00:33:20]

LGTM.. only a couple of trivials:

https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:142: args),
This is getting a bit unreadable. I suggest reorganizing it creating the
parameter for the Cmd field upfront:

format := "%s/jujud %s --zookeeper-servers '%s' --log-file
/var/log/juju/%s-agent.log %s"
cmd := fmt.Sprintf(format, cfg.jujuTools(), name, cfg.zookeeperHostAddrs(),
name, args)

https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:146: return fmt.Errorf("cannot make cloudinit %s agent
upstart script: %v", name, err)
Nitpick: s/cloudinit/cloud-init/ (that's the software name)

[niemeyer, 2012-07-18 00:35:11]

Btw, I also think the quoting of Cmd is a red-herring.

[dave_cheney.net, 2012-07-18 00:36:50]

> environs/ec2/cloudinit.go:146: return fmt.Errorf("cannot make cloudinit %s
agent
> upstart script: %v", name, err)
> Nitpick: s/cloudinit/cloud-init/ (that's the software name)

I will remove that TODO, it's caused more raised eyebrows that it is worth.

[dave_cheney.net, 2012-07-18 01:11:51]

*** Submitted:

environs/ec2: start machine agent on machine/0

Start a machine agent on the bootstrap node. 

TODO(dfc) add tests to ssh to the host to confirm 
machine agent started correctly.

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

https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:142: args),
On 2012/07/18 00:33:20, niemeyer wrote:
> This is getting a bit unreadable. I suggest reorganizing it creating the
> parameter for the Cmd field upfront:
> 
> format := "%s/jujud %s --zookeeper-servers '%s' --log-file
> /var/log/juju/%s-agent.log %s"
> cmd := fmt.Sprintf(format, cfg.jujuTools(), name, cfg.zookeeperHostAddrs(),
> name, args)

Nice, done.

https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:146: return fmt.Errorf("cannot make cloudinit %s agent
upstart script: %v", name, err)
On 2012/07/18 00:33:20, niemeyer wrote:
> Nitpick: s/cloudinit/cloud-init/ (that's the software name)

Done.

[rog, 2012-07-18 09:14:08]

https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go
File environs/ec2/cloudinit.go (right):

https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go#ne...
environs/ec2/cloudinit.go:142: args),
On 2012/07/18 00:33:20, niemeyer wrote:
> This is getting a bit unreadable. I suggest reorganizing it creating the
> parameter for the Cmd field upfront:
> 
> format := "%s/jujud %s --zookeeper-servers '%s' --log-file
> /var/log/juju/%s-agent.log %s"
> cmd := fmt.Sprintf(format, cfg.jujuTools(), name, cfg.zookeeperHostAddrs(),
> name, args)

can I suggest that the format parameter is kept as an arg to Sprintf, so it's
amenable to checking by govet?

perhaps move the Sprintf line outside the struct initialisation.

[dave_cheney.net, 2012-07-18 09:19:19]

I can do that, but we have a unit test that will blow up if we screw
up the format string. Is that sufficient ?

On Wed, Jul 18, 2012 at 7:14 PM,  <rogpeppe@gmail.com> wrote:
>
> https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go
> File environs/ec2/cloudinit.go (right):
>
>
https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go#ne...
> environs/ec2/cloudinit.go:142: args),
> On 2012/07/18 00:33:20, niemeyer wrote:
>>
>> This is getting a bit unreadable. I suggest reorganizing it creating
>
> the
>>
>> parameter for the Cmd field upfront:
>
>
>> format := "%s/jujud %s --zookeeper-servers '%s' --log-file
>> /var/log/juju/%s-agent.log %s"
>> cmd := fmt.Sprintf(format, cfg.jujuTools(), name,
>
> cfg.zookeeperHostAddrs(),
>>
>> name, args)
>
>
> can I suggest that the format parameter is kept as an arg to Sprintf, so
> it's amenable to checking by govet?
>
> perhaps move the Sprintf line outside the struct initialisation.
>
> https://codereview.appspot.com/6405046/

[rog, 2012-07-18 11:12:01]

On 18 July 2012 10:19, Dave Cheney <dave@cheney.net> wrote:
> I can do that, but we have a unit test that will blow up if we screw
> up the format string. Is that sufficient ?

i'd prefer to let govet do its job; and it's nice to keep the format string
and the arguments close together anyway.

something like this might look nice, with the arguments reading
in parallel with the format:

      cmd := fmt.Sprintf(
              "%s/jujud %s --zookeeper-servers '%s' --log-file
/var/log/juju/%s-agent.log %s",
              cfg.jujuTools(), name, cfg.zookeeperHostAddrs(), name, args),

> On Wed, Jul 18, 2012 at 7:14 PM,  <rogpeppe@gmail.com> wrote:
>>
>> https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go
>> File environs/ec2/cloudinit.go (right):
>>
>>
https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go#ne...
>> environs/ec2/cloudinit.go:142: args),
>> On 2012/07/18 00:33:20, niemeyer wrote:
>>>
>>> This is getting a bit unreadable. I suggest reorganizing it creating
>>
>> the
>>>
>>> parameter for the Cmd field upfront:
>>
>>
>>> format := "%s/jujud %s --zookeeper-servers '%s' --log-file
>>> /var/log/juju/%s-agent.log %s"
>>> cmd := fmt.Sprintf(format, cfg.jujuTools(), name,
>>
>> cfg.zookeeperHostAddrs(),
>>>
>>> name, args)
>>
>>
>> can I suggest that the format parameter is kept as an arg to Sprintf, so
>> it's amenable to checking by govet?
>>
>> perhaps move the Sprintf line outside the struct initialisation.
>>
>> https://codereview.appspot.com/6405046/

[dave_cheney.net, 2012-07-18 11:20:56]

np, i'll do a branch now.

On Wed, Jul 18, 2012 at 9:12 PM, roger peppe <rogpeppe@gmail.com> wrote:
> On 18 July 2012 10:19, Dave Cheney <dave@cheney.net> wrote:
>> I can do that, but we have a unit test that will blow up if we screw
>> up the format string. Is that sufficient ?
>
> i'd prefer to let govet do its job; and it's nice to keep the format string
> and the arguments close together anyway.
>
> something like this might look nice, with the arguments reading
> in parallel with the format:
>
>       cmd := fmt.Sprintf(
>               "%s/jujud %s --zookeeper-servers '%s' --log-file
> /var/log/juju/%s-agent.log %s",
>               cfg.jujuTools(), name, cfg.zookeeperHostAddrs(), name, args),
>
>> On Wed, Jul 18, 2012 at 7:14 PM,  <rogpeppe@gmail.com> wrote:
>>>
>>> https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go
>>> File environs/ec2/cloudinit.go (right):
>>>
>>>
https://codereview.appspot.com/6405046/diff/9001/environs/ec2/cloudinit.go#ne...
>>> environs/ec2/cloudinit.go:142: args),
>>> On 2012/07/18 00:33:20, niemeyer wrote:
>>>>
>>>> This is getting a bit unreadable. I suggest reorganizing it creating
>>>
>>> the
>>>>
>>>> parameter for the Cmd field upfront:
>>>
>>>
>>>> format := "%s/jujud %s --zookeeper-servers '%s' --log-file
>>>> /var/log/juju/%s-agent.log %s"
>>>> cmd := fmt.Sprintf(format, cfg.jujuTools(), name,
>>>
>>> cfg.zookeeperHostAddrs(),
>>>>
>>>> name, args)
>>>
>>>
>>> can I suggest that the format parameter is kept as an arg to Sprintf, so
>>> it's amenable to checking by govet?
>>>
>>> perhaps move the Sprintf line outside the struct initialisation.
>>>
>>> https://codereview.appspot.com/6405046/

[dave_cheney.net, 2012-07-18 11:26:15]

Please take a look.

[dave_cheney.net, 2012-07-18 11:28:13]

*** abandoned *** lbox screwup

On Wed, Jul 18, 2012 at 9:26 PM,  <dave@cheney.net> wrote:
> Please take a look.
>
> https://codereview.appspot.com/6405046/