Skip to content

Commit

Permalink
functional: change logic of destroying all units
Browse files Browse the repository at this point in the history
So far the logic of destroyUnitRetrying() was sub-optimal, as it
repeatedly tried to destroy every unit. To improve its logic for better
efficiency, change its logic into waiting for every unit being removed
only at the end of tests.

Also convert the helper destroyUnitRetrying into a method of
nspawnCluster, WaitForNAllUnits(), just like the existing method
WaitForNActiveUnits().
  • Loading branch information
Dongsu Park committed Mar 21, 2016
1 parent 6f6cf81 commit 8a22f07
Show file tree
Hide file tree
Showing 3 changed files with 42 additions and 34 deletions.
1 change: 1 addition & 0 deletions functional/platform/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ type Cluster interface {
// client operations
Fleetctl(m Member, args ...string) (string, string, error)
FleetctlWithInput(m Member, input string, args ...string) (string, string, error)
WaitForNAllUnits(Member, int) error

This comment has been minimized.

Copy link
@antrik

antrik Mar 22, 2016

The naming is a bit weird... Maybe simply WaitForNUnits?

WaitForNActiveUnits(Member, int) (map[string][]util.UnitState, error)
WaitForNMachines(Member, int) ([]string, error)
}
Expand Down
25 changes: 25 additions & 0 deletions functional/platform/nspawn.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,6 +104,31 @@ func (nc *nspawnCluster) FleetctlWithInput(m Member, input string, args ...strin
return util.RunFleetctlWithInput(input, args...)
}

func (nc *nspawnCluster) WaitForNAllUnits(m Member, count int) error {
timeout := 15 * time.Second
alarm := time.After(timeout)

ticker := time.Tick(250 * time.Millisecond)
loop:
for {
select {
case <-alarm:
return fmt.Errorf("failed to find %d units within %v", count, timeout)
case <-ticker:
stdout, _, err := nc.Fleetctl(m, "list-units", "--no-legend")
if err != nil {
continue
}
units := strings.Split(strings.TrimSpace(stdout), "\n")
if (count == 0 && len(stdout) == 0) || len(units) == count {
break loop
}
}
}

return nil
}

This comment has been minimized.

Copy link
@antrik

antrik Mar 22, 2016

This looks much better :-)

And once coreos#1501 is merged, you can use the new common WaitForState() function for implementing this too...


func (nc *nspawnCluster) WaitForNActiveUnits(m Member, count int) (map[string][]util.UnitState, error) {
var nactive int
states := make(map[string][]util.UnitState)
Expand Down
50 changes: 16 additions & 34 deletions functional/unit_action_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,12 @@ func replaceUnitCommon(cmd string) error {

os.Remove(tmpHelloService)

if err := destroyUnitRetrying(cluster, m, fxtHelloService); err != nil {
return fmt.Errorf("Cannot destroy unit %v", fxtHelloService)
if _, _, err := cluster.Fleetctl(m, "destroy", fxtHelloService); err != nil {
return fmt.Errorf("Failed to destroy unit: %v", err)
}

if err := cluster.WaitForNAllUnits(m, 0); err != nil {
return fmt.Errorf("Failed to get every unit to be cleaned up: %v", err)
}

if err := waitForActiveUnitsReplaceCmds(cluster, m, cmd, 0); err != nil {

This comment has been minimized.

Copy link
@antrik

antrik Mar 22, 2016

I don't think you still need that now?...

Expand Down Expand Up @@ -473,12 +477,19 @@ func replaceUnitMultiple(cmd string, n int) error {
// clean up temp services under /tmp
for i := 1; i <= n; i++ {
curHelloService := fmt.Sprintf("/tmp/hello%d.service", i)
os.Remove(curHelloService)

if err := destroyUnitRetrying(cluster, m, fxtHelloService); err != nil {
return fmt.Errorf("Cannot destroy unit %v", fxtHelloService)
if _, _, err := cluster.Fleetctl(m, "destroy", curHelloService); err != nil {
fmt.Printf("Failed to destroy unit: %v", err)
continue
}

os.Remove(curHelloService)
}

if err := cluster.WaitForNAllUnits(m, 0); err != nil {
return fmt.Errorf("Failed to get every unit to be cleaned up: %v", err)
}

os.Remove(tmpFixtures)

if err := waitForActiveUnitsReplaceCmds(cluster, m, cmd, 0); err != nil {
Expand Down Expand Up @@ -552,32 +563,3 @@ func waitForActiveUnitsReplaceCmds(cluster platform.Cluster, m platform.Member,

return nil
}

// destroyUnitRetrying() destroys the unit and ensure it disappears from the
// unit list. It could take a little time until the unit gets destroyed.
func destroyUnitRetrying(cluster platform.Cluster, m platform.Member, serviceFile string) error {
maxAttempts := 3
found := false
var stdout string
var err error
for {
if _, _, err := cluster.Fleetctl(m, "destroy", serviceFile); err != nil {
return fmt.Errorf("Failed to destroy unit: %v", err)
}
stdout, _, err = cluster.Fleetctl(m, "list-units", "--no-legend")
if err != nil {
return fmt.Errorf("Failed to run list-units: %v", err)
}
if strings.TrimSpace(stdout) == "" || maxAttempts == 0 {
found = true
break
}
maxAttempts--
}

if !found {
return fmt.Errorf("Did not find 0 units in cluster: \n%s", stdout)
}

return nil
}

0 comments on commit 8a22f07

Please sign in to comment.