fix: schedulers created only when data plane is present #4612
Conversation
Signed-off-by: Calum Murray <[email protected]>
|
/cc @creydr @twoGiants |
knative-prow
bot
added
size/L
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Cali0707 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
knative-prow
bot
added
the
approved
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4612 +/- ##
=======================================
Coverage 28.77% 28.78%
=======================================
Files 294 294
Lines 16169 16253 +84
=======================================
+ Hits 4653 4678 +25
- Misses 11063 11120 +57
- Partials 453 455 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/cherry-pick release-1.20 |
|
@Cali0707: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
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. |
|
/cherry-pick release-1.19 |
|
@Cali0707: once the present PR merges, I will cherry-pick it on top of DetailsIn response to this:
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. |
|
/retest-required |
1 similar comment
|
/retest-required |
|
/hold This failure seems related to the changes I made - will look into it |
knative-prow
bot
added
the
do-not-merge/hold
Signed-off-by: Calum Murray <[email protected]>
Cali0707
force-pushed
the
fix-scheduler-errors
branch
from
1b1fd6d to
594ffed
Compare
|
/retest-required |
|
/unhold |
knative-prow
bot
removed
the
do-not-merge/hold
|
/test reconciler-tests-keda Failure was unrelated - cert-manager did not become ready in time |
twoGiants
left a comment
twoGiants
left a comment
There was a problem hiding this comment.
Thank you for the fix 😸 👍
I will continue with the review next week. It's more complex than it looks, I wasn't able to finish. There are nuances here and there. I left a few comments.
A regression unit tests which captures the bug would be great. Also tests which cover all the new logic.
See my comments below.
| logger := logging.FromContext(sm.ctx) | ||
| logger.Infow("Removing scheduler for deleted StatefulSet", zap.String("statefulset", ssName), zap.String("scheduler", schedulerKey)) | ||
|
|
||
| delete(sm.schedulers, schedulerKey) |
There was a problem hiding this comment.
If the scheduler is a leader it is not demoted before it's removed. This will probably be an issue.
| for ssName := range statefulSetToSchedulerKey { | ||
| if _, err := statefulSetLister.Get(ssName); err == nil { | ||
| schedulerMgr.createSchedulerForStatefulSet(ssName) | ||
| } else if !apierrors.IsNotFound(err) { |
There was a problem hiding this comment.
This it a bit difficult to read. I would invert the error here => else if apierrors.IsNotFound(err) { and then log the infow. And on other errors I would not proceed but exit with an error. Otherwise we continue with a partial setup.
3 participants
Fixes #4604
Proposed Changes
Release Note