✨improve Extension status conditions #683
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
| Type: ocv1alpha1.TypeDeleting, | ||
| Status: metav1.ConditionFalse, |
There was a problem hiding this comment.
This one is tricky. I think this combo of TypeDeleting and ConditionFalse would be interpreted as "this extension is not in the deleting state"
And I suppose that "DeletionFailed" could mean "I'm no longer actively in the deleting state because the deletion attempt failed".
But what happens at this point? Does kapp-controller keep retry-ing the deletion?
There was a problem hiding this comment.
Digging into the codebase (haven't verified yet by trying it out), looks like it doesn't. If the deletion fails, the process is still marked as completed (https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/cli/pkg/kctrl/cmd/app/app_tailer.go#L52) and the watches are closed (https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/cli/pkg/kctrl/cmd/app/app_tailer.go#L58).
Going a bit further and looking into the PackageInstall reconciler to see what happens - looks like if deletion fails, and the App's status conveys the same - the packageInstall just updates its status: https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/pkg/packageinstall/packageinstall.go#L408-L428.
This makes sense, because the reconcile period for the App CR is set by the user (and defaulted to 30sec if it's unset).
This is why they seem to have the DeleteFailed condition which is set to true when Deleting is set to false.
There was a problem hiding this comment.
From what I can tell, kapp-controller does seem to keep retrying the App delete, though I'm not sure if there's anything to limit the retries: https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/pkg/app/app_reconcile.go#L36
There was a problem hiding this comment.
@ankitathomas Let's ask the question in #carvel channel upstream for clarification. They would be able to point us to right code.
| @@ -77,6 +77,7 @@ const ( | |||
| // TODO(user): add more Types, here and into init() | |||
| TypeInstalled = "Installed" | |||
| TypeResolved = "Resolved" | |||
| TypeDeleting = "Deleting" | |||
There was a problem hiding this comment.
Could we have a more generic "Progressing" status instead? That way, whether we are in the process of installing, upgrading or deleting, we can set Progressing=True (and conversely Progressing=False would mean we're at a steady state).
There was a problem hiding this comment.
Do we need a separate condition for this? I think we could achieve the same thing with Installed, with Installed=True being our steady state and False or Unknown for other states, with the Reason giving more details.
There was a problem hiding this comment.
I'm worried if this may cause confusion - what would it mean when: Install is either fail/succeeded, with Progressing - True. How does the user know what action is being taken next? We would have to ensure that when Progressing is set to true, then other statuses are moved to Unknown.
There was a problem hiding this comment.
Wondering if we can instead re-name this condition to capture just the Delete action similar to how we handle others.
TypeDeleteFail - means App'skcv1alpha1.DeleteFailedis True,kcv1alpha1.Deletingis false.TypeDeleteUnknown - means App'skcv1alpha1.Deletingis true or any other combination which we are unsure of what it represents.
Looking into how Kapp is handling their status references - https://github.com/carvel-dev/kapp-controller/blob/d89101eb78b2c26a73a7afd8e8ae3111a6731dde/cli/pkg/kctrl/cmd/app/app.go#L39-L64 - All they are trying to map is Reconcile Fail/Succeeded/Progressing, Delete Progressing/Failed. So I'm hoping that the above should capture what we want.
Thoughts?
There was a problem hiding this comment.
I can try this option if it's ok to have the TypeDelete condition on every Extension all the time. kapp-controller only allows one of the Reconciling/Deleting condition sets on an App at a time, I wasn't sure if we wanted to switch the way we mandate all defined conditions presence on each Extension.
There was a problem hiding this comment.
I'm worried if this may cause confusion - what would it mean when: Install is either fail/succeeded, with Progressing - True. How does the user know what action is being taken next?
We can use the Reason and Message to get into more specifics.
Taking a stab at enumerating various states:
| State | Progressing | Installed |
| Initial install is in progress | True Reason=Installing Message="installing $bundleID" |
False Reason=NothingInstalled |
| Initial install is failing with a non-terminal error (i.e. retries might resolve the error in the future) | True Reason=RetryingInstallation Message="retrying installation of $bundleID due to error: $error" |
False Reason=NothingInstalled |
| Initial install failed with a terminal error (i.e. retries will never resolve in the future without some external change) | False Reason=InstallFailed Message="failed installation of $bundleID: $error |
False Reason=NothingInstalled |
| Install failed, deletion is in progress | True Reason= Uninstalling Message="uninstalling failed installation of $bundleID" |
False Reason=NothingInstalled |
| Initial install failed, deletion is failing | True Reason=RetryingUninstall Message="retrying uninstall of $bundleID due to error: $error" |
False Reason=NothingInstalled |
| Initial install failed, deletion failed | False Reason=UninstallFailed Message="failed uninstall of $bundleID: $error>" |
False Reason=NothingInstalled |
| Install/upgrade succeeded, steady state | False Reason=NothingToDo |
True Reason=InstallSucceeded Message="successfully installed $bundleID" |
| Install succeeded, upgrade is in progress | True Reason=Upgrading Message="upgrading to $newBundleID |
True Reason=InstallSucceeded Message="successfully installed $oldBundleID" |
| Install succeeded, upgrade is failing with a non-terminal error | True Reason=RetryingUpgrade Message="retrying upgrade of $newBundleID due to error: $error" |
True Reason=InstallSucceeded Message="successfully installed $oldBundleID" |
| Install succeeded, upgrade failed with a terminal error | False Reason=UpgradeFailed Message="failed upgrade to $newBundleID: $error" |
True Reason=InstallSucceeded Message="successfully installed $oldBundleID" |
| Install succeeded, deletion is in progress | True Reason=Uninstalling Message="uninstalling $bundleID" |
True Reason=InstallSucceeded Message="successfully installed $bundleID" |
| Install succeeded, deletion is failing with a non-terminal error | True Reason=RetryingUninstall Message="retrying uninstall of $bundleID due to error: $error" |
True Reason=InstallSucceeded Message="successfully installed $bundleID" |
| Install succeeded, deletion is failing with a terminal error | False Reason=UninstallFailed Message="failed uninstall of $bundleID: $error" |
True Reason=InstallSucceeded Message="successfully installed $bundleID" |
Notice how Progressing and Installed are actually orthogonal. Installed does not depend on Progressing or vice-versa.
We would have to ensure that when Progressing is set to true, then other statuses are moved to Unknown.
I don't think so. My idea and goal here is actually that a Progressing condition gives us a place to keep some status about what we're doing so that we don't have to wipe out some status about what we did before.
For example, if someone installs v1, and then tries to upgrade to v2, but v2 fails the permissions preflight check, the actual state of the system is:
- v1 is still doing its thing untouched
- v2 failed the upgrade preflight check (which might resolve in the future if the admin reconfigures the permissions of the Extension/App's service account).
It would be really nice to be able to say "v1 is still installed and running, but here's why we can't upgrade to v2."
There was a problem hiding this comment.
Wouldn't a progressing/failing update/delete possibly impact the currently installed extension? If the idea is to have a separate condition specific to preflight checks, we may have to add more support for that separately from the kapp-controller App conditions.
There was a problem hiding this comment.
It might impact it, yes. Hopefully App's status would tell us enough about the failure to help us know if the current installation was impacted or not.
This is probably a good discussion topic with the Carvel folks.
ankitathomas
force-pushed
the
kapp-status
branch
from
4b5bbf4
to
c069984
Compare
ankitathomas
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
ankitathomas
force-pushed
the
kapp-status
branch
from
c069984
to
b24b20d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #683 +/- ##
==========================================
- Coverage 68.15% 67.41% -0.75%
==========================================
Files 22 22
Lines 1429 1470 +41
==========================================
+ Hits 974 991 +17
- Misses 390 414 +24
Partials 65 65
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ankitathomas
force-pushed
the
kapp-status
branch
2 times, most recently
from
4ef9059
to
3157a50
Compare
ankitathomas
force-pushed
the
kapp-status
branch
from
3157a50
to
03904d9
Compare
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| Status: metav1.ConditionFalse, | ||
| Reason: ocv1alpha1.ReasonResolutionFailed, | ||
| Reason: ocv1alpha1.ReasonPending, |
There was a problem hiding this comment.
If we're not progressing, doesn't that mean we're at steady state with nothing to do? Pending seems like it would indicate that we are progressing and waiting on some action to occur.
There was a problem hiding this comment.
Agree to that. ProgressingFalse can indicate a few things:
- Installation of the bundle was successful, we are in a stable state.
- Installation of bundle failed, we are again in a stable state, but look to the install status to know more.
- We are at the Resolution step for the next reconcile probably caused by upgrade - haven't yet reached to the point of install.
There was a problem hiding this comment.
+1. If it is assumed that when Progressing is False we are in a stable state maybe Stable as the reason is sufficient?
|
|
||
| mediaType, err := bundle.MediaType() | ||
| if err != nil { | ||
| setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
| if ext.Status.InstalledBundleResource == "" { |
There was a problem hiding this comment.
Don't read our own status here to determine if there is an installed bundle. Instead, we need to Get the App that would be managed by this Extension.
If it does not exist:
- installedBundleResource = ""
- installed condition = False
If we encountered some other error:
- don't touch installedBundleResource?
- installed condition = Unknown
If it does exist:
- installedBundleResource =
- installed condition = true
| setDeprecationStatusesUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration()) | ||
| return ctrl.Result{}, errkappAPIUnavailable | ||
| } | ||
|
|
||
| // TODO: Improve the resolution logic. | ||
| bundle, err := r.resolve(ctx, *ext) | ||
| if err != nil { | ||
| ext.Status.InstalledBundleResource = "" | ||
| setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration()) | ||
| if ext.Status.InstalledBundleResource == "" { |
There was a problem hiding this comment.
Same comment here about not reading our own status.
| ext.Status.ResolvedBundleResource = "" | ||
| setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
| setProgressingStatusConditionResolutionFailed(&ext.Status.Conditions, "resolution failed: "+err.Error(), ext.GetGeneration()) | ||
| setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as resolution failed", ext.GetGeneration()) |
There was a problem hiding this comment.
I know you didn't touch this part of the code, but we should be able to determine deprecation status:
- for package and channel if we have a catalog
- for bundle if we have an installed bundle
Can you make a follow-up issue to improve this?
| // hasn't been attempted yet, due to the spec being invalid. | ||
| setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
| } | ||
| setProgressingStatusConditionUnsupportedOrInvalidBundle(&ext.Status.Conditions, "failed to read bundle mediaType: "+err.Error(), ext.GetGeneration()) |
There was a problem hiding this comment.
It is generally prefered to use fmt.Sprintf over string concatenation via +.
| setProgressingStatusConditionUnsupportedOrInvalidBundle(&ext.Status.Conditions, "failed to read bundle mediaType: "+err.Error(), ext.GetGeneration()) | |
| setProgressingStatusConditionUnsupportedOrInvalidBundle(&ext.Status.Conditions, fmt.Sprintf("failed to read bundle mediaType: %v", err), ext.GetGeneration()) |
| if ext.Status.InstalledBundleResource == "" { | ||
| setInstalledStatusConditionUnknown(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration()) | ||
| } |
There was a problem hiding this comment.
I would only be expecting changes to the InstalledBundleResource and the Installed condition based on Get-ing the existing App.
Resolution and Installation are orthogonal. A failure computing a resolution shouldn't impact our ability to assess the installed state and vice versa.
This is similar to the point I made above about assessing Deprecation status separately.
| @@ -194,6 +198,8 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext | |||
| // originally Reason: ocv1alpha1.ReasonInstallationFailed | |||
| ext.Status.InstalledBundleResource = "" | |||
| setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | |||
| setProgressingStatusConditionFalse(&ext.Status.Conditions, "app install failed", ext.GetGeneration()) | |||
There was a problem hiding this comment.
This seems like a case where I'd say Progressing is still True because we encountered a failure but are going to retry.
| @@ -203,11 +209,15 @@ func (r *ExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.Ext | |||
| // originally Reason: ocv1alpha1.ReasonInstallationStatusUnknown | |||
| ext.Status.InstalledBundleResource = "" | |||
| setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | |||
| setProgressingStatusConditionFalse(&ext.Status.Conditions, "app install failed", ext.GetGeneration()) | |||
There was a problem hiding this comment.
Same comment here as above. A rule of thumb is: if we're going to end up returning an error from Reconcile, that means we're going to retry, which means we're still progressing.
| Status: metav1.ConditionFalse, | ||
| Reason: ocv1alpha1.ReasonResolutionFailed, | ||
| Reason: ocv1alpha1.ReasonPending, |
There was a problem hiding this comment.
Agree to that. ProgressingFalse can indicate a few things:
- Installation of the bundle was successful, we are in a stable state.
- Installation of bundle failed, we are again in a stable state, but look to the install status to know more.
- We are at the Resolution step for the next reconcile probably caused by upgrade - haven't yet reached to the point of install.
| // setResolvedStatusConditionUnknown sets the resolved status condition to unknown. | ||
| func setResolvedStatusConditionUnknown(conditions *[]metav1.Condition, message string, generation int64) { | ||
| // setProgressingStatusConditionUnknown sets the progressing status condition to unknown. | ||
| func setProgressingStatusConditionUnknown(conditions *[]metav1.Condition, message string, generation int64) { |
There was a problem hiding this comment.
I've commented below on trying to avoid Unknown for Progressing.
| Reason: ocv1alpha1.ReasonUnsupportedOrInvalidBundle, | ||
| Message: message, | ||
| ObservedGeneration: generation, | ||
| }) |
There was a problem hiding this comment.
Nit: We could also pass in the the reason to the method signature, such that these helpers are used to just indicate if the particular Status is set to false or true. Its a personal preference, so whatever you think makes it easier to read the code.
| ext.Status.ResolvedBundleResource = "" | ||
| setResolvedStatusConditionUnknown(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration()) | ||
| setProgressingStatusConditionUnknown(&ext.Status.Conditions, "extension feature is disabled", ext.GetGeneration()) |
There was a problem hiding this comment.
Ah, I see here.
Do we need to reset the progressing condition here to unknown? Can't it just be false? Having too many combinations can also be confusing, it would be nice if we can reduce the dimensionality of Progressing to either False or True. Especially given we have clarity on other stages.
There was a problem hiding this comment.
+1, but as I mentioned on our call, I'm also personally okay with following the existing pattern in this PR and then working in a separate followup to reorganize the code around the fact that the various condition types can be calculated and determined in isolation from each other.
| ext.Status.ResolvedBundleResource = "" | ||
| setResolvedStatusConditionUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration()) | ||
|
|
||
| setProgressingStatusConditionUnknown(&ext.Status.Conditions, "kapp apis are unavailable", ext.GetGeneration()) |
There was a problem hiding this comment.
I would also suggest to make this False instead of Unknown. This bit of code that checks if the Apis exists is going to be removed anyway though. The change is in the unit test PR.
| ext.Status.ResolvedBundleResource = "" | ||
| setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
| setProgressingStatusConditionResolutionFailed(&ext.Status.Conditions, "resolution failed: "+err.Error(), ext.GetGeneration()) | ||
| setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as resolution failed", ext.GetGeneration()) |
| ext.Status.InstalledBundleResource = "" | ||
| setInstalledStatusConditionUnknown(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration()) | ||
| if ext.Status.InstalledBundleResource == "" { | ||
| setInstalledStatusConditionUnknown(&ext.Status.Conditions, fmt.Sprintf("bundle type %s not supported currently", mediaType), ext.GetGeneration()) |
There was a problem hiding this comment.
I'm starting to re-think based on my previous comment of avoiding Unknown in Progressing. Can we also make this to False?
Resolution hasn't begun, spec is invalid, and no instance of bundle exists on the cluster. The above does give us some confidence to set Install to false.
| setInstalledStatusConditionUnknown(&ext.Status.Conditions, "install status unknown", ext.Generation) | ||
| // mapAppStatusToCondition maps the reconciling/deleting App conditions to the installed/deleting conditions on the Extension. | ||
| func MapAppStatusToCondition(existingApp *kappctrlv1alpha1.App, ext *ocv1alpha1.Extension) { | ||
| // Note: App.Status.Inspect errors are never surfaced to App conditions, so are currently ignored when determining App status. |
There was a problem hiding this comment.
iirc, also Status.Inspect appears if the knob to inspect resources is set to true in the spec. We would have to probably cover this aspect better with the health checks.
| for _, cond := range orderedAppStatuses { | ||
| if c := findStatusCondition(existingApp.Status.GenericStatus.Conditions, cond); c != nil && c.Status == corev1.ConditionTrue { | ||
| if len(message) == 0 { | ||
| message = c.Message | ||
| } | ||
| appStatusMapFn[cond](&ext.Status.Conditions, message, ext.Generation) | ||
| return | ||
| } | ||
| } |
There was a problem hiding this comment.
Instead of looping through these orderedAppStatuses and then doing a loop through the conditions to find them, could we just loop through all the App conditions, check if there is a status mapping function, and if there is one call it?
openshift-merge-robot
added
the
needs-rebase
ankitathomas
force-pushed
the
kapp-status
branch
3 times, most recently
from
139a87d
to
fe5b3c5
Compare
ankitathomas
force-pushed
the
kapp-status
branch
7 times, most recently
from
6deccc5
to
30709da
Compare
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
ankitathomas
force-pushed
the
kapp-status
branch
from
30709da
to
781f915
Compare
openshift-merge-robot
removed
the
needs-rebase
ankitathomas
force-pushed
the
kapp-status
branch
from
781f915
to
55085aa
Compare
ankitathomas
changed the title
ankitathomas
force-pushed
the
kapp-status
branch
from
55085aa
to
0e1abc3
Compare
There was a problem hiding this comment.
Nice job @ankitathomas! Just one nit. Once you decide on that, let's merge it!
| apimeta.SetStatusCondition(conditions, metav1.Condition{ | ||
| Type: ocv1alpha1.TypeProgressing, | ||
| Status: metav1.ConditionFalse, | ||
| Reason: ocv1alpha1.ReasonSuccess, |
There was a problem hiding this comment.
Nit: something like ReasonReachedDesiredIntent seems more apt for the False status of the progressing condition. WDYT?
| // Populate the deprecation status using the resolved bundle | ||
| SetDeprecationStatusInExtension(ext, bundle) | ||
|
|
There was a problem hiding this comment.
Nit: Based on the current state of the logic in determining deprecation status, and without increasing scope to include modifications to it, I think this shouldn't be called until installation has succeeded.
There is some nuance where we theoretically could set the package and/or channel deprecation status after resolution but IMO any changes to the existing deprecation logic is out of scope of this PR.
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
ankitathomas
dismissed stale reviews from everettraven and joelanford
via
ce3a005
ankitathomas
force-pushed
the
kapp-status
branch
2 times, most recently
from
90fc6ba
to
ba6ec56
Compare
Signed-off-by: Ankita Thomas <ankithom@redhat.com>
ankitathomas
force-pushed
the
kapp-status
branch
from
ba6ec56
to
27a6d32
Compare
| appStatusMapFn := map[kappctrlv1alpha1.ConditionType]func(*[]metav1.Condition, string, int64){ | ||
| kappctrlv1alpha1.Deleting: setProgressingStatusConditionProgressing, | ||
| kappctrlv1alpha1.Reconciling: setProgressingStatusConditionProgressing, | ||
| kappctrlv1alpha1.DeleteFailed: setProgressingStatusConditionFailed, | ||
| kappctrlv1alpha1.ReconcileFailed: setProgressingStatusConditionFailed, | ||
| kappctrlv1alpha1.ReconcileSucceeded: setProgressingStatusConditionSuccess, | ||
| } | ||
| for cond := range appStatusMapFn { | ||
| if c := findStatusCondition(existingApp.Status.GenericStatus.Conditions, cond); c != nil && c.Status == corev1.ConditionTrue { | ||
| if len(message) == 0 { | ||
| message = c.Message | ||
| } | ||
| appStatusMapFn[cond](&ext.Status.Conditions, fmt.Sprintf("App %s: %s", c.Type, message), ext.Generation) | ||
| return | ||
| } | ||
| } | ||
| if len(message) == 0 { | ||
| message = "waiting for app" | ||
| } | ||
| setProgressingStatusConditionProgressing(&ext.Status.Conditions, message, ext.Generation) |
There was a problem hiding this comment.
I'm not a huge fan of looping through all the status conditions 5 times but since we are moving away from building on top of the App CR and this is an optimization that could be made in the future I'm fine with it for now
Description
Decouple the extension status conditions and introduce a new Progressing condition.
See #655
The installed condition now indicates whether the last installed App was successfully applied.
The progressing condition indicates whether resolution, installation, upgrade or app reconciliation is in flight, with the reason and message on the condition providing more information.
Reviewer Checklist