feat: Adopt-or-Create #179

michaelhtm · 2025-04-04T22:58:54Z

Description of changes:
The changes handle the addition of a new adoption-policy called adopt-or-create.
The adopt-or-create policy requires users to define the Spec fields in the resource Spec,
and the Status fields in the adoption-fields annotation. Requiring the Spec fields be in Spec
for adopt-or-create ensures that the users provide fields that may also be required for a create
operation.

Here is the controller lifecycle of adopt-or-create policy:

Controller looks for resource in AWS
If found
   check if there are any differences between user resource definition (desired) and 
    resource definition in AWS (latest). Update if there are any differences
If not Found
  create the resource

Unlike the adopt adoption policy, if a resource has adopt-or-create policy and read-only, and it is not found
in AWS, the controller will mark that resource with a terminal error.

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

ack-prow · 2025-04-04T22:58:58Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: michaelhtm
Once this PR has been reviewed and has the lgtm label, please assign a-hilaly for approval by writing /assign @a-hilaly 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

Allow users to create the resource if adoption fails due to a `NotFound` error. With these changes, we are expecting users to populate all the necessary fields for creation in the reosurce spec, and the status fileds in the `adoption-fields` annotation

gfaraj · 2025-04-07T16:21:18Z

Hi @michaelhtm! Quick question -- will this overwrite the entire Kubernetes manifest if it's able to adopt an existing resource? From the code I'm seeing, it seems like that's the case.

In my case, I would like to avoid that if possible. Overwriting the manifest from the adopted resource means I have to hit "sync" again for my argo app. Really, I just want to upsert my resource to match my ACK spec.

While I think overwriting the whole thing does seem like a valid use case, maybe it's possible to support both cases? Might be looking at a separate adoption policy of "upsert" or something like that?

michaelhtm · 2025-04-07T16:24:57Z

@gfaraj Hello, The entire manifest would be replaced with the spec defined in AWS if the adoption-policy is adopt. With the current implementation, if it's adopt-or-create, the controller will do an update after the adoption, if not it will create the resource. Maybe we need a better name than adopt-or-create..

gfaraj · 2025-04-07T16:56:32Z

@michaelhtm oh nice, thanks for confirming that, I may have misread part of the changes. Yeah then this seems more like an "upsert" -- maybe that should be the policy name instead?

michaelhtm · 2025-04-21T21:03:43Z

/test sagemaker-controller-test

a-hilaly

Thanks @michaelhtm ! Looks good overall, I left a few comments below

pkg/runtime/reconciler.go

a-hilaly · 2025-04-22T18:23:38Z

pkg/runtime/reconciler.go


 	rlog.Info("Adopting Resource")
-	extractedFields, err := ExtractAdoptionFields(desired)
+	adoptionFields, err := ExtractAdoptionFields(desired)


does ExtractAdoptionFields need to be public?

not really. privatizing...

Actually there's more functions in util.go that are public without needing to be..we can change their signature during the next refactor..

a-hilaly · 2025-04-22T18:25:06Z

pkg/runtime/reconciler.go

+	err = populated.PopulateResourceFromAnnotation(adoptionFields)
+	// maybe don't return errors when it's adopt-or-create?
+	// TODO (michaelhtm) change PopulateResourceFromAnnotation to understand
+	// adopt-or-create, and validate Spec fields are not nil...


or maybe PopulateResourceFromAnnotation shouldn't return an error? how about a boolean, stating whether a resource got populated or not.

For this, i was mostly thinking we can pass the adoptionPolicy to PopulateResourceFromAnnotation. Doing so, we can decide not to populate Spec fields if the policy is Adopt-Or-Create...

Or..we can divide this up into two..PopulateSpecFromAnnotations, and PopulateStatusFromAnnotation..(we need a better name)

pkg/runtime/reconciler_test.go

a-hilaly · 2025-04-22T18:32:28Z

pkg/runtime/reconciler.go

 			return latest, err
 		}
-		if isAdopted {
+		if adoptionPolicy == AdoptPolicy || isAdopted {


do you also want to check for needAdoption

i check that early on..adoptionPolicy = "" if !needAdoption

a-hilaly · 2025-04-22T18:32:44Z

pkg/runtime/reconciler.go

 			return latest, err
 		}
-	} else {
+	} else if adoptionPolicy == AdoptPolicy {


a-hilaly · 2025-04-22T18:36:31Z

pkg/runtime/reconciler.go

nit: maybe it's about time to factor the Sync function in multiple small functions? maybe in a seperate PR? looks like it's growing exponentially, and most of it's logic is really the core of ACK.

rushmash91

Nice @michaelhtm !! would be great to have more details in the PR description.

pkg/runtime/reconciler.go

rushmash91 · 2025-04-22T18:50:02Z

pkg/runtime/reconciler.go

 	resolved, hasReferences, err := rm.ResolveReferences(ctx, r.apiReader, desired)
 	rlog.Exit("rm.ResolveReferences", err)
-	if err != nil {
+	// TODO (michaelhtm): should we fail here for `adopt-or-create` adoption policy?


Failing is consistent here.. this is why we see the issues like the adoption fails in VPC Peering in ec2 (for just adopt)?

The VPCPeering error was mostly happening after the adoption path was successful..

michaelhtm · 2025-04-25T18:13:36Z

/test sagemaker-controller-test

ack-prow · 2025-04-25T18:45:14Z

@michaelhtm: 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
verify-attribution	`6f7e297`	link	false	`/test verify-attribution`
sagemaker-controller-test	`6f7e297`	link	true	`/test sagemaker-controller-test`

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.

ack-prow bot requested review from a-hilaly and jlbutler April 4, 2025 22:58

michaelhtm changed the title ~~feat: Adopt-or-Create feature~~ feat: Adopt-or-Create Apr 4, 2025

feat: Adopt-or-Create feature

2163f84

Allow users to create the resource if adoption fails due to a `NotFound` error. With these changes, we are expecting users to populate all the necessary fields for creation in the reosurce spec, and the status fileds in the `adoption-fields` annotation

michaelhtm force-pushed the feat/adoptorcreate branch from efde0ab to 2163f84 Compare April 5, 2025 01:14

michaelhtm force-pushed the feat/adoptorcreate branch from 2310560 to 8bf6a25 Compare April 8, 2025 22:07

michaelhtm added 2 commits April 21, 2025 09:31

test: Ensure Unit tests reflect adopt-or-create feature

d40c004

minor refactors

e9c2402

michaelhtm force-pushed the feat/adoptorcreate branch from 6bb2bdd to e9c2402 Compare April 21, 2025 18:59

test: unit test Adopt-or-Create

0a84239

michaelhtm force-pushed the feat/adoptorcreate branch from 7a922a2 to 0a84239 Compare April 22, 2025 18:06

a-hilaly reviewed Apr 22, 2025

View reviewed changes

rushmash91 reviewed Apr 22, 2025

View reviewed changes

ADdress comments

6f7e297

michaelhtm force-pushed the feat/adoptorcreate branch from 0016367 to 6f7e297 Compare April 22, 2025 20:14

rushmash91 merged commit 910ed2a into aws-controllers-k8s:main Apr 25, 2025
5 of 8 checks passed

feat: Adopt-or-Create #179

feat: Adopt-or-Create #179

Uh oh!

Conversation

michaelhtm commented Apr 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-prow bot commented Apr 4, 2025

Uh oh!

gfaraj commented Apr 7, 2025

Uh oh!

michaelhtm commented Apr 7, 2025

Uh oh!

gfaraj commented Apr 7, 2025

Uh oh!

michaelhtm commented Apr 21, 2025

Uh oh!

a-hilaly left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

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!

rushmash91 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rushmash91 Apr 22, 2025 • 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!

michaelhtm Apr 22, 2025 • 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!

michaelhtm commented Apr 25, 2025

Uh oh!

ack-prow bot commented Apr 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

michaelhtm commented Apr 4, 2025 •

edited

Loading

rushmash91 Apr 22, 2025 •

edited

Loading

michaelhtm Apr 22, 2025 •

edited

Loading