Skip to content

Update tags proposal#2609

Open
arshadd-b wants to merge 3 commits intokubernetes-sigs:mainfrom
arshadd-b:update-tag-proposal
Open

Update tags proposal#2609
arshadd-b wants to merge 3 commits intokubernetes-sigs:mainfrom
arshadd-b:update-tag-proposal

Conversation

@arshadd-b
Copy link
Contributor

@arshadd-b arshadd-b commented Jan 27, 2026

What this PR does / why we need it:
Improves tags proposal
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

/area provider/ibmcloud

  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

Update tags proposal

@k8s-ci-robot k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label Jan 27, 2026
@netlify
Copy link

netlify bot commented Jan 27, 2026

Deploy Preview for kubernetes-sigs-cluster-api-ibmcloud ready!

Name Link
🔨 Latest commit a20e0b8
🔍 Latest deploy log https://app.netlify.com/projects/kubernetes-sigs-cluster-api-ibmcloud/deploys/69a6d4075e38ce0008821690
😎 Deploy Preview https://deploy-preview-2609.cluster-api-ibmcloud.sigs.k8s.io
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: arshadd-b
Once this PR has been reviewed and has the lgtm label, please assign prajyot-parab for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 27, 2026
@arshadd-b
Copy link
Contributor Author

Hi @Amulyam24
Actually while we are implementing this tags feature , have encountered one scenario when we are creating cluster infra
what if resource is created and attaching tag fails . In some resources we are making different call for resource creation and attach tag like LB, VPC. In this case we are thinking if attach tag fails then we will also delete the resource before we return error.
Could you please let me know your thoughts. Thanks
cc: @Karthik-K-N

Below are the cluster creation scenarios.
#### Creating a new cluster
- When resources will be created for new cluster in the cloud the tag will be attached. During deletion flow, will check for tag `powervs.cluster.x-k8s.io/cluster-uuid: UUID` and delete the resources.
- When resources will be created for new cluster in the cloud the tag will be attached. During deletion flow, will check for tag `powervs.cluster.x-k8s.io/cluster-uuid: UUID` and delete the resources. During creation if resource is created successfully but failed to attach the tag, will clean up the created resource as well before returning error.
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure what else we can do, If we relax resource creation then deletion will be an issue.

I think we need to set some condition or event or something to let the user explicitly know that tag is failing so not able to proceed in creating the resource?

/cc @dharaneeshvrd

Copy link
Contributor Author

@arshadd-b arshadd-b Feb 2, 2026

Choose a reason for hiding this comment

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

But the thing is we have different calls for creation and attach . So the resource is created but attached failed we will error out to user and set condition but what about that created resource ?

Copy link
Contributor

Choose a reason for hiding this comment

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

What if we retry the attach tag alone for a few times and if it doesn't succeed, then proceed with deletion, notifying the user to try a different region? Not sure if tagging is affected across regions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah makes sense to me , we can have retry on attach tag itself and after that as well if it is failing, we will delete the resource and ask the user to try in different region

Copy link
Contributor

Choose a reason for hiding this comment

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

Say the tag fails for multiple retries and you want to delete all the resources created by the controller then in the next reconcile the the controller will again creates the resources. Isn't it?

Copy link
Contributor Author

@arshadd-b arshadd-b Feb 11, 2026

Choose a reason for hiding this comment

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

yeah actually what I am thinking if attach tag is failing , we will retry there itself instead of reconcile. If after retries still it fails we will delete resource and error to user to try in different region. If again user trigger cluster creation in the same region then yes again reconcile will happen and same process will be repeated . But not sure what else we can do here .

Copy link
Contributor

@Karthik-K-N Karthik-K-N Feb 11, 2026

Choose a reason for hiding this comment

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

If after retries still it fails we will delete resource and error to user trying in different region.

How do you do this in controller?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If after retries still it fails we will delete resource and error to user trying in different region.

How do you do this in controller?

I think we will just error out to user, User has to provide different region

Copy link
Contributor

Choose a reason for hiding this comment

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

Had a discussion in CAPIBM meeting and also with Arshad and here are the points we can do

  1. Lets create a condition type something like AttachTag and set it on the IBMPowerVSCluster whenever a tag creation fails
  2. The condition message should be descriptive, Should be like "Failed to attach tag to newly created Workspace "workspace-name", either manually attach tag or delete the cluster and retry it in different region.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @Karthik-K-N I will update the proposal as well with these details

Below are the cluster creation scenarios.
#### Creating a new cluster
- When resources will be created for new cluster in the cloud the tag will be attached. During deletion flow, will check for tag `powervs.cluster.x-k8s.io/cluster-uuid: UUID` and delete the resources.
- When resources will be created for new cluster in the cloud the tag will be attached. During deletion flow, will check for tag `powervs.cluster.x-k8s.io/cluster-uuid: UUID` and delete the resources. During creation if resource is created successfully but failed to attach the tag, will retry for some time and if it still fails, we will set condition with descriptive message `Failed to attach tag to newly created Workspace workspace-name, either manually attach tag or delete the cluster and retry it in different region.`
Copy link
Contributor

Choose a reason for hiding this comment

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

This approach LGTM but I think it should be documented in generic format, . @Amulyam24 Please suggest

Copy link
Contributor

@Amulyam24 Amulyam24 Feb 24, 2026

Choose a reason for hiding this comment

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

I think it will be good to list down failure and success scenarios separately for readability.

You might want to add details about the new condition as well with an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @Amulyam24 I am done with suggested changes
Thanks

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 3, 2026
@arshadd-b arshadd-b requested a review from Karthik-K-N March 3, 2026 12:19
@arshadd-b arshadd-b force-pushed the update-tag-proposal branch from b4b218a to fb3be78 Compare March 3, 2026 12:26
@arshadd-b arshadd-b force-pushed the update-tag-proposal branch from fb3be78 to a20e0b8 Compare March 3, 2026 12:28
Copy link
Contributor

@Amulyam24 Amulyam24 left a comment

Choose a reason for hiding this comment

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

Overall LGTM, have added minor comments.

Comment on lines +54 to +55
When the user triggers deletion of a cluster where tag attachment previously failed, the controller will check the condition status. Based on this condition, the controller will determine whether to proceed with resource deletion.

Copy link
Contributor

Choose a reason for hiding this comment

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

This point seems vague, you might have to add specific details on the action being taken by the controller.

Below are the cluster creation scenarios.
#### Creating a new cluster
- When resources will be created for new cluster in the cloud the tag will be attached. During deletion flow, will check for tag `powervs.cluster.x-k8s.io/cluster-uuid: UUID` and delete the resources.
##### Tag Attachment Scenarios
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you PTAL at the format of this section?

1. Retry Attempts: The controller retries attaching the tag multiple times over a configured period.
2. After Retries Fail: If all retries fail, the controller:
- Sets a warning condition on the cluster
- Adds an error message like: "Failed to attach tag to newly created Workspace <workspace-name>. Please manually attach the tag or delete the cluster and recreate it."
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: an example of the condition in the status field would be helpful

1. Retry Attempts: The controller retries attaching the tag multiple times over a configured period.
2. After Retries Fail: If all retries fail, the controller:
- Sets a warning condition on the cluster
- Adds an error message like: "Failed to attach tag to newly created Workspace <workspace-name>. Please manually attach the tag or delete the cluster and recreate it."
Copy link
Contributor

Choose a reason for hiding this comment

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

 Please manually attach the tag or delete the cluster and recreate it."

@arshadd-b @Karthik-K-N, for my understanding, what would be the purpose of user manually attaching the tag if the tag creation fails when we would proceed with deletion based on the condition being set and it is being handled for success and failure case?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants