🐛 add orderKappsValidateErr in crdupgradesafety preflight #1640
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
orderKappsValidateErr() is meant as a temporary solution to an external (ie. dependency) problem. carvel.dev/kapp/pkg/kapp/crdupgradesafety Validate() can return a multi-line error message which comes in random order. Until that is changed upstream, we need to fix this on our side to avoid falling into cycle of constantly trying to reconcile ClusterExtension's status due to random error message we set in its conditions.
azych
force-pushed
the
tmp-ensure-order-in-kapp-validation-error
branch
from
0845f38 to
ed16ae9
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1640 +/- ##
==========================================
+ Coverage 67.35% 67.60% +0.24%
==========================================
Files 55 55
Lines 4555 4571 +16
==========================================
+ Hits 3068 3090 +22
+ Misses 1261 1257 -4
+ Partials 226 224 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| // github.com/carvel-dev/kapp/pull/1047 (PR to ensure order in upstream) | ||
| // | ||
| // TODO: remove this once ordering has been handled by the upstream. | ||
| func orderKappsValidateErr(err error) error { |
There was a problem hiding this comment.
Well written (as with the unit tests). Thank you! Qq: is it worth surfacing whether the function had to fallback on the original? My guess is that if we fallback, we may run into the continuous reconcile issue. Wondering if we should make that noisy somehow...?
There was a problem hiding this comment.
thank you @perdasilva I thought about it too and it's a tricky situation, because while the first two steps in the unwrapping logic are true to all internal validators (as of now at least), the last one (most nested error.Join) might only be valid for CheckValidator internal validator. Since fallback might as well be a false negative here, I'd normally log this via Debug, but I noticed Debug isn't being used anywhere in the code so ultimately thought against it.
In case of a non-empty error here and regardless of fallback, we are still logging the full reconciliation error message every time we try to reconcile and that should make spotting the random order possible, though I agree an added hint from failed fallback might definitely help, so maybe it would be ok.
WDYT?
There was a problem hiding this comment.
yeah. Ultimately, I guess, it becomes a debug artifice. Which, while it undoubtedly helps with debugging, we'd still be reconciling repeatedly. If we have exp. backoff, then maybe it's not a big issue, and it's also temporary. I was thinking if we should panic (since the library shouldn't change out from under us during runtime), but then we'd need to be very careful about testing each error condition the underlying library checks for, which might not be a good use of time atm. Let's proceed ^^ thanks, dude =D
orderKappsValidateErr() is meant as a temporary solution to an external (ie. dependency) problem. carvel.dev/kapp/pkg/kapp/crdupgradesafety Validate() can return a multi-line error message which comes in random order. Until that is changed upstream, we need to fix this on our side to avoid falling into cycle of constantly trying to reconcile ClusterExtension's status due to random error message we set in its conditions.
An upstream PR is already created, but it might be some time before it makes it into the carvel's codebase and then to ours, hence this change.
For full context please see #1456 and carvel-dev/kapp#1047, especially carvel-dev/kapp#1047 (comment)
Description
Reviewer Checklist