Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(feature): cleanup funcs should not rely on feature's data #1184

Merged

Commits on Aug 16, 2024

  1. fix(feature): cleanup funcs should not rely on feature's data

    Initially, `Cleanup` functions did not rely on loading Feature's
    data for invocation, as certain data was held as fields of the Feature
    struct, making it implicitly available. With the refactoring of Feature
    to become a "glorified map" container, this behavior has changed,
    leading to errors under certain circumstances.
    
    With `DSCI.ServiceMesh` now being a pointer to a struct, there is a corner
    case where the (now needed) ServiceMesh spec is `nil`, leading to a panic.
    
    When the reconciliation of DSC is triggered, it is invoked for each
    component, even if it is never defined or changed. In a simple
    case where DSCI does not have a specified ServiceMesh (`nil` instead of the default),
    and DSC has a single `Managed` component that is not KServe/Serverless,
    the reconciliation will trigger the removal of KServe resources regardless.
    This, in turn, will call for the removal of Serverless features, which rely on
    SMCP config when they are applied. With data now being loaded in the cleanup
    step, this leads to an error, as there is no SMCP config to read from.
    
    By default, the cleanup logic removes the owner FeatureTracker for each Feature,
    and this does not require loading Feature's data, as FeatureTracker is used
    as `OwnerReference` for each resources created for a given Feature.
    
    The API allows defining custom functions that can be invoked in
    addition. There is one custom cleanup function requiring the Service Mesh
    control plane spec to remove part of the config introduced by its patch.
    The code in question only worked because it had default values to work
    on and was not failing if patched object was not found.
    
    The refactoring wrongly enforced loading of Feature data during cleanup for
    this single case, and this cascaded to other Feature sets where it was
    unnecessary exposing this faulty behaviour.
    
    With this change, no reference to Feature is enforced at the API
    level by introducing the `CleanupFunc(ctx, client)` type for defining
    custom cleanup logic. The custom patch function has been reworked
    to comply with this.
    bartoszmajsak committed Aug 16, 2024
    Configuration menu
    Copy the full SHA
    5bb6291 View commit details
    Browse the repository at this point in the history