Skip to content

Conversation

@arshadd-b
Copy link
Contributor

@arshadd-b arshadd-b commented May 20, 2025

What this PR does / why we need it:
Proposal for adding the tags to PowerVS Cluster resources and performing delete of resources on the bases of tags
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:

Proposal to use tags for tracking PowerVS cluster resources

@k8s-ci-robot k8s-ci-robot added the area/provider/ibmcloud Issues or PRs related to ibmcloud provider label May 20, 2025
@k8s-ci-robot k8s-ci-robot requested a review from Karthik-K-N May 20, 2025 08:00
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 20, 2025
@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 20, 2025
@k8s-ci-robot
Copy link
Contributor

Hi @arshadd-b. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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-sigs/prow repository.

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 20, 2025
@netlify
Copy link

netlify bot commented May 20, 2025

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

Name Link
🔨 Latest commit 3f3d33d
🔍 Latest deploy log https://app.netlify.com/projects/kubernetes-sigs-cluster-api-ibmcloud/deploys/693928dd4deb490008812346
😎 Deploy Preview https://deploy-preview-2364.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.

@Amulyam24
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 20, 2025
@Amulyam24
Copy link
Contributor

/retitle Proposal to use tags for tracking PowerVS cluster resources

@k8s-ci-robot k8s-ci-robot changed the title Add tags to PowerVS Cluster resources and delete resources based on tags Proposal to use tags for tracking PowerVS cluster resources May 22, 2025
Copy link
Contributor

@Karthik-K-N Karthik-K-N left a comment

Choose a reason for hiding this comment

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

In the delete flow diagram, I think If is not needed inside the rhombus as its already a decision block
In create flow diagram I think you missed to consider COSInstance

Copy link
Contributor

@Karthik-K-N Karthik-K-N left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, Most of the things are good,lets update it bit more to make it better.

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 29, 2025
@arshadd-b arshadd-b force-pushed the add-tags-proposal branch from 48c48f4 to e0b2d89 Compare May 29, 2025 13:48
@arshadd-b
Copy link
Contributor Author

In the delete flow diagram, I think If is not needed inside the rhombus as its already a decision block In create flow diagram I think you missed to consider COSInstance

done

@arshadd-b arshadd-b requested a review from Karthik-K-N May 30, 2025 10:22
@arshadd-b arshadd-b force-pushed the add-tags-proposal branch 2 times, most recently from d862267 to 0f4cb6b Compare May 30, 2025 13:29
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 30, 2025
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.

Hi @arshadd-b, Thank you for the proposal!

I'm not clear on how user tags will be used apart from the controller tag. Can you please share more details on it?

@arshadd-b
Copy link
Contributor Author

Hi @arshadd-b, Thank you for the proposal!

I'm not clear on how user tags will be used apart from the controller tag. Can you please share more details on it?

Hi @Amulyam24 , It is the same functionality that we do from UI, adding tags to IBM Cloud resources if user wants to tag resources. It can help to user in resource management.

@arshadd-b arshadd-b requested a review from Amulyam24 June 11, 2025 14:25
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.

Hi @Amulyam24 , It is the same functionality that we do from UI, adding tags to IBM Cloud resources if user wants to tag resources. It can help to user in resource management.

Got it, Thanks for the clarification.

A couple of suggestions have been added.

In the create flow diagram, how about we enhance the condition such as cluster.spec.UserTags > 0 , then proceed to attach user provided tags.

Comment on lines 37 to 38
- Currently TransitGateway Connections doesn't support tagging, So we will handle deletion of connections based on VPC.
- DHCP Server doesn't support tagging, So we will tag DHCP Network and handle deletion based on Network.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- Currently TransitGateway Connections doesn't support tagging, So we will handle deletion of connections based on VPC.
- DHCP Server doesn't support tagging, So we will tag DHCP Network and handle deletion based on Network.
Currently transit gateway connections and DHCP server don't support tagging. We will handle their deletion using the VPC and network tag respectively.

When DHCP server is created, we use its network right, is that taggable?

should we depend on workspace resource tag instead?

cc @Karthik-K-N

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Amulyam24 Actually there is ause case when workspace is already created but DHCP network is newly created,
in that case we want to delete the network only not workspace. So for this case we have to tag the network.
I have already checked for network tagging is supported .

@arshadd-b arshadd-b force-pushed the add-tags-proposal branch from c7fec6b to 14da794 Compare June 16, 2025 08:41
@arshadd-b
Copy link
Contributor Author

