Wait for deployments by skonto · Pull Request #2010 · knative/operator

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

@skonto: 4 warnings.

Details

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.81%. Comparing base (e9077ba) to head (77f3f27).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2010      +/-   ##
==========================================
+ Coverage   62.76%   62.81%   +0.04%     
==========================================
  Files          49       49              
  Lines        2291     2294       +3     
==========================================
+ Hits         1438     1441       +3     
  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

 	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?

@houshengbo I added a new predicate ByGK because ByGVK

func ByGVK(gvk schema.GroupVersionKind) Predicate { return func(u *unstructured.Unstructured) bool { return u.GroupVersionKind() == gvk } }

would not work, as u will have a version so I cannot leave version as empty, comparison would fail.

houshengbo · 2025-03-13T14:59:45Z

/lgtm
/approve

knative-prow · 2025-03-13T14:59:55Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: houshengbo, skonto

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [houshengbo,skonto]

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

dsimansk · 2025-03-18T12:00:34Z

/cherry-pick release-1.17
/cherry-pick release-1.16
/cherry-pick release-1.15

/cc @skonto @pierDipi

knative-prow · 2025-03-18T12:00:37Z

@dsimansk: GitHub didn't allow me to request PR reviews from the following users: skonto.

Note that only knative members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

/cherry-pick release-1.17
/cherry-pick release-1.16
/cherry-pick release-1.15

/cc @skonto @pierDipi

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-robot · 2025-03-18T12:01:24Z

@dsimansk: #2010 failed to apply on top of branch "release-1.15":

Applying: Wait for deployments
Applying: enable telemetry to debug
Using index info to reconstruct a base tree...
M	test/e2e-common.sh
Falling back to patching base and 3-way merge...
Auto-merging test/e2e-common.sh
CONFLICT (content): Merge conflict in test/e2e-common.sh
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0002 enable telemetry to debug

Details

In response to this:

/cherry-pick release-1.17
/cherry-pick release-1.16
/cherry-pick release-1.15

/cc @skonto @pierDipi

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.

dsimansk · 2025-03-18T12:12:48Z

/cherry-pick release-1.16

dsimansk · 2025-03-18T12:12:53Z

/cherry-pick release-1.17

knative-prow-robot · 2025-03-18T12:13:27Z

@dsimansk: new pull request created: #2034

Details

In response to this:

/cherry-pick release-1.16

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-robot · 2025-03-18T12:13:31Z

@dsimansk: new pull request created: #2035

Details

In response to this:

/cherry-pick release-1.17

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.

* Wait for deployments * enable telemetry to debug * fix Istio cni * typos etc * lint * skip net stages if deployments are not ready * fix tests * use gk only for Certificate

* Wait for deployments * enable telemetry to debug * fix Istio cni * typos etc * lint * skip net stages if deployments are not ready * fix tests * use gk only for Certificate Co-authored-by: Stavros Kontopoulos <st.kontopoulos@gmail.com>

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

use gk only for Certificate

77f3f27

knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 13, 2025

knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2025

knative-prow bot merged commit be75448 into knative:main Mar 13, 2025
25 checks passed

knative-prow bot requested a review from pierDipi March 18, 2025 12:00

knative-prow-robot mentioned this pull request Mar 18, 2025

[release-1.16] Wait for deployments #2034

Merged

knative-prow-robot mentioned this pull request Mar 18, 2025

[release-1.17] Wait for deployments #2035

Merged

dsimansk mentioned this pull request Mar 18, 2025

[release-1.15] Wait for deployments (#2010) #2037

Merged

linkvt mentioned this pull request Nov 18, 2025

Post "https://webhook.knative-servin │ │ g.svc:443/defaulting?timeout=10s": context deadline exceeded knative/serving#16033

Closed

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

Conversation

skonto commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed Changes

Uh oh!

knative-prow bot left a comment

Choose a reason for hiding this comment

Proposed Changes

Uh oh!

knative-prow bot Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

knative-prow bot Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

knative-prow bot Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

knative-prow bot Mar 6, 2025

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

skonto commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

skonto commented Mar 6, 2025

Uh oh!

skonto commented Mar 6, 2025

Uh oh!

skonto commented Mar 6, 2025

Uh oh!

skonto commented Mar 6, 2025

Uh oh!

skonto commented Mar 10, 2025

Uh oh!

pierDipi Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pierDipi Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

skonto Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

skonto commented Mar 10, 2025

Uh oh!

skonto commented Mar 11, 2025

Uh oh!

houshengbo commented Mar 11, 2025

Uh oh!

skonto commented Mar 11, 2025

Uh oh!

houshengbo Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skonto Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houshengbo commented Mar 13, 2025

Uh oh!

knative-prow bot commented Mar 13, 2025

Uh oh!

Uh oh!

dsimansk commented Mar 18, 2025

Uh oh!

knative-prow bot commented Mar 18, 2025

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

skonto Mar 13, 2025 •

edited

Loading