Merge pull request #12766 from sbueringer/pr-fix-e2e-chained

🐛 Fix e2e test issues introduced by chained upgrades

Author: k8s-ci-robot

internal/webhooks/cluster.go

@@ -974,6 +974,10 @@ func (webhook *Cluster) pollClusterClassForCluster(ctx context.Context, cluster
 		return true, nil
 	})
 	if clusterClassPollErr != nil {
+		if errors.Is(clusterClassPollErr, errClusterClassNotReconciled) {
+			// Return ClusterClass if we were able to get it and it's just not reconciled.
+			return clusterClass, clusterClassPollErr
+		}
 		return nil, clusterClassPollErr
 	}
 	return clusterClass, nil

test/e2e/cluster_upgrade_runtimesdk.go

@@ -892,13 +892,7 @@ func runtimeHookTestHandler(ctx context.Context, c client.Client, cluster *clust
 
 	// Check if the hook keeps blocking.
 	Consistently(func(_ Gomega) bool {
-		if !topologyConditionCheck() {
-			return false
-		}
-		if !blockingCondition() {
-			return false
-		}
-		return true
+		return topologyConditionCheck() && blockingCondition()
 	}, 60*time.Second).Should(BeTrue(),
 		fmt.Sprintf("Cluster Topology reconciliation continued unexpectedly: hook %s not blocking", hookName))
 
@@ -924,19 +918,19 @@ func runtimeHookTestHandler(ctx context.Context, c client.Client, cluster *clust
 	Byf("Waiting for %s hook to stop blocking", hookName)
 
 	Eventually(func(_ Gomega) bool {
-		if topologyConditionCheck() {
-			return false
+		if hook == "BeforeClusterDelete" {
+			// Only check blockingCondition for BeforeClusterDelete, because topologyConditionCheck
+			// always returns true for the BeforeClusterDelete hook.
+			return blockingCondition()
 		}
-		if blockingCondition() {
-			return false
-		}
-		return true
+		return topologyConditionCheck() || blockingCondition()
 	}, intervals...).Should(BeFalse(),
 		fmt.Sprintf("ClusterTopology reconcile did not proceed as expected when calling %s", hookName))
 }
 
 func computeHookName(hook string, attributes []string) string {
-	return strings.Join(append([]string{hook}, attributes...), "-")
+	// Note: + is not a valid character for ConfigMap keys (only alphanumeric characters, '-', '_' or '.')
+	return strings.ReplaceAll(strings.Join(append([]string{hook}, attributes...), "-"), "+", "_")
 }
 
 // clusterConditionShowsHookBlocking checks if the TopologyReconciled condition message contains both the hook name and hookFailedMessage.

test/e2e/cluster_upgrade_runtimesdk_test.go

@@ -100,8 +100,12 @@ var _ = Describe("When performing chained upgrades for workload cluster using Cl
 			DeployClusterClassInSeparateNamespace: true,
 			// Setting Kubernetes version from
 			KubernetesVersionFrom: e2eConfig.MustGetVariable(KubernetesVersionChainedUpgradeFrom),
-			// use Kubernetes versions from the kind mapper.
-			KubernetesVersions: kind.GetKubernetesVersions(),
+			// use Kubernetes versions from the kind mapper
+			// Note: Ensure KUBERNETES_VERSION_UPGRADE_TO is always part of the list, this is required for cases where
+			// KUBERNETES_VERSION_UPGRADE_TO is not in the kind mapper, e.g. in the `e2e-latestk8s` prowjob.
+			// Note: KUBERNETES_VERSION_UPGRADE_TO has to be set either to one version in kind.GetKubernetesVersions() or
+			// to a version greater than the last in the list by at most one minor version.
+			KubernetesVersions: appendIfNecessary(kind.GetKubernetesVersions(), e2eConfig.MustGetVariable(KubernetesVersionUpgradeTo)),
 			// The runtime extension gets deployed to the test-extension-system namespace and is exposed
 			// by the test-extension-webhook-service.
 			// The below values are used when creating the cluster-wide ExtensionConfig to refer
@@ -112,3 +116,12 @@ var _ = Describe("When performing chained upgrades for workload cluster using Cl
 		}
 	})
 })
+
+func appendIfNecessary(versions []string, v string) []string {
+	for _, version := range versions {
+		if version == v {
+			return versions
+		}
+	}
+	return append(versions, v)
+}

test/e2e/config/docker.yaml

@@ -410,8 +410,8 @@ intervals:
   default/wait-machine-pool-nodes: ["5m", "10s"]
   default/wait-delete-cluster: ["3m", "10s"]
   default/wait-machine-upgrade: ["20m", "10s"]
