Wait for deployments #2010

skonto · 2025-03-06T12:14:14Z

Fixes #2009

Proposed Changes

Skip applying webhook resources until deployments are available. This makes sure resources can be reconciled properly and any resources depending on webhooks will be deployed after webhooks are up.

Release Note

Proper order is enforced now during manifest installation.

knative-prow · 2025-03-06T12:14:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: skonto

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [skonto]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

knative-prow

@skonto: 4 warnings.

In response to this:

Fixes #2009

Proposed Changes

Re-enqueue until deployments are available. This make sure resources can be reconciled properly.

Release Note
Proper order is enforced now during manifest installation.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

knative-prow · 2025-03-06T12:14:32Z

pkg/reconciler/common/install.go

+	return nil
+}
+
+func InstallWebhookConfigs(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {


Golint comments: exported function InstallWebhookConfigs should have comment or be unexported. More info.

knative-prow · 2025-03-06T12:14:32Z

pkg/reconciler/common/install.go

+	return nil
+}
+
+func InstallWebhookDepResources(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {


Golint comments: exported function InstallWebhookDepResources should have comment or be unexported. More info.

knative-prow · 2025-03-06T12:14:32Z

pkg/reconciler/common/install.go

+	return nil
+}
+
+func MarkStatusSuccess(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {


Golint comments: exported function MarkStatusSuccess should have comment or be unexported. More info.

knative-prow · 2025-03-06T12:14:32Z

pkg/reconciler/manifests/install.go

+	return nil
+}
+
+func SetManifestPaths(ctx context.Context, manifest *mf.Manifest, instance base.KComponent) error {


Golint comments: exported function SetManifestPaths should have comment or be unexported. More info.

codecov · 2025-03-06T12:16:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.79%. Comparing base (e9077ba) to head (a6a9921).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2010      +/-   ##
==========================================
+ Coverage   62.76%   62.79%   +0.03%     
==========================================
  Files          49       49              
  Lines        2291     2293       +2     
==========================================
+ Hits         1438     1440       +2     
  Misses        761      761              
  Partials       92       92

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

skonto · 2025-03-06T12:30:01Z

Istio cni is not coming up for e2e tests

✘ CNI encountered an error: failed to wait for resource: resources not ready after 5m0s: context deadline exceeded
  Error: failed to install manifests: failed to wait for resource: resources not ready after 5m0s: context deadline exceeded

skonto · 2025-03-06T12:35:37Z

        waiting:
          message: back-off 2m40s restarting failed container=install-cni pod=istio-cni-node-wndwj_kube-system(692908a1-4a41-4cc7-b918-72f8a177f824)
          reason: CrashLoopBackOff

skonto · 2025-03-06T13:02:56Z

skonto · 2025-03-06T13:11:42Z

istio/istio#53849

skonto · 2025-03-06T15:06:54Z

verify should be fixed with #2011

skonto · 2025-03-10T08:29:15Z

@houshengbo gentle ping.

pierDipi · 2025-03-10T09:03:44Z

pkg/reconciler/common/deployments.go

@@ -52,7 +55,7 @@ func CheckDeployments(ctx context.Context, manifest *mf.Manifest, instance base.

 	if len(nonReadyDeployments) > 0 {
 		status.MarkDeploymentsNotReady(nonReadyDeployments)
-		return nil
+		return controller.NewRequeueAfter(1 * time.Second)


Do we need to requeue blindly as this could create a hard loop when there is not actual progress?

The controllers are setting up informers for deployments, so the resource will get requeued anyway when they change (including when they become ready)

operator/pkg/reconciler/knativeeventing/controller.go

Lines 71 to 74 in 59db3c0

deploymentInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{

FilterFunc: controller.FilterControllerGVK(v1beta1.SchemeGroupVersion.WithKind("KnativeEventing")),

Handler: controller.HandleAll(impl.EnqueueControllerOf),

})

operator/pkg/reconciler/knativeserving/controller.go

Lines 71 to 74 in 59db3c0

deploymentInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{

FilterFunc: controller.FilterControllerGVK(v1beta1.SchemeGroupVersion.WithKind("KnativeServing")),

Handler: controller.HandleAll(impl.EnqueueControllerOf),

})

and if that's not happening, then the informer handling is where we need to tweak to fix the issue

I am aware of that, I wanted to make sure it happens faster in case something is slow.
The idea is that you cannot progress with an install if your deployments are not up and I wanted to enforce that. So it is not exactly blindly done.

The other thing is that if I dont re-enqueue there and block the logic we still face the problem (an error is printed) as the dependent resources will be deployed e.g. Certificate. We need to interrupt the deployment (returning an error) or block until deployments are up. I have tested both options, here I have the former.

These are the stages:

stages := common.Stages{ common.AppendTarget, ingress.AppendTargetIngress, security.AppendTargetSecurity, common.AppendAdditionalManifests, r.appendExtensionManifests, r.transform, manifests.Install, common.CheckDeployments, // <- Make sure we are up here otherwise don't progress bellow common.InstallWebhookConfigs, common.InstallWebhookDependentResources, manifests.SetManifestPaths, common.MarkStatusSuccess, common.DeleteObsoleteResources(ctx, ks, r.installed), }

skonto · 2025-03-10T11:32:08Z

@pierDipi I removed the re-enqueue thing and now I just skip the next stages until we are up. If no deployment change happens no reconciliation will happen.

skonto · 2025-03-11T08:47:42Z

@houshengbo gentle ping

houshengbo · 2025-03-11T15:19:41Z

@skonto I just fixed the CI issue, could you rebase this PR?

skonto · 2025-03-11T15:37:53Z

Sure.

houshengbo · 2025-03-12T18:45:45Z

pkg/reconciler/common/install.go

+	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 = mf.ByGVK(schema.GroupVersionKind{Group: "networking.internal.knative.dev", Version: "v1alpha1", Kind: "Certificate"})


Do we need to specify Version: "v1alpha1" here, as it may change in future? If version is empty, it will just return a GroupKind. It should be fine, right?

knative-prow bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 6, 2025

knative-prow bot requested review from aliok and houshengbo March 6, 2025 12:14

knative-prow bot reviewed Mar 6, 2025

View reviewed changes

knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 6, 2025

skonto force-pushed the fix_wbh branch from bdf03c2 to 12ea5dc Compare March 7, 2025 10:26

skonto assigned houshengbo Mar 7, 2025

skonto mentioned this pull request Mar 10, 2025

[wip][SRVKS-1301][RELEASE-1.35] Wait for deployments openshift-knative/serverless-operator#3484

Closed

pierDipi reviewed Mar 10, 2025

View reviewed changes

skonto added 7 commits March 11, 2025 17:38

Wait for deployments

646a93e

enable telemetry to debug

f9ed5f9

fix Istio cni

0c04fa6

typos etc

6b60ec4

lint

0e5604d

skip net stages if deployments are not ready

81490eb

fix tests

a6a9921

skonto force-pushed the fix_wbh branch from 2f74f20 to a6a9921 Compare March 11, 2025 15:38

houshengbo reviewed Mar 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for deployments #2010

Wait for deployments #2010

skonto commented Mar 6, 2025 •

edited

Loading

knative-prow bot commented Mar 6, 2025

knative-prow bot left a comment

knative-prow bot Mar 6, 2025

knative-prow bot Mar 6, 2025

knative-prow bot Mar 6, 2025

knative-prow bot Mar 6, 2025

codecov bot commented Mar 6, 2025 •

edited

Loading

skonto commented Mar 6, 2025 •

edited

Loading

skonto commented Mar 6, 2025

skonto commented Mar 6, 2025

skonto commented Mar 6, 2025

skonto commented Mar 6, 2025

skonto commented Mar 10, 2025

pierDipi Mar 10, 2025 •

edited

Loading

pierDipi Mar 10, 2025

skonto Mar 10, 2025 •

edited

Loading

skonto Mar 10, 2025 •

edited

Loading

skonto Mar 10, 2025

skonto commented Mar 10, 2025

skonto commented Mar 11, 2025

houshengbo commented Mar 11, 2025

skonto commented Mar 11, 2025

houshengbo Mar 12, 2025 •

edited

Loading

	deploymentInformer.Informer().AddEventHandler(cache.FilteringResourceEventHandler{
	FilterFunc: controller.FilterControllerGVK(v1beta1.SchemeGroupVersion.WithKind("KnativeEventing")),
	Handler: controller.HandleAll(impl.EnqueueControllerOf),
	})

Wait for deployments #2010

Are you sure you want to change the base?

Wait for deployments #2010

Conversation

skonto commented Mar 6, 2025 • edited Loading

Proposed Changes

knative-prow bot commented Mar 6, 2025

knative-prow bot left a comment

Choose a reason for hiding this comment

Proposed Changes

knative-prow bot Mar 6, 2025

Choose a reason for hiding this comment

knative-prow bot Mar 6, 2025

Choose a reason for hiding this comment

knative-prow bot Mar 6, 2025

Choose a reason for hiding this comment

knative-prow bot Mar 6, 2025

Choose a reason for hiding this comment

codecov bot commented Mar 6, 2025 • edited Loading

Codecov Report

skonto commented Mar 6, 2025 • edited Loading

skonto commented Mar 6, 2025

skonto commented Mar 6, 2025

skonto commented Mar 6, 2025

skonto commented Mar 6, 2025

skonto commented Mar 10, 2025

pierDipi Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

pierDipi Mar 10, 2025

Choose a reason for hiding this comment

skonto Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

skonto Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

skonto Mar 10, 2025

Choose a reason for hiding this comment

skonto commented Mar 10, 2025

skonto commented Mar 11, 2025

houshengbo commented Mar 11, 2025

skonto commented Mar 11, 2025

houshengbo Mar 12, 2025 • edited Loading

Choose a reason for hiding this comment

skonto commented Mar 6, 2025 •

edited

Loading

codecov bot commented Mar 6, 2025 •

edited

Loading

skonto commented Mar 6, 2025 •

edited

Loading

pierDipi Mar 10, 2025 •

edited

Loading

skonto Mar 10, 2025 •

edited

Loading

skonto Mar 10, 2025 •

edited

Loading

houshengbo Mar 12, 2025 •

edited

Loading