From b9f335bdcf403d547e68fd55b3a5966264d767ef Mon Sep 17 00:00:00 2001 From: Dongsu Park Date: Wed, 9 Nov 2016 16:22:56 +0100 Subject: [PATCH] functional: allow MemberCommand to return stderr Remove Cluster.MemberCommand(), rename MemberCommandStderr() to MemberCommand(), and update every call site accordingly. That way we can clean up leftovers from https://github.com/coreos/fleet/pull/1700. --- functional/connectivity-loss_test.go | 20 +++++++-------- functional/node_test.go | 37 ++++++++++++++-------------- functional/platform/cluster.go | 3 +-- functional/platform/nspawn.go | 23 +---------------- functional/server_test.go | 20 +++++++-------- functional/shutdown_test.go | 33 +++++++++++++------------ functional/unit_action_test.go | 16 ++++++------ 7 files changed, 65 insertions(+), 87 deletions(-) diff --git a/functional/connectivity-loss_test.go b/functional/connectivity-loss_test.go index 702dbd268..7b704e934 100644 --- a/functional/connectivity-loss_test.go +++ b/functional/connectivity-loss_test.go @@ -142,8 +142,9 @@ func TestSingleNodeConnectivityLoss(t *testing.T) { // Cut connection to etcd. // // We use REJECT here, so fleet knows immediately that it's disconnected, rather than waiting for a timeout. - if _, err = cluster.MemberCommand(m0, "sudo", "iptables", "-I", "OUTPUT", "-p", "tcp", "-m", "multiport", "--dports=2379,4001", "-j", "REJECT"); err != nil { - t.Fatal(err) + stdout, stderr, err := cluster.MemberCommand(m0, "sudo", "iptables", "-I", "OUTPUT", "-p", "tcp", "-m", "multiport", "--dports=2379,4001", "-j", "REJECT") + if err != nil { + t.Fatalf("Failed to cut connection to etcd.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Wait long enough to be reasonably confident that no more state changes will happen. @@ -159,9 +160,8 @@ func TestSingleNodeConnectivityLoss(t *testing.T) { // as fleet is not available to give us this information... // We have to go deeper, and try to obtain the information from systemd directly. actualSystemdFiles := map[string]string{} - var stdout string for name, _ := range expectedSystemdFiles { - stdout, _ := cluster.MemberCommand(m0, "systemctl", "is-enabled", name) + stdout, _, _ := cluster.MemberCommand(m0, "systemctl", "is-enabled", name) // do not check for error, as systemctl is-enabled returns exit status 1 for linked-runtime. stdout = strings.TrimSpace(stdout) if stdout == "" { @@ -174,9 +174,9 @@ func TestSingleNodeConnectivityLoss(t *testing.T) { t.Fatalf("Units files not in expected state after losing connectivity.\nExpected: %v\nActual: %v", expectedSystemdFiles, actualSystemdFiles) } - stdout, err = cluster.MemberCommand(m0, "systemctl", "list-units", "-t", "service", "--no-legend", "single@*.service", "global@*.service") + stdout, stderr, err = cluster.MemberCommand(m0, "systemctl", "list-units", "-t", "service", "--no-legend", "single@*.service", "global@*.service") if err != nil { - t.Fatalf("Failed to retrieve systemd unit states: %v", err) + t.Fatalf("Failed to retrieve systemd unit states.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } stdout = strings.TrimSpace(stdout) actualSystemdStates := map[string][]string{} @@ -191,8 +191,8 @@ func TestSingleNodeConnectivityLoss(t *testing.T) { } // Restore etcd connection. - if _, err = cluster.MemberCommand(m0, "sudo", "iptables", "-D", "OUTPUT", "-p", "tcp", "-m", "multiport", "--dports=2379,4001", "-j", "REJECT"); err != nil { - t.Fatal(err) + if stdout, stderr, err = cluster.MemberCommand(m0, "sudo", "iptables", "-D", "OUTPUT", "-p", "tcp", "-m", "multiport", "--dports=2379,4001", "-j", "REJECT"); err != nil { + t.Fatalf("Failed to restore etcd connection.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Again, wait long enough to be reasonably confident that no more state changes will happen. @@ -219,9 +219,9 @@ func TestSingleNodeConnectivityLoss(t *testing.T) { } // Additionally check the logs of all active units for possible temporary state flapping. - stdout, err = cluster.MemberCommand(m0, "journalctl", "_PID=1") + stdout, stderr, err = cluster.MemberCommand(m0, "journalctl", "_PID=1") if err != nil { - t.Fatalf("Failed to retrieve journal: %v", err) + t.Fatalf("Failed to retrieve journal.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } if strings.Contains(stdout, "Stopping single@") || strings.Contains(stdout, "Stopping global@") { t.Fatalf("Units were unexpectedly stopped at some point:\n%s", stdout) diff --git a/functional/node_test.go b/functional/node_test.go index 6beca2bb7..3192f2516 100644 --- a/functional/node_test.go +++ b/functional/node_test.go @@ -62,8 +62,8 @@ func TestNodeShutdown(t *testing.T) { } // Stop the fleet process on the first member - if stdout, err = cluster.MemberCommand(m0, "sudo", "systemctl", "stop", "fleet"); err != nil { - t.Fatalf("Failed stopping fleet service: %v\nstdout: %s\n", err, stdout) + if stdout, stderr, err = cluster.MemberCommand(m0, "sudo", "systemctl", "stop", "fleet"); err != nil { + t.Fatalf("Failed stopping fleet service:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // The first member should quickly remove itself from the published @@ -81,11 +81,10 @@ func TestNodeShutdown(t *testing.T) { // NOTE: In case of no units, systemd v230 or older prints out // "Active: inactive" to stdout, while systemd v231 or newer prints out // "Unit NAME could not be found" to stderr. So we need to check for - // both cases. Use MemberCommandStderr() to retrieve both stdout and stderr, - // and check for each case. - stdout, stderr, err = cluster.MemberCommandStderr(m0, "systemctl", "status", "hello.service") + // both cases. + stdout, stderr, err = cluster.MemberCommand(m0, "systemctl", "status", "hello.service") if !strings.Contains(stdout, "Active: inactive") && !strings.Contains(stderr, "could not be found") { - t.Fatalf("Unit hello.service not reported as inactive:\n%s\n", stdout) + t.Fatalf("Unit hello.service not reported as inactive:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } } @@ -118,35 +117,35 @@ func TestDetectMachineId(t *testing.T) { // Restart fleet service, and check if its systemd status is still active. restartFleetService := func(m platform.Member) error { - stdout, err := cluster.MemberCommand(m, "sudo", "systemctl", "restart", "fleet.service") + stdout, stderr, err := cluster.MemberCommand(m, "sudo", "systemctl", "restart", "fleet.service") if err != nil { - return fmt.Errorf("Failed to restart fleet service\nstdout: %s\nerr: %v", stdout, err) + return fmt.Errorf("Failed to restart fleet service\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } - stdout, err = cluster.MemberCommand(m, "systemctl", "show", "--property=ActiveState", "fleet") + stdout, stderr, err = cluster.MemberCommand(m, "systemctl", "show", "--property=ActiveState", "fleet") if strings.TrimSpace(stdout) != "ActiveState=active" { - return fmt.Errorf("Fleet unit not reported as active:\nstdout:%s\nerr: %v", stdout, err) + return fmt.Errorf("Fleet unit not reported as active:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } - stdout, err = cluster.MemberCommand(m, "systemctl", "show", "--property=Result", "fleet") + stdout, stderr, err = cluster.MemberCommand(m, "systemctl", "show", "--property=Result", "fleet") if strings.TrimSpace(stdout) != "Result=success" { - return fmt.Errorf("Result for fleet unit not reported as success:\nstdout:%s\nerr: %v", stdout, err) + return fmt.Errorf("Result for fleet unit not reported as success:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } return nil } - stdout, err := cluster.MemberCommand(m0, "cat", machineIdFile) + stdout, stderr, err := cluster.MemberCommand(m0, "cat", machineIdFile) if err != nil { - t.Fatalf("Failed to get machine-id\nstdout: %s\nerr: %v", stdout, err) + t.Fatalf("Failed to get machine-id\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } m0_machine_id := strings.TrimSpace(stdout) // If the two machine IDs are different with each other, // set the m1's ID to the same one as m0, to intentionally // trigger an error case of duplication of machine ID. - stdout, err = cluster.MemberCommand(m1, + stdout, stderr, err = cluster.MemberCommand(m1, "echo", m0_machine_id, "|", "sudo", "tee", machineIdFile) if err != nil { - t.Fatalf("Failed to replace machine-id\nstdout: %s\nerr: %v", stdout, err) + t.Fatalf("Failed to replace machine-id\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } if err := restartFleetService(m1); err != nil { @@ -156,7 +155,7 @@ func TestDetectMachineId(t *testing.T) { // fleetd should actually be running, but failing to list machines. // So we should expect a specific error after running fleetctl list-machines, // like "googlapi: Error 503: fleet server unable to communicate with etcd". - stdout, stderr, err := cluster.Fleetctl(m1, "list-machines", "--no-legend") + stdout, stderr, err = cluster.Fleetctl(m1, "list-machines", "--no-legend") if err != nil { if !strings.Contains(err.Error(), "exit status 1") || !strings.Contains(stderr, "fleet server unable to communicate with etcd") { @@ -171,10 +170,10 @@ func TestDetectMachineId(t *testing.T) { // Trigger another test case of m0's ID getting different from m1's. // Then it's expected that m0 and m1 would be working properly with distinct // machine IDs, after having restarted fleet.service both on m0 and m1. - stdout, err = cluster.MemberCommand(m0, + stdout, stderr, err = cluster.MemberCommand(m0, "echo", util.NewMachineID(), "|", "sudo", "tee", machineIdFile) if err != nil { - t.Fatalf("m0: Failed to replace machine-id\nstdout: %s\nerr: %v", stdout, err) + t.Fatalf("m0: Failed to replace machine-id\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Restart fleet service on m0, and see that it's still working. diff --git a/functional/platform/cluster.go b/functional/platform/cluster.go index a03a7dd3d..dc51afeb4 100644 --- a/functional/platform/cluster.go +++ b/functional/platform/cluster.go @@ -31,8 +31,7 @@ type Cluster interface { DestroyMember(Member) error ReplaceMember(Member) (Member, error) Members() []Member - MemberCommand(Member, ...string) (string, error) - MemberCommandStderr(Member, ...string) (string, string, error) + MemberCommand(Member, ...string) (string, string, error) Destroy(t *testing.T) error // client operations diff --git a/functional/platform/nspawn.go b/functional/platform/nspawn.go index 11d727a00..0ee68cc1b 100644 --- a/functional/platform/nspawn.go +++ b/functional/platform/nspawn.go @@ -379,28 +379,7 @@ func (nc *nspawnCluster) Members() []Member { return ms } -func (nc *nspawnCluster) MemberCommand(m Member, args ...string) (string, error) { - baseArgs := []string{"-o", "UserKnownHostsFile=/dev/null", "-o", "StrictHostKeyChecking=no", fmt.Sprintf("core@%s", m.IP())} - args = append(baseArgs, args...) - log.Printf("ssh %s", strings.Join(args, " ")) - var stdoutBytes bytes.Buffer - cmd := exec.Command("ssh", args...) - cmd.Stdout = &stdoutBytes - err := cmd.Run() - return stdoutBytes.String(), err -} - -// MemberCommandStderr() is like MemberCommand(), except that it returns -// both stdout and stderr from the running command. This is definitely useful -// for most cases, especially when the caller needs to check for both stdout -// and stderr. -// -// NOTE: Ideally we should remove MemberCommand() above, and rename -// MemberCommandStderr() to MemberCommand(). For doing that, however, we need -// to change every call site to replace MemberCommand() with MemberCommandStderr(). -// Of course that would be a lot of work. I'll leave that to future work. -// - dpark 20161102 -func (nc *nspawnCluster) MemberCommandStderr(m Member, args ...string) (string, string, error) { +func (nc *nspawnCluster) MemberCommand(m Member, args ...string) (string, string, error) { baseArgs := []string{"-o", "UserKnownHostsFile=/dev/null", "-o", "StrictHostKeyChecking=no", fmt.Sprintf("core@%s", m.IP())} args = append(baseArgs, args...) log.Printf("ssh %s", strings.Join(args, " ")) diff --git a/functional/server_test.go b/functional/server_test.go index 7de0a2072..1a54044c6 100644 --- a/functional/server_test.go +++ b/functional/server_test.go @@ -65,9 +65,9 @@ func TestReconfigureServer(t *testing.T) { // Send a SIGHUP to fleetd, and periodically checks if a message // "Reloading configuration" appears in fleet's journal, up to timeout (15) seconds. - stdout, _ = cluster.MemberCommand(m0, "sudo", "systemctl", "kill", "-s", "SIGHUP", "fleet") + stdout, stderr, err = cluster.MemberCommand(m0, "sudo", "systemctl", "kill", "-s", "SIGHUP", "fleet") if strings.TrimSpace(stdout) != "" { - t.Fatalf("Sending SIGHUP to fleetd returned: %s", stdout) + t.Fatalf("Sending SIGHUP to fleetd returned.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Watch the logs if fleet was correctly reloaded @@ -97,19 +97,19 @@ func TestReconfigureServer(t *testing.T) { } // Check for HTTP listener error looking into the fleetd journal - stdout, _ = cluster.MemberCommand(m0, "journalctl _PID=$(pidof fleetd)") + stdout, stderr, err = cluster.MemberCommand(m0, "journalctl _PID=$(pidof fleetd)") if strings.Contains(strings.TrimSpace(stdout), "Failed serving HTTP on listener:") { - t.Fatalf("Fleetd log returned error on HTTP listeners: %s", stdout) + t.Fatalf("Fleetd log returned error on HTTP listeners.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Check expected state after reconfiguring fleetd - stdout, _ = cluster.MemberCommand(m0, "systemctl", "show", "--property=ActiveState", "fleet") + stdout, stderr, err = cluster.MemberCommand(m0, "systemctl", "show", "--property=ActiveState", "fleet") if strings.TrimSpace(stdout) != "ActiveState=active" { - t.Fatalf("Fleet unit not reported as active: %s", stdout) + t.Fatalf("Fleet unit not reported as active.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } - stdout, _ = cluster.MemberCommand(m0, "systemctl", "show", "--property=Result", "fleet") + stdout, stderr, err = cluster.MemberCommand(m0, "systemctl", "show", "--property=Result", "fleet") if strings.TrimSpace(stdout) != "Result=success" { - t.Fatalf("Result for fleet unit not reported as success: %s", stdout) + t.Fatalf("Result for fleet unit not reported as success.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } } @@ -123,7 +123,7 @@ func waitForReloadConfig(cluster platform.Cluster, m0 platform.Member) (err erro // "journalctl -u fleet | grep \"Reloading configuration\"" is racy // in a subtle way, so that it sometimes fails only on semaphoreci. // - dpark 20160408 - stdout, _ := cluster.MemberCommand(m0, "sudo", "journalctl --priority=info _PID=$(pidof fleetd)") + stdout, _, _ := cluster.MemberCommand(m0, "sudo", "journalctl --priority=info _PID=$(pidof fleetd)") journalfleet := strings.TrimSpace(stdout) if !strings.Contains(journalfleet, "Reloading configuration") { fmt.Errorf("Fleetd is not fully reconfigured, retrying... entire fleet journal:\n%v", journalfleet) @@ -144,7 +144,7 @@ func waitForReloadConfig(cluster platform.Cluster, m0 platform.Member) (err erro func waitForFleetdSocket(cluster platform.Cluster, m0 platform.Member) (err error) { _, err = util.WaitForState( func() bool { - stdout, _ := cluster.MemberCommand(m0, "test -S /var/run/fleet.sock && echo 1") + stdout, _, _ := cluster.MemberCommand(m0, "test -S /var/run/fleet.sock && echo 1") if strings.TrimSpace(stdout) == "" { fmt.Errorf("Fleetd is not fully started, retrying...") return false diff --git a/functional/shutdown_test.go b/functional/shutdown_test.go index cfc60a756..33e9573e7 100644 --- a/functional/shutdown_test.go +++ b/functional/shutdown_test.go @@ -39,18 +39,19 @@ func TestShutdown(t *testing.T) { } // Stop the fleet process. - if _, err = cluster.MemberCommand(m0, "sudo", "systemctl", "stop", "fleet"); err != nil { - t.Fatal(err) + stdout, stderr, err := cluster.MemberCommand(m0, "sudo", "systemctl", "stop", "fleet") + if err != nil { + t.Fatalf("Failed to stop fleet process.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Check expected state after stop. - stdout, _ := cluster.MemberCommand(m0, "systemctl", "show", "--property=ActiveState", "fleet") + stdout, stderr, err = cluster.MemberCommand(m0, "systemctl", "show", "--property=ActiveState", "fleet") if strings.TrimSpace(stdout) != "ActiveState=inactive" { - t.Fatalf("Fleet unit not reported as inactive: %s", stdout) + t.Fatalf("Fleet unit not reported as inactive.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } - stdout, _ = cluster.MemberCommand(m0, "systemctl", "show", "--property=Result", "fleet") + stdout, stderr, err = cluster.MemberCommand(m0, "systemctl", "show", "--property=Result", "fleet") if strings.TrimSpace(stdout) != "Result=success" { - t.Fatalf("Result for fleet unit not reported as success: %s", stdout) + t.Fatalf("Result for fleet unit not reported as success.\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } } @@ -74,32 +75,32 @@ func TestShutdownVsMonitor(t *testing.T) { // Cut connection to etcd. // // This will result in a failed health check, and consequently the monitor will attempt a restart. - stdout, err := cluster.MemberCommand(m0, "sudo", "iptables", "-I", "OUTPUT", "-p", "tcp", "-m", "multiport", "--dports=2379,4001", "-j", "DROP") + stdout, stderr, err := cluster.MemberCommand(m0, "sudo", "iptables", "-I", "OUTPUT", "-p", "tcp", "-m", "multiport", "--dports=2379,4001", "-j", "DROP") if err != nil { - t.Fatalf("Failed inserting iptables rule:\nstdout: %s\nerr: %v", stdout, err) + t.Fatalf("Failed inserting iptables rule:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Wait for the monitor to trigger the restart. // // This will never complete, as long as there is no connectivity. - stdout, err = cluster.MemberCommand(m0, "sudo", "sh", "-c", `'until journalctl -u fleet | grep -q "Server monitor triggered: Monitor timed out before successful heartbeat"; do sleep 1; done'`) + stdout, stderr, err = cluster.MemberCommand(m0, "sudo", "sh", "-c", `'until journalctl -u fleet | grep -q "Server monitor triggered: Monitor timed out before successful heartbeat"; do sleep 1; done'`) if err != nil { - t.Fatalf("Failed checking journal message:\nstdout: %s\nerr: %v", stdout, err) + t.Fatalf("Failed checking journal message:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Stop fleetd while the restart is still in progress. - stdout, err = cluster.MemberCommand(m0, "sudo", "systemctl", "stop", "fleet") + stdout, stderr, err = cluster.MemberCommand(m0, "sudo", "systemctl", "stop", "fleet") if err != nil { - t.Fatalf("Failed stopping fleet service:\nstdout: %s\nerr: %v", stdout, err) + t.Fatalf("Failed stopping fleet service:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } // Verify that fleetd was shut down cleanly in spite of the concurrent restart. - stdout, _ = cluster.MemberCommand(m0, "systemctl", "show", "--property=ActiveState", "fleet") + stdout, stderr, err = cluster.MemberCommand(m0, "systemctl", "show", "--property=ActiveState", "fleet") if strings.TrimSpace(stdout) != "ActiveState=inactive" { - t.Fatalf("Fleet unit not reported as inactive: %s", stdout) + t.Fatalf("Fleet unit not reported as inactive:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } - stdout, _ = cluster.MemberCommand(m0, "systemctl", "show", "--property=Result", "fleet") + stdout, stderr, err = cluster.MemberCommand(m0, "systemctl", "show", "--property=Result", "fleet") if strings.TrimSpace(stdout) != "Result=success" { - t.Fatalf("Result for fleet unit not reported as success: %s", stdout) + t.Fatalf("Result for fleet unit not reported as success:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } } diff --git a/functional/unit_action_test.go b/functional/unit_action_test.go index 8a5d95122..613161d45 100644 --- a/functional/unit_action_test.go +++ b/functional/unit_action_test.go @@ -307,8 +307,8 @@ func TestUnitStatus(t *testing.T) { stdout, stderr, err = cluster.Fleetctl(m, "--strict-host-key-checking=false", "status", path.Base(unitFile)) if !strings.Contains(stdout, "Loaded: loaded") { - t.Errorf("Could not find expected string in status output:\n%s\nstderr:\n%s", - stdout, stderr) + t.Errorf("Could not find expected string in status output:\nstdout: %s\nstderr:\nerr: %s", + stdout, stderr, err) } } @@ -345,12 +345,12 @@ func TestListUnitFilesOrder(t *testing.T) { // make sure that all unit files will show up _, err = cluster.WaitForNUnitFiles(m, 20) if err != nil { - t.Fatal("Failed to run list-unit-files: %v", err) + t.Fatalf("Failed to run list-unit-files: %v", err) } stdout, stderr, err := cluster.Fleetctl(m, "list-unit-files", "--no-legend", "--fields", "unit") if err != nil { - t.Fatal("Failed to run list-unit-files:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) + t.Fatalf("Failed to run list-unit-files:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } outUnits := strings.Split(strings.TrimSpace(stdout), "\n") @@ -607,13 +607,13 @@ func checkListUnits(cl platform.Cluster, m platform.Member, cmd string, ufs []st if cmd == "start" { // Check expected systemd state after starting units - stdout, _ := cl.MemberCommand(m, "systemctl", "show", "--property=ActiveState", ufs[i]) + stdout, stderr, err := cl.MemberCommand(m, "systemctl", "show", "--property=ActiveState", ufs[i]) if strings.TrimSpace(stdout) != "ActiveState=active" { - return fmt.Errorf("Fleet unit not reported as active: %s", stdout) + return fmt.Errorf("Fleet unit not reported as active:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } - stdout, _ = cl.MemberCommand(m, "systemctl", "show", "--property=Result", ufs[i]) + stdout, stderr, err = cl.MemberCommand(m, "systemctl", "show", "--property=Result", ufs[i]) if strings.TrimSpace(stdout) != "Result=success" { - return fmt.Errorf("Result for fleet unit not reported as success: %s", stdout) + return fmt.Errorf("Result for fleet unit not reported as success:\nstdout: %s\nstderr: %s\nerr: %v", stdout, stderr, err) } } }