From d5983e8d4b7ff12be1e3bed0c44f13ffe483af5b Mon Sep 17 00:00:00 2001 From: Blaine Gardner Date: Thu, 25 Feb 2021 11:38:17 -0700 Subject: [PATCH] ceph: change osd on pvc unit test to use t.Log() Change the TestOSDsOnPVC unit test to use t.Log()/t.Logf() to avoid having a special infof() function that was unnecessary since our unit tests run with `go test -v` where the `-v` flag interleaves t.Log() output. Signed-off-by: Blaine Gardner --- pkg/operator/ceph/cluster/osd/osd_pvc_test.go | 47 +++++++++---------- pkg/operator/test/client.go | 19 +++----- 2 files changed, 28 insertions(+), 38 deletions(-) diff --git a/pkg/operator/ceph/cluster/osd/osd_pvc_test.go b/pkg/operator/ceph/cluster/osd/osd_pvc_test.go index 7d26f11ba..18a1eef18 100644 --- a/pkg/operator/ceph/cluster/osd/osd_pvc_test.go +++ b/pkg/operator/ceph/cluster/osd/osd_pvc_test.go @@ -49,11 +49,6 @@ import ( k8stesting "k8s.io/client-go/testing" ) -// make infof function for helping identify logs from unit test helpers versus from runtime code. -func infof(t *testing.T, format string, args ...interface{}) { - logger.Infof(t.Name()+": "+format, args...) -} - // The definition for this test is a wrapper for the test function that adds a timeout func TestOSDsOnPVC(t *testing.T) { oldLogger := *logger @@ -112,7 +107,7 @@ func testOSDsOnPVC(t *testing.T) { // can't use mockNodeOrchestrationCompleted here b/c it uses a context.CoreV1() method that // is mutex locked when a reactor is processing status := parseOrchestrationStatus(cm.Data) - infof(t, "configmap reactor: updating configmap %q status to completed", cm.Name) + t.Logf("configmap reactor: updating configmap %q status to completed", cm.Name) // configmap names are deterministic can be mapped indirectly to an OSD ID, and since the // configmaps are used to report completion status of OSD provisioning, we use this property in // thse unit tests @@ -165,11 +160,11 @@ func testOSDsOnPVC(t *testing.T) { status := parseOrchestrationStatus(cm.Data) if status.Status == OrchestrationStatusAlreadyExists { - infof(t, "configmap reactor: delete: OSD for configmap %q was updated", cm.Name) + t.Logf("configmap reactor: delete: OSD for configmap %q was updated", cm.Name) // allow tests to specify a custom callback for this case deleteConfigMapWithStatusAlreadyExistsCallback(cm, action) } else if status.Status == OrchestrationStatusCompleted { - infof(t, "configmap reactor: delete: OSD for configmap %q was created", cm.Name) + t.Logf("configmap reactor: delete: OSD for configmap %q was created", cm.Name) } } @@ -197,7 +192,7 @@ func testOSDsOnPVC(t *testing.T) { // deployment already exists, so this isn't be a valid create return false, nil, nil } - infof(t, "creating deployment %q", d.Name) + t.Logf("creating deployment %q", d.Name) deploymentOps.Add(d.Name, "create") return false, nil, nil } @@ -210,7 +205,7 @@ func testOSDsOnPVC(t *testing.T) { updateDeploymentAndWait = oldUDAW }() updateDeploymentAndWait = func(context *clusterd.Context, clusterInfo *cephclient.ClusterInfo, deployment *appsv1.Deployment, daemonType string, daemonName string, skipUpgradeChecks bool, continueUpgradeAfterChecksEvenIfNotHealthy bool) error { - infof(t, "updating deployment %q", deployment.Name) + t.Logf("updating deployment %q", deployment.Name) deploymentOps.Add(deployment.Name, "update") return nil } @@ -269,7 +264,7 @@ func testOSDsOnPVC(t *testing.T) { } // ============================================================================================= - infof(t, "Step 1: create new PVCs") + t.Log("Step 1: create new PVCs") // when creating new configmaps with status "starting", simulate them becoming "completed" // before any opportunistic updates can happen by changing the status to "completed" before @@ -286,10 +281,10 @@ func testOSDsOnPVC(t *testing.T) { waitForDone := func() { for { if provisionDone { - infof(t, "provisioning done") + t.Log("provisioning done") break } - infof(t, "provisioning not done. waiting...") + t.Log("provisioning not done. waiting...") time.Sleep(time.Millisecond) } } @@ -318,12 +313,12 @@ func testOSDsOnPVC(t *testing.T) { assert.Equal(t, 5, deploymentOps.Len()) // all 5 should be create operations assert.Len(t, deploymentOps.ResourcesWithOperation("create"), 5) - infof(t, "deployments successfully created for new PVCs") + t.Log("deployments successfully created for new PVCs") waitForDone() // ============================================================================================= - infof(t, "Step 2: verify deployments are updated when run again") + t.Log("Step 2: verify deployments are updated when run again") // clean the create times maps reset := func() { deploymentOps = newResourceOperationList() @@ -348,7 +343,7 @@ func testOSDsOnPVC(t *testing.T) { waitForDone() // ============================================================================================= - infof(t, "Step 3: verify new deployments are created before existing ones are updated") + t.Log("Step 3: verify new deployments are created before existing ones are updated") reset() spec.Storage.StorageClassDeviceSets[0].Count = 8 @@ -383,7 +378,7 @@ func testOSDsOnPVC(t *testing.T) { waitForDone() // ============================================================================================= - infof(t, "Step 4: verify updates can happen opportunistically") + t.Log("Step 4: verify updates can happen opportunistically") reset() spec.Storage.StorageClassDeviceSets[0].Count = 10 @@ -397,7 +392,7 @@ func testOSDsOnPVC(t *testing.T) { // in update-then-create fashion until all creates are done, followed by all updates. configMapsThatNeedUpdatedToCompleted := []string{} createConfigMapWithStatusStartingCallback = func(cm *corev1.ConfigMap) { - infof(t, "configmap reactor: create: marking that configmap %q needs to be completed later", cm.Name) + t.Logf("configmap reactor: create: marking that configmap %q needs to be completed later", cm.Name) configMapsThatNeedUpdatedToCompleted = append(configMapsThatNeedUpdatedToCompleted, cm.Name) } deleteConfigMapWithStatusAlreadyExistsCallback = func(cm *corev1.ConfigMap, action k8stesting.DeleteActionImpl) { @@ -453,7 +448,7 @@ func testOSDsOnPVC(t *testing.T) { waitForDone() // ============================================================================================= - infof(t, "Step 5: verify opportunistic updates can all happen before creates") + t.Log("Step 5: verify opportunistic updates can all happen before creates") reset() spec.Storage.StorageClassDeviceSets[0].Count = 12 @@ -465,7 +460,7 @@ func testOSDsOnPVC(t *testing.T) { configMapsThatNeedUpdatedToCompleted = []string{} createConfigMapWithStatusStartingCallback = func(cm *corev1.ConfigMap) { // re-define this behavior as a reminder for readers of the test - infof(t, "configmap reactor: create: marking that configmap %q needs to be completed later", cm.Name) + t.Logf("configmap reactor: create: marking that configmap %q needs to be completed later", cm.Name) configMapsThatNeedUpdatedToCompleted = append(configMapsThatNeedUpdatedToCompleted, cm.Name) } deleteConfigMapWithStatusAlreadyExistsCallback = func(cm *corev1.ConfigMap, action k8stesting.DeleteActionImpl) { @@ -509,7 +504,7 @@ func testOSDsOnPVC(t *testing.T) { } waitForDone() - infof(t, "success") + t.Log("success") } /* @@ -519,7 +514,7 @@ func testOSDsOnPVC(t *testing.T) { func osdPVCTestExecutor(t *testing.T, clientset *fake.Clientset, namespace string) *exectest.MockExecutor { return &exectest.MockExecutor{ MockExecuteCommandWithOutputFile: func(command string, outFileArg string, args ...string) (string, error) { - infof(t, "command: %s %v", command, args) + t.Logf("command: %s %v", command, args) if command != "ceph" { return "", errors.Errorf("unexpected command %q with args %v", command, args) } @@ -571,7 +566,7 @@ func waitForNumPVCs(t *testing.T, clientset *fake.Clientset, namespace string, c l, err := clientset.CoreV1().PersistentVolumeClaims(namespace).List(context.TODO(), metav1.ListOptions{}) assert.NoError(t, err) if len(l.Items) >= count { - infof(t, "PVCs for OSDs on PVC all exist") + t.Log("PVCs for OSDs on PVC all exist") break } <-time.After(1 * time.Millisecond) @@ -583,7 +578,7 @@ func waitForNumDeployments(t *testing.T, clientset *fake.Clientset, namespace st l, err := clientset.AppsV1().Deployments(namespace).List(context.TODO(), metav1.ListOptions{}) assert.NoError(t, err) if len(l.Items) >= count { - infof(t, "Deployments for OSDs on PVC all exist") + t.Log("Deployments for OSDs on PVC all exist") break } <-time.After(1 * time.Millisecond) @@ -608,13 +603,13 @@ func newOSDIDGenerator() osdIDGenerator { func (g *osdIDGenerator) osdID(t *testing.T, namedResource string) int { if id, ok := g.osdIDMap[namedResource]; ok { - infof(t, "resource %q has existing OSD ID %d", namedResource, id) + t.Logf("resource %q has existing OSD ID %d", namedResource, id) return id } id := g.nextOSDID g.osdIDMap[namedResource] = id g.nextOSDID++ - infof(t, "generated new OSD ID %d for resource %q", id, namedResource) + t.Logf("generated new OSD ID %d for resource %q", id, namedResource) return id } diff --git a/pkg/operator/test/client.go b/pkg/operator/test/client.go index cf614a900..b65d676e9 100644 --- a/pkg/operator/test/client.go +++ b/pkg/operator/test/client.go @@ -68,7 +68,7 @@ func AddReadyNode(t *testing.T, clientset *fake.Clientset, name, ip string) { _, err := clientset.CoreV1().Nodes().Create(context.TODO(), n, metav1.CreateOptions{}) if err != nil { if errors.IsAlreadyExists(err) { - infof(t, "AddReadyNode: node %q already exists; not treating this as an error") + t.Logf("AddReadyNode: node %q already exists; not treating this as an error", n.Name) } panic(fmt.Errorf("failed to create node %q: %+v", name, err)) } @@ -119,11 +119,6 @@ var ( nodeGVR schema.GroupVersionResource = corev1.SchemeGroupVersion.WithResource("nodes") ) -// make infof function for helping identify logs from unit test helpers versus from runtime code. -func infof(t *testing.T, format string, args ...interface{}) { - logger.Infof(t.Name()+": "+format, args...) -} - // NewComplexClientset is a reusable clientset for Rook unit tests that adds some complex behavior // to the clientset to mimic more of what K8s does in the real world. // - Generate a name for resources that have 'generateName' set and 'name' unset. @@ -154,7 +149,7 @@ func NewComplexClientset(t *testing.T) *fake.Clientset { b32 := base32.StdEncoding.EncodeToString(b[:]) // includes trailing equal signs newName := genName + "-" + strings.Trim(b32, "=") // trim off the trailing equal signs objMeta.SetName(newName) - infof(t, "generateName reactor: generated name for %s: %s", resource, objMeta.GetName()) + t.Logf("generateName reactor: generated name for %s: %s", resource, objMeta.GetName()) } // setting obj.Name above modifies the action in-place before future reactors occur // we want the default reactor to create the resource, so return false as if we did nothing @@ -196,18 +191,18 @@ func PrependComplexJobReactor(t *testing.T, clientset *fake.Clientset, assignPod err := clientset.Tracker().Create(podGVR, &pod, action.GetNamespace()) if err != nil { if errors.IsAlreadyExists(err) { - infof(t, "job reactor: pod %q is already created for job %q; not treating this as an error", pod.GetName(), job.GetName()) + t.Logf("job reactor: pod %q is already created for job %q; not treating this as an error", pod.GetName(), job.GetName()) } panic(fmt.Errorf("failed to create Pod %q for job %+v. %v", pod.Name, job, err)) } - infof(t, "job reactor: created pod %q for job %q", pod.Name, job.Name) + t.Logf("job reactor: created pod %q for job %q", pod.Name, job.Name) case k8stesting.DeleteActionImpl: jobName := action.GetName() obj, err := clientset.Tracker().Get(action.GetResource(), action.GetNamespace(), jobName) if err != nil && !errors.IsNotFound(err) { if errors.IsNotFound(err) { - infof(t, "job reactor: job %q being deleted does not exist; will not delete a pod", jobName) + t.Logf("job reactor: job %q being deleted does not exist; will not delete a pod", jobName) return false, nil, nil } panic(fmt.Errorf("failed to get info about job %q being deleted. %+v", jobName, err)) @@ -220,12 +215,12 @@ func PrependComplexJobReactor(t *testing.T, clientset *fake.Clientset, assignPod err = clientset.Tracker().Delete(podGVR, action.GetNamespace(), podName) if err != nil { if errors.IsNotFound(err) { - infof(t, "job reactor: pod %q does not exist to be deleted while deleting job %q", podName, jobName) + t.Logf("job reactor: pod %q does not exist to be deleted while deleting job %q", podName, jobName) return false, nil, nil } panic(fmt.Errorf("failed to delete pod %q while deleting job %+v", podName, job)) } - infof(t, "job reactor: deleted pod %q while deleting job %q", podName, jobName) + t.Logf("job reactor: deleted pod %q while deleting job %q", podName, jobName) } return false, nil, nil