fix: update condition before calling delete and decrementing counter #1074
Conversation
MatousJobanek
requested review from
xcoulon,
alexeykazakov,
rajivnathan,
ranakan19,
sbryzak and
mfrancisc
as code owners
| @@ -474,6 +478,10 @@ func (r *Reconciler) ensureSpaceDeletion(ctx context.Context, space *toolchainv1 | |||
| logger := log.FromContext(ctx) | |||
|
|
|||
| logger.Info("terminating Space") | |||
| if err := r.setStatusTerminating(ctx, space); err != nil { | |||
| logger.Error(err, "error updating status") | |||
There was a problem hiding this comment.
what about wrapping the underlying error and returning the new err? Otherwise, we'll get 2 lines in logs, which is harder to track :/
There was a problem hiding this comment.
I'm sorry, but I don't follow.
What do you propose to do differently compared to what we currently have in the controller? Do you want to drop the line 482 (and wrap the error with the message instead)? If yes, then this change doesn't affect only this line, but all other lines in the whole controller and most likely in all other controllers as well. I only kept the consistency with the current code.
TBH, I'm fine with removing the duplication in the logs, but this is much bigger effort.
There was a problem hiding this comment.
sure, let's address that in a separate PR later, then
There was a problem hiding this comment.
Good catch and great investigation!
Thanks for describing the scenario. I have only few minor comments.
Also I have some concerns about the 0 value meaning infinity , should we change that and use something like -1 when we want to configure a member cluster with no space limits ?
|
/retest |
|
New changes are detected. LGTM label has been removed. |
|
interestingly is still failing on the same test apparently 🤔 |
|
Also I'm not sure I understand why the first cluster has the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MatousJobanek, mfrancisc, rajivnathan, xcoulon 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:
Approvers can indicate their approval by writing |
Co-authored-by: Rajiv Senthilnathan <[email protected]>
Co-authored-by: Rajiv Senthilnathan <[email protected]>
|
there is apparently still a problem with the cache not being updated with the NSTemplateSet CR that are already being deleted (the controller called "delete" on them before) |
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1074 +/- ##
==========================================
+ Coverage 85.10% 85.17% +0.06%
==========================================
Files 55 55
Lines 5041 5044 +3
==========================================
+ Hits 4290 4296 +6
+ Misses 580 578 -2
+ Partials 171 170 -1
|
|
|
@MatousJobanek: The following test failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
fix for flakiness in
space_autocompletion_test.goan example: https://prow.ci.openshift.org/view/gs/test-platform-results/pr-logs/pull/codeready-toolchain_toolchain-e2e/1033/pull-ci-codeready-toolchain-toolchain-e2e-master-e2e/1823343090958602240what happened:
0, but the e2e test expects that at least one Space should be present. It sets the max number of Spaces (for member1) to the same value as is in the ToolchainStatus (in this case to0)0means no limit, then the Space controller provisioned the next Space to member1, but member1 was supposed to be full, and the Space was expected to be provisoned in member2.Thus, to avoid similar problems in the future, I updated the controller so it updates the status before calling delete on NSTemplateSet and decrementing the counter.