Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow add-machine to take a -n param #198

Merged
merged 4 commits into from Jul 4, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
63 changes: 51 additions & 12 deletions cmd/juju/addmachine.go
Expand Up @@ -4,7 +4,9 @@
package main

import (
"errors"
"fmt"
"strings"

"github.com/juju/cmd"
"github.com/juju/names"
Expand Down Expand Up @@ -44,7 +46,9 @@ access the environment storage.

Examples:
juju add-machine (starts a new machine)
juju add-machine -n 2 (starts 2 new machines)
juju add-machine lxc (starts a new machine with an lxc container)
juju add-machine lxc -n 2 (starts 2 new machines with an lxc container)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work. "lxc" is a placement directive.

EDIT: ah, I see what you did now. Never mind.

juju add-machine lxc:4 (starts a new lxc container on machine 4)
juju add-machine --constraints mem=8G (starts a machine with at least 8GB RAM)
juju add-machine ssh:user@10.10.0.3 (manually provisions a machine with ssh)
Expand All @@ -62,6 +66,8 @@ type AddMachineCommand struct {
Constraints constraints.Value
// Placement is passed verbatim to the API, to be parsed and evaluated server-side.
Placement *instance.Placement

NumMachines int
}

func (c *AddMachineCommand) Info() *cmd.Info {
Expand All @@ -75,6 +81,7 @@ func (c *AddMachineCommand) Info() *cmd.Info {

func (c *AddMachineCommand) SetFlags(f *gnuflag.FlagSet) {
f.StringVar(&c.Series, "series", "", "the charm series")
f.IntVar(&c.NumMachines, "n", 1, "The number of machines to add")
f.Var(constraints.ConstraintsValue{Target: &c.Constraints}, "constraints", "additional machine constraints")
}

Expand All @@ -94,9 +101,22 @@ func (c *AddMachineCommand) Init(args []string) error {
if err != nil {
return err
}
if c.NumMachines > 1 && c.Placement != nil && c.Placement.Directive != "" {
return fmt.Errorf("cannot use -n when specifying a placement directive")
}
return nil
}

type AddMachineAPI interface {
Close() error
AddMachines([]params.AddMachineParams) ([]params.AddMachinesResult, error)
AddMachines1dot18([]params.AddMachineParams) ([]params.AddMachinesResult, error)
}

var getAddMachineAPI = func(envname string) (AddMachineAPI, error) {
return juju.NewAPIClientFromName(envname)
}

func (c *AddMachineCommand) Run(ctx *cmd.Context) error {
if c.Placement != nil && c.Placement.Scope == "ssh" {
args := manual.ProvisionMachineArgs{
Expand All @@ -110,7 +130,7 @@ func (c *AddMachineCommand) Run(ctx *cmd.Context) error {
return err
}

client, err := juju.NewAPIClientFromName(c.EnvName)
client, err := getAddMachineAPI(c.EnvName)
if err != nil {
return err
}
Expand All @@ -127,7 +147,12 @@ func (c *AddMachineCommand) Run(ctx *cmd.Context) error {
Constraints: c.Constraints,
Jobs: []params.MachineJob{params.JobHostUnits},
}
results, err := client.AddMachines([]params.AddMachineParams{machineParams})
machines := make([]params.AddMachineParams, c.NumMachines)
for i := 0; i < c.NumMachines; i++ {
machines[i] = machineParams
}

results, err := client.AddMachines(machines)
if params.IsCodeNotImplemented(err) {
if c.Placement != nil {
containerType, parseErr := instance.ParseContainerType(c.Placement.Scope)
Expand All @@ -150,17 +175,31 @@ func (c *AddMachineCommand) Run(ctx *cmd.Context) error {
return err
}

// Currently, only one machine is added, but in future there may be several added in one call.
machineInfo := results[0]
if machineInfo.Error != nil {
return machineInfo.Error
}
machineId := machineInfo.Machine
errs := []error{}
for _, machineInfo := range results {
if machineInfo.Error != nil {
errs = append(errs, machineInfo.Error)
continue
}
machineId := machineInfo.Machine

if names.IsContainerMachine(machineId) {
ctx.Infof("created container %v", machineId)
} else {
ctx.Infof("created machine %v", machineId)
if names.IsContainerMachine(machineId) {
ctx.Infof("created container %v", machineId)
} else {
ctx.Infof("created machine %v", machineId)
}
}
if len(errs) == 1 {
fmt.Fprintf(ctx.Stderr, "failed to create 1 machine\n")
return errs[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 on returning the original error if there's only one failure, but I think we need to be consistent with the error output.

I think it might be better just to print out the "failed to create N machine(s)" regardless of how many there are, and then return cmd.ErrSilent. Also, I think you want to be printing to ctx.Stderr() for errors -- they shouldn't be hidden by -q IMO.

}
if len(errs) > 1 {
fmt.Fprintf(ctx.Stderr, "failed to create %d machines\n", len(errs))
returnErr := []string{}
for _, e := range errs {
returnErr = append(returnErr, fmt.Sprintf("%s", e))
}
return errors.New(strings.Join(returnErr, ", "))
}
return nil
}
77 changes: 76 additions & 1 deletion cmd/juju/addmachine_test.go
Expand Up @@ -16,6 +16,7 @@ import (
"github.com/juju/juju/instance"
jujutesting "github.com/juju/juju/juju/testing"
"github.com/juju/juju/state"
"github.com/juju/juju/state/api/params"
"github.com/juju/juju/testing"
)

Expand Down Expand Up @@ -63,6 +64,35 @@ func (s *AddMachineSuite) TestAddMachineWithConstraints(c *gc.C) {
c.Assert(mcons, gc.DeepEquals, expectedCons)
}

func (s *AddMachineSuite) TestAddTwoMachinesWithConstraints(c *gc.C) {
context, err := runAddMachine(c, "--constraints", "mem=4G", "-n", "2")
c.Assert(err, gc.IsNil)
c.Assert(testing.Stderr(context), gc.Equals, "created machine 0\ncreated machine 1\n")
for i := 0; i < 2; i++ {
m, err := s.State.Machine(strconv.Itoa(i))
c.Assert(err, gc.IsNil)
mcons, err := m.Constraints()
c.Assert(err, gc.IsNil)
expectedCons := constraints.MustParse("mem=4G")
c.Assert(mcons, gc.DeepEquals, expectedCons)
}
}

func (s *AddMachineSuite) TestAddTwoMachinesWithContainers(c *gc.C) {
context, err := runAddMachine(c, "lxc", "-n", "2")
c.Assert(err, gc.IsNil)
c.Assert(testing.Stderr(context), gc.Equals, "created container 0/lxc/0\ncreated container 1/lxc/0\n")
for i := 0; i < 2; i++ {
machine := fmt.Sprintf("%d/%s/0", i, instance.LXC)
s._assertAddContainer(c, strconv.Itoa(i), machine, instance.LXC)
}
}

func (s *AddMachineSuite) TestAddTwoMachinesWithContainerDirective(c *gc.C) {
_, err := runAddMachine(c, "lxc:1", "-n", "2")
c.Assert(err, gc.ErrorMatches, "cannot use -n when specifying a placement directive")
}

func (s *AddMachineSuite) _assertAddContainer(c *gc.C, parentId, containerId string, ctype instance.ContainerType) {
m, err := s.State.Machine(parentId)
c.Assert(err, gc.IsNil)
Expand Down Expand Up @@ -114,7 +144,7 @@ func (s *AddMachineSuite) TestAddUnsupportedContainerToMachine(c *gc.C) {
m.SetSupportedContainers([]instance.ContainerType{instance.KVM})
context, err = runAddMachine(c, "lxc:0")
c.Assert(err, gc.ErrorMatches, "cannot add a new machine: machine 0 cannot host lxc containers")
c.Assert(testing.Stderr(context), gc.Equals, "")
c.Assert(testing.Stderr(context), gc.Equals, "failed to create 1 machine\n")
}

func (s *AddMachineSuite) TestAddMachineErrors(c *gc.C) {
Expand All @@ -133,3 +163,48 @@ func (s *AddMachineSuite) TestAddMachineErrors(c *gc.C) {
_, err = runAddMachine(c, "lxc", "--constraints", "container=lxc")
c.Check(err, gc.ErrorMatches, `container constraint "lxc" not allowed when adding a machine`)
}

func (s *AddMachineSuite) TestAddThreeMachinesWithTwoFailures(c *gc.C) {
fakeApi := fakeAddMachineAPI{}
s.PatchValue(&getAddMachineAPI, func(envName string) (AddMachineAPI, error) {
return &fakeApi, nil
})
fakeApi.successOrder = []bool{true, false, false}
expectedOutput := `created machine 0
failed to create 2 machines
`
context, err := runAddMachine(c, "-n", "3")
c.Assert(err, gc.ErrorMatches, "something went wrong, something went wrong")
c.Assert(testing.Stderr(context), gc.Equals, expectedOutput)
}

type fakeAddMachineAPI struct {
successOrder []bool
currentOp int
}

func (f *fakeAddMachineAPI) Close() error {
return nil
}

func (f *fakeAddMachineAPI) AddMachines(args []params.AddMachineParams) ([]params.AddMachinesResult, error) {
results := []params.AddMachinesResult{}
for i := range args {
if f.successOrder[i] {
results = append(results, params.AddMachinesResult{
Machine: strconv.Itoa(i),
Error: nil,
})
} else {
results = append(results, params.AddMachinesResult{
Machine: string(i),
Error: &params.Error{"something went wrong", "1"},
})
}
f.currentOp++
}
return results, nil
}
func (f *fakeAddMachineAPI) AddMachines1dot18(args []params.AddMachineParams) ([]params.AddMachinesResult, error) {
return f.AddMachines(args)
}