Skip to content

Clean up error logging in test #1110

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 18, 2023
Merged

Clean up error logging in test #1110

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions test/e2e/tests/multi_zone_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,7 @@ func testAttachWriteReadDetach(volID string, volName string, instance *remote.In
testFile := filepath.Join(a.publishDir, "testfile")
err := testutils.WriteFile(instance, testFile, testFileContents)
if err != nil {
return fmt.Errorf("Failed to write file: %v", err)
return fmt.Errorf("Failed to write file: %v", err.Error())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. what is the reason we need err.Error() here?
  2. .Error() returns a string so perhaps %s or %q instead of %v?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep consistency in logging. Right now we use either %w with err or %v with err.Error() when the error is not nil.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I still don't quite understand what's the context behind the need to add ".Error()" for all err messaging, is there a custom Error() function we're preparing?

Copy link
Contributor Author

@sunnylovestiramisu sunnylovestiramisu Jan 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.Error() unwrap and return the error message only, err obj itself printing in %v may contain some other information depending on the error, like error code string etc.

}
}
return nil
Expand All @@ -168,7 +168,7 @@ func testAttachWriteReadDetach(volID string, volName string, instance *remote.In
secondTestFile := filepath.Join(a.publishDir, "testfile")
readContents, err := testutils.ReadFile(instance, secondTestFile)
if err != nil {
return fmt.Errorf("ReadFile failed with error: %v", err)
return fmt.Errorf("ReadFile failed with error: %v", err.Error())
}
if strings.TrimSpace(string(readContents)) != testFileContents {
return fmt.Errorf("wanted test file content: %s, got content: %s", testFileContents, readContents)
Expand All @@ -184,7 +184,7 @@ func testLifecycleWithVerify(volID string, volName string, instance *remote.Inst
// Attach Disk
err = client.ControllerPublishVolume(volID, instance.GetNodeID())
if err != nil {
return fmt.Errorf("ControllerPublishVolume failed with error for disk %v on node %v: %v", volID, instance.GetNodeID(), err)
return fmt.Errorf("ControllerPublishVolume failed with error for disk %v on node %v: %v", volID, instance.GetNodeID(), err.Error())
}

defer func() {
Expand Down Expand Up @@ -232,11 +232,11 @@ func testLifecycleWithVerify(volID string, volName string, instance *remote.Inst
}

if err != nil {
return fmt.Errorf("NodePublishVolume failed with error: %v", err)
return fmt.Errorf("NodePublishVolume failed with error: %v", err.Error())
}
err = testutils.ForceChmod(instance, filepath.Join("/tmp/", volName), "777")
if err != nil {
return fmt.Errorf("Chmod failed with error: %v", err)
return fmt.Errorf("Chmod failed with error: %v", err.Error())
}

a := verifyArgs{
Expand All @@ -251,7 +251,7 @@ func testLifecycleWithVerify(volID string, volName string, instance *remote.Inst
// Unmount Disk
err = client.NodeUnpublishVolume(volID, publishDir)
if err != nil {
return fmt.Errorf("NodeUnpublishVolume failed with error: %v", err)
return fmt.Errorf("NodeUnpublishVolume failed with error: %v", err.Error())
}

if secondMountVerify != nil {
Expand All @@ -263,7 +263,7 @@ func testLifecycleWithVerify(volID string, volName string, instance *remote.Inst
err = client.NodePublishVolume(volID, stageDir, secondPublishDir)
}
if err != nil {
return fmt.Errorf("NodePublishVolume failed with error: %v", err)
return fmt.Errorf("NodePublishVolume failed with error: %v", err.Error())
}
err = testutils.ForceChmod(instance, filepath.Join("/tmp/", volName), "777")
if err != nil {
Expand All @@ -275,13 +275,13 @@ func testLifecycleWithVerify(volID string, volName string, instance *remote.Inst
}
err = secondMountVerify(b)
if err != nil {
return fmt.Errorf("failed to verify after second mount to %s: %v", publishDir, err)
return fmt.Errorf("failed to verify after second mount to %s: %v", publishDir, err.Error())
}

// Unmount Disk
err = client.NodeUnpublishVolume(volID, secondPublishDir)
if err != nil {
return fmt.Errorf("NodeUnpublishVolume failed with error: %v", err)
return fmt.Errorf("NodeUnpublishVolume failed with error: %v", err.Error())
}
}

Expand Down
8 changes: 4 additions & 4 deletions test/e2e/tests/single_zone_e2e_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -692,7 +692,7 @@ var _ = Describe("GCE PD CSI Driver", func() {
verifyVolumeStats := func(a verifyArgs) error {
available, capacity, used, inodesFree, inodes, inodesUsed, err := client.NodeGetVolumeStats(volID, a.publishDir)
if err != nil {
return fmt.Errorf("failed to get node volume stats: %v", err)
return fmt.Errorf("failed to get node volume stats: %v", err.Error())
}
if available != 0 || capacity != common.GbToBytes(defaultSizeGb) || used != 0 ||
inodesFree != 0 || inodes != 0 || inodesUsed != 0 {
Expand Down Expand Up @@ -729,7 +729,7 @@ var _ = Describe("GCE PD CSI Driver", func() {
verifyVolumeStats := func(a verifyArgs) error {
available, capacity, used, inodesFree, inodes, inodesUsed, err := client.NodeGetVolumeStats(volID, a.publishDir)
if err != nil {
return fmt.Errorf("failed to get node volume stats: %v", err)
return fmt.Errorf("failed to get node volume stats: %v", err.Error())
}
if !equalWithinEpsilon(available, common.GbToBytes(defaultSizeGb), defaultEpsilon) || !equalWithinEpsilon(capacity, common.GbToBytes(defaultSizeGb), defaultEpsilon) || !equalWithinEpsilon(used, 0, defaultEpsilon) ||
inodesFree == 0 || inodes == 0 || inodesUsed == 0 {
Expand Down Expand Up @@ -795,14 +795,14 @@ var _ = Describe("GCE PD CSI Driver", func() {
writeFunc := func(a verifyArgs) error {
err := testutils.WriteBlock(instance, a.publishDir, testFileContents)
if err != nil {
return fmt.Errorf("Failed to write file: %v", err)
return fmt.Errorf("Failed to write file: %v", err.Error())
}
return nil
}
verifyReadFunc := func(a verifyArgs) error {
readContents, err := testutils.ReadBlock(instance, a.publishDir, len(testFileContents))
if err != nil {
return fmt.Errorf("ReadFile failed with error: %v", err)
return fmt.Errorf("ReadFile failed with error: %v", err.Error())
}
if strings.TrimSpace(string(readContents)) != testFileContents {
return fmt.Errorf("wanted test file content: %s, got content: %s", testFileContents, readContents)
Expand Down
34 changes: 17 additions & 17 deletions test/e2e/utils/utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,43 +147,43 @@ func SetupProwConfig(resourceType string) (project, serviceAccount string) {
func ForceChmod(instance *remote.InstanceInfo, filePath string, perms string) error {
originalumask, err := instance.SSHNoSudo("umask")
if err != nil {
return fmt.Errorf("failed to umask. Output: %v, errror: %v", originalumask, err)
return fmt.Errorf("failed to umask. Output: %v, errror: %v", originalumask, err.Error())
}
output, err := instance.SSHNoSudo("umask", "0000")
if err != nil {
return fmt.Errorf("failed to umask. Output: %v, errror: %v", output, err)
return fmt.Errorf("failed to umask. Output: %v, errror: %v", output, err.Error())
}
output, err = instance.SSH("chmod", "-R", perms, filePath)
if err != nil {
return fmt.Errorf("failed to chmod file %s. Output: %v, errror: %v", filePath, output, err)
return fmt.Errorf("failed to chmod file %s. Output: %v, errror: %v", filePath, output, err.Error())
}
output, err = instance.SSHNoSudo("umask", originalumask)
if err != nil {
return fmt.Errorf("failed to umask. Output: %v, errror: %v", output, err)
return fmt.Errorf("failed to umask. Output: %v, errror: %v", output, err.Error())
}
return nil
}

func WriteFile(instance *remote.InstanceInfo, filePath, fileContents string) error {
output, err := instance.SSHNoSudo("echo", fileContents, ">", filePath)
if err != nil {
return fmt.Errorf("failed to write test file %s. Output: %v, errror: %v", filePath, output, err)
return fmt.Errorf("failed to write test file %s. Output: %v, errror: %v", filePath, output, err.Error())
}
return nil
}

func ReadFile(instance *remote.InstanceInfo, filePath string) (string, error) {
output, err := instance.SSHNoSudo("cat", filePath)
if err != nil {
return "", fmt.Errorf("failed to read test file %s. Output: %v, errror: %v", filePath, output, err)
return "", fmt.Errorf("failed to read test file %s. Output: %v, errror: %v", filePath, output, err.Error())
}
return output, nil
}

func WriteBlock(instance *remote.InstanceInfo, path, fileContents string) error {
output, err := instance.SSHNoSudo("echo", fileContents, "|", "dd", "of="+path)
if err != nil {
return fmt.Errorf("failed to write test file %s. Output: %v, errror: %v", path, output, err)
return fmt.Errorf("failed to write test file %s. Output: %v, errror: %v", path, output, err.Error())
}
return nil
}
Expand All @@ -192,15 +192,15 @@ func ReadBlock(instance *remote.InstanceInfo, path string, length int) (string,
lengthStr := strconv.Itoa(length)
output, err := instance.SSHNoSudo("dd", "if="+path, "bs="+lengthStr, "count=1", "2>", "/dev/null")
if err != nil {
return "", fmt.Errorf("failed to read test file %s. Output: %v, errror: %v", path, output, err)
return "", fmt.Errorf("failed to read test file %s. Output: %v, errror: %v", path, output, err.Error())
}
return output, nil
}

func GetFSSizeInGb(instance *remote.InstanceInfo, mountPath string) (int64, error) {
output, err := instance.SSH("df", "--output=size", "-BG", mountPath, "|", "awk", "'NR==2'")
if err != nil {
return -1, fmt.Errorf("failed to get size of path %s. Output: %v, error: %v", mountPath, output, err)
return -1, fmt.Errorf("failed to get size of path %s. Output: %v, error: %v", mountPath, output, err.Error())
}
output = strings.TrimSuffix(strings.TrimSpace(output), "G")
n, err := strconv.ParseInt(output, 10, 64)
Expand All @@ -213,7 +213,7 @@ func GetFSSizeInGb(instance *remote.InstanceInfo, mountPath string) (int64, erro
func GetBlockSizeInGb(instance *remote.InstanceInfo, devicePath string) (int64, error) {
output, err := instance.SSH("blockdev", "--getsize64", devicePath)
if err != nil {
return -1, fmt.Errorf("failed to get size of path %s. Output: %v, error: %v", devicePath, output, err)
return -1, fmt.Errorf("failed to get size of path %s. Output: %v, error: %v", devicePath, output, err.Error())
}
n, err := strconv.ParseInt(strings.TrimSpace(output), 10, 64)
if err != nil {
Expand All @@ -225,31 +225,31 @@ func GetBlockSizeInGb(instance *remote.InstanceInfo, devicePath string) (int64,
func Symlink(instance *remote.InstanceInfo, src, dest string) error {
output, err := instance.SSH("ln", "-s", src, dest)
if err != nil {
return fmt.Errorf("failed to symlink from %s to %s. Output: %v, errror: %v", src, dest, output, err)
return fmt.Errorf("failed to symlink from %s to %s. Output: %v, errror: %v", src, dest, output, err.Error())
}
return nil
}

func RmAll(instance *remote.InstanceInfo, filePath string) error {
output, err := instance.SSH("rm", "-rf", filePath)
if err != nil {
return fmt.Errorf("failed to delete all %s. Output: %v, errror: %v", filePath, output, err)
return fmt.Errorf("failed to delete all %s. Output: %v, errror: %v", filePath, output, err.Error())
}
return nil
}

func MkdirAll(instance *remote.InstanceInfo, dir string) error {
output, err := instance.SSH("mkdir", "-p", dir)
if err != nil {
return fmt.Errorf("failed to mkdir -p %s. Output: %v, errror: %v", dir, output, err)
return fmt.Errorf("failed to mkdir -p %s. Output: %v, errror: %v", dir, output, err.Error())
}
return nil
}

func CopyFile(instance *remote.InstanceInfo, src, dest string) error {
output, err := instance.SSH("cp", src, dest)
if err != nil {
return fmt.Errorf("failed to copy %s to %s. Output: %v, errror: %v", src, dest, output, err)
return fmt.Errorf("failed to copy %s to %s. Output: %v, errror: %v", src, dest, output, err.Error())
}
return nil
}
Expand All @@ -273,7 +273,7 @@ func ValidateLogicalLinkIsDisk(instance *remote.InstanceInfo, link, diskName str

devFsPath, err := instance.SSH("find", link, "-printf", "'%l'")
if err != nil {
return false, fmt.Errorf("failed to find symbolic link for %s. Output: %v, errror: %v", link, devFsPath, err)
return false, fmt.Errorf("failed to find symbolic link for %s. Output: %v, errror: %v", link, devFsPath, err.Error())
}
if len(devFsPath) == 0 {
return false, nil
Expand All @@ -282,7 +282,7 @@ func ValidateLogicalLinkIsDisk(instance *remote.InstanceInfo, link, diskName str
fullDevPath := path.Join("/dev/", string(sdx))
scsiIDOut, err := instance.SSH("/lib/udev_containerized/scsi_id", "--page=0x83", "--whitelisted", fmt.Sprintf("--device=%v", fullDevPath))
if err != nil {
return false, fmt.Errorf("failed to find %s's SCSI ID. Output: %v, errror: %v", devFsPath, scsiIDOut, err)
return false, fmt.Errorf("failed to find %s's SCSI ID. Output: %v, errror: %v", devFsPath, scsiIDOut, err.Error())
}
scsiID := scsiRegex.FindStringSubmatch(scsiIDOut)
if len(scsiID) == 0 {
Expand All @@ -296,7 +296,7 @@ func ValidateLogicalLinkIsDisk(instance *remote.InstanceInfo, link, diskName str
fullDevPath := path.Join("/dev/", string(nvmex))
nvmeIDOut, err := instance.SSH("/lib/udev_containerized/google_nvme_id", fmt.Sprintf("-d%v", fullDevPath))
if err != nil {
return false, fmt.Errorf("failed to find %s's NVME ID. Output: %v, errror: %v", devFsPath, nvmeIDOut, err)
return false, fmt.Errorf("failed to find %s's NVME ID. Output: %v, errror: %v", devFsPath, nvmeIDOut, err.Error())
}
nvmeID := nvmeSerialRegex.FindStringSubmatch(nvmeIDOut)
if len(nvmeID) == 0 {
Expand Down
24 changes: 12 additions & 12 deletions test/k8s-integration/cluster.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func clusterDownGKE(gceZone, gceRegion string) error {
locationArg, locationVal, "--quiet")
err = runCommand("Bringing Down E2E Cluster on GKE", cmd)
if err != nil {
return fmt.Errorf("failed to bring down kubernetes e2e cluster on gke: %v", err)
return fmt.Errorf("failed to bring down kubernetes e2e cluster on gke: %v", err.Error())
}
return nil
}
Expand Down Expand Up @@ -126,7 +126,7 @@ func clusterUpGCE(k8sDir, gceZone string, numNodes int, numWindowsNodes int, ima
cmd.Env = os.Environ()
err = runCommand("Starting E2E Cluster on GCE", cmd)
if err != nil {
return fmt.Errorf("failed to bring up kubernetes e2e cluster on gce: %v", err)
return fmt.Errorf("failed to bring up kubernetes e2e cluster on gce: %v", err.Error())
}

return nil
Expand Down Expand Up @@ -170,7 +170,7 @@ func clusterUpGKE(gceZone, gceRegion string, numNodes int, numWindowsNodes int,
fmt.Sprintf("name=%s", *gkeTestClusterName)).CombinedOutput()

if err != nil {
return fmt.Errorf("failed to check for previous test cluster: %v %s", err, out)
return fmt.Errorf("failed to check for previous test cluster: %v %s", err.Error(), out)
}
if len(out) > 0 {
klog.Infof("Detected previous cluster %s. Deleting so a new one can be created...", *gkeTestClusterName)
Expand Down Expand Up @@ -203,7 +203,7 @@ func clusterUpGKE(gceZone, gceRegion string, numNodes int, numWindowsNodes int,
cmd = exec.Command("gcloud", cmdParams...)
err = runCommand("Starting E2E Cluster on GKE", cmd)
if err != nil {
return fmt.Errorf("failed to bring up kubernetes e2e cluster on gke: %v", err)
return fmt.Errorf("failed to bring up kubernetes e2e cluster on gke: %v", err.Error())
}

// Because gcloud cannot disable addons on cluster create, the deployment has
Expand All @@ -216,7 +216,7 @@ func clusterUpGKE(gceZone, gceRegion string, numNodes int, numWindowsNodes int,
"--update-addons", "GcePersistentDiskCsiDriver=DISABLED")
err = runCommand("Updating E2E Cluster on GKE to disable driver deployment", cmd)
if err != nil {
return fmt.Errorf("failed to update kubernetes e2e cluster on gke: %v", err)
return fmt.Errorf("failed to update kubernetes e2e cluster on gke: %v", err.Error())
}
}

Expand Down Expand Up @@ -245,15 +245,15 @@ func downloadKubernetesSource(pkgDir, k8sIoDir, kubeVersion string) error {
klog.Info("cloning k8s master")
out, err := exec.Command("git", "clone", "https://github.com/kubernetes/kubernetes", k8sDir).CombinedOutput()
if err != nil {
return fmt.Errorf("failed to clone kubernetes master: %s, err: %v", out, err)
return fmt.Errorf("failed to clone kubernetes master: %s, err: %v", out, err.Error())
}
} else {
// Shallow clone of a release branch.
vKubeVersion := "v" + kubeVersion
klog.Infof("shallow clone of k8s %s", vKubeVersion)
out, err := exec.Command("git", "clone", "--depth", "1", "https://github.com/kubernetes/kubernetes", k8sDir).CombinedOutput()
if err != nil {
return fmt.Errorf("failed to clone kubernetes %s: %s, err: %v", vKubeVersion, out, err)
return fmt.Errorf("failed to clone kubernetes %s: %s, err: %v", vKubeVersion, out, err.Error())
}
}
return nil
Expand Down Expand Up @@ -289,7 +289,7 @@ func getGKEKubeTestArgs(gceZone, gceRegion, imageType string, useKubetest2 bool)
cmd := exec.Command("gcloud", "config", "get-value", "project")
project, err := cmd.Output()
if err != nil {
return nil, fmt.Errorf("failed to get current project: %v", err)
return nil, fmt.Errorf("failed to get current project: %v", err.Error())
}

// kubetest arguments
Expand Down Expand Up @@ -353,7 +353,7 @@ func getNormalizedVersion(kubeVersion, gkeVersion string) (string, error) {
func getKubeClusterVersion() (string, error) {
out, err := exec.Command("kubectl", "version", "-o=json").Output()
if err != nil {
return "", fmt.Errorf("failed to obtain cluster version, error: %v; output was %s", err, out)
return "", fmt.Errorf("failed to obtain cluster version, error: %v; output was %s", err.Error(), out)
}
type version struct {
ClientVersion *apimachineryversion.Info `json:"clientVersion,omitempty" yaml:"clientVersion,omitempty"`
Expand All @@ -363,7 +363,7 @@ func getKubeClusterVersion() (string, error) {
var v version
err = json.Unmarshal(out, &v)
if err != nil {
return "", fmt.Errorf("Failed to parse kubectl version output, error: %v", err)
return "", fmt.Errorf("Failed to parse kubectl version output, error: %v", err.Error())
}

return v.ServerVersion.GitVersion, nil
Expand Down Expand Up @@ -401,11 +401,11 @@ func getKubeClient() (kubernetes.Interface, error) {
}
config, err := clientcmd.BuildConfigFromFlags("", kubeConfig)
if err != nil {
return nil, fmt.Errorf("failed to create config: %v", err)
return nil, fmt.Errorf("failed to create config: %v", err.Error())
}
kubeClient, err := kubernetes.NewForConfig(config)
if err != nil {
return nil, fmt.Errorf("failed to create client: %v", err)
return nil, fmt.Errorf("failed to create client: %v", err.Error())
}
return kubeClient, nil
}
Expand Down
Loading