✨Add support to MachinePool to reference template objects #4112

felipeweb · 2021-01-23T21:22:02Z

What this PR does / why we need it:
MachinePool must support to reference template objects like MachineDeployment supports

Which issue(s) this PR fixes
kubernetes-sigs/cluster-api-provider-aws/issues/2224

Signed-off-by: Felipe Oliveira <fpo@felipeweb.dev>

k8s-ci-robot · 2021-01-23T21:22:09Z

Welcome @felipeweb!

It looks like this is your first PR to kubernetes-sigs/cluster-api 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/cluster-api has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2021-01-23T21:22:10Z

Hi @felipeweb. 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/test-infra repository.

k8s-ci-robot · 2021-01-23T21:22:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vincepri after the PR has been reviewed.
You can assign the PR to them by writing /assign @vincepri in a comment when ready.

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

fabriziopandini

/ok-to-test

fabriziopandini · 2021-01-25T09:57:36Z

exp/controllers/machinepool_controller_phases.go

+		obj, err = external.Get(ctx, r.Client, ref, m.Namespace)
+		if err != nil {
+			if apierrors.IsNotFound(errors.Cause(err)) {
+				return external.ReconcileOutput{}, errors.Wrapf(err, "could not find %v %q for MachinePool %q in namespace %q, requeuing",


Should this be could not find the MachinePool template?
Also, for my understanding, what kind of error are we trying to catch by getting the cloned template after it is cloned? If the clone operation passed, the template should be there...

@fabriziopandini I may be wrong because I don't have a deep knowledge of the codebase of the cluster-api, but by my understanding the object to be reconciled must be the concrete object and not the template so I do Get after the clone and if I can't get the concrete object I can't continue the reconciliation

CecileRobertMichon · 2021-01-28T23:33:56Z

Is this PR still needed? I see kubernetes-sigs/cluster-api-provider-aws#2226 was opened to fix the same problem

felipeweb · 2021-01-29T01:19:36Z

Yes, it is not the same @CecileRobertMichon . This PR adds funcionality to refer template objects (kubeadmConfigTemplate and EKSConfigTemplate) in MachinePool and other PR adds support to add an AWS Launch template in MachinePool object

CecileRobertMichon · 2021-01-29T01:22:19Z

/assign @devigned

devigned

/lgtm

This was something I had wanted to fix, but had forgotten about. Thank you!!

felipeweb · 2021-02-01T17:31:47Z

/assign @vincepri

vincepri · 2021-02-03T18:01:47Z

exp/controllers/machinepool_controller_phases.go

+		owner := &metav1.OwnerReference{
+			APIVersion: expv1.GroupVersion.String(),
+			Kind:       "MachinePool",
+			Name:       m.Name,
+			UID:        m.UID,
+		}


Isn't the owner reference already set below?

I don't t think so

Current line 118, there is a call that sets it controllerutil.SetControllerReference(m, obj, r.Client.Scheme())

I use this owner in line 123 passing owner as input in ColneTemplate func so i can't remove it.

vincepri · 2021-02-03T18:05:58Z

exp/controllers/machinepool_controller_phases.go

+		ref, err = external.CloneTemplate(ctx, &external.CloneTemplateInput{
+			Client:      r.Client,
+			TemplateRef: ref,
+			Namespace:   m.Namespace,
+			ClusterName: cluster.Name,
+			OwnerRef:    owner,
+		})


/hold

Wouldn't this code clone the reference every time it reconciles?

cc @CecileRobertMichon @fabriziopandini

In my point of view no because reference is replaced by concrete object reference, so in next reconciliation the code will do not pass inside if block

The ref, err = external.CloneTemplate assignment would only replace the reference for the current function, am I missing something?

line 128 is getting the ref object and this object returned by function

To clarify my earlier statement, the MachinePools and MachineDeployments incongruity with KubeadmConfig and KubeadmConfigTemplate respectively caused me some confusion when I first started writing cluster yamls for the AzureMachinePool implementation.

When I initially authored the cluster yamls for AzureMachinePool, I started with a working set of resources from the CAPZ default cluster-template.yaml. I started replacing the MachineDeployment with a MachinePool and kept the KubeadmConfigTemplate. After writing a bit of code, I started testing and found that CAPI MachinePool controller didn't pick up on the KubeadmConfigTemplate, but rather, was looking for a KubeadmConfig. It took a bit of debugging and getting into the internals of the MachinePool controller (I was really new to the project at the time and still trying to wrap my head around the project.)

However much confusion I had previously, I feel better about about the distinction today since, as @CecileRobertMichon pointed out, MachinePools reference 1 bootstrap config, not many instances of a config template. That distinction may not make sense to a newcomer to the project. They may try to write MachinePools in a similar manor as a MachineDeployment without regard to the subtle distinction between them.

I don't feel strongly one way or another. Just thought I'd share my early experiences in case it can provide some color.

What @devigned described was exactly what I went through as well. I don't have a use case for this yet, only those pains that @devigned pointed out.

Perhaps we could put in the developer documentation how and when to use template objects. This is a little confusing for those who are starting to understand the project and looking at MachineDeployment and MachinePool

Got it. In that case, how can we make this subtle distinction very explicit so it's not so confusing for newcomers? I don't think allowing templates and making MachinePool more like MachineDeployments is the answer here, but we should make it clear why and how they are different.

Maybe pictures would tell the story our words have failed to tell.

Documentation showing a visual representation comparing a MachineDeployment and MachinePool would be super helpful. Perhaps, a multi-step visualization starting from initial apply and ending with deployment completed with multiple machines that describes the anatomy, relationships and choreography of the resources the user specified and the ones created by the controller. This documentation could be less developer focused and more user focused; less state diagrams and more context / intent.

@felipeweb do you think that would have helped you?

@devigned sure!

fabriziopandini · 2021-03-05T19:35:57Z

/milestone v0.4.0

CecileRobertMichon · 2021-03-10T23:29:41Z

/close

opened an issue to track docs improvements #4288

k8s-ci-robot · 2021-03-10T23:29:49Z

@CecileRobertMichon: Closed this PR.

In response to this:

/close

opened an issue to track docs improvements #4288

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.

felipeweb added 2 commits January 23, 2021 18:09

fix duplicate import path

9c2a034

Signed-off-by: Felipe Oliveira <fpo@felipeweb.dev>

add support to reference template objects in machinepool

1f87ca9

Signed-off-by: Felipe Oliveira <fpo@felipeweb.dev>

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 23, 2021

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 Jan 23, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 23, 2021

k8s-ci-robot requested review from JoelSpeed and vincepri January 23, 2021 21:22

fabriziopandini reviewed Jan 25, 2021

View reviewed changes

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 Jan 25, 2021

k8s-ci-robot assigned devigned Jan 29, 2021

devigned approved these changes Jan 29, 2021

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 29, 2021

k8s-ci-robot assigned vincepri Feb 1, 2021

vincepri requested changes Feb 3, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2021

k8s-ci-robot added this to the v0.4.0 milestone Mar 5, 2021

CecileRobertMichon mentioned this pull request Mar 10, 2021

Add a side by side comparison of MachinePools and MachineDeployments in book #4288

Closed

k8s-ci-robot closed this Mar 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨Add support to MachinePool to reference template objects #4112

✨Add support to MachinePool to reference template objects #4112

felipeweb commented Jan 23, 2021

k8s-ci-robot commented Jan 23, 2021

k8s-ci-robot commented Jan 23, 2021

k8s-ci-robot commented Jan 23, 2021

fabriziopandini left a comment

fabriziopandini Jan 25, 2021

felipeweb Jan 25, 2021 •

edited

Loading

CecileRobertMichon commented Jan 28, 2021

felipeweb commented Jan 29, 2021 via email •

edited

Loading

CecileRobertMichon commented Jan 29, 2021

devigned left a comment

felipeweb commented Feb 1, 2021

vincepri Feb 3, 2021

felipeweb Feb 3, 2021

vincepri Feb 3, 2021

felipeweb Feb 3, 2021

vincepri Feb 3, 2021

vincepri Feb 3, 2021

felipeweb Feb 3, 2021

vincepri Feb 3, 2021

felipeweb Feb 3, 2021

devigned Feb 10, 2021

felipeweb Feb 10, 2021 •

edited

Loading

CecileRobertMichon Feb 10, 2021

devigned Feb 13, 2021

felipeweb Feb 13, 2021

fabriziopandini commented Mar 5, 2021

CecileRobertMichon commented Mar 10, 2021

k8s-ci-robot commented Mar 10, 2021

✨Add support to MachinePool to reference template objects #4112

✨Add support to MachinePool to reference template objects #4112

Conversation

felipeweb commented Jan 23, 2021

k8s-ci-robot commented Jan 23, 2021

k8s-ci-robot commented Jan 23, 2021

k8s-ci-robot commented Jan 23, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felipeweb Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

CecileRobertMichon commented Jan 28, 2021

felipeweb commented Jan 29, 2021 via email • edited Loading

CecileRobertMichon commented Jan 29, 2021

devigned left a comment

Choose a reason for hiding this comment

felipeweb commented Feb 1, 2021

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

Choose a reason for hiding this comment

felipeweb Feb 10, 2021 • 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

fabriziopandini commented Mar 5, 2021

CecileRobertMichon commented Mar 10, 2021

k8s-ci-robot commented Mar 10, 2021

felipeweb Jan 25, 2021 •

edited

Loading

felipeweb commented Jan 29, 2021 via email •

edited

Loading

felipeweb Feb 10, 2021 •

edited

Loading