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

Get status.conditions from CAPI operator during updateComponentsStatus #341

Merged
merged 4 commits into from
Oct 31, 2024

Conversation

squizzi
Copy link
Contributor

@squizzi squizzi commented Sep 17, 2024

In this PR we now update our component statuses by iterating the provider CRDs affiliated with CAPI operator and returning a status error if any of the status.conditions are False or if we cannot obtain any information pertaining to status condition. For example:

In this case cluster-api-provider-aws and -azure are not yet installed:

  status:
    availableProviders: {}
    components:
      cluster-api:
        success: true
      cluster-api-provider-aws:
        error: 'failed to find any provider resources affiliated with template: cluster-api-provider-aws'
      cluster-api-provider-azure:
        error: 'failed to find any provider resources affiliated with template: cluster-api-provider-azure'
      hmc:
        success: true
      k0smotron:
        success: true

In this case cluster-api-provider-aws is reporting some status conditions false:

  status:
    availableProviders: {}
    components:
      cluster-api:
        success: true
      cluster-api-provider-aws:
        error: 'InfrastructureProvider: aws is not yet ready, has false conditions: MinimumReplicasAvailable'
      cluster-api-provider-azure:
        success: true
      hmc:
        success: true
      k0smotron:
        success: true

In this case cluster-api-provider-aws has no status.conditions because something is going wrong. Perhaps we could add some additional statuses for special circumstances; here it's because aws-variables does not yet exist yet, but I couldn't find anyway to assume this type of circumstance, capi-operator doesn't produce any event in this case, it only generates a log.

    components:
      cluster-api:
        success: true
      cluster-api-provider-aws:
        error: 'failed to get status for template: cluster-api-provider-aws: failed
          to get conditions: no status conditions found for InfrastructureProvider:
          aws'
      cluster-api-provider-azure:
        success: true
      hmc:
        success: true
      k0smotron:
        success: true

Closes #235

@squizzi squizzi force-pushed the patch_component-status-check-readiness branch from 06fe1e2 to 83d09f5 Compare September 18, 2024 15:52
@squizzi
Copy link
Contributor Author

squizzi commented Sep 18, 2024

I can probably add some unit tests to this using envtest, I just wanted to get some thoughts/eyes on before I do that.

Edit: This apparently isn't trivial to test in envtest, perhaps I could add an e2e test to the controller suite, I'm working on breaking the providers/controllers tests apart for the vsphere tests to work right anyways so that might work well.

wahabmk
wahabmk previously approved these changes Sep 24, 2024
internal/utils/conditions.go Outdated Show resolved Hide resolved
internal/controller/management_controller.go Outdated Show resolved Hide resolved
internal/controller/management_controller.go Outdated Show resolved Hide resolved
@squizzi
Copy link
Contributor Author

squizzi commented Sep 24, 2024

I'll plan to add an e2e test in a follow-up once the controller tests have been refactored as part of #360

@squizzi squizzi force-pushed the patch_component-status-check-readiness branch from 7e84fac to 62b1c3a Compare September 24, 2024 18:10
@squizzi squizzi requested a review from wahabmk September 25, 2024 18:06
@squizzi squizzi force-pushed the patch_component-status-check-readiness branch 7 times, most recently from 8f4437e to 552ec53 Compare October 1, 2024 16:19
internal/utils/status/status.go Outdated Show resolved Hide resolved
internal/controller/management_controller.go Outdated Show resolved Hide resolved
internal/utils/status/status.go Outdated Show resolved Hide resolved
internal/utils/status/status.go Outdated Show resolved Hide resolved
internal/controller/management_controller.go Outdated Show resolved Hide resolved
@squizzi squizzi force-pushed the patch_component-status-check-readiness branch 2 times, most recently from 2134943 to f6441f1 Compare October 2, 2024 23:10
@squizzi squizzi requested a review from zerospiel October 2, 2024 23:10
zerospiel
zerospiel previously approved these changes Oct 3, 2024
@squizzi squizzi force-pushed the patch_component-status-check-readiness branch from 53ba0e5 to 559d10b Compare October 11, 2024 17:22
slysunkin
slysunkin previously approved these changes Oct 11, 2024
@squizzi squizzi force-pushed the patch_component-status-check-readiness branch from 559d10b to 3fd8c05 Compare October 14, 2024 19:07
@squizzi squizzi force-pushed the patch_component-status-check-readiness branch 5 times, most recently from 721fd70 to 23df058 Compare October 17, 2024 21:53
a13x5
a13x5 previously requested changes Oct 18, 2024
Copy link
Collaborator

@a13x5 a13x5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some resolved conversations aren't resolved for some reason.
Several other notes, mostly minor

internal/controller/management_controller.go Outdated Show resolved Hide resolved
internal/controller/management_controller.go Outdated Show resolved Hide resolved
internal/utils/status/status.go Outdated Show resolved Hide resolved
internal/utils/status/status.go Show resolved Hide resolved
internal/utils/status/status.go Outdated Show resolved Hide resolved
@squizzi squizzi force-pushed the patch_component-status-check-readiness branch from 23df058 to 2905c9a Compare October 21, 2024 18:40
@squizzi squizzi requested a review from a13x5 October 21, 2024 21:07
@squizzi squizzi requested a review from eromanova October 22, 2024 20:21
eromanova
eromanova previously approved these changes Oct 23, 2024
@squizzi squizzi force-pushed the patch_component-status-check-readiness branch from acc8d45 to 23fe371 Compare October 31, 2024 14:19
@squizzi
Copy link
Contributor Author

squizzi commented Oct 31, 2024

@eromanova another rebase cleared your review, if you don't mind approving again

Signed-off-by: Kyle Squizzato <[email protected]>
@squizzi squizzi merged commit 29bc4b4 into k0rdent:main Oct 31, 2024
5 checks passed
@squizzi squizzi deleted the patch_component-status-check-readiness branch October 31, 2024 19:49
bnallapeta pushed a commit to bnallapeta/hmc that referenced this pull request Nov 15, 2024
…us` (k0rdent#341)

* Differentiate status into own package
* Use struct for resource condition return, address review
* Filter by namespace within GetResourceConditions
* Fix linting issues

Signed-off-by: Kyle Squizzato <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Wait for Infrastructure/ControlPlane/BootstrapProvider readiness before reporting success
7 participants