From 5b302d5d3172430564e81928ef661d651ccab640 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:16:17 +0100 Subject: [PATCH 01/21] 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 71785606d..09a75b6aa 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -746,6 +746,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 bd37783955bd8392c28cc87c4f6224b6a7e97b71 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:17:47 +0100 Subject: [PATCH 02/21] 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 474e4d5870898daa86981de4ef67807be792140b Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:28:14 +0100 Subject: [PATCH 03/21] 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 c29a358cf1d7182c6b8de4ac346af20c7d1affdd Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:28:29 +0100 Subject: [PATCH 04/21] 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 ca00298fc0957221c5acba5ddab50242a89c1838 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:28:40 +0100 Subject: [PATCH 05/21] 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 538cbcea8ee96229ce4b35152f058a5ebb5c9316 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:44:00 +0100 Subject: [PATCH 06/21] 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 09a75b6aa..7c00e6d7d 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -497,6 +497,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, ":") { @@ -626,30 +667,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 537d516a3bf104454d9fc89902440a28afa61762 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:51:07 +0100 Subject: [PATCH 07/21] 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 7c00e6d7d..bb59134d4 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -483,6 +483,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) { @@ -656,24 +681,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 e116a2cc439304999ed8caa3004a7331852d5a27 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 12:56:39 +0100 Subject: [PATCH 08/21] 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 bb59134d4..99ec3640e 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -485,17 +485,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 @@ -505,6 +509,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 } @@ -543,9 +548,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) @@ -555,9 +564,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 @@ -681,6 +687,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 9cc141ad677cf542b70c0b861009945a4a07e086 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Tue, 16 Feb 2016 15:37:33 +0100 Subject: [PATCH 09/21] 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 99ec3640e..3b99f7f2a 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -671,8 +671,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 2d2c3a3dcbffa1ecaca62ae24bf4109e5e76b02d Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 16:15:10 +0100 Subject: [PATCH 10/21] fleetctl: inline getBlockAttempts() in tryWaitForUnitStates() calls --- fleetctl/load.go | 4 +--- fleetctl/start.go | 4 +--- fleetctl/stop.go | 4 +--- fleetctl/unload.go | 4 +--- 4 files changed, 4 insertions(+), 12 deletions(-) diff --git a/fleetctl/load.go b/fleetctl/load.go index 8659a1498..0f7b0923c 100644 --- a/fleetctl/load.go +++ b/fleetctl/load.go @@ -46,8 +46,6 @@ func init() { } func runLoadUnits(args []string) (exit int) { - attempts := getBlockAttempts() - if err := lazyCreateUnits(args); err != nil { stderr("Error creating units: %v", err) return 1 @@ -68,7 +66,7 @@ func runLoadUnits(args []string) (exit int) { } } - exit = tryWaitForUnitStates(loading, "load", job.JobStateLoaded, attempts, os.Stdout) + exit = tryWaitForUnitStates(loading, "load", job.JobStateLoaded, getBlockAttempts(), os.Stdout) return } diff --git a/fleetctl/start.go b/fleetctl/start.go index 17c8a53da..2ded29539 100644 --- a/fleetctl/start.go +++ b/fleetctl/start.go @@ -54,8 +54,6 @@ func init() { } func runStartUnit(args []string) (exit int) { - attempts := getBlockAttempts() - if err := lazyCreateUnits(args); err != nil { stderr("Error creating units: %v", err) return 1 @@ -76,7 +74,7 @@ func runStartUnit(args []string) (exit int) { } } - exit = tryWaitForUnitStates(starting, "start", job.JobStateLaunched, attempts, os.Stdout) + exit = tryWaitForUnitStates(starting, "start", job.JobStateLaunched, getBlockAttempts(), os.Stdout) return } diff --git a/fleetctl/stop.go b/fleetctl/stop.go index c63904107..941b33d7b 100644 --- a/fleetctl/stop.go +++ b/fleetctl/stop.go @@ -52,8 +52,6 @@ func init() { } func runStopUnit(args []string) (exit int) { - attempts := getBlockAttempts() - units, err := findUnits(args) if err != nil { stderr("%v", err) @@ -81,7 +79,7 @@ func runStopUnit(args []string) (exit int) { } } - exit = tryWaitForUnitStates(stopping, "stop", job.JobStateLoaded, attempts, os.Stdout) + exit = tryWaitForUnitStates(stopping, "stop", job.JobStateLoaded, getBlockAttempts(), os.Stdout) return } diff --git a/fleetctl/unload.go b/fleetctl/unload.go index c9700496a..48eb833fc 100644 --- a/fleetctl/unload.go +++ b/fleetctl/unload.go @@ -36,8 +36,6 @@ func init() { } func runUnloadUnit(args []string) (exit int) { - attempts := getBlockAttempts() - units, err := findUnits(args) if err != nil { stderr("%v", err) @@ -62,7 +60,7 @@ func runUnloadUnit(args []string) (exit int) { } } - exit = tryWaitForUnitStates(wait, "unload", job.JobStateInactive, attempts, os.Stdout) + exit = tryWaitForUnitStates(wait, "unload", job.JobStateInactive, getBlockAttempts(), os.Stdout) return } From f1bc4c343134a11896c1c2bb5fc2dc5c7281a89d Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 17 Feb 2016 16:16:39 +0100 Subject: [PATCH 11/21] fleetctl: improve code comment about getUnitFileFromTemplate() Improve code comments about getUnitFileFromTemplate() and kill some other useless code comments. --- fleetctl/fleetctl.go | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 3b99f7f2a..716deb54c 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -499,14 +499,14 @@ func getUnitFile(file string) (*unit.UnitFile, 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 or disk + // a corresponding template unit in the Registry or disk. + // If we found a template unit, later we create a + // near-identical instance unit in the Registry - same + // unit file as the template, but different name 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 } log.Debugf("Found Unit(%s)", name) @@ -792,12 +792,10 @@ 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 } From 5afa52dada59158ef445a8ea65b8536c94395205 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Sat, 20 Feb 2016 09:50:33 +0100 Subject: [PATCH 12/21] fleetctl: fix debug message in getUnitFile() --- fleetctl/fleetctl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 716deb54c..67fab4562 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -487,7 +487,7 @@ func getUnitFile(file string) (*unit.UnitFile, error) { var uf *unit.UnitFile name := unitNameMangle(file) - log.Debugf("Looking up for Unit(%s) or its corresponding template", name) + log.Debugf("Looking 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 From a59196c760712cfad5c413c28a1d4d16fe1a5aed Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Sat, 20 Feb 2016 10:22:40 +0100 Subject: [PATCH 13/21] fleetctl: getBlockAttempts() standarise on negative meaning do not block --- fleetctl/fleetctl.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 67fab4562..9a7d76a4a 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -785,19 +785,19 @@ func setTargetStateOfUnits(units []string, state job.JobState) ([]*schema.Unit, // 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 +// It returns a negative value which means do not block, if zero is +// returned then it means try forever, 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 + var attempts int = 0 if sharedFlags.BlockAttempts > 0 { attempts = sharedFlags.BlockAttempts } if sharedFlags.NoBlock { - attempts = 0 + attempts = -1 } return attempts @@ -808,14 +808,14 @@ func getBlockAttempts() int { // 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 +// reach the desired state. If maxAttempts is negative, 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 maxAttempts is zero 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 { + if maxAttempts <= -1 { for _, name := range units { stdout("Triggered unit %s %s", name, state) } From ab02d54cd3ce4c6f4d64637ce480a10f266562c1 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Sat, 20 Feb 2016 10:24:44 +0100 Subject: [PATCH 14/21] fleetctl:test: push tests for getBlockAttempts() --- fleetctl/fleetctl_test.go | 45 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 45 insertions(+) diff --git a/fleetctl/fleetctl_test.go b/fleetctl/fleetctl_test.go index 0d50de40c..d8f2a5129 100644 --- a/fleetctl/fleetctl_test.go +++ b/fleetctl/fleetctl_test.go @@ -181,6 +181,51 @@ func TestUnitNameMangle(t *testing.T) { } } +func TestGetBlockAttempts(t *testing.T) { + oldNoBlock := sharedFlags.NoBlock + oldBlockAttempts := sharedFlags.BlockAttempts + sharedFlags.NoBlock = true + sharedFlags.BlockAttempts = 0 + + if n := getBlockAttempts(); n != -1 { + t.Errorf("got %d, want -1", n) + } + + sharedFlags.BlockAttempts = -1 + if n := getBlockAttempts(); n != -1 { + t.Errorf("got %d, want -1", n) + } + + sharedFlags.BlockAttempts = 9999 + if n := getBlockAttempts(); n != -1 { + t.Errorf("got %d, want -1", n) + } + + sharedFlags.NoBlock = false + sharedFlags.BlockAttempts = 0 + if n := getBlockAttempts(); n != 0 { + t.Errorf("got %d, want 0", n) + } + + sharedFlags.BlockAttempts = -1 + if n := getBlockAttempts(); n != 0 { + t.Errorf("got %d, want 0", n) + } + + sharedFlags.BlockAttempts = 0 + if n := getBlockAttempts(); n != 0 { + t.Errorf("got %d, want 0", n) + } + + sharedFlags.BlockAttempts = 9999 + if n := getBlockAttempts(); n != 9999 { + t.Errorf("got %d, want 9999", n) + } + + sharedFlags.NoBlock = oldNoBlock + sharedFlags.BlockAttempts = oldBlockAttempts +} + func newUnitFile(t *testing.T, contents string) *unit.UnitFile { uf, err := unit.NewUnitFile(contents) if err != nil { From 8240d03819cce94f66c52aabe90aeb7f73cb2755 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Thu, 25 Feb 2016 13:55:59 +0100 Subject: [PATCH 15/21] fleetctl: add getUnitInstanceInfo() and improve unit error handling Add getUnitInstanceInfo() to check if the unit name is an instance of a template unit and while we are it improve the error handling. --- fleetctl/fleetctl.go | 44 ++++++++++++++++++++++++++------------------ unit/unit.go | 10 ++++++++++ 2 files changed, 36 insertions(+), 18 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 9a7d76a4a..89d326c87 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -498,14 +498,20 @@ func getUnitFile(file string) (*unit.UnitFile, 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 or disk. + // name appears to be an instance of a template unit + info, ret := getUnitInstanceInfo(name) + if ret != 0 { + return nil, fmt.Errorf("failed getting template Unit(%s) info: %s", name, unit.ErrorDescription[ret]) + } + + // If it is an instance check for a corresponding template + // unit in the Registry or disk. // If we found a template unit, later we create a // near-identical instance unit in the Registry - same // unit file as the template, but different name - uf, err = getUnitFileFromTemplate(file) + uf, err = getUnitFileFromTemplate(file, info) if err != nil { - return nil, err + return nil, fmt.Errorf("failed getting Unit(%s) from template: %v", file, err) } } @@ -527,25 +533,27 @@ 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 +func getUnitInstanceInfo(name string) (*unit.UnitNameInfo, int) { + // Check if the name appears to be an instance unit uni := unit.NewUnitNameInfo(name) if uni == nil { - return nil, fmt.Errorf("error extracting information from unit name %s", name) + return nil, unit.ErrNameInfo } else if !uni.IsInstance() { - return nil, fmt.Errorf("unable to find Unit(%s) in Registry or on filesystem", name) + return nil, unit.ErrNotTemplateInstance } + return uni, 0 +} + +// 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, uni *unit.UnitNameInfo) (*unit.UnitFile, error) { + var uf *unit.UnitFile + tmpl, err := cAPI.Unit(uni.Template) if err != nil { - return nil, fmt.Errorf("error retrieving template Unit(%s) from Registry: %v", uni.Template, err) + return nil, fmt.Errorf("unable to retrieve Unit(%s) from Registry: %v", uni.Template, err) } if tmpl != nil { @@ -557,12 +565,12 @@ func getUnitFileFromTemplate(arg string) (*unit.UnitFile, error) { // 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) + return nil, fmt.Errorf("unable to find Unit(%s) on filesystem", file) } uf, err = getUnitFromFile(file) if err != nil { - return nil, fmt.Errorf("failed getting template Unit(%s) from file: %v", uni.Template, err) + return nil, fmt.Errorf("unable to load Unit(%s) from file: %v", file, err) } } diff --git a/unit/unit.go b/unit/unit.go index 2da58a6bb..7c79450f5 100644 --- a/unit/unit.go +++ b/unit/unit.go @@ -25,6 +25,16 @@ import ( "github.com/coreos/fleet/Godeps/_workspace/src/github.com/coreos/go-systemd/unit" ) +const ( + ErrNameInfo = 1 + ErrNotTemplateInstance = 2 +) + +var ErrorDescription = []string{ + ErrNameInfo: "Unable to extract information from unit name", + ErrNotTemplateInstance: "Not an instance of a template unit", +} + func NewUnitFile(raw string) (*UnitFile, error) { reader := strings.NewReader(raw) opts, err := unit.Deserialize(reader) From c283f2c9c0ae4bae8a19b44034e9972607bad3e5 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Mon, 29 Feb 2016 15:33:51 +0100 Subject: [PATCH 16/21] fleetctl:test: use test tables for TestGetBlockAttempts --- fleetctl/fleetctl_test.go | 56 +++++++++++++++------------------------ 1 file changed, 21 insertions(+), 35 deletions(-) diff --git a/fleetctl/fleetctl_test.go b/fleetctl/fleetctl_test.go index d8f2a5129..643292809 100644 --- a/fleetctl/fleetctl_test.go +++ b/fleetctl/fleetctl_test.go @@ -184,46 +184,32 @@ func TestUnitNameMangle(t *testing.T) { func TestGetBlockAttempts(t *testing.T) { oldNoBlock := sharedFlags.NoBlock oldBlockAttempts := sharedFlags.BlockAttempts - sharedFlags.NoBlock = true - sharedFlags.BlockAttempts = 0 - if n := getBlockAttempts(); n != -1 { - t.Errorf("got %d, want -1", n) - } - - sharedFlags.BlockAttempts = -1 - if n := getBlockAttempts(); n != -1 { - t.Errorf("got %d, want -1", n) - } - - sharedFlags.BlockAttempts = 9999 - if n := getBlockAttempts(); n != -1 { - t.Errorf("got %d, want -1", n) - } - - sharedFlags.NoBlock = false - sharedFlags.BlockAttempts = 0 - if n := getBlockAttempts(); n != 0 { - t.Errorf("got %d, want 0", n) - } - - sharedFlags.BlockAttempts = -1 - if n := getBlockAttempts(); n != 0 { - t.Errorf("got %d, want 0", n) - } + defer func() { + sharedFlags.NoBlock = oldNoBlock + sharedFlags.BlockAttempts = oldBlockAttempts + }() - sharedFlags.BlockAttempts = 0 - if n := getBlockAttempts(); n != 0 { - t.Errorf("got %d, want 0", n) + var blocktests = []struct { + noBlock bool + blockAttempts int + expected int + }{ + {true, 0, -1}, + {true, -1, -1}, + {true, 9999, -1}, + {false, 0, 0}, + {false, -1, 0}, + {false, 9999, 9999}, } - sharedFlags.BlockAttempts = 9999 - if n := getBlockAttempts(); n != 9999 { - t.Errorf("got %d, want 9999", n) + for _, tt := range blocktests { + sharedFlags.NoBlock = tt.noBlock + sharedFlags.BlockAttempts = tt.blockAttempts + if n := getBlockAttempts(); n != tt.expected { + t.Errorf("got %d, want %d", n, tt.expected) + } } - - sharedFlags.NoBlock = oldNoBlock - sharedFlags.BlockAttempts = oldBlockAttempts } func newUnitFile(t *testing.T, contents string) *unit.UnitFile { From e569fa776fca189d83c83b2cb59b96f898d487d6 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Mon, 29 Feb 2016 15:50:29 +0100 Subject: [PATCH 17/21] fleetctl:unit: imporve error handling in case getUnitInstanceInfo() fails --- fleetctl/fleetctl.go | 14 +++++++------- unit/unit.go | 10 ---------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 89d326c87..8064fd20c 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -499,9 +499,9 @@ func getUnitFile(file string) (*unit.UnitFile, error) { } else { // Otherwise (if the unit file does not exist), check if the // name appears to be an instance of a template unit - info, ret := getUnitInstanceInfo(name) - if ret != 0 { - return nil, fmt.Errorf("failed getting template Unit(%s) info: %s", name, unit.ErrorDescription[ret]) + info, err := getUnitInstanceInfo(name) + if err != nil { + return nil, fmt.Errorf("failed getting template Unit(%s) info: %v", name, err) } // If it is an instance check for a corresponding template @@ -533,16 +533,16 @@ func getUnitFromFile(file string) (*unit.UnitFile, error) { return unit.NewUnitFile(string(out)) } -func getUnitInstanceInfo(name string) (*unit.UnitNameInfo, int) { +func getUnitInstanceInfo(name string) (*unit.UnitNameInfo, error) { // Check if the name appears to be an instance unit uni := unit.NewUnitNameInfo(name) if uni == nil { - return nil, unit.ErrNameInfo + return nil, errors.New("unable to extract information from unit name") } else if !uni.IsInstance() { - return nil, unit.ErrNotTemplateInstance + return nil, errors.New("Not an instance of a template unit") } - return uni, 0 + return uni, nil } // getUnitFileFromTemplate checks if the name appears to be an instance unit diff --git a/unit/unit.go b/unit/unit.go index 7c79450f5..2da58a6bb 100644 --- a/unit/unit.go +++ b/unit/unit.go @@ -25,16 +25,6 @@ import ( "github.com/coreos/fleet/Godeps/_workspace/src/github.com/coreos/go-systemd/unit" ) -const ( - ErrNameInfo = 1 - ErrNotTemplateInstance = 2 -) - -var ErrorDescription = []string{ - ErrNameInfo: "Unable to extract information from unit name", - ErrNotTemplateInstance: "Not an instance of a template unit", -} - func NewUnitFile(raw string) (*UnitFile, error) { reader := strings.NewReader(raw) opts, err := unit.Deserialize(reader) From 73c455d6f9e95428dcd8a5133ed56f275d6f00ca Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Mon, 29 Feb 2016 18:02:47 +0100 Subject: [PATCH 18/21] fleetctl: restore previous error message where a unit is not intended to be an instance --- fleetctl/fleetctl.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 8064fd20c..523362982 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -501,7 +501,7 @@ func getUnitFile(file string) (*unit.UnitFile, error) { // name appears to be an instance of a template unit info, err := getUnitInstanceInfo(name) if err != nil { - return nil, fmt.Errorf("failed getting template Unit(%s) info: %v", name, err) + return nil, fmt.Errorf("failed getting Unit(%s) info: %v", name, err) } // If it is an instance check for a corresponding template @@ -539,7 +539,7 @@ func getUnitInstanceInfo(name string) (*unit.UnitNameInfo, error) { if uni == nil { return nil, errors.New("unable to extract information from unit name") } else if !uni.IsInstance() { - return nil, errors.New("Not an instance of a template unit") + return nil, errors.New("unable to find Unit in Registry or on filesystem") } return uni, nil From a7bcde1f91253b6933b8ae01571bb48db91b6a34 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Mon, 29 Feb 2016 18:16:38 +0100 Subject: [PATCH 19/21] fleetctl: on failures indicate that getUnitFileFromTemplate() tried filesystem and registry --- fleetctl/fleetctl.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 523362982..5e7d2e8c4 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -565,7 +565,7 @@ func getUnitFileFromTemplate(arg string, uni *unit.UnitNameInfo) (*unit.UnitFile // 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) on filesystem", file) + return nil, fmt.Errorf("unable to find Unit(%s) in Registry or on filesystem", uni.Template) } uf, err = getUnitFromFile(file) From 84748aa4e726ad9834446a78e5da706ae0e1fc31 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 2 Mar 2016 13:07:21 +0100 Subject: [PATCH 20/21] fleetctl: improve getUnitFile() and getUnitFileFromTemplate() Godoc --- fleetctl/fleetctl.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 5e7d2e8c4..8d4954088 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -483,6 +483,13 @@ func getChecker() *ssh.HostKeyChecker { return ssh.NewHostKeyChecker(keyFile) } +// getUnitFile attempts to get a UnitFile configuration +// It takes a unit file name as a parameter and tries first to lookup +// the unit from the local disk. If it fails, it checks if the provided +// file name may reference an instance of a template unit, if so, it +// tries to get the template configuration either from the registry or +// the local disk. +// It returns a UnitFile configuration or nil; and any error ecountered func getUnitFile(file string) (*unit.UnitFile, error) { var uf *unit.UnitFile name := unitNameMangle(file) @@ -509,7 +516,7 @@ func getUnitFile(file string) (*unit.UnitFile, error) { // If we found a template unit, later we create a // near-identical instance unit in the Registry - same // unit file as the template, but different name - uf, err = getUnitFileFromTemplate(file, info) + uf, err = getUnitFileFromTemplate(info, file) if err != nil { return nil, fmt.Errorf("failed getting Unit(%s) from template: %v", file, err) } @@ -545,10 +552,11 @@ func getUnitInstanceInfo(name string) (*unit.UnitNameInfo, error) { return uni, nil } -// getUnitFileFromTemplate checks if the name appears to be an instance unit -// and gets its corresponding template unit from the registry or local disk +// getUnitFileFromTemplate attempts to get a Unit from a template unit that +// is either in the registry or on the file system +// It takes two arguments, the template information and the unit file name // It returns the Unit or nil; and any error encountered -func getUnitFileFromTemplate(arg string, uni *unit.UnitNameInfo) (*unit.UnitFile, error) { +func getUnitFileFromTemplate(uni *unit.UnitNameInfo, fileName string) (*unit.UnitFile, error) { var uf *unit.UnitFile tmpl, err := cAPI.Unit(uni.Template) @@ -557,20 +565,20 @@ func getUnitFileFromTemplate(arg string, uni *unit.UnitNameInfo) (*unit.UnitFile } if tmpl != nil { - warnOnDifferentLocalUnit(arg, tmpl) + warnOnDifferentLocalUnit(fileName, 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) { + filePath := path.Join(path.Dir(fileName), uni.Template) + if _, err := os.Stat(filePath); os.IsNotExist(err) { return nil, fmt.Errorf("unable to find Unit(%s) in Registry or on filesystem", uni.Template) } - uf, err = getUnitFromFile(file) + uf, err = getUnitFromFile(filePath) if err != nil { - return nil, fmt.Errorf("unable to load Unit(%s) from file: %v", file, err) + return nil, fmt.Errorf("unable to load Unit(%s) from file: %v", filePath, err) } } From 685e9d6a9e162283ae603513aec0d47c56ef23b6 Mon Sep 17 00:00:00 2001 From: Djalal Harouni Date: Wed, 2 Mar 2016 13:17:33 +0100 Subject: [PATCH 21/21] fleetctl: just inline getUnitInstanceInfo() and restore previous error messages --- fleetctl/fleetctl.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/fleetctl/fleetctl.go b/fleetctl/fleetctl.go index 8d4954088..d16863635 100644 --- a/fleetctl/fleetctl.go +++ b/fleetctl/fleetctl.go @@ -506,9 +506,11 @@ func getUnitFile(file string) (*unit.UnitFile, error) { } else { // Otherwise (if the unit file does not exist), check if the // name appears to be an instance of a template unit - info, err := getUnitInstanceInfo(name) + info := unit.NewUnitNameInfo(name) if err != nil { - return nil, fmt.Errorf("failed getting Unit(%s) info: %v", name, err) + return nil, fmt.Errorf("error extracting information from unit name %s", name) + } else if !info.IsInstance() { + return nil, fmt.Errorf("unable to find Unit(%s) in Registry or on filesystem", name) } // If it is an instance check for a corresponding template @@ -540,18 +542,6 @@ func getUnitFromFile(file string) (*unit.UnitFile, error) { return unit.NewUnitFile(string(out)) } -func getUnitInstanceInfo(name string) (*unit.UnitNameInfo, error) { - // Check if the name appears to be an instance unit - uni := unit.NewUnitNameInfo(name) - if uni == nil { - return nil, errors.New("unable to extract information from unit name") - } else if !uni.IsInstance() { - return nil, errors.New("unable to find Unit in Registry or on filesystem") - } - - return uni, nil -} - // getUnitFileFromTemplate attempts to get a Unit from a template unit that // is either in the registry or on the file system // It takes two arguments, the template information and the unit file name