revert trustbundle propagation in sb lifecycle Do#8985
revert trustbundle propagation in sb lifecycle Do#8985maschmid wants to merge 6 commits intoknative:mainfrom
Conversation
knative-prow
bot
added
the
size/M
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: maschmid The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/test reconciler-tests |
looks relevant. Without the trustbundle propagation in Do, we'd be reverting to being just eventually consistent. |
|
Let's see if we can moving that logic that passes the CMs from PropagateTrustBundles directly to ApiServerSource reconciliation. Makes sense to use those CMs directly, instead of using stale values from the CM lister cache. |
|
/hold If that works for apiserversource, we should check if we should do the same for other sources that propagate bundles at the same time as they create deployments mounting them. |
knative-prow
bot
added
do-not-merge/hold
|
/unhold |
knative-prow
bot
removed
the
do-not-merge/hold
|
/test reconciler-tests |
Looking at the replicasets of the apiserversource-two-kmrmshbe-e8874f5d64f6d4ffacf50942f8e1b30ae4 Deployment, we see 4 replicasets in total, differing only in trust bundles being removed. As we now reconcile the trustbundle changes faster, it may be that the other tests that add trust-bundles to the system namespace may cause this one to flake (as "Expected 1 ready replica, got: 2" is likely the caused by having two replicasets, differing only by the trust bundle, with the old one still running) |
|
Anyway, the TestApiServerSourceSkipPermissionsFeature looks like a known flake ( #8903 ) , so let's try fixing that in a separate PR. |
maschmid
force-pushed
the
maschmid-main-revert-sinkbinding-lifecycle-trustbundle-propagation-in-do
branch
from
62a9d11 to
20e9722
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8985 +/- ##
=======================================
Coverage 51.04% 51.05%
=======================================
Files 409 409
Lines 21996 21986 -10
=======================================
- Hits 11228 11224 -4
+ Misses 9905 9899 -6
Partials 863 863 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/test reconciler-tests |
|
@maschmid: The following test failed, say
DetailsInstructions 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. I understand the commands that are listed here. |
1 participant
Fixes #8984
Basically reverts f88d983
Invoking PropagateTrustBundles in sinkbinding Do means it is being invoked per each sinkbinding-bound resource, not just during sinkbinding reconciliation. As the PropagateTrustBundles synchonously gets and updates ConfigMaps, it may also significantly delay the webhook's admission review of the resources. I believe the introduction of PropagateTrustBundles in
Dohas been a mistake (it should only by invoked during the SinkBinding reconciliation, not in the Do for each bound resource, as that doesn't really make sense)Additionally, we now pass the configmaps returned from PropagateTrustBundles to the resource adapters in apiserversource and integrationsink , as those invoke PropagateTrustBundles just before listing the configmaps from the lister, so they should now use the current CMs directly.
Proposed Changes
DoPre-review Checklist