Hi @Amulyam24 , It is the same functionality that we do from UI, adding tags to IBM Cloud resources if user wants to tag resources. It can help to user in resource management.

Got it, Thanks for the clarification.

A couple of suggestions have been added.

In the create flow diagram, how about we enhance the condition such as cluster.spec.UserTags > 0 , then proceed to attach user provided tags.

Updated the flow diagram

@Amulyam24
Copy link
Contributor

@arshadd-b, PTAL at the failing verify check

@arshadd-b
Copy link
Contributor Author

@arshadd-b, PTAL at the failing verify check

sure will check

@arshadd-b
Copy link
Contributor Author

/retest

@arshadd-b
Copy link
Contributor Author

Looks like some links are changed and returning 500 now. will have a look

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.

IMO, tagging a resource with UUID alone should suffice right?
As in either case we check with it at the end and decide the flow based on that. wdyt?

@arshadd-b
Copy link
Contributor Author

IMO, tagging a resource with UUID alone should suffice right? As in either case we check with it at the end and decide the flow based on that. wdyt?

I think it should be okay to use only UUID alone

So before creating we check if resource exist with same UUID, then error out to user. If it doesn't exist then create.
what do you think ? @dharaneeshvrd @Karthik-K-N

@Karthik-K-N
Copy link
Contributor

IMO, tagging a resource with UUID alone should suffice right? As in either case we check with it at the end and decide the flow based on that. wdyt?

I think it should be okay to use only UUID alone

So before creating we check if resource exist with same UUID, then error out to user. If it doesn't exist then create. what do you think ? @dharaneeshvrd @Karthik-K-N

I think that should be perfect, I can't recall why we didn't drop using name when we discoverd UUID is necessary

### Controller tags
When cluster creation is triggered, resources gets created in the cloud. So to distinguish whether resources are newly created or user has given pre-existing resources,
tags of format`powervs.cluster.x-k8s.io/owner: <cluster-name>` and `powervs.cluster.x-k8s.io/cluster-uuid: UUID` will be added by the controller to newly created cloud resources marking the resource as created by controller.
During cluster creation with infrastructure creation if the resources are already present with the same name in the cloud. It will lead to security issues because there is a possibilty the existing resources in the cloud belong to different user. So to handle this scenario this tag `powervs.cluster.x-k8s.io/cluster-uuid: UUID` is added. UUID in tag `powervs.cluster.x-k8s.io/cluster-uuid: UUID` represents cluster object ID.
Copy link
Contributor

Choose a reason for hiding this comment

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

Are you planning to tag existing resource or not? Below statement stats that you are not and that makes sense. Maybe missed to remove?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No if user is using existing resources to create cluster we won't add any tag
here we have mentioned https://github.com/kubernetes-sigs/cluster-api-provider-ibmcloud/pull/2364/files#diff-702383f3b28082fe9eabfb59e5f348409edd0882e4f3a682690b3787847dd29eR36

@arshadd-b
Copy link
Contributor Author

Hi @Amulyam24 @Karthik-K-N @dharaneeshvrd
I had removed the tag powervs.cluster.x-k8s.io/owner: <cluster-name> based on discussion #2364 (comment)
Please take a look
Thanks

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.

/lgtm

Thank you @arshadd-b!

Please squash the commits.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2025
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 10, 2025
@arshadd-b
Copy link
Contributor Author

/lgtm

Thank you @arshadd-b!

Please squash the commits.

done

Copy link
Member

@Prajyot-Parab Prajyot-Parab left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 11, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Amulyam24, arshadd-b, Prajyot-Parab

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

The pull request process is described here

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 11, 2025
@Prajyot-Parab
Copy link
Member

Thanks! @arshadd-b

@Prajyot-Parab
Copy link
Member

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2025
@Prajyot-Parab
Copy link
Member

/test pull-cluster-api-provider-ibmcloud-verify

@Prajyot-Parab
Copy link
Member

/retest-required

@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Dec 11, 2025

@arshadd-b: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-cluster-api-provider-ccm-image 090d991 link true /test pull-cluster-api-provider-ccm-image
pull-cluster-api-provider-ibmcloud-verify 3f3d33d link true /test pull-cluster-api-provider-ibmcloud-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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-sigs/prow repository. I understand the commands that are listed here.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/provider/ibmcloud Issues or PRs related to ibmcloud provider cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants