environs/ec2: make format strings govet friendly

environs/ec2/cloudinit.go

 package ec2
 
 import (
         "fmt"
         "launchpad.net/juju-core/cloudinit"
         "launchpad.net/juju-core/log"
         "launchpad.net/juju-core/state"
         "launchpad.net/juju-core/upstart"
         "path"
         "strings"
 )
 
 // machineConfig represents initialization information for a new juju machine.
 // Creation of cloudinit data from this struct is largely provider-independent,
 // but we'll keep it internal until we need to factor it out.
 type machineConfig struct {
         // provisioner specifies whether the new machine will run a provisioning agent.
         provisioner bool
 
+        // machiner specifies whether the new machine will run a machine agent.
+        machiner bool

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

I think this field is redundant: we should always run a machine agent.

[dfc, 2012/07/17 00:53:22]

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

+
         // zookeeper specifies whether the new machine will run a zookeeper instance.
         zookeeper bool
 
         // instanceIdAccessor holds bash code that evaluates to the current instance id.
         instanceIdAccessor string
 
         // providerType identifies the provider type so the host
         // knows which kind of provider to use.
         providerType string
 
@@ skipping 76 matching lines @@
         if cfg.zookeeper {
                 addScripts(c,
                         jujutools+"/jujud initzk"+
                                 " --instance-id "+cfg.instanceIdAccessor+
                                 " --env-type "+shquote(cfg.providerType)+
                                 " --zookeeper-servers localhost"+zkPortSuffix+
                                 debugFlag,
                 )
         }
 
-        // TODO start machine agent

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

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...)

[dfc, 2012/07/17 00:53:22]

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.

[dfc, 2012/07/17 00:53:22]

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 :)

-
         if cfg.provisioner {
                 svc := upstart.NewService("jujud-provisioning")
                 // 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 provisioning agent",
                         Cmd: jujutools + "/jujud provisioning" +
                                 " --zookeeper-servers " + fmt.Sprintf("'%s'", cfg.zookeeperHostAddrs()) +
                                 " --log-file /var/log/juju/provision-agent.log" +
                                 debugFlag,
                 }
                 cmds, err := conf.InstallCommands()
                 if err != nil {
                         return nil, fmt.Errorf("cannot make cloudinit provisioning agent upstart script: %v", err)
                 }
                 addScripts(c, cmds...)
         }
 
+        if cfg.machiner {
+                svc := upstart.NewService("jujud-machine-agent")
+                // TODO(rogerpeppe) change upstart.Conf.Cmd to []string so that
+                // we don't have to second-guess upstart's quoting rules.

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

Not really relevant to this CL, but don't we have to guess upstart's quoting
rules regardless?

[dfc, 2012/07/17 00:53:22]

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

+                conf := &upstart.Conf{

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

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
}

[dfc, 2012/07/17 00:53:22]

Done.

+                        Service: *svc,
+                        Desc:    "juju machine agent",
+                        Cmd: jujutools + "/jujud machine" +
+                                fmt.Sprintf(" --zookeeper-servers '%s'", cfg.zookeeperHostAddrs()) +
+                                fmt.Sprintf(" --machine-id %d", cfg.machineId) +
+                                " --log-file /var/log/juju/machine-agent.log" +
+                                debugFlag,
+                }
+                cmds, err := conf.InstallCommands()
+                if err != nil {
+                        return nil, fmt.Errorf("cannot make cloudinit machine agent upstart script: %v", err)
+                }
+                addScripts(c, cmds...)
+        }
+
         // general options
+
+        // general options
         c.SetAptUpgrade(true)
         c.SetAptUpdate(true)
         c.SetOutput(cloudinit.OutAll, "| tee -a /var/log/cloud-init-output.log", "")
         return c, nil
 }
 
 // versionDir converts a tools URL into a name
 // to use as a directory for storing the tools executables in
 // by using the last element stripped of its extension.
 func versionDir(toolsURL string) string {
@@ skipping 34 matching lines @@
                 if cfg.instanceIdAccessor == "" {
                         return requiresError("instance id accessor")
                 }
         } else {
                 if cfg.stateInfo == nil || len(cfg.stateInfo.Addrs) == 0 {
                         return requiresError("zookeeper hosts")
                 }
         }
         return nil
 }