Skip to content

KAAP-657: Remove pf9 cluster label as a part of scaling down or byoh cluster deletion #38

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

vaibhavd21
Copy link

@vaibhavd21 vaibhavd21 commented May 15, 2025

UI adds a label kaapi.pf9.io/cluster-name on the byohost object whenever it is selected or is a part of some cluster.

Whenever cluster is deleted or that specific byohost is scaled down, the respective byomachine is deleted and byohost is cleaned up and ready to attach to other cluster. Removing kaapi.pf9.io/cluster-name as a part of this.

Summary by Bito

This PR updates the management of cluster labels for byohost objects by introducing a standardized cluster label constant and improving the cleanup process in the byomachine controller. These changes ensure proper removal of unwanted cluster labels during scaling down or deletion, enhancing overall cluster state management reliability.

Unit tests added: False

Estimated effort to review (1-5, lower is better): 1

Copy link

bito-code-review bot commented May 15, 2025

Code Review Agent Run #26d9d5

Actionable Suggestions - 0
Additional Suggestions - 2
  • controllers/infrastructure/byomachine_controller.go - 1
    • Incorrect logger formatting in markHostForCleanup function · Line 614-614
  • apis/infrastructure/v1beta1/byohost_types.go - 1
    • Inconsistent namespace prefix in constant definition · Line 23-24
Review Details
  • Files reviewed - 2 · Commit Range: f3f9360..f3f9360
    • apis/infrastructure/v1beta1/byohost_types.go
    • controllers/infrastructure/byomachine_controller.go
  • Files skipped - 0
  • Tools
    • Whispers (Secret Scanner) - ✔︎ Successful
    • Detect-secrets (Secret Scanner) - ✔︎ Successful

Bito Usage Guide

Commands

Type the following command in the pull request comment and save the comment.

  • /review - Manually triggers a full AI review.

  • /pause - Pauses automatic reviews on this pull request.

  • /resume - Resumes automatic reviews.

  • /resolve - Marks all Bito-posted review comments as resolved.

  • /abort - Cancels all in-progress reviews.

Refer to the documentation for additional commands.

Configuration

This repository uses Default Agent You can customize the agent settings here or contact your Bito workspace admin at [email protected].

Documentation & Help

AI Code Review powered by Bito Logo

Copy link

Changelist by Bito

This pull request implements the following key changes.

Key Change Files Impacted
New Feature - Introducing Cluster Label Constant

byohost_types.go - Added a new constant 'ClusterLabel' to mark the cluster association on the byohost object.

Bug Fix - Removal of Cluster Label During Cleanup

byomachine_controller.go - Enhanced the cleanup process by logging and removing the pf9 cluster label from the byohost, ensuring proper state reset during scaling down or deletion.

Copy link

@cruizen cruizen left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -20,6 +20,8 @@ const (
AttachedByoMachineLabel = "byoh.infrastructure.cluster.x-k8s.io/byomachine-name"
// BundleLookupBaseRegistryAnnotation annotation used to store the base registry for the bundle lookup
BundleLookupBaseRegistryAnnotation = "byoh.infrastructure.cluster.x-k8s.io/bundle-registry"
// ClusterLabel label is used to mark a cluster where it is attached to
ClusterLabel = "kaapi.pf9.io/cluster-name"
Copy link

Choose a reason for hiding this comment

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

Can we make this a constant defined in a common global module?

Copy link

@srm6867 srm6867 left a comment

Choose a reason for hiding this comment

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

/lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants