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

✨ Add healthy status condition to ClusterExtension #600

Conversation

everettraven
Copy link
Contributor

@everettraven everettraven commented Jan 31, 2024

Description

  • Adds a Healthy status condition type to the ClusterExtension status conditions. This value is populated based on two things:
    • If installation status is failed or unknown, the Healthy status will be unknown
    • The healthy status condition of the associated BundleDeployment

Reviewer Checklist

  • API Go Documentation
  • Tests: Unit Tests (and E2E Tests, if appropriate)
  • Comprehensive Commit Messages
  • Links to related GitHub Issue(s)

that is based on the corresponding BundleDeployment's
Healthy status condition setting

Signed-off-by: everettraven <everettraven@gmail.com>
@everettraven everettraven requested a review from a team as a code owner January 31, 2024 20:00
…onsets

Signed-off-by: everettraven <everettraven@gmail.com>
@tmshort
Copy link
Contributor

tmshort commented Feb 8, 2024

Should revisit after #598 when Extension is added.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 21, 2024
@openshift-merge-robot
Copy link

PR needs rebase.

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/test-infra repository.

@perdasilva
Copy link
Contributor

Quick question: if the installation is failed, why healthy is unknown? Would false make more sense? Or is this more metaphysical question like: if something doesn't exist it can't be anything, healthy or otherwise XD. Just wondering if there's a standard/best practice here.

@joelanford
Copy link
Member

I think we need to refactor this PR based on some discussion that happened over in Extension land with @ankitathomas's work on conditions there. #683

TL;DR is that we should structure the conditions to be as orthogonal/decoupled as possible:

  • Resolution depends only on having catalog data available
  • Installed determination depends only on assessing what is currently installed (does not depend on resolution)
  • Deprecation depends on:
    • For package and channel, only having catalog data available
    • For bundle, having catalog data available and knowledge of what is currently installed
  • Healthy determination depends on having something currently installed, and being able to assess its health.
  • Progressing is a new one we're adding that helps conveying concepts like "v1 is currently installed, but we're trying to upgrade to v2"

@everettraven
Copy link
Contributor Author

@joelanford What do you think about closing out this PR, waiting for the work to port some of the Extension controller logic over to the ClusterExtension, then opening a new PR with these changes?

I am leaning towards this approach for two reasons:

  1. Keeps the scope of the work to only adding the healthy status condition and making sure it follows the condition structure you outlined (should be straightforward if all the other conditions are in place)
  2. This PR still builds on the fact that it is pulling the health condition from Rukpak's BundleDeployment API which will shortly no longer be the way we are deploying and monitoring content for the ClusterExtension API

Overall I think it would be less work in the long run to hold this functionality until the larger refactoring work with our new direction is done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants