Repository: knative/operator
Pull request #2037 (2025-03-18 16:51:26)

[release-1.15] Wait for deployments (#2010)
## Proposed Changes

* cherry-pick of Wait for deployments

https://github.com/knative/operator/pull/2010

/cc @skonto 

knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/operator/pull/2037#" title="Author self-approved">dsimansk</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/operator/blob/release-1.15/OWNERS)~~ [dsimansk]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/knative/operator/pull/2037?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) Report
Attention: Patch coverage is `42.85714%` with `20 lines` in your changes missing coverage. Please review.
> Project coverage is 57.03%. Comparing base [(`5c18464`)](https://app.codecov.io/gh/knative/operator/commit/5c184640903addd495808cf41bebee661d583bc0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) to head [(`b12e9cf`)](https://app.codecov.io/gh/knative/operator/commit/b12e9cf8870813361b9243956ad5492c5cdc2bcf?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

| [Files with missing lines](https://app.codecov.io/gh/knative/operator/pull/2037?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) | Patch % | Lines |
|---|---|---|
| [pkg/reconciler/common/install.go](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&filepath=pkg%2Freconciler%2Fcommon%2Finstall.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvY29tbW9uL2luc3RhbGwuZ28=) | 61.11% | [7 Missing :warning: ](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) |
| [pkg/reconciler/common/deployments.go](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&filepath=pkg%2Freconciler%2Fcommon%2Fdeployments.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvY29tbW9uL2RlcGxveW1lbnRzLmdv) | 33.33% | [4 Missing :warning: ](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) |
| [pkg/reconciler/knativeserving/knativeserving.go](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&filepath=pkg%2Freconciler%2Fknativeserving%2Fknativeserving.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIva25hdGl2ZXNlcnZpbmcva25hdGl2ZXNlcnZpbmcuZ28=) | 0.00% | [4 Missing :warning: ](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) |
| [pkg/reconciler/knativeeventing/knativeeventing.go](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&filepath=pkg%2Freconciler%2Fknativeeventing%2Fknativeeventing.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIva25hdGl2ZWV2ZW50aW5nL2tuYXRpdmVldmVudGluZy5nbw==) | 0.00% | [3 Missing :warning: ](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) |
| [pkg/reconciler/common/stages.go](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&filepath=pkg%2Freconciler%2Fcommon%2Fstages.go&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative#diff-cGtnL3JlY29uY2lsZXIvY29tbW9uL3N0YWdlcy5nbw==) | 0.00% | [2 Missing :warning: ](https://app.codecov.io/gh/knative/operator/pull/2037?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative) |

:x: Your project check has failed because the head coverage (57.03%) is below the target coverage (80.00%). You can increase the head coverage or adjust the [target](https://docs.codecov.com/docs/commit-status#target) coverage.

<details><summary>Additional details and impacted files</summary>


```diff
@@               Coverage Diff                @@
##           release-1.15    #2037      +/-   ##
================================================
- Coverage         63.69%   57.03%   -6.67%     
================================================
  Files                53       53              
  Lines              2592     2623      +31     
================================================
- Hits               1651     1496     -155     
- Misses              826     1022     +196     
+ Partials            115      105      -10     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/knative/operator/pull/2037?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=knative).

<details><summary>🚀 New features to boost your workflow: </summary>

- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
</details>
knative-prow[bot]: @dsimansk: Overrode contexts on behalf of dsimansk: codecov/project

<details>

In response to [this](https://github.com/knative/operator/pull/2037#issuecomment-2735881877):

>/override codecov/project


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
dsimansk: /override "style / Golang / Lint"
knative-prow[bot]: @dsimansk: Overrode contexts on behalf of dsimansk: style / Golang / Lint

<details>

In response to [this](https://github.com/knative/operator/pull/2037#issuecomment-2735883180):

>/override "style / Golang / Lint"


Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
</details>
dsimansk: /approve
/lgtm
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/operator/pull/2037#issuecomment-2735883380" title="LGTM">dsimansk</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/operator/blob/release-1.15/OWNERS)~~ [dsimansk]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->
dsimansk: /cc @matzew 
knative-prow[bot]: [APPROVALNOTIFIER] This PR is **APPROVED**

This pull-request has been approved by: *<a href="https://github.com/knative/operator/pull/2037#" title="Author self-approved">dsimansk</a>*, *<a href="https://github.com/knative/operator/pull/2037#pullrequestreview-2697866562" title="Approved">matzew</a>*

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=knative%2Foperator).

The pull request process is described [here](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process)

<details >
Needs approval from an approver in each of these files:

- ~~[OWNERS](https://github.com/knative/operator/blob/release-1.15/OWNERS)~~ [dsimansk,matzew]

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":[]} -->

Patch:
diff --git a/pkg/reconciler/common/deployments.go b/pkg/reconciler/common/deployments.go
index c0c5afcea..ae3c703e6 100644
--- a/pkg/reconciler/common/deployments.go
+++ b/pkg/reconciler/common/deployments.go
@@ -18,15 +18,31 @@ package common
 
 import (
 	"context"
+	"errors"
 
 	mf "github.com/manifestival/manifestival"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
-	"k8s.io/apimachinery/pkg/api/errors"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/client-go/kubernetes/scheme"
+
 	"knative.dev/operator/pkg/apis/operator/base"
 )
 
+type deploymentsNotReadyError struct{}
+
+var _ error = deploymentsNotReadyError{}
+
+// Error implements the Error() interface of error.
+func (err deploymentsNotReadyError) Error() string {
+	return "deployments not ready"
+}
+
+// IsDeploymentsNotReadyError returns true if the given error is a deploymentsNotReadyError.
+func IsDeploymentsNotReadyError(err error) bool {
+	return errors.Is(err, deploymentsNotReadyError{})
+}
+
 // CheckDeployments checks all deployments in the given manifest and updates the given
 // status with the status of the deployments.
 func CheckDeployments(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
@@ -36,7 +52,7 @@ func CheckDeployments(ctx context.Context, manifest *mf.Manifest, instance base.
 		resource, err := manifest.Client.Get(&u)
 		if err != nil {
 			status.MarkDeploymentsNotReady([]string{"all"})
-			if errors.IsNotFound(err) {
+			if apierrors.IsNotFound(err) {
 				return nil
 			}
 			return err
@@ -52,7 +68,7 @@ func CheckDeployments(ctx context.Context, manifest *mf.Manifest, instance base.
 
 	if len(nonReadyDeployments) > 0 {
 		status.MarkDeploymentsNotReady(nonReadyDeployments)
-		return nil
+		return deploymentsNotReadyError{}
 	}
 
 	status.MarkDeploymentsAvailable()
diff --git a/pkg/reconciler/common/install.go b/pkg/reconciler/common/install.go
index 4b3b3adaf..83170d87c 100644
--- a/pkg/reconciler/common/install.go
+++ b/pkg/reconciler/common/install.go
@@ -22,17 +22,20 @@ import (
 	"strings"
 
 	mf "github.com/manifestival/manifestival"
-	"knative.dev/pkg/logging"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"knative.dev/operator/pkg/apis/operator/base"
 	"knative.dev/operator/pkg/apis/operator/v1beta1"
+	"knative.dev/pkg/logging"
 )
 
 var (
-	role            mf.Predicate = mf.Any(mf.ByKind("ClusterRole"), mf.ByKind("Role"))
-	rolebinding     mf.Predicate = mf.Any(mf.ByKind("ClusterRoleBinding"), mf.ByKind("RoleBinding"))
-	webhook         mf.Predicate = mf.Any(mf.ByKind("MutatingWebhookConfiguration"), mf.ByKind("ValidatingWebhookConfiguration"))
-	gatewayNotMatch              = "no matches for kind \"Gateway\""
+	role                      mf.Predicate = mf.Any(mf.ByKind("ClusterRole"), mf.ByKind("Role"))
+	rolebinding               mf.Predicate = mf.Any(mf.ByKind("ClusterRoleBinding"), mf.ByKind("RoleBinding"))
+	webhook                   mf.Predicate = mf.Any(mf.ByKind("MutatingWebhookConfiguration"), mf.ByKind("ValidatingWebhookConfiguration"))
+	webhookDependentResources mf.Predicate = byGV(schema.GroupKind{Group: "networking.internal.knative.dev", Kind: "Certificate"})
+	gatewayNotMatch                        = "no matches for kind \"Gateway\""
 )
 
 // Install applies the manifest resources for the given version and updates the given
@@ -52,7 +55,7 @@ func Install(ctx context.Context, manifest *mf.Manifest, instance base.KComponen
 		status.MarkInstallFailed(err.Error())
 		return fmt.Errorf("failed to apply (cluster)rolebindings: %w", err)
 	}
-	if err := manifest.Filter(mf.Not(mf.Any(role, rolebinding, webhook))).Apply(); err != nil {
+	if err := manifest.Filter(mf.Not(mf.Any(role, rolebinding, webhook, webhookDependentResources))).Apply(); err != nil {
 		status.MarkInstallFailed(err.Error())
 		if ks, ok := instance.(*v1beta1.KnativeServing); ok && strings.Contains(err.Error(), gatewayNotMatch) &&
 			(ks.Spec.Ingress == nil || ks.Spec.Ingress.Istio.Enabled) {
@@ -63,10 +66,32 @@ func Install(ctx context.Context, manifest *mf.Manifest, instance base.KComponen
 
 		return fmt.Errorf("failed to apply non rbac manifest: %w", err)
 	}
+	return nil
+}
+
+// InstallWebhookConfigs applies the Webhook manifest resources and updates the given status accordingly.
+func InstallWebhookConfigs(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
+	status := instance.GetStatus()
 	if err := manifest.Filter(webhook).Apply(); err != nil {
 		status.MarkInstallFailed(err.Error())
 		return fmt.Errorf("failed to apply webhooks: %w", err)
 	}
+	return nil
+}
+
+// InstallWebhookDependentResources applies the Webhook dependent resources updates the given status accordingly.
+func InstallWebhookDependentResources(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
+	status := instance.GetStatus()
+	if err := manifest.Filter(webhookDependentResources).Apply(); err != nil {
+		status.MarkInstallFailed(err.Error())
+		return fmt.Errorf("failed to apply webhooks: %w", err)
+	}
+	return nil
+}
+
+// MarkStatusSuccess updates the component status to success.
+func MarkStatusSuccess(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
+	status := instance.GetStatus()
 	status.MarkInstallSucceeded()
 	status.SetVersion(TargetVersion(instance))
 	return nil
@@ -83,3 +108,9 @@ func Uninstall(manifest *mf.Manifest) error {
 	}
 	return nil
 }
+
+func byGV(gk schema.GroupKind) mf.Predicate {
+	return func(u *unstructured.Unstructured) bool {
+		return u.GroupVersionKind().GroupKind() == gk
+	}
+}
diff --git a/pkg/reconciler/common/stages.go b/pkg/reconciler/common/stages.go
index 66f1fc7d6..47edd79b1 100644
--- a/pkg/reconciler/common/stages.go
+++ b/pkg/reconciler/common/stages.go
@@ -38,6 +38,9 @@ type Stages []Stage
 func (stages Stages) Execute(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
 	for _, stage := range stages {
 		if err := stage(ctx, manifest, instance); err != nil {
+			if IsDeploymentsNotReadyError(err) {
+				break
+			}
 			return err
 		}
 	}
diff --git a/pkg/reconciler/knativeeventing/knativeeventing.go b/pkg/reconciler/knativeeventing/knativeeventing.go
index 959656ef0..dab94829f 100644
--- a/pkg/reconciler/knativeeventing/knativeeventing.go
+++ b/pkg/reconciler/knativeeventing/knativeeventing.go
@@ -142,6 +142,9 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ke *v1beta1.KnativeEvent
 		r.handleTLSResources,
 		manifests.Install,
 		common.CheckDeployments,
+		common.InstallWebhookConfigs,
+		manifests.SetManifestPaths,
+		common.MarkStatusSuccess,
 		common.DeleteObsoleteResources(ctx, ke, r.installed),
 	}
 	manifest := r.manifest.Append()
diff --git a/pkg/reconciler/knativeserving/knativeserving.go b/pkg/reconciler/knativeserving/knativeserving.go
index d554b73dd..cf54f4aeb 100644
--- a/pkg/reconciler/knativeserving/knativeserving.go
+++ b/pkg/reconciler/knativeserving/knativeserving.go
@@ -124,6 +124,10 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, ks *v1beta1.KnativeServi
 		r.transform,
 		manifests.Install,
 		common.CheckDeployments,
+		common.InstallWebhookConfigs,
+		common.InstallWebhookDependentResources,
+		manifests.SetManifestPaths,
+		common.MarkStatusSuccess,
 		common.DeleteObsoleteResources(ctx, ks, r.installed),
 	}
 	manifest := r.manifest.Append()
diff --git a/pkg/reconciler/manifests/install.go b/pkg/reconciler/manifests/install.go
index dc5bf0d77..0c4c60a2c 100644
--- a/pkg/reconciler/manifests/install.go
+++ b/pkg/reconciler/manifests/install.go
@@ -34,6 +34,11 @@ func Install(ctx context.Context, manifest *mf.Manifest, instance base.KComponen
 	if err != nil {
 		return err
 	}
+	return nil
+}
+
+// SetManifestPaths set the manifest paths in the status of the component
+func SetManifestPaths(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {
 	status := instance.GetStatus()
 	path := common.TargetManifestPathArray(instance)
 	version := common.TargetVersion(instance)


Test patch:
diff --git a/pkg/reconciler/common/deployments_test.go b/pkg/reconciler/common/deployments_test.go
index 5ea3fdedf..865f3768c 100644
--- a/pkg/reconciler/common/deployments_test.go
+++ b/pkg/reconciler/common/deployments_test.go
@@ -78,7 +78,7 @@ func TestCheckDeployments(t *testing.T) {
 			*NamespacedResource("apps/v1", "Deployment", "test", "notReady"),
 		},
 		inAPI:      []runtime.Object{notReadyDeployment},
-		wantError:  false,
+		wantError:  true,
 		wantStatus: corev1.ConditionFalse,
 	}, {
 		name: "ready and not ready deployment",
@@ -86,8 +86,8 @@ func TestCheckDeployments(t *testing.T) {
 			*NamespacedResource("apps/v1", "Deployment", "test", "ready"),
 			*NamespacedResource("apps/v1", "Deployment", "test", "notReady"),
 		},
-		inAPI:      []runtime.Object{},
-		wantError:  false,
+		inAPI:      []runtime.Object{readyDeployment, notReadyDeployment},
+		wantError:  true,
 		wantStatus: corev1.ConditionFalse,
 	}, {
 		name: "not found deployment",
diff --git a/pkg/reconciler/common/install_test.go b/pkg/reconciler/common/install_test.go
index b99a6231f..842562d95 100644
--- a/pkg/reconciler/common/install_test.go
+++ b/pkg/reconciler/common/install_test.go
@@ -81,6 +81,14 @@ func TestInstall(t *testing.T) {
 		t.Fatalf("Install() = %v, want no error", err)
 	}
 
+	if err := InstallWebhookConfigs(context.TODO(), &manifest, instance); err != nil {
+		t.Fatalf("InstallWebhookConfigs() = %v, want no error", err)
+	}
+
+	if err := MarkStatusSuccess(context.TODO(), &manifest, instance); err != nil {
+		t.Fatalf("MarkStatusSuccess() = %v, want no error", err)
+	}
+
 	if !cmp.Equal(client.creates, want) {
 		t.Fatalf("Unexpected creates: %s", cmp.Diff(client.creates, want))
 	}
diff --git a/pkg/reconciler/manifests/install_test.go b/pkg/reconciler/manifests/install_test.go
index b9884b951..4d8b0bf0c 100644
--- a/pkg/reconciler/manifests/install_test.go
+++ b/pkg/reconciler/manifests/install_test.go
@@ -107,6 +107,12 @@ func TestInstall(t *testing.T) {
 			manifest, _ := mf.ManifestFrom(mf.Slice{})
 			err := Install(context.TODO(), &manifest, tt.instance)
 			util.AssertEqual(t, err, nil)
+			err = common.InstallWebhookConfigs(context.TODO(), &manifest, tt.instance)
+			util.AssertEqual(t, err, nil)
+			err = SetManifestPaths(context.TODO(), &manifest, tt.instance)
+			util.AssertEqual(t, err, nil)
+			err = common.MarkStatusSuccess(context.TODO(), &manifest, tt.instance)
+			util.AssertEqual(t, err, nil)
 			util.AssertEqual(t, tt.instance.GetStatus().GetVersion(), tt.version)
 			util.AssertDeepEqual(t, tt.instance.GetStatus().GetManifests(), tt.expectedPath)
 		})