Create adoption reconciler #2

RedbackThomson · 2021-02-10T00:26:36Z

Issue #, if available:

Description of changes:

Rename reconciler struct to ACKReconciler to allow for new reconciler types other than AWSResourceReconciler
Create an adoption reconciler which supports adoption through string names and IDs (not yet ARNs)

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

jaypipes

This is a great start, @RedbackThomson! Thanks!

Some suggestions and comments inline for you to ponder.

jaypipes · 2021-02-10T16:12:48Z

pkg/runtime/reconciler.go

+	reconciler
+	rmf acktypes.AWSResourceManagerFactory
+	rd  acktypes.AWSResourceDescriptor
+}


Nice. This is exactly the kind of polymorphism I was thinking of when we chatted about the implementation. ++

pkg/runtime/service_controller_test.go

jaypipes · 2021-02-10T16:16:42Z

pkg/types/ack_adoption_reconciler.go

+// ACKAdoptionReconciler
+type ACKAdoptionReconciler interface {
+	ACKReconciler
+}


Is there a need for this interface?

Just futureproofing. But not at this stage, no

OK, let's leave it out for now, then.

pkg/types/ack_reconciler.go

jaypipes · 2021-02-10T16:23:38Z

pkg/types/aws_resource_descriptor.go

 	MarkUnmanaged(AWSResource)
+	// MarkAdopted places descriptors on the custom resource that indicate the
+	// resource was not created from within ACK.
+	MarkAdopted(AWSResource)


This should be fine in the resource descriptor interface. Not sure whether in the future the process of marking adoption will mean setting AWS tags on the resource. If it does, then this may need to move to the AWSResourceManager interface.... we'll just have to wait and see.

jaypipes · 2021-02-10T16:25:10Z

scripts/lib/k8s.sh

+#   if controller_gen_version_equals "v0.4.0"; then
+#       echo "controller-gen is at version 0.4.0"
+#   fi
+k8s_controller_gen_version_equals() {


This is unused I think?

Yeah I was thinking of checking within the build-runtime.sh script, again. Since there is a chance someone is calling the script directly, rather than through any of the scripts in community. But I don't like the code duplication.

scripts/build-runtime.sh

templates/boilerplate.txt

scripts/build-runtime.sh

pkg/types/ack_reconciler.go

jaypipes · 2021-02-13T01:02:38Z

pkg/types/aws_resource.go

+	// SetObjectMeta sets the ObjectMeta field for the resource
+	SetObjectMeta(meta metav1.ObjectMeta)
+	// SetARN will set the ARN field in the resource's metadata status field
+	SetARN(*ackv1alpha1.AWSResourceName)


What we need is an interface that will help the AWSResourceManager determine which field in the acktypes.AWSResource's underlyihng Spec or Status struct maps to each field in a ackcorev1alpha1.AWSIdentifiers struct. We already know that the ackcorev1alpha1.AWSIdentifiers.ARN field maps to the Status.ACKMetadata.ARN field because we standardize on that. What we don't really know is which field in the Spec corresponds to the ackcorev1alpha1.AWSIdentifiers.Name and ackcorev1alpha1.AWSIdentifiers.ID field.

Something like this might work?

// AWSIdentifierFieldMapper helps a resource manager determine which fields // in a resource represent name or generated IDs. type AWSIdentifierFieldMapper interface { // NameField returns the path to the field in the underlying resource's `Spec` // or `Status` struct that represents the resource's name. This will be empty // if there is no name field for the resource. NameField() string // IDField returns the path to the field in the underlying resource's `Spec` // or `Status` struct that represents the resource's AWS-generated ID field. // This will be empty if there is no such field for the resource. IDField() string }

We could then have the AWSResourceDescriptor interface have a method that returns a AWSIdentifierFieldMapper-implementing struct:

type AWSResourceDescriptor interface { // ... // IdentifierFieldMapper returns an `AWSIdentifierFieldMapper` that instructs callers // which fields in the `AWSResource` represent name or generated IDs. IdentifierFieldMapper() AWSIdentifierFieldMapper }

Furthemore, instead of a SetNameField and SetARN method (and I presume you would eventually want a SetID method?) on the AWSResource interface above, how about just adding a SetIdentifiers method that accepts a single parameter of type ackcorev1alpha1.AWSIdentifiers? That way, the AdoptedResourceReconciler can construct a new AWSResource and set its identifier fields using a single method.

So, instead of this code that you have above in the AdoptedResourceReconciler:

// Set spec fields prior to reading if desired.Spec.AWS.Name != nil { readableResource.SetNameField(*desired.Spec.AWS.Name) } else if desired.Spec.AWS.ID != nil { readableResource.SetNameField(*desired.Spec.AWS.ID) } else if desired.Spec.AWS.ARN != nil { readableResource.SetARN(desired.Spec.AWS.ARN) } else { return fmt.Errorf("must provide at least one value for identifier") }

you could just do this instead:

readableResource.SetIdentifiers(*desired.Spec.AWS)

and let the plumbing inside the controller's pkg/resource/$RESOURCE/descriptor.go and pkg/resource/$RESOURCE/resource.go files use the AWSIdentiferFieldMapper to determine the right fields in the underlying resource to pull out.

jaypipes · 2021-03-09T12:55:47Z

apis/core/v1alpha1/annotations.go

+	// the resource into its management). If this annotation is set to false on
+	// a CR, that means the user expects the ACK service controller to create
+	// the backend AWS service API resource.
+	AnnotationAdopted = AnnotationPrefix + "adopted"


Been thinking more about this annotation and whether we really need it. I'm not so sure we do, at least in a boolean form. A couple questions that highlight my concerns:

If this annotation is not present on a CR, what does that mean?

Above, it states "If this annotation is set to false on a CR, that means the user expects the ACK service controller to create the backend AWS service API resource." But this isn't really true, right? It's not the annotation's presence or lack of presence that dictates whether the service controller takes some action. Instead, it's the creation of an AdoptedResource CR explicitly by the Kubernetes user that is the signal for a service controller to grab the latest observed state of the referred-to resource instead of calling the API's Create operation.

Given 2), would it make more sense to add an annotation called services.k8s.aws/adopted-on that is a timestamp of when the AdoptedResourceReconciler successfully created the resource CR after pulling the latest observed state from the AWS API?

Sure, I could see more value in this becoming a timestamp. It was initially a boolean so that you could easily run a filter of services.k8s.aws/adopted=='true' on all resource annotations, but I understand that timestamp probably provide that plus more.
This annotation isn't being used by any system, it really is just informational to the user.

jaypipes · 2021-03-09T12:55:56Z

apis/core/v1alpha1/conditions.go

 const (
+	// ConditionTypeAdopted indicates that the adopted resource custom resource
+	// has been successfully reconciled and the target has been created
+	ConditionTypeAdopted ConditionType = "ACK.Adopted"


Is this condition to be applied to the AdoptedResource CR or to the target resource CR? If the latter, then perhaps we don't need a services.k8s.aws/adopted-on annotation at all, since the Condition has a timestamp assocated with its creation.

This condition is currently being applied to the AdoptedResource CR to describe to the user that it has essentially reached the terminal condition.

Rename ACKReconciler to AWSResourceReconciler

jaypipes

I'm eager to see e2e tests for this functionality. :)

jaypipes · 2021-04-19T23:47:27Z

pkg/runtime/service_controller.go

+// GetResourceManagerFactories returns the map of resource manager
+// factories, keyed by the GroupKind of the resource managed by the resource
+// manager produced by that factory
+func (c *serviceController) GetResourceManagerFactories() map[string]acktypes.AWSResourceManagerFactory {


a-hilaly

Neat !! Very nice to see the adoption reconciler in action! 👍
I left few non-blocking comments/questions

a-hilaly · 2021-04-20T12:17:50Z

pkg/runtime/adoption_reconciler.go

+	r.patchAdoptedCondition(ctx, res, err)
+	return err


Error return value is not check here, did you mean return r.patchAdoptedCondition(ctx, res, err) ?

This is correct. This is meant to wrap an error, ensuring that it is implanted into the adopted resource before we raise it. It's a wrapper around return err in other methods.

a-hilaly · 2021-04-20T12:19:09Z

pkg/runtime/service_controller.go

+	rec := NewAdoptionReconciler(c, c.log, cfg, c.metrics)
+	if err := rec.BindControllerManager(mgr); err != nil {
+		return err
+	}
+	c.adoptionReconciler = rec


nit/QQ: Would it be valuable to add a flag to enable/disable the adoption reconciler? i'm thinking about --enable-adoption-reconciler and default it to true

Since resources can be opted out of adoption (via the generator.yaml), I'm OK with adding a CLI flag after this first patch is merged. We can even consider a CRD that allows the service controller to be configured without restarting the service controller -- and that CRD could allow configuring things like the resources that should be adoptable.

a-hilaly · 2021-04-20T12:21:09Z

pkg/runtime/adoption_reconciler.go

+
+// handleReconcileError will handle errors from reconcile handlers, which
+// respects runtime errors.
+func (r *adoptionReconciler) handleReconcileError(err error) (ctrlrt.Result, error) {


nit: similar to resourceReconciler.handleReconcileError - in the future we can merge them

a-hilaly · 2021-04-20T12:23:46Z

apis/core/v1alpha1/types.go

+type AdoptionStatus string
+
+const (
+	AdoptionStatus_Adopted AdoptionStatus = "Adopted"
+	AdoptionStatus_Error   AdoptionStatus = "Error"
+)


QQ: i can't find where these constants are used, are they?

Good point. I don't believe this is used anywhere and probably should be removed.

a-hilaly · 2021-04-20T12:24:49Z

pkg/runtime/adoption_reconciler.go

+	ackv1alpha1 "github.com/aws-controllers-k8s/runtime/apis/core/v1alpha1"
+	ackcfg "github.com/aws-controllers-k8s/runtime/pkg/config"
+	ackerr "github.com/aws-controllers-k8s/runtime/pkg/errors"
+	ackmetrics "github.com/aws-controllers-k8s/runtime/pkg/metrics"
+	"github.com/aws-controllers-k8s/runtime/pkg/requeue"
+	ackrtcache "github.com/aws-controllers-k8s/runtime/pkg/runtime/cache"
+	acktypes "github.com/aws-controllers-k8s/runtime/pkg/types"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	k8sctrlutil "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"


tiny nit: running goimports can help organizing imports

a-hilaly · 2021-04-20T12:48:55Z

pkg/runtime/adoption_reconciler.go

+// we look for the namespace associated region, if that is set we use it. Finally
+// if none of these annotations are set we use the use the region specified in the
+// configuration is used
+func (r *adoptionReconciler) getRegion(


same thing as for handleReconcileError

jaypipes

@RedbackThomson just remove the unused string constants for adopted status and I'll merge this PR.

jaypipes · 2021-04-20T15:24:04Z

apis/core/v1alpha1/types.go

+type AdoptionStatus string
+
+const (
+	AdoptionStatus_Adopted AdoptionStatus = "Adopted"
+	AdoptionStatus_Error   AdoptionStatus = "Error"
+)


Good point. I don't believe this is used anywhere and probably should be removed.

jaypipes · 2021-04-20T15:27:03Z

pkg/runtime/service_controller.go

+	rec := NewAdoptionReconciler(c, c.log, cfg, c.metrics)
+	if err := rec.BindControllerManager(mgr); err != nil {
+		return err
+	}
+	c.adoptionReconciler = rec


Since resources can be opted out of adoption (via the generator.yaml), I'm OK with adding a CLI flag after this first patch is merged. We can even consider a CRD that allows the service controller to be configured without restarting the service controller -- and that CRD could allow configuring things like the resources that should be adoptable.

jaypipes

muy bueno :)

jaypipes requested changes Feb 10, 2021

View reviewed changes

jaypipes reviewed Feb 11, 2021

View reviewed changes

pkg/types/ack_reconciler.go Show resolved Hide resolved

RedbackThomson requested a review from jaypipes February 11, 2021 21:23

jaypipes requested changes Feb 13, 2021

View reviewed changes

RedbackThomson requested a review from jaypipes February 24, 2021 19:45

jaypipes reviewed Mar 9, 2021

View reviewed changes

akartsky approved these changes Apr 16, 2021

View reviewed changes

RedbackThomson force-pushed the resource-adoption branch from 88e6c2f to ba8304d Compare April 19, 2021 19:14

Create adoption reconciler

67f49bb

Rename ACKReconciler to AWSResourceReconciler

RedbackThomson force-pushed the resource-adoption branch from ba8304d to 67f49bb Compare April 19, 2021 21:29

jaypipes approved these changes Apr 19, 2021

View reviewed changes

a-hilaly reviewed Apr 20, 2021

View reviewed changes

jaypipes requested changes Apr 20, 2021

View reviewed changes

Remove unused adoption strings

ebb0160

jaypipes approved these changes Apr 20, 2021

View reviewed changes

jaypipes merged commit 8cff8f7 into aws-controllers-k8s:main Apr 20, 2021

RedbackThomson deleted the resource-adoption branch April 20, 2021 17:26

Create adoption reconciler #2

Create adoption reconciler #2

Uh oh!

Conversation

RedbackThomson commented Feb 10, 2021

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

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!

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!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

RedbackThomson Apr 20, 2021 • 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!

a-hilaly Apr 20, 2021 • 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!

jaypipes 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!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

RedbackThomson Apr 20, 2021 •

edited

Loading

a-hilaly Apr 20, 2021 •

edited

Loading