From 75b60886e6f698845db93e1cd9db89e0ebcb4398 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:16:17 +0100 Subject: [PATCH 01/18] fleetctl: add tryWaitForUnitStates() and getBlockAttempts() * tryWaitForUnitStates() tries to wait for units to reach the desired state. * getBlockAttempts() gets the correct value of how many attempts to try before giving up on an operation. These helpers will be used to make the code more consistent and clean. We do not intended to change any behaviour here. --- --- fleetctl/fleetctl.go | 50 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 951536f20..caae29c71 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -745,6 +745,56 @@ func setTargetStateOfUnits(units []string, state job.JobState) ([]*schema.Unit, return triggered, nil } +// getBlockAttempts gets the correct value of how many attempts to try +// before giving up on an operation. +// It returns a negative value which means try forever, if zero is +// returned then do not make any attempt, and if a positive value is +// returned then try up to that value +func getBlockAttempts() int { + // By default we wait forever + var attempts int = -1 + + // Up to BlockAttempts + if sharedFlags.BlockAttempts > 0 { + attempts = sharedFlags.BlockAttempts + } + + // NoBlock we do not wait + if sharedFlags.NoBlock { + attempts = 0 + } + + return attempts +} + +// tryWaitForUnitStates tries to wait for units to reach the desired state. +// It takes 5 arguments, the units to wait for, the desired state, the +// desired JobState, how many attempts before timing out and a writer +// interface. +// tryWaitForUnitStates polls each of the indicated units until they +// reach the desired state. If maxAttempts is zero, then it will not +// wait, it will assume that all units reached their desired state. +// If maxAttempts is negative tryWaitForUnitStates will retry forever, and +// if it is greater than zero, it will retry up to the indicated value. +// It returns 0 on success or 1 on errors. +func tryWaitForUnitStates(units []string, state string, js job.JobState, maxAttempts int, out io.Writer) (ret int) { + // We do not wait just assume we reached the desired state + if maxAttempts == 0 { + for _, name := range units { + stdout("Triggered unit %s %s", name, state) + } + return + } + + errchan := waitForUnitStates(units, js, maxAttempts, out) + for err := range errchan { + stderr("Error waiting for units: %v", err) + ret = 1 + } + + return +} + // waitForUnitStates polls each of the indicated units until each of their // states is equal to that which the caller indicates, or until the // polling operation times out. waitForUnitStates will retry forever, or From ea24c8c4deb387d8e3247fb22ee14384792cf9ad Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:17:47 +0100 Subject: [PATCH 02/18] load: Use the new tryWaitForUnitStates() and getBlockAttempts() --- fleetctl/load.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/fleetctl/load.go b/fleetctl/load.go index 497f86058..8659a1498 100644 --- a/fleetctl/load.go +++ b/fleetctl/load.go @@ -46,6 +46,8 @@ func init() { } func runLoadUnits(args []string) (exit int) { + attempts := getBlockAttempts() + if err := lazyCreateUnits(args); err != nil { stderr("Error creating units: %v", err) return 1 @@ -66,17 +68,7 @@ func runLoadUnits(args []string) (exit int) { } } - if !sharedFlags.NoBlock { - errchan := waitForUnitStates(loading, job.JobStateLoaded, sharedFlags.BlockAttempts, os.Stdout) - for err := range errchan { - stderr("Error waiting for units: %v", err) - exit = 1 - } - } else { - for _, name := range loading { - stdout("Triggered unit %s load", name) - } - } + exit = tryWaitForUnitStates(loading, "load", job.JobStateLoaded, attempts, os.Stdout) return } From 62d5b6a23988b48d1ad96c9b83d138644eaec2de Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:28:14 +0100 Subject: [PATCH 03/18] start: Use the new tryWaitForUnitStates() and getBlockAttempts() --- fleetctl/start.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/fleetctl/start.go b/fleetctl/start.go index 77ea0d343..17c8a53da 100644 --- a/fleetctl/start.go +++ b/fleetctl/start.go @@ -54,6 +54,8 @@ func init() { } func runStartUnit(args []string) (exit int) { + attempts := getBlockAttempts() + if err := lazyCreateUnits(args); err != nil { stderr("Error creating units: %v", err) return 1 @@ -74,17 +76,7 @@ func runStartUnit(args []string) (exit int) { } } - if !sharedFlags.NoBlock { - errchan := waitForUnitStates(starting, job.JobStateLaunched, sharedFlags.BlockAttempts, os.Stdout) - for err := range errchan { - stderr("Error waiting for units: %v", err) - exit = 1 - } - } else { - for _, name := range starting { - stdout("Triggered unit %s start", name) - } - } + exit = tryWaitForUnitStates(starting, "start", job.JobStateLaunched, attempts, os.Stdout) return } From 2e90705f0c5bd08ca4b84c8e7e47430a468c671a Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:28:29 +0100 Subject: [PATCH 04/18] stop: Use the new tryWaitForUnitStates() and getBlockAttempts() --- fleetctl/stop.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/fleetctl/stop.go b/fleetctl/stop.go index 262946bfb..c63904107 100644 --- a/fleetctl/stop.go +++ b/fleetctl/stop.go @@ -52,6 +52,8 @@ func init() { } func runStopUnit(args []string) (exit int) { + attempts := getBlockAttempts() + units, err := findUnits(args) if err != nil { stderr("%v", err) @@ -79,17 +81,7 @@ func runStopUnit(args []string) (exit int) { } } - if !sharedFlags.NoBlock { - errchan := waitForUnitStates(stopping, job.JobStateLoaded, sharedFlags.BlockAttempts, os.Stdout) - for err := range errchan { - stderr("Error waiting for units: %v", err) - exit = 1 - } - } else { - for _, name := range stopping { - stdout("Triggered unit %s stop", name) - } - } + exit = tryWaitForUnitStates(stopping, "stop", job.JobStateLoaded, attempts, os.Stdout) return } From 411940eb5e66c3600a35cf745b60a4d24c263752 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:28:40 +0100 Subject: [PATCH 05/18] unload: Use the new tryWaitForUnitStates() and getBlockAttempts() --- fleetctl/unload.go | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/fleetctl/unload.go b/fleetctl/unload.go index 6758f2825..c9700496a 100644 --- a/fleetctl/unload.go +++ b/fleetctl/unload.go @@ -36,6 +36,8 @@ func init() { } func runUnloadUnit(args []string) (exit int) { + attempts := getBlockAttempts() + units, err := findUnits(args) if err != nil { stderr("%v", err) @@ -60,17 +62,7 @@ func runUnloadUnit(args []string) (exit int) { } } - if !sharedFlags.NoBlock { - errchan := waitForUnitStates(wait, job.JobStateInactive, sharedFlags.BlockAttempts, os.Stdout) - for err := range errchan { - stderr("Error waiting for units: %v", err) - exit = 1 - } - } else { - for _, name := range wait { - stdout("Triggered unit %s unload", name) - } - } + exit = tryWaitForUnitStates(wait, "unload", job.JobStateInactive, attempts, os.Stdout) return } From d7787d02415d2603f6d5cc7363f979dfb0f1e584 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:44:00 +0100 Subject: [PATCH 06/18] fleetctl: move logic that lookus up a unit from a template into its own function --- fleetctl/fleetctl.go | 66 +++++++++++++++++++++++++++++--------------- 1 file changed, 43 insertions(+), 23 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index caae29c71..30098b49e 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -496,6 +496,47 @@ func getUnitFromFile(file string) (*unit.UnitFile, error) { return unit.NewUnitFile(string(out)) } +// getUnitFileFromTemplate checks if the name appears to be an instance unit +// and gets its corresponding template unit from the registry or local disk +// It returns the Unit or nil; and any error encountered +func getUnitFileFromTemplate(arg string) (*unit.UnitFile, error) { + var uf *unit.UnitFile + name := unitNameMangle(arg) + + // Check if the name appears to be an instance unit, and if so, + // check for a corresponding template unit in the Registry + uni := unit.NewUnitNameInfo(name) + if uni == nil { + return nil, fmt.Errorf("error extracting information from unit name %s", name) + } else if !uni.IsInstance() { + return nil, fmt.Errorf("unable to find Unit(%s) in Registry or on filesystem", name) + } + + tmpl, err := cAPI.Unit(uni.Template) + if err != nil { + return nil, fmt.Errorf("error retrieving template Unit(%s) from Registry: %v", uni.Template, err) + } + + // Finally, if we could not find a template unit in the Registry, + // check the local disk for one instead + if tmpl == nil { + file := path.Join(path.Dir(arg), uni.Template) + if _, err := os.Stat(file); os.IsNotExist(err) { + return nil, fmt.Errorf("unable to find Unit(%s) or template Unit(%s) in Registry or on filesystem", name, uni.Template) + } + + uf, err = getUnitFromFile(file) + if err != nil { + return nil, fmt.Errorf("failed getting template Unit(%s) from file: %v", uni.Template, err) + } + } else { + warnOnDifferentLocalUnit(arg, tmpl) + uf = schema.MapSchemaUnitOptionsToUnitFile(tmpl.Options) + } + + return uf, nil +} + func getTunnelFlag() string { tun := globalFlags.Tunnel if tun != "" && !strings.Contains(tun, ":") { @@ -625,30 +666,9 @@ func lazyCreateUnits(args []string) error { } else { // Otherwise (if the unit file does not exist), check if the name appears to be an instance unit, // and if so, check for a corresponding template unit in the Registry - uni := unit.NewUnitNameInfo(name) - if uni == nil { - return fmt.Errorf("error extracting information from unit name %s", name) - } else if !uni.IsInstance() { - return fmt.Errorf("unable to find Unit(%s) in Registry or on filesystem", name) - } - tmpl, err := cAPI.Unit(uni.Template) + uf, err = getUnitFileFromTemplate(arg) if err != nil { - return fmt.Errorf("error retrieving template Unit(%s) from Registry: %v", uni.Template, err) - } - - // Finally, if we could not find a template unit in the Registry, check the local disk for one instead - if tmpl == nil { - file := path.Join(path.Dir(arg), uni.Template) - if _, err := os.Stat(file); os.IsNotExist(err) { - return fmt.Errorf("unable to find Unit(%s) or template Unit(%s) in Registry or on filesystem", name, uni.Template) - } - uf, err = getUnitFromFile(file) - if err != nil { - return fmt.Errorf("failed getting template Unit(%s) from file: %v", uni.Template, err) - } - } else { - warnOnDifferentLocalUnit(arg, tmpl) - uf = schema.MapSchemaUnitOptionsToUnitFile(tmpl.Options) + return err } // If we found a template unit, create a near-identical instance unit in From 49344ff6454da25fc736e8998e84e04f40fe8091 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:51:07 +0100 Subject: [PATCH 07/18] fleetctl: more cleaning and consolidate the logic to get a Unit from disk or the Registry --- fleetctl/fleetctl.go | 46 +++++++++++++++++++++++++++----------------- 1 file changed, 28 insertions(+), 18 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 30098b49e..df90ed5b1 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -482,6 +482,31 @@ func getChecker() *ssh.HostKeyChecker { return ssh.NewHostKeyChecker(keyFile) } +func getUnitFile(file string) (*unit.UnitFile, error) { + var uf *unit.UnitFile + + // Failing that, assume the name references a local unit file on disk, + // and attempt to load that, if it exists + if _, err := os.Stat(file); !os.IsNotExist(err) { + uf, err = getUnitFromFile(file) + if err != nil { + return nil, fmt.Errorf("failed getting Unit(%s) from file: %v", file, err) + } + } else { + // Otherwise (if the unit file does not exist), check if the name appears to be an instance unit, + // and if so, check for a corresponding template unit in the Registry + uf, err = getUnitFileFromTemplate(file) + if err != nil { + return nil, err + } + + // If we found a template unit, create a near-identical instance unit in + // the Registry - same unit file as the template, but different name + } + + return uf, nil +} + // getUnitFromFile attempts to load a Unit from a given filename // It returns the Unit or nil, and any error encountered func getUnitFromFile(file string) (*unit.UnitFile, error) { @@ -655,24 +680,9 @@ func lazyCreateUnits(args []string) error { continue } - var uf *unit.UnitFile - // Failing that, assume the name references a local unit file on disk, and attempt to load that, if it exists - // TODO(mischief): consolidate these two near-identical codepaths - if _, err := os.Stat(arg); !os.IsNotExist(err) { - uf, err = getUnitFromFile(arg) - if err != nil { - return fmt.Errorf("failed getting Unit(%s) from file: %v", arg, err) - } - } else { - // Otherwise (if the unit file does not exist), check if the name appears to be an instance unit, - // and if so, check for a corresponding template unit in the Registry - uf, err = getUnitFileFromTemplate(arg) - if err != nil { - return err - } - - // If we found a template unit, create a near-identical instance unit in - // the Registry - same unit file as the template, but different name + uf, err := getUnitFile(arg) + if err != nil { + return err } _, err = createUnit(name, uf) From 4f7fca8fad6bd34532dfccceeae702b8947170be Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:56:39 +0100 Subject: [PATCH 08/18] fleetctl: add debug messages arround getUnit*() functions * Cover code path of getUnit*() functions with debug messages * Improve code comments --- fleetctl/fleetctl.go | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index df90ed5b1..2ecd0818c 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -484,17 +484,21 @@ func getChecker() *ssh.HostKeyChecker { func getUnitFile(file string) (*unit.UnitFile, error) { var uf *unit.UnitFile + name := unitNameMangle(file) - // Failing that, assume the name references a local unit file on disk, - // and attempt to load that, if it exists + log.Debugf("Looking up for Unit(%s) or its corresponding template", name) + + // Assume that the file references a local unit file on disk and + // attempt to load it, if it exists if _, err := os.Stat(file); !os.IsNotExist(err) { uf, err = getUnitFromFile(file) if err != nil { return nil, fmt.Errorf("failed getting Unit(%s) from file: %v", file, err) } } else { - // Otherwise (if the unit file does not exist), check if the name appears to be an instance unit, - // and if so, check for a corresponding template unit in the Registry + // Otherwise (if the unit file does not exist), check if the + // name appears to be an instance unit, and if so, check for + // a corresponding template unit in the Registry or disk uf, err = getUnitFileFromTemplate(file) if err != nil { return nil, err @@ -504,6 +508,7 @@ func getUnitFile(file string) (*unit.UnitFile, error) { // the Registry - same unit file as the template, but different name } + log.Debugf("Found Unit(%s)", name) return uf, nil } @@ -542,9 +547,13 @@ func getUnitFileFromTemplate(arg string) (*unit.UnitFile, error) { return nil, fmt.Errorf("error retrieving template Unit(%s) from Registry: %v", uni.Template, err) } - // Finally, if we could not find a template unit in the Registry, - // check the local disk for one instead - if tmpl == nil { + if tmpl != nil { + warnOnDifferentLocalUnit(arg, tmpl) + uf = schema.MapSchemaUnitOptionsToUnitFile(tmpl.Options) + log.Debugf("Template Unit(%s) found in registry", uni.Template) + } else { + // Finally, if we could not find a template unit in the Registry, + // check the local disk for one instead file := path.Join(path.Dir(arg), uni.Template) if _, err := os.Stat(file); os.IsNotExist(err) { return nil, fmt.Errorf("unable to find Unit(%s) or template Unit(%s) in Registry or on filesystem", name, uni.Template) @@ -554,9 +563,6 @@ func getUnitFileFromTemplate(arg string) (*unit.UnitFile, error) { if err != nil { return nil, fmt.Errorf("failed getting template Unit(%s) from file: %v", uni.Template, err) } - } else { - warnOnDifferentLocalUnit(arg, tmpl) - uf = schema.MapSchemaUnitOptionsToUnitFile(tmpl.Options) } return uf, nil @@ -680,6 +686,9 @@ func lazyCreateUnits(args []string) error { continue } + // Assume that the name references a local unit file on + // disk or if it is an instance unit and if so get its + // corresponding unit uf, err := getUnitFile(arg) if err != nil { return err From de05a8d7ad25995bb6fef39c3959dd98c3f65595 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Tue, 16 Feb 2016 15:37:33 +0100 Subject: [PATCH 09/18] fleetctl: remove dead code comment --- fleetctl/fleetctl.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 2ecd0818c..923e4470d 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -670,8 +670,6 @@ func lazyCreateUnits(args []string) error { errchan := make(chan error) var wg sync.WaitGroup for _, arg := range args { - // TODO(jonboulle): this loop is getting too unwieldy; factor it out - arg = maybeAppendDefaultUnitType(arg) name := unitNameMangle(arg) From fd173a01ced24065a3824db9919c1a04a3a6a8a5 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Tue, 16 Feb 2016 16:01:05 +0100 Subject: [PATCH 10/18] fleetctl: avoid hard coding sleep time values inside calls Just make it a const var that can be used or adapted if needed later. --- fleetctl/destroy.go | 2 +- fleetctl/fleetctl.go | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/fleetctl/destroy.go b/fleetctl/destroy.go index 0d319174d..54c6a57b8 100644 --- a/fleetctl/destroy.go +++ b/fleetctl/destroy.go @@ -71,7 +71,7 @@ func runDestroyUnits(args []string) (exit int) { if u == nil { break } - time.Sleep(500 * time.Millisecond) + time.Sleep(defaultSleepTime) } } diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 923e4470d..3b99f7f2a 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -59,7 +59,8 @@ recommended to upgrade fleetctl to prevent incompatibility issues. clientDriverAPI = "API" clientDriverEtcd = "etcd" - defaultEndpoint = "unix:///var/run/fleet.sock" + defaultEndpoint = "unix:///var/run/fleet.sock" + defaultSleepTime = 500 * time.Millisecond ) var ( @@ -858,7 +859,7 @@ func waitForUnitStates(units []string, js job.JobState, maxAttempts int, out io. func checkUnitState(name string, js job.JobState, maxAttempts int, out io.Writer, wg *sync.WaitGroup, errchan chan error) { defer wg.Done() - sleep := 500 * time.Millisecond + sleep := defaultSleepTime if maxAttempts < 1 { for { From 76d2f1a08ec67cebca7b397db221f747eb10edd2 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 10:04:10 +0100 Subject: [PATCH 11/18] fleetctl:destroy: on destroy check if the unit does exist or not Do not error out directly if Destroy command fails, check first if the unit does really exist if no then ignore the destroy error and continue. Follow-up fix for: https://github.com/coreos/fleet/issues/1383 --- fleetctl/destroy.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fleetctl/destroy.go b/fleetctl/destroy.go index 54c6a57b8..5e0aea5bf 100644 --- a/fleetctl/destroy.go +++ b/fleetctl/destroy.go @@ -42,6 +42,11 @@ func runDestroyUnits(args []string) (exit int) { for _, v := range units { err := cAPI.DestroyUnit(v.Name) if err != nil { + // If unit does not exist do not error out + u, _ := cAPI.Unit(v.Name) + if u == nil { + continue + } stderr("Error destroying units: %v", err) exit = 1 continue From 3933145c8a79676352416bae3a3de8a81a9ebfc5 Mon Sep 17 00:00:00 2001 From: Stefan Junker Date: Fri, 12 Feb 2016 15:57:56 +0100 Subject: [PATCH 12/18] tests fleetctl destroy behavior --- fleetctl/destroy_test.go | 97 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 97 insertions(+) create mode 100644 fleetctl/destroy_test.go diff --git a/fleetctl/destroy_test.go b/fleetctl/destroy_test.go new file mode 100644 index 000000000..508047877 --- /dev/null +++ b/fleetctl/destroy_test.go @@ -0,0 +1,97 @@ +// Copyright 2014 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "testing" + + "github.com/coreos/fleet/client" + "github.com/coreos/fleet/job" + "github.com/coreos/fleet/machine" + "github.com/coreos/fleet/registry" + "github.com/coreos/fleet/unit" +) + +func newFakeRegistryForDestroy() client.API { + // clear machineStates for every invocation + machineStates = nil + machines := []machine.MachineState{ + newMachineState("c31e44e1-f858-436e-933e-59c642517860", "1.2.3.4", map[string]string{"ping": "pong"}), + newMachineState("595989bb-cbb7-49ce-8726-722d6e157b4e", "5.6.7.8", map[string]string{"foo": "bar"}), + } + + jobs := []job.Job{ + job.Job{Name: "j1.service", Unit: unit.UnitFile{}, TargetMachineID: machines[0].ID}, + job.Job{Name: "j2.service", Unit: unit.UnitFile{}, TargetMachineID: machines[1].ID}, + } + + states := []unit.UnitState{ + unit.UnitState{ + UnitName: "j1.service", + LoadState: "loaded", + ActiveState: "active", + SubState: "listening", + MachineID: machines[0].ID, + }, + unit.UnitState{ + UnitName: "j2.service", + LoadState: "loaded", + ActiveState: "inactive", + SubState: "dead", + MachineID: machines[1].ID, + }, + } + + reg := registry.NewFakeRegistry() + reg.SetMachines(machines) + reg.SetUnitStates(states) + reg.SetJobs(jobs) + + return &client.RegistryClient{Registry: reg} +} + +// TestRunDestroyUnits checks for correct unit destruction +func TestRunDestroyUnits(t *testing.T) { + for _, s := range []struct { + Description string + DestroyUnits []string + ExpectedExit int + }{ + { + "destroy available units", + []string{"j1", "j2"}, + 0, + }, + { + "attempt to destroy available and non-available units", + []string{"j1", "j2", "j3"}, + 0, + }, + } { + cAPI = newFakeRegistryForDestroy() + exit := runDestroyUnits(s.DestroyUnits) + if exit != s.ExpectedExit { + t.Errorf("%s: expected exit code %d but received %d", + s.Description, s.ExpectedExit, exit) + } + for _, destroyedUnit := range s.DestroyUnits { + u, _ := cAPI.Unit(destroyedUnit) + if u != nil { + t.Errorf("%s: unit %s was not destroyed as requested", + s.Description, destroyedUnit) + } + } + } +} From 8c74542d1b34719f5d074d10f1c3c3664a016d17 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 16:29:59 +0100 Subject: [PATCH 13/18] destroy_test: add a destroy test for non-existent units --- fleetctl/destroy_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/fleetctl/destroy_test.go b/fleetctl/destroy_test.go index 508047877..36410b4b7 100644 --- a/fleetctl/destroy_test.go +++ b/fleetctl/destroy_test.go @@ -74,6 +74,11 @@ func TestRunDestroyUnits(t *testing.T) { []string{"j1", "j2"}, 0, }, + { + "destroy non-existent units", + []string{"y1", "y2"}, + 0, + }, { "attempt to destroy available and non-available units", []string{"j1", "j2", "j3"}, From 59fcef31e04b8a9c3e2f0d4719574a476d169548 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Thu, 18 Feb 2016 15:20:08 +0100 Subject: [PATCH 14/18] fleetctl:test: add appendJobsForTests() helper to automatically append jobs --- fleetctl/fleetctl_test.go | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/fleetctl/fleetctl_test.go b/fleetctl/fleetctl_test.go index ca635dab6..51ac7372e 100644 --- a/fleetctl/fleetctl_test.go +++ b/fleetctl/fleetctl_test.go @@ -15,9 +15,11 @@ package main import ( + "fmt" "testing" "github.com/coreos/fleet/client" + "github.com/coreos/fleet/job" "github.com/coreos/fleet/machine" "github.com/coreos/fleet/registry" "github.com/coreos/fleet/unit" @@ -26,6 +28,19 @@ import ( "github.com/coreos/fleet/Godeps/_workspace/src/github.com/coreos/go-semver/semver" ) +func appendJobsForTests(jobs *[]job.Job, machine machine.MachineState, prefix string, unitCnt int) { + for i := 1; i <= unitCnt; i++ { + j := job.Job{ + Name: fmt.Sprintf("%s%d.service", prefix, i), + Unit: unit.UnitFile{}, + TargetMachineID: machine.ID, + } + *jobs = append(*jobs, j) + } + + return +} + func newFakeRegistryForCheckVersion(v string) registry.ClusterRegistry { sv, err := semver.NewVersion(v) if err != nil { From 6ac77f6ee864a9e57ba6a6e7da6762554eb68d65 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Thu, 18 Feb 2016 15:21:30 +0100 Subject: [PATCH 15/18] fleetctl:destroy_test: make the test more smarter by checking for race conditions Make the test more smarter by checking for race conditions and output result. Sometimes you may see normal output sometimes no output between the two goroutines which is normal, the thing to worry about is if Destroy did success or not. --- fleetctl/destroy_test.go | 105 +++++++++++++++++++++++++++------------ 1 file changed, 73 insertions(+), 32 deletions(-) diff --git a/fleetctl/destroy_test.go b/fleetctl/destroy_test.go index 36410b4b7..a6d1b513e 100644 --- a/fleetctl/destroy_test.go +++ b/fleetctl/destroy_test.go @@ -15,7 +15,10 @@ package main import ( + "fmt" + "sync" "testing" + "time" "github.com/coreos/fleet/client" "github.com/coreos/fleet/job" @@ -24,7 +27,13 @@ import ( "github.com/coreos/fleet/unit" ) -func newFakeRegistryForDestroy() client.API { +type DestroyTestResults struct { + Description string + DestroyUnits []string + ExpectedExit int +} + +func newFakeRegistryForDestroy(prefix string, unitCnt int) client.API { // clear machineStates for every invocation machineStates = nil machines := []machine.MachineState{ @@ -32,26 +41,31 @@ func newFakeRegistryForDestroy() client.API { newMachineState("595989bb-cbb7-49ce-8726-722d6e157b4e", "5.6.7.8", map[string]string{"foo": "bar"}), } - jobs := []job.Job{ - job.Job{Name: "j1.service", Unit: unit.UnitFile{}, TargetMachineID: machines[0].ID}, - job.Job{Name: "j2.service", Unit: unit.UnitFile{}, TargetMachineID: machines[1].ID}, - } + jobs := make([]job.Job, 0) + appendJobsForTests(&jobs, machines[0], prefix, unitCnt) + appendJobsForTests(&jobs, machines[1], prefix, unitCnt) - states := []unit.UnitState{ - unit.UnitState{ - UnitName: "j1.service", + states := make([]unit.UnitState, 0) + for i := 1; i <= unitCnt; i++ { + state := unit.UnitState{ + UnitName: fmt.Sprintf("%s%d.service", prefix, i), LoadState: "loaded", ActiveState: "active", SubState: "listening", MachineID: machines[0].ID, - }, - unit.UnitState{ - UnitName: "j2.service", + } + states = append(states, state) + } + + for i := 1; i <= unitCnt; i++ { + state := unit.UnitState{ + UnitName: fmt.Sprintf("%s%d.service", prefix, i), LoadState: "loaded", ActiveState: "inactive", SubState: "dead", MachineID: machines[1].ID, - }, + } + states = append(states, state) } reg := registry.NewFakeRegistry() @@ -62,16 +76,26 @@ func newFakeRegistryForDestroy() client.API { return &client.RegistryClient{Registry: reg} } +func doDestroyUnits(r DestroyTestResults, errchan chan error) { + exit := runDestroyUnits(r.DestroyUnits) + if exit != r.ExpectedExit { + errchan <- fmt.Errorf("%s: expected exit code %d but received %d", r.Description, r.ExpectedExit, exit) + } + for _, destroyedUnit := range r.DestroyUnits { + u, _ := cAPI.Unit(destroyedUnit) + if u != nil { + errchan <- fmt.Errorf("%s: unit %s was not destroyed as requested", r.Description, destroyedUnit) + } + } +} + // TestRunDestroyUnits checks for correct unit destruction func TestRunDestroyUnits(t *testing.T) { - for _, s := range []struct { - Description string - DestroyUnits []string - ExpectedExit int - }{ + unitPrefix := "j" + results := []DestroyTestResults{ { "destroy available units", - []string{"j1", "j2"}, + []string{"j1", "j2", "j3", "j4", "j5"}, 0, }, { @@ -81,22 +105,39 @@ func TestRunDestroyUnits(t *testing.T) { }, { "attempt to destroy available and non-available units", - []string{"j1", "j2", "j3"}, + []string{"y1", "y2", "y3", "y4", "j1", "j2", "j3", "j4", "j5", "y0"}, 0, }, - } { - cAPI = newFakeRegistryForDestroy() - exit := runDestroyUnits(s.DestroyUnits) - if exit != s.ExpectedExit { - t.Errorf("%s: expected exit code %d but received %d", - s.Description, s.ExpectedExit, exit) - } - for _, destroyedUnit := range s.DestroyUnits { - u, _ := cAPI.Unit(destroyedUnit) - if u != nil { - t.Errorf("%s: unit %s was not destroyed as requested", - s.Description, destroyedUnit) - } + } + + // Check with two goroutines we don't care we should just get + // the right result. If you happen to inspect this code for + // errors then you probably got hit by a race condition in + // Destroy command that should not happen + for _, r := range results { + var wg sync.WaitGroup + errchan := make(chan error) + + cAPI = newFakeRegistryForDestroy(unitPrefix, len(r.DestroyUnits)) + + wg.Add(2) + go func() { + defer wg.Done() + time.Sleep(2 * time.Microsecond) + doDestroyUnits(r, errchan) + }() + go func() { + defer wg.Done() + doDestroyUnits(r, errchan) + }() + + go func() { + wg.Wait() + close(errchan) + }() + + for err := range errchan { + t.Errorf("%v", err) } } } From d6750dbb509107f8f032b5fb9db914bc818a5002 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Thu, 18 Feb 2016 16:22:33 +0100 Subject: [PATCH 16/18] fleetctl:stop_test: some tests for fleetctl stop code --- fleetctl/stop_test.go | 141 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 141 insertions(+) create mode 100644 fleetctl/stop_test.go diff --git a/fleetctl/stop_test.go b/fleetctl/stop_test.go new file mode 100644 index 000000000..dc353ed42 --- /dev/null +++ b/fleetctl/stop_test.go @@ -0,0 +1,141 @@ +// Copyright 2014 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "fmt" + "sync" + "testing" + "time" + + "github.com/coreos/fleet/client" + "github.com/coreos/fleet/job" + "github.com/coreos/fleet/machine" + "github.com/coreos/fleet/registry" + "github.com/coreos/fleet/unit" +) + +type StopTestResults struct { + Description string + Units []string + ExpectedExit int +} + +func newFakeRegistryForStop(prefix string, unitCnt int) client.API { + // clear machineStates for every invocation + machineStates = nil + machines := []machine.MachineState{ + newMachineState("c31e44e1-f858-436e-933e-59c642517860", "1.2.3.4", map[string]string{"ping": "pong"}), + newMachineState("595989bb-cbb7-49ce-8726-722d6e157b4e", "5.6.7.8", map[string]string{"foo": "bar"}), + } + + jobs := make([]job.Job, 0) + appendJobsForTests(&jobs, machines[0], prefix, unitCnt) + appendJobsForTests(&jobs, machines[1], prefix, unitCnt) + + states := make([]unit.UnitState, 0) + for i := 1; i <= unitCnt; i++ { + state := unit.UnitState{ + UnitName: fmt.Sprintf("%s%d.service", prefix, i), + LoadState: "loaded", + ActiveState: "active", + SubState: "listening", + MachineID: machines[0].ID, + } + states = append(states, state) + } + + for i := 1; i <= unitCnt; i++ { + state := unit.UnitState{ + UnitName: fmt.Sprintf("%s%d.service", prefix, i), + LoadState: "loaded", + ActiveState: "inactive", + SubState: "dead", + MachineID: machines[1].ID, + } + states = append(states, state) + } + + reg := registry.NewFakeRegistry() + reg.SetMachines(machines) + reg.SetUnitStates(states) + reg.SetJobs(jobs) + + return &client.RegistryClient{Registry: reg} +} + +func doStopUnits(r StopTestResults, errchan chan error) { + sharedFlags.NoBlock = true + + exit := runStopUnit(r.Units) + if exit != r.ExpectedExit { + errchan <- fmt.Errorf("%s: expected exit code %d but received %d", r.Description, r.ExpectedExit, exit) + } + for _, unit := range r.Units { + u, _ := cAPI.Unit(unit) + if u != nil { + errchan <- fmt.Errorf("%s: unit %s was not stopped as requested", r.Description, unit) + } + } +} + +// TestRunStopUnits checks +func TestRunStopUnits(t *testing.T) { + unitPrefix := "stop" + results := []StopTestResults{ + { + "stop available units", + []string{"stop1", "stop2", "stop3", "stop4", "stop5"}, + 0, + }, + { + "stop non-existent units", + []string{"y1", "y2"}, + 0, + }, + { + "attempt to stop available and non-available units", + []string{"y1", "y2", "y3", "y4", "stop1", "stop2", "stop3", "stop4", "stop5", "y0"}, + 0, + }, + } + + for _, r := range results { + var wg sync.WaitGroup + errchan := make(chan error) + + cAPI = newFakeRegistryForStop(unitPrefix, len(r.Units)) + + wg.Add(2) + go func() { + defer wg.Done() + time.Sleep(2 * time.Microsecond) + doStopUnits(r, errchan) + }() + go func() { + defer wg.Done() + doStopUnits(r, errchan) + }() + + go func() { + wg.Wait() + close(errchan) + }() + + for err := range errchan { + t.Errorf("%v", err) + } + } +} From 56d717052affd5737c99c279ebd7ec74b1241b72 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Mon, 22 Feb 2016 12:23:53 +0100 Subject: [PATCH 17/18] fleetctl:stop_test: check if we reached the desired state Fix and enforce the check for the intended desired state. --- fleetctl/stop_test.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/fleetctl/stop_test.go b/fleetctl/stop_test.go index dc353ed42..38c58b83d 100644 --- a/fleetctl/stop_test.go +++ b/fleetctl/stop_test.go @@ -83,10 +83,17 @@ func doStopUnits(r StopTestResults, errchan chan error) { if exit != r.ExpectedExit { errchan <- fmt.Errorf("%s: expected exit code %d but received %d", r.Description, r.ExpectedExit, exit) } - for _, unit := range r.Units { - u, _ := cAPI.Unit(unit) - if u != nil { - errchan <- fmt.Errorf("%s: unit %s was not stopped as requested", r.Description, unit) + + real_units, err := findUnits(r.Units) + if err != nil { + errchan <- fmt.Errorf("%v", err) + return + } + + // We assume that we reached the desired state + for _, v := range real_units { + if job.JobState(v.DesiredState) != job.JobStateLoaded { + errchan <- fmt.Errorf("Error: unit %s was not stopped as requested", v.Name) } } } From 69f6a1b86632433ea1e95061618905c3a946e7c0 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Mon, 22 Feb 2016 12:32:07 +0100 Subject: [PATCH 18/18] fleetctl:unload_test: more tests for the unload fleetctl path More tests for the Inactive desired state. --- fleetctl/unload_test.go | 148 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 148 insertions(+) create mode 100644 fleetctl/unload_test.go diff --git a/fleetctl/unload_test.go b/fleetctl/unload_test.go new file mode 100644 index 000000000..0ad8c30e5 --- /dev/null +++ b/fleetctl/unload_test.go @@ -0,0 +1,148 @@ +// Copyright 2014 CoreOS, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "fmt" + "sync" + "testing" + "time" + + "github.com/coreos/fleet/client" + "github.com/coreos/fleet/job" + "github.com/coreos/fleet/machine" + "github.com/coreos/fleet/registry" + "github.com/coreos/fleet/unit" +) + +type UnloadTestResults struct { + Description string + Units []string + ExpectedExit int +} + +func newFakeRegistryForUnload(prefix string, unitCnt int) client.API { + // clear machineStates for every invocation + machineStates = nil + machines := []machine.MachineState{ + newMachineState("c31e44e1-f858-436e-933e-59c642517860", "1.2.3.4", map[string]string{"ping": "pong"}), + newMachineState("595989bb-cbb7-49ce-8726-722d6e157b4e", "5.6.7.8", map[string]string{"foo": "bar"}), + } + + jobs := make([]job.Job, 0) + appendJobsForTests(&jobs, machines[0], prefix, unitCnt) + appendJobsForTests(&jobs, machines[1], prefix, unitCnt) + + states := make([]unit.UnitState, 0) + for i := 1; i <= unitCnt; i++ { + state := unit.UnitState{ + UnitName: fmt.Sprintf("%s%d.service", prefix, i), + LoadState: "loaded", + ActiveState: "active", + SubState: "listening", + MachineID: machines[0].ID, + } + states = append(states, state) + } + + for i := 1; i <= unitCnt; i++ { + state := unit.UnitState{ + UnitName: fmt.Sprintf("%s%d.service", prefix, i), + LoadState: "loaded", + ActiveState: "inactive", + SubState: "dead", + MachineID: machines[1].ID, + } + states = append(states, state) + } + + reg := registry.NewFakeRegistry() + reg.SetMachines(machines) + reg.SetUnitStates(states) + reg.SetJobs(jobs) + + return &client.RegistryClient{Registry: reg} +} + +func doUnloadUnits(r UnloadTestResults, errchan chan error) { + sharedFlags.NoBlock = true + + exit := runUnloadUnit(r.Units) + if exit != r.ExpectedExit { + errchan <- fmt.Errorf("%s: expected exit code %d but received %d", r.Description, r.ExpectedExit, exit) + } + + real_units, err := findUnits(r.Units) + if err != nil { + errchan <- fmt.Errorf("%v", err) + return + } + + // We assume that we reached the desired state + for _, v := range real_units { + if job.JobState(v.DesiredState) != job.JobStateInactive { + errchan <- fmt.Errorf("Error: unit %s was not unloaded as requested", v.Name) + } + } +} + +// TestRunUnloadUnits checks +func TestRunUnloadUnits(t *testing.T) { + unitPrefix := "unload" + results := []UnloadTestResults{ + { + "unload available units", + []string{"unload1", "unload2", "unload3", "unload4", "unload5"}, + 0, + }, + { + "unload non-existent units", + []string{"y1", "y2"}, + 0, + }, + { + "attempt to unload available and non-available units", + []string{"y1", "y2", "y3", "y4", "unload1", "unload2", "unload3", "unload4", "unload5", "y0"}, + 0, + }, + } + + for _, r := range results { + var wg sync.WaitGroup + errchan := make(chan error) + + cAPI = newFakeRegistryForUnload(unitPrefix, len(r.Units)) + + wg.Add(2) + go func() { + defer wg.Done() + time.Sleep(2 * time.Microsecond) + doUnloadUnits(r, errchan) + }() + go func() { + defer wg.Done() + doUnloadUnits(r, errchan) + }() + + go func() { + wg.Wait() + close(errchan) + }() + + for err := range errchan { + t.Errorf("%v", err) + } + } +}