Feature - Add/Modify/Delete tags for model resource #261

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

@@ -1,8 +1,8 @@
 apiVersion: v1
 name: sagemaker-chart
 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

@@ -3,7 +3,8 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 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.

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.

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.

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 surajkota and ryansteakley 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 requested 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature - Add/Modify/Delete tags for model resource #261

Feature - Add/Modify/Delete tags for model resource #261

rkurduka commented Mar 1, 2024 •

edited

Loading

a-hilaly commented Mar 2, 2024

a-hilaly left a comment

a-hilaly Mar 2, 2024

rkurduka Mar 5, 2024

surajkota Mar 5, 2024

rkurduka Mar 6, 2024 •

edited

Loading

a-hilaly Mar 2, 2024

rkurduka Mar 5, 2024

a-hilaly Mar 2, 2024

rkurduka Mar 5, 2024

a-hilaly Mar 2, 2024

rkurduka Mar 5, 2024

a-hilaly Mar 2, 2024

rkurduka Mar 5, 2024

a-hilaly commented Mar 2, 2024

rkurduka commented Mar 5, 2024

ack-prow bot commented Mar 5, 2024

surajkota left a comment •

edited

Loading

surajkota Mar 5, 2024

rkurduka Mar 6, 2024

ack-prow bot commented Aug 30, 2024

Feature - Add/Modify/Delete tags for model resource #261

Are you sure you want to change the base?

Feature - Add/Modify/Delete tags for model resource #261

Conversation

rkurduka commented Mar 1, 2024 • edited Loading

a-hilaly commented Mar 2, 2024

a-hilaly left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rkurduka Mar 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-hilaly commented Mar 2, 2024

rkurduka commented Mar 5, 2024

ack-prow bot commented Mar 5, 2024

surajkota left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ack-prow bot commented Aug 30, 2024

rkurduka commented Mar 1, 2024 •

edited

Loading

rkurduka Mar 6, 2024 •

edited

Loading

surajkota left a comment •

edited

Loading