Feature - Add/Modify/Delete tags for model resource by rkurduka · Pull Request #261 · aws-controllers-k8s/sagemaker-controller

rkurduka · 2024-03-01T10:25:33Z

Issue 1732, if available:

Description of changes:
Currently tags modification is not supported for model resource . Hence added require changes to support tags modification.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

a-hilaly · 2024-03-02T23:42:48Z

/retest

a-hilaly

@rkurduka we have plans to start generating Tag handlers for all the ACK controllers. But nothing stops us from implement some manually if there is urgent need for them :)
I left few comments in-line

a-hilaly · 2024-03-02T23:43:57Z

apis/v1alpha1/generator.yaml

+  AddTags:
+    operation_type: Update
+    resource_name: Model


This generates code that treats AddTags as the Update API Call to update the model.

@a-hilaly - Thanks for review.
Sagemaker model API do not have any modify /update method , as fields are immutable except tags . Also Sagemaker API resource only have "ADDTAG" and "DELETE TAG" . So to handle update on this resource will have to use one of these two. So chose "ADD TAGS" and handling "DELETE tags" using template hooks.

Let me know if you think any other approach we can consider

Agree with Amine, need to discuss this further as we need to throw appropriate error if any other fields except Tags are modified as Model does not have udpate API, maybe implement it completely via hooks

@surajkota - Added check in update_pre hooks to validate field to update, if it is other than tag , reconciler will update "message" status. Please review

Please let me know if you feel anything else is require

a-hilaly · 2024-03-02T23:45:10Z

helm/Chart.yaml

 description: A Helm chart for the ACK service controller for Amazon SageMaker (SageMaker)
-version: 1.2.7
-appVersion: 1.2.7
+version: 0.0.0-non-release-version


Please pull the controller tags

a-hilaly · 2024-03-02T23:45:28Z

config/crd/common/bases/services.k8s.aws_fieldexports.yaml

 metadata:
  annotations:
-    controller-gen.kubebuilder.io/version: v0.14.0
+    controller-gen.kubebuilder.io/version: v0.9.2


You need to install controller-gen v0.14.0

a-hilaly · 2024-03-02T23:46:02Z

templates/model/sdk_update_pre_build_request.go.tpl

+// this to handle delete/remove tags 
+_ , err = rm.deleteTags(ctx,desired,latest)
+if err != nil {
+	return nil, err
+}


We don't need to delete the tags before performing a tag update.

@a-hilaly - as I mentioned above , SDK update method is handling ADDTAGS only and hooks are deleting tags , there is no modify API available for sagemaker model resource . So in SDKUPDATE , first "DeletTags" method look for any deletion request , if not then proceed and look for add tags request .

Let me know if anything else is require.

a-hilaly · 2024-03-02T23:48:22Z

pkg/resource/model/hooks.go

+)
+
+// deleteTags is used to keep tags in sync by calling Create and Delete API's
+func (rm *resourceManager) deleteTags(


This function should stay, syncTags and also handle tag creation

@a-hilaly - same comments as above , let me know if any other approach we should consider here

a-hilaly · 2024-03-02T23:48:46Z

/hold

rkurduka · 2024-03-05T06:17:52Z

@rkurduka we have plans to start generating Tag handlers for all the ACK controllers. But nothing stops us from implement some manually if there is urgent need for them :) I left few comments in-line

@a-hilaly - thanks for update . Yes there was customer ask for this . Issue 1732 has details

ack-prow · 2024-03-05T06:22:15Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rkurduka
Once this PR has been reviewed and has the lgtm label, please ask for approval from ryansteakley by writing /assign @ryansteakley in a comment. For more information see the Kubernetes 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:

OWNERS

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

surajkota

See #261 (comment)

surajkota · 2024-03-05T16:47:44Z

pkg/resource/model/hooks.go

+}
+
+// computeTagsDelta returns tags to be added and removed from the resource
+func computeTagsDelta(


Sagemaker tag API pattern(addTag/deleteTag) resembles memorydb pattern (TagResource/UntagResource).

We have a reference implementation in memorydb which uses ackcompare.GetTagsDifference helper method which can reduce maintenance of SM controller code. https://github.com/aws-controllers-k8s/memorydb-controller/blob/main/pkg/resource/acl/hooks.go#L92

Can you please look into adopting similar pattern?

FYI @ryansteakley @a-hilaly

Thanks @surajkota for sharing this , and looking into it . But it seems memoryDB - ACL resource do have "UpdateACL" API method . So we can plug tags manipulation in it , using hooks. But in sagemaker -model case , there is NO update method available , so "SDKUPDATE block doesn't get generated by code-generator . So had to generate that using "ADDTags" in "Operation" Block , and later handling tags CRUD operations in it . Let me know if you have any suggestion in this case. Thanks again

ack-prow · 2024-08-30T17:18:42Z

@rkurduka: 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
sagemaker-unit-test	`99037c2`	link	true	`/test sagemaker-unit-test`
sagemaker-kind-e2e	`99037c2`	link	true	`/test sagemaker-kind-e2e`
sagemaker-verify-attribution	`99037c2`	link	true	`/test sagemaker-verify-attribution`

Full PR test history. Your PR dashboard.

Details

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

ack-bot · 2025-02-26T19:18:56Z

Issues go stale after 180d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 60d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle stale

ack-bot · 2025-04-27T20:05:11Z

Stale issues rot after 60d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 60d of inactivity.
If this issue is safe to close now please do so with /close.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/lifecycle rotten

ack-bot · 2025-06-26T20:53:15Z

Rotten issues close after 60d of inactivity.
Reopen the issue with /reopen.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/close

ack-prow · 2025-06-26T20:53:19Z

@ack-bot: Closed this PR.

Details

In response to this:

Rotten issues close after 60d of inactivity.
Reopen the issue with /reopen.
Provide feedback via https://github.com/aws-controllers-k8s/community.
/close

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.

rkurduka added 3 commits March 1, 2024 07:52

regenerated code again

f5b80a3

added a hook to handle modeldatasource updates

d5ca8ae

fixed update modeldatasource issue in sdkfind

63768ef

rkurduka requested review from ryansteakley and surajkota as code owners March 1, 2024 10:25

ack-prow bot requested a review from vijtrip2 March 1, 2024 10:25

ack-prow bot assigned ryansteakley Mar 2, 2024

a-hilaly reviewed Mar 2, 2024

View reviewed changes

ack-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 2, 2024

fixed versioning for code-generator and on helm values

0e99f49

surajkota suggested changes Mar 5, 2024

View reviewed changes

rkurduka added 2 commits March 6, 2024 08:56

added check to validate update field

9eca23c

added check to validate field for update

99037c2

ack-prow bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 26, 2025

ack-prow bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Apr 27, 2025

ack-prow bot closed this Jun 26, 2025

Conversation

rkurduka commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

a-hilaly commented Mar 2, 2024

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rkurduka Mar 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-hilaly commented Mar 2, 2024

Uh oh!

rkurduka commented Mar 5, 2024

Uh oh!

ack-prow bot commented Mar 5, 2024

Uh oh!

surajkota left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ack-prow bot commented Aug 30, 2024

Uh oh!

ack-bot commented Feb 26, 2025

Uh oh!

ack-bot commented Apr 27, 2025

Uh oh!

ack-bot commented Jun 26, 2025

Uh oh!

ack-prow bot commented Jun 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rkurduka commented Mar 1, 2024 •

edited

Loading

rkurduka Mar 6, 2024 •

edited

Loading

surajkota left a comment •

edited

Loading