Repository: GoogleContainerTools/kpt
Pull request #3751 (2023-01-24 22:55:59)

tests: Add wait for function and packagerevision readiness
The agggregated apiserver can sometimes be slow to respond, for
example if it is polling git synchronously.  After creating a
Repository in our tests, wait for the aggregated apiserver to start
responding before returning.

This should clean up some test flakes.


justinsb: /retest

Looks like we have more than one class of flake :-)

Patch:
diff --git a/porch/controllers/remoterootsyncsets/pkg/applyset/applyset.go b/porch/controllers/remoterootsyncsets/pkg/applyset/applyset.go
index 7c733ee7..775e04bf 100644
--- a/porch/controllers/remoterootsyncsets/pkg/applyset/applyset.go
+++ b/porch/controllers/remoterootsyncsets/pkg/applyset/applyset.go
@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/dynamic"
 	"k8s.io/klog/v2"
+	"sigs.k8s.io/cli-utils/pkg/kstatus/status"
 )
 
 // ApplySet is a set of objects that we want to apply to the cluster.
@@ -210,7 +211,24 @@ func (a *ApplySet) ApplyOnce(ctx context.Context) (*ApplyResults, error) {
 		tracker.lastApplied = applied
 		results.applySuccess(gvk, nn)
 
-		tracker.isHealthy = isHealthy(applied)
+		health, err := computeHealth(applied)
+		if err != nil {
+			klog.Warningf("error computing health: %v", err)
+			tracker.isHealthy = false
+		} else {
+			switch health.Status {
+			case status.CurrentStatus:
+				tracker.isHealthy = true
+			case status.InProgressStatus:
+				// TODO: Do we want a different status here?
+				tracker.isHealthy = false
+			case status.FailedStatus:
+				tracker.isHealthy = false
+			default:
+				klog.Warningf("unexpected health status %v", health)
+				tracker.isHealthy = false
+			}
+		}
 		results.reportHealth(gvk, nn, tracker.isHealthy)
 	}
 	return results, nil
diff --git a/porch/controllers/remoterootsyncsets/pkg/applyset/health.go b/porch/controllers/remoterootsyncsets/pkg/applyset/health.go
index adee9093..13c626d1 100644
--- a/porch/controllers/remoterootsyncsets/pkg/applyset/health.go
+++ b/porch/controllers/remoterootsyncsets/pkg/applyset/health.go
@@ -15,74 +15,11 @@
 package applyset
 
 import (
-	"encoding/json"
-
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
-	"k8s.io/klog/v2"
-)
-
-// isHealthy reports whether the object should be considered "healthy"
-// TODO: Replace with kstatus library
-func isHealthy(u *unstructured.Unstructured) bool {
-	ready := true
-
-	statusConditions, found, err := unstructured.NestedFieldNoCopy(u.Object, "status", "conditions")
-	if err != nil || !found {
-		klog.Infof("status conditions not found for %s", u.GroupVersionKind())
-		return true
-	}
-
-	statusConditionsList, ok := statusConditions.([]interface{})
-	if !ok {
-		klog.Warningf("expected status.conditions to be list, got %T", statusConditions)
-		return true
-	}
-	for i := range statusConditionsList {
-		condition := statusConditionsList[i]
-		conditionMap, ok := condition.(map[string]interface{})
-		if !ok {
-			klog.Warningf("expected status.conditions[%d] to be map, got %T", i, condition)
-			return true
-		}
-
-		conditionType := ""
-		conditionStatus := ""
-		for k, v := range conditionMap {
-			switch k {
-			case "type":
-				s, ok := v.(string)
-				if !ok {
-					klog.Warningf("expected status.conditions[].type to be string, got %T", v)
-				} else {
-					conditionType = s
-				}
-			case "status":
-				s, ok := v.(string)
-				if !ok {
-					klog.Warningf("expected status.conditions[].status to be string, got %T", v)
-				} else {
-					conditionStatus = s
-				}
-			}
-		}
 
-		// TODO: Check conditionType?
-
-		switch conditionStatus {
-		case "True":
-			// ready
-
-		case "False":
-			j, _ := json.Marshal(condition)
-			klog.Infof("status.conditions indicates object is not ready: %v", string(j))
-			ready = false
-
-		case "":
-			klog.Warningf("ignoring status.conditions[] type %q with unknown status %q", conditionType, conditionStatus)
-
-		}
-	}
+	"sigs.k8s.io/cli-utils/pkg/kstatus/status"
+)
 
-	klog.Infof("isHealthy %s => %v", u.GroupVersionKind(), ready)
-	return ready
+func computeHealth(u *unstructured.Unstructured) (*status.Result, error) {
+	return status.Compute(u)
 }
diff --git a/porch/go.mod b/porch/go.mod
index fa7f85e4..435c0b5e 100644
--- a/porch/go.mod
+++ b/porch/go.mod
@@ -53,6 +53,7 @@ require (
 	k8s.io/klog/v2 v2.70.1
 	k8s.io/kube-aggregator v0.24.0-beta.0
 	k8s.io/utils v0.0.0-20220823124924-e9cbc92d1a73
+	sigs.k8s.io/cli-utils v0.34.0
 	sigs.k8s.io/controller-runtime v0.13.0
 	sigs.k8s.io/kustomize/kyaml v0.13.9
 	sigs.k8s.io/yaml v1.3.0
@@ -187,7 +188,6 @@ require (
 	k8s.io/kube-openapi v0.0.0-20220803162953-67bda5d908f1 // indirect
 	k8s.io/kubectl v0.25.3 // indirect
 	sigs.k8s.io/apiserver-network-proxy/konnectivity-client v0.0.33 // indirect
-	sigs.k8s.io/cli-utils v0.34.0 // indirect
 	sigs.k8s.io/json v0.0.0-20220713155537-f223a00ba0e2 // indirect
 	sigs.k8s.io/kustomize/api v0.12.1 // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect


Test patch:
diff --git a/porch/test/e2e/e2e_test.go b/porch/test/e2e/e2e_test.go
index a45c52a8..8dbd1a04 100644
--- a/porch/test/e2e/e2e_test.go
+++ b/porch/test/e2e/e2e_test.go
@@ -2152,11 +2152,6 @@ func (t *PorchSuite) registerGitRepositoryF(ctx context.Context, repo, name, dir
 		},
 	})
 
-	// TODO: Replace with readiness check or similar, once we get to CRDs
-	// Sometimes we see "failed to list resources" here, I believe because we need to wait for the repository to be crawled.
-	t.Logf("HACK: sleeping for 5 seconds to allow for repository registration")
-	time.Sleep(5 * time.Second)
-
 	t.Cleanup(func() {
 		t.DeleteL(ctx, &configapi.Repository{
 			ObjectMeta: metav1.ObjectMeta{
@@ -2165,6 +2160,10 @@ func (t *PorchSuite) registerGitRepositoryF(ctx context.Context, repo, name, dir
 			},
 		})
 	})
+
+	// Make sure the repository is ready before we test to (hopefully)
+	// avoid flakiness.
+	t.waitUntilRepositoryReady(ctx, name, t.namespace)
 }
 
 type repositoryOption func(*configapi.Repository)
@@ -2310,6 +2309,9 @@ func (t *PorchSuite) mustNotExist(ctx context.Context, obj client.Object) {
 
 // waitUntilRepositoryReady waits for up to 10 seconds for the repository with the
 // provided name and namespace is ready, i.e. the Ready condition is true.
+// It also queries for Functions and PackageRevisions, to ensure these are also
+// ready - this is an artifact of the way we've implemented the aggregated apiserver,
+// where the first fetch can sometimes be synchronous.
 func (t *PorchSuite) waitUntilRepositoryReady(ctx context.Context, name, namespace string) {
 	nn := types.NamespacedName{
 		Name:      name,
@@ -2332,6 +2334,31 @@ func (t *PorchSuite) waitUntilRepositoryReady(ctx context.Context, name, namespa
 	if err != nil {
 		t.Errorf("Repository not ready after wait: %v", innerErr)
 	}
+
+	// While we're using an aggregated apiserver, make sure we can query the generated objects
+	if err := wait.PollImmediateWithContext(ctx, time.Second, 10*time.Second, func(ctx context.Context) (bool, error) {
+		var revisions porchapi.PackageRevisionList
+		if err := t.client.List(ctx, &revisions, client.InNamespace(nn.Namespace)); err != nil {
+			innerErr = err
+			return false, nil
+		}
+		return true, nil
+	}); err != nil {
+		t.Errorf("unable to query PackageRevisions after wait: %v", innerErr)
+	}
+
+	// Check for functions also (until we move them to CRDs)
+	if err := wait.PollImmediateWithContext(ctx, time.Second, 10*time.Second, func(ctx context.Context) (bool, error) {
+		var functions porchapi.FunctionList
+		if err := t.client.List(ctx, &functions, client.InNamespace(nn.Namespace)); err != nil {
+			innerErr = err
+			return false, nil
+		}
+		return true, nil
+	}); err != nil {
+		t.Errorf("unable to query Functions after wait: %v", innerErr)
+	}
+
 }
 
 func (t *PorchSuite) waitUntilRepositoryDeleted(ctx context.Context, name, namespace string) {