-  default/wait-control-plane-upgrade: ["5m", "10s"]
-  default/wait-machine-deployment-upgrade: ["5m", "10s"]
+  default/wait-control-plane-upgrade: ["15m", "10s"]
+  default/wait-machine-deployment-upgrade: ["10m", "10s"]
   default/wait-machine-pool-upgrade: ["5m", "10s"]
   default/wait-nodes-ready: ["10m", "10s"]
   default/wait-machine-remediation: ["5m", "10s"]
@@ -426,3 +426,4 @@ intervals:
   scale/wait-cluster: ["10m", "10s"]
   scale/wait-control-plane: ["20m", "10s"]
   scale/wait-worker-nodes: ["20m", "10s"]
+  self-hosted/wait-control-plane-upgrade: ["20m", "10s"]

test/extension/config/default/kustomization.yaml

@@ -6,7 +6,7 @@ labels:
 - includeSelectors: true
   pairs:
     # Label to identify all the providers objects; As per the clusterctl contract the value should be unique.
-    cluster.x-k8s.io/provider: runtime-extension-test
+    cluster.x-k8s.io/provider: runtime-extension-test-extension
 
 resources:
 - namespace.yaml

test/extension/handlers/lifecycle/handlers.go

@@ -293,5 +293,6 @@ func configMapName(clusterName, extensionConfigName string) string {
 }
 
 func computeHookName(hook runtimecatalog.Hook, attributes []string) string {
-	return strings.Join(append([]string{runtimecatalog.HookName(hook)}, attributes...), "-")
+	// Note: + is not a valid character for ConfigMap keys (only alphanumeric characters, '-', '_' or '.')
+	return strings.ReplaceAll(strings.Join(append([]string{runtimecatalog.HookName(hook)}, attributes...), "-"), "+", "_")
 }

test/framework/deployment_helpers.go

@@ -412,12 +412,12 @@ func dumpPodMetrics(ctx context.Context, client *kubernetes.Clientset, metricsPa
 		}
 
 		if !errorRetrievingMetrics {
-			Expect(verifyMetrics(data)).To(Succeed())
+			Expect(verifyMetrics(data, &pod)).To(Succeed())
 		}
 	}
 }
 
-func verifyMetrics(data []byte) error {
+func verifyMetrics(data []byte, pod *corev1.Pod) error {
 	var parser expfmt.TextParser
 	mf, err := parser.TextToMetricFamilies(bytes.NewReader(data))
 	if err != nil {
@@ -458,7 +458,7 @@ func verifyMetrics(data []byte) error {
 	}
 
 	if len(errs) > 0 {
-		return kerrors.NewAggregate(errs)
+		return errors.WithMessagef(kerrors.NewAggregate(errs), "panics occurred in Pod %s", klog.KObj(pod))
 	}
 
 	return nil

test/framework/deployment_helpers_test.go

@@ -20,6 +20,8 @@ import (
 	"testing"
 
 	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 func Test_verifyMetrics(t *testing.T) {
@@ -68,7 +70,7 @@ controller_runtime_reconcile_panics_total{controller="clusterclass"} 0
 # TYPE controller_runtime_webhook_panics_total counter
 controller_runtime_webhook_panics_total 0
 `),
-			wantErr: "1 panics occurred in \"cluster\" controller (check logs for more details)",
+			wantErr: "panics occurred in Pod default/pod1: 1 panics occurred in \"cluster\" controller (check logs for more details)",
 		},
 		{
 			name: "panic occurred in webhooks",
@@ -88,7 +90,7 @@ controller_runtime_webhook_panics_total 1
 # TYPE controller_runtime_conversion_webhook_panics_total counter
 controller_runtime_conversion_webhook_panics_total 0
 `),
-			wantErr: "1 panics occurred in webhooks (check logs for more details)",
+			wantErr: "panics occurred in Pod default/pod1: 1 panics occurred in webhooks (check logs for more details)",
 		},
 		{
 			name: "panics occurred in conversion webhooks",
@@ -108,14 +110,14 @@ controller_runtime_webhook_panics_total 0
 # TYPE controller_runtime_conversion_webhook_panics_total counter
 controller_runtime_conversion_webhook_panics_total 2
 `),
-			wantErr: "2 panics occurred in conversion webhooks (check logs for more details)",
+			wantErr: "panics occurred in Pod default/pod1: 2 panics occurred in conversion webhooks (check logs for more details)",
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
-			err := verifyMetrics(tt.data)
+			err := verifyMetrics(tt.data, &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "pod1"}})
 			if tt.wantErr == "" {
 				g.Expect(err).ToNot(HaveOccurred())
 			} else {