diff --git a/evetest/README.md b/evetest/README.md index ab59426a68e..c19213c6ae4 100644 --- a/evetest/README.md +++ b/evetest/README.md @@ -302,10 +302,10 @@ stdout, stderr, err := device.RunShellScript("uptime", timeout, stdoutWatchdogTi // Read EVE's internal published state (pubsub) var dpcl pillartypes.DevicePortConfigList -evetest.ReadPublication(device, "nim", true, "global", &dpcl) +err := evetest.ReadPublication(device, "nim", true, "global", &dpcl) // Read all publications of a type -items := evetest.ReadAllPublications[pillartypes.AppInstanceStatus]( +items, err := evetest.ReadAllPublications[pillartypes.AppInstanceStatus]( device, "zedmanager", false) // Get the latest device info/metrics (or nil if not yet received) diff --git a/evetest/VERSION b/evetest/VERSION index d7cec1d7df4..4ac0b5130c9 100644 --- a/evetest/VERSION +++ b/evetest/VERSION @@ -1,2 +1,2 @@ # Evetest version. Increment this manually whenever changes are made to the evetest framework. -1.1 +1.2 diff --git a/evetest/edgedevice.go b/evetest/edgedevice.go index 64d8b033646..261910a206e 100644 --- a/evetest/edgedevice.go +++ b/evetest/edgedevice.go @@ -1830,13 +1830,13 @@ func (d *EdgeDevice) FileExists(fileName string) bool { } // ReadFile reads the contents of a file from the device. -func (d *EdgeDevice) ReadFile(fileName string) []byte { +func (d *EdgeDevice) ReadFile(fileName string) ([]byte, error) { ctx, cancel := context.WithTimeout(d.th.ctx, fileTransferTimeout) defer cancel() tmpFile, err := os.CreateTemp("", "eve-file-*") if err != nil { - d.th.t.Fatalf("ReadFile: failed to create temp file: %v", err) + return nil, fmt.Errorf("ReadFile: failed to create temp file: %w", err) } tmpPath := tmpFile.Name() tmpFile.Close() @@ -1844,15 +1844,15 @@ func (d *EdgeDevice) ReadFile(fileName string) []byte { err = d.th.scpFromEVE(ctx, d.devName, fileName, tmpPath, false) if err != nil { - d.th.t.Fatalf("ReadFile: failed to copy %q from device %q: %v", + return nil, fmt.Errorf("ReadFile: failed to copy %q from device %q: %w", fileName, d.devName, err) } data, err := os.ReadFile(tmpPath) if err != nil { - d.th.t.Fatalf("ReadFile: failed to read temp file: %v", err) + return nil, fmt.Errorf("ReadFile: failed to read temp file: %w", err) } - return data + return data, nil } // WriteFile writes content to a file on the device. @@ -2782,12 +2782,11 @@ func (d *EdgeDevice) WatchClusterMetrics() ( // - key: identifies the specific message within the topic to fetch // - output: pointer to a value of type T to unmarshal the message into // -// Returns false if the topic or message does not exist yet (e.g. before the -// agent has first published it) -- callers that need to wait for it should -// poll on the returned bool instead of treating absence as an error. Calls -// t.Fatalf on any other read or unmarshal failure. +// Returns an error if the message does not exist yet (e.g. before the agent has +// first published it), cannot be read, or does not unmarshal into T. Callers +// waiting for a message to appear should poll until the error clears. func ReadPublication[T any](d *EdgeDevice, fromAgent string, persistent bool, - key string, output *T) bool { + key string, output *T) error { fullName := fmt.Sprintf("%T", *new(T)) typeName := fullName[strings.LastIndex(fullName, ".")+1:] var path string @@ -2796,15 +2795,15 @@ func ReadPublication[T any](d *EdgeDevice, fromAgent string, persistent bool, } else { path = fmt.Sprintf("/run/%s/%s/%s.json", fromAgent, typeName, key) } - if !d.FileExists(path) { - return false + data, err := d.ReadFile(path) + if err != nil { + return fmt.Errorf("ReadPublication: %w", err) } - data := d.ReadFile(path) if err := json.Unmarshal(data, output); err != nil { - d.th.t.Fatalf("ReadPublication: failed to unmarshal %q from device %q: %v", + return fmt.Errorf("ReadPublication: failed to unmarshal %q from device %q: %w", path, d.devName, err) } - return true + return nil } // ReadAllPublications retrieves all messages from a pub-sub topic published by @@ -2816,7 +2815,8 @@ func ReadPublication[T any](d *EdgeDevice, fromAgent string, persistent bool, // // Returns a slice of values of type T representing all messages from the topic, // or an error if reading or unmarshaling fails. -func ReadAllPublications[T any](d *EdgeDevice, fromAgent string, persistent bool) []T { +func ReadAllPublications[T any](d *EdgeDevice, fromAgent string, + persistent bool) ([]T, error) { fullName := fmt.Sprintf("%T", *new(T)) typeName := fullName[strings.LastIndex(fullName, ".")+1:] var dir string @@ -2830,20 +2830,29 @@ func ReadAllPublications[T any](d *EdgeDevice, fromAgent string, persistent bool "find "+shellEscape(dir)+" -maxdepth 1 -name '*.json' -type f 2>/dev/null || true", quickSSHCommandTimeout, 0) if err != nil { - d.th.t.Fatalf("ReadAllPublications: failed to list %q on device %q: %v", + return nil, fmt.Errorf("ReadAllPublications: failed to list %q on device %q: %w", dir, d.devName, err) } var results []T - for _, file := range strings.Fields(stdout) { - data := d.ReadFile(file) + // Pubsub keys become file names and may contain spaces. + for _, file := range strings.Split(stdout, "\n") { + file = strings.TrimRight(file, "\r") + if file == "" { + continue + } + data, err := d.ReadFile(file) + if err != nil { + return nil, fmt.Errorf("ReadAllPublications: %w", err) + } var item T if err := json.Unmarshal(data, &item); err != nil { - d.th.t.Fatalf("ReadAllPublications: failed to unmarshal %q from device %q: %v", + return nil, fmt.Errorf( + "ReadAllPublications: failed to unmarshal %q from device %q: %w", file, d.devName, err) } results = append(results, item) } - return results + return results, nil } // getDevUUID returns the device UUID, calling t.Fatalf if not found/onboarded. diff --git a/evetest/ssh.go b/evetest/ssh.go index 6f4d812bfda..9bad2a2bc8b 100644 --- a/evetest/ssh.go +++ b/evetest/ssh.go @@ -210,19 +210,26 @@ func (th *TestHarness) scpFromEVE(ctx context.Context, if recursive { scpArgs = append(scpArgs, "-r") } - // The path after the colon is interpreted by a remote shell, so it needs - // its own shell quoting independent of how this argv element is split - // locally (scp itself is invoked directly via exec, with no local shell - // involved) -- otherwise a remote path containing spaces (e.g. a pubsub - // key like "Application Data Store") gets split into multiple arguments - // remotely. + // Pass the remote path verbatim: scp speaks SFTP (the OpenSSH 9.x default, + // and no -O here selects the legacy protocol), so no remote shell splits + // it and quoting it would make the quotes part of the file name. Spaces + // are safe because this is a single argv element -- exec runs scp + // directly, with no local shell. scpArgs = append(scpArgs, "-i", "/root/.ssh/eve_rsa", - "root@"+eveIP+":"+shellEscape(remotePath), + "root@"+eveIP+":"+remotePath, localPath, ) cmd := exec.CommandContext(ctx, "scp", scpArgs...) - return cmd.Run() + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + if msg := strings.TrimSpace(stderr.String()); msg != "" { + return fmt.Errorf("%w: %s", err, msg) + } + return err + } + return nil } // scpToEVE copies a file (or, when recursive is true, a directory) from a @@ -242,10 +249,18 @@ func (th *TestHarness) scpToEVE(ctx context.Context, scpArgs = append(scpArgs, "-i", "/root/.ssh/eve_rsa", localPath, - "root@"+eveIP+":"+shellEscape(remotePath), + "root@"+eveIP+":"+remotePath, ) cmd := exec.CommandContext(ctx, "scp", scpArgs...) - return cmd.Run() + var stderr bytes.Buffer + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + if msg := strings.TrimSpace(stderr.String()); msg != "" { + return fmt.Errorf("%w: %s", err, msg) + } + return err + } + return nil } // getReachableEVEAddr finds a reachable IP for the given device at the specified diff --git a/evetest/tests/networking/dns_test.go b/evetest/tests/networking/dns_test.go index 4a400955506..bb203c31d5a 100644 --- a/evetest/tests/networking/dns_test.go +++ b/evetest/tests/networking/dns_test.go @@ -319,8 +319,9 @@ func TestDNSFunctionality(test *testing.T) { // ------------------------------------------------------------------ log.Infof("Phase 2: verifying /etc/resolv.conf and mgmt dnsmasq config...") - resolvConf := string(device.ReadFile("/etc/resolv.conf")) - t.Expect(resolvConf).To(ContainSubstring("nameserver 127.0.0.1")) + resolvConf, err := device.ReadFile("/etc/resolv.conf") + t.Expect(err).NotTo(HaveOccurred()) + t.Expect(string(resolvConf)).To(ContainSubstring("nameserver 127.0.0.1")) // The main config file contains static options only; upstream server // entries live in the separate servers file re-read on SIGHUP. @@ -328,13 +329,16 @@ func TestDNSFunctionality(test *testing.T) { configFilePath = "/run/nim/dnsmasq.mgmt.conf" serversFilePath = "/run/nim/dnsmasq.mgmt.servers" ) - dnsmasqConf := string(device.ReadFile(configFilePath)) - t.Expect(dnsmasqConf).To(ContainSubstring("servers-file=" + serversFilePath)) + dnsmasqConf, err := device.ReadFile(configFilePath) + t.Expect(err).NotTo(HaveOccurred()) + t.Expect(string(dnsmasqConf)).To(ContainSubstring("servers-file=" + serversFilePath)) // The servers file may still be updating after Phase 1 — wait for it // to contain all expected servers in cost-ascending order. t.Eventually(func(g Gomega) { - dnsmasqServers := string(device.ReadFile(serversFilePath)) + serversFile, err := device.ReadFile(serversFilePath) + g.Expect(err).NotTo(HaveOccurred()) + dnsmasqServers := string(serversFile) // All expected upstream servers must be present. g.Expect(dnsmasqServers).To(ContainSubstring(badDNS3IP), "eth3 (cost=0) server must appear in mgmt dnsmasq servers file") diff --git a/evetest/tests/networking/pciback_error_test.go b/evetest/tests/networking/pciback_error_test.go index b2cd68252b8..f523bc15b58 100644 --- a/evetest/tests/networking/pciback_error_test.go +++ b/evetest/tests/networking/pciback_error_test.go @@ -74,7 +74,8 @@ func portPciLong(t *WithT, device *evetest.EdgeDevice, phylabel string) string { var pci string t.Eventually(func() string { var aa pillartypes.AssignableAdapters - if !evetest.ReadPublication(device, "domainmgr", false, "global", &aa) { + if err := evetest.ReadPublication( + device, "domainmgr", false, "global", &aa); err != nil { return "" } for _, b := range aa.IoBundleList { diff --git a/evetest/tests/storage/vault_trim_test.go b/evetest/tests/storage/vault_trim_test.go index 4eb25634c19..c28f11e348e 100644 --- a/evetest/tests/storage/vault_trim_test.go +++ b/evetest/tests/storage/vault_trim_test.go @@ -69,8 +69,8 @@ func TestVaultZvolTrimReclaimsBlocks(test *testing.T) { // Wait for vaultmgr to report the default vault ConversionComplete. t.Eventually(func() bool { var status pillartypes.VaultStatus - if !evetest.ReadPublication(device, "vaultmgr", false, - pillartypes.DefaultVaultName, &status) { + if err := evetest.ReadPublication(device, "vaultmgr", false, + pillartypes.DefaultVaultName, &status); err != nil { return false // status not published yet } return status.ConversionComplete diff --git a/evetest/tests/storage/zvol_provisioned_size_test.go b/evetest/tests/storage/zvol_provisioned_size_test.go index aaa50abd6b5..ad9511fd24d 100644 --- a/evetest/tests/storage/zvol_provisioned_size_test.go +++ b/evetest/tests/storage/zvol_provisioned_size_test.go @@ -155,9 +155,11 @@ func TestZVolProvisionedSizeReported(test *testing.T) { // volumemgr pubsub object: AppDiskMetric.ProvisionedBytes is the value the // fix populates. The zvol device path embeds the volume UUID, so match on // it. Poll because volumemgr recomputes disk metrics on its own interval. - t.Eventually(func() uint64 { - for _, m := range evetest.ReadAllPublications[pillartypes.AppDiskMetric]( - device, "volumemgr", false) { + t.Eventually(func(g Gomega) uint64 { + diskMetrics, err := evetest.ReadAllPublications[pillartypes.AppDiskMetric]( + device, "volumemgr", false) + g.Expect(err).NotTo(HaveOccurred()) + for _, m := range diskMetrics { if strings.Contains(m.DiskPath, volUUID.String()) { return m.ProvisionedBytes }