Populate ControlPlaneEndpoint when ManagedCluster update is not needed. #2134

karthikbalasub · 2022-02-28T21:57:32Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
Fixes a bug in managed clusters service that results in control plane endpoint not getting updated correctly.
Before this fix, update flow will not read the FQDN from the the ManagedCluster fetched from cloud when no update was needed on the managed cluster.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

TODOs:

squashed commits
includes documentation
adds unit tests

Release note:

Fixed a bug in managed clusters service that results in control plane endpoint not getting updated correctly.

linux-foundation-easycla · 2022-02-28T21:57:35Z

The committers are authorized under a signed CLA.

✅ Karthik Balasubramanian (1a7a4c7)

k8s-ci-robot · 2022-02-28T21:57:39Z

Welcome @karthikbalasub!

It looks like this is your first PR to kubernetes-sigs/cluster-api-provider-azure 🎉. 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-provider-azure 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 · 2022-02-28T21:57:41Z

Hi @karthikbalasub. 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.

shysank · 2022-02-28T22:01:26Z

/ok-to-test

jackfrancis · 2022-02-28T22:17:09Z

azure/services/managedclusters/managedclusters.go

+			// No update required, but use the MC fetched from Azure for reading fields below.
+			// This is to ensure the read-only fields like Fqdn from the existing MC are used for updating the
+			// AzureManagedCluster.
+			managedCluster = existingMC


Is this something that we only need to do some of the time in practice? If not, how did this updates ever work at all?

Also, would it just be simpler to modify L335 below to use:

Host: *existingMC.ManagedClusterProperties.Fqdn,

Is this something that we only need to do some of the time in practice? If not, how did this updates ever work at all?

The issue happens when the create response doesn't include the FQDN (can happen due to timeout, for example). When that happens, subsequent update calls will fail to update the controlplane endpoint.

Also, would it just be simpler to modify L335 below to use:

Host: *existingMC.ManagedClusterProperties.Fqdn,

This would be incorrect when there was an update needed (diff != "") as the response from the update call is assigned to managedCluster in this line above:

managedCluster, err = s.Client.CreateOrUpdate(ctx, managedClusterSpec.ResourceGroupName, managedClusterSpec.Name, managedCluster, customHeaders)

But the FQDN will never change, right? So even if there is a diff to process we can always conclude that the FQDN is not one of the updated properties.

existingMC would also be null for the create flow.

@CecileRobertMichon @zmalik @michalno1 does anyone else have thoughts here? I think @karthikbalasub has done a great job of describing in detail what's going on.

so for other services we've refactored the reconcile logic to use a common CreateResource func which returns either 1) the existing resource if no update was made or 2) the resulting resource returned from the CreateOrUpdate code. Then each service can do what it needs with resulting resource (eg. update Status)

Looks like we're trying to achieve the same here and that's fine, but I think the code is slightly confusing just because it's not really clear what managedCluster is supposed to be since we use the same variable to send the PUT and to receive the return. What do you think of adding a new variable called result instead or maybe reusing the existingMC variable to assign the return of CreateOrUpdate functions (then we don't even need this else call)?
This would achieve the same thing, just read a bit differently.

I'll look into refactoring managed clusters to async soon.

@CecileRobertMichon I refactored the code based on your suggestion. PTAL, thanks!

lgtm

@jackfrancis thoughts?

much cleaner, thanks @karthikbalasub !

CecileRobertMichon · 2022-02-28T22:27:36Z

@karthikbalasub looks like there's a merge conflict, please rebase

karthikbalasub · 2022-03-01T00:20:09Z

@karthikbalasub looks like there's a merge conflict, please rebase

Done

CecileRobertMichon · 2022-03-02T22:37:48Z

/area managedclusters

CecileRobertMichon · 2022-03-07T18:05:01Z

@karthikbalasub could you please squash commits?

…edControlPlane correctly. Before this fix, the issue happens when the create request failed/timed out and later update flow was not reading from the cluster fetched from cloud.

karthikbalasub · 2022-03-07T18:28:27Z

@karthikbalasub could you please squash commits?

Done

jackfrancis · 2022-03-07T20:44:42Z

/retest

CecileRobertMichon · 2022-03-08T17:48:53Z

/lgtm
/approve

k8s-ci-robot · 2022-03-08T17:49:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CecileRobertMichon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [CecileRobertMichon]

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

CecileRobertMichon · 2022-03-08T17:49:32Z

/cherry-pick release-1.2

k8s-infra-cherrypick-robot · 2022-03-08T17:49:33Z

@CecileRobertMichon: once the present PR merges, I will cherry-pick it on top of release-1.2 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.2

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.

jackfrancis

/lgtm

k8s-infra-cherrypick-robot · 2022-03-08T19:01:43Z

@CecileRobertMichon: new pull request created: #2153

In response to this:

/cherry-pick release-1.2

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 added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Feb 28, 2022

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Feb 28, 2022

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 Feb 28, 2022

k8s-ci-robot requested review from alexeldeib and shysank February 28, 2022 21:57

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 Feb 28, 2022

jackfrancis reviewed Feb 28, 2022

View reviewed changes

karthikbalasub force-pushed the fix_controlplane_endpoint branch from 1a7a4c7 to a6843ee Compare March 1, 2022 00:19

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 1, 2022

karthikbalasub force-pushed the fix_controlplane_endpoint branch 4 times, most recently from 6c08249 to de30378 Compare March 1, 2022 01:38

k8s-ci-robot added the area/managedclusters Issues related to managed AKS clusters created through the CAPZ ManagedCluster Type label Mar 2, 2022

Fix the issue when ControlPlaneEndpoint was not updated on AzureManag…

679375f

…edControlPlane correctly. Before this fix, the issue happens when the create request failed/timed out and later update flow was not reading from the cluster fetched from cloud.

karthikbalasub force-pushed the fix_controlplane_endpoint branch from 1c3e1a9 to 679375f Compare March 7, 2022 18:28

k8s-ci-robot assigned CecileRobertMichon Mar 8, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2022

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 8, 2022

jackfrancis approved these changes Mar 8, 2022

View reviewed changes

k8s-ci-robot assigned jackfrancis Mar 8, 2022

k8s-ci-robot merged commit f7a2786 into kubernetes-sigs:main Mar 8, 2022

k8s-ci-robot added this to the v1.3 milestone Mar 8, 2022

k8s-infra-cherrypick-robot mentioned this pull request Mar 8, 2022

[release-1.2] Populate ControlPlaneEndpoint when ManagedCluster update is not needed. #2153

Merged

karthikbalasub deleted the fix_controlplane_endpoint branch March 8, 2022 19:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Populate ControlPlaneEndpoint when ManagedCluster update is not needed. #2134

Populate ControlPlaneEndpoint when ManagedCluster update is not needed. #2134

karthikbalasub commented Feb 28, 2022 •

edited by CecileRobertMichon

Loading

linux-foundation-easycla bot commented Feb 28, 2022 •

edited

Loading

k8s-ci-robot commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

shysank commented Feb 28, 2022

jackfrancis Feb 28, 2022

jackfrancis Feb 28, 2022

karthikbalasub Mar 1, 2022 •

edited

Loading

jackfrancis Mar 1, 2022

karthikbalasub Mar 1, 2022

jackfrancis Mar 1, 2022

CecileRobertMichon Mar 2, 2022 •

edited

Loading

karthikbalasub Mar 3, 2022

CecileRobertMichon Mar 4, 2022

jackfrancis Mar 7, 2022

CecileRobertMichon commented Feb 28, 2022

karthikbalasub commented Mar 1, 2022

CecileRobertMichon commented Mar 2, 2022

CecileRobertMichon commented Mar 7, 2022

karthikbalasub commented Mar 7, 2022

jackfrancis commented Mar 7, 2022

CecileRobertMichon commented Mar 8, 2022

k8s-ci-robot commented Mar 8, 2022

CecileRobertMichon commented Mar 8, 2022

k8s-infra-cherrypick-robot commented Mar 8, 2022

jackfrancis left a comment

k8s-infra-cherrypick-robot commented Mar 8, 2022

Populate ControlPlaneEndpoint when ManagedCluster update is not needed. #2134

Populate ControlPlaneEndpoint when ManagedCluster update is not needed. #2134

Conversation

karthikbalasub commented Feb 28, 2022 • edited by CecileRobertMichon Loading

linux-foundation-easycla bot commented Feb 28, 2022 • edited Loading

k8s-ci-robot commented Feb 28, 2022

k8s-ci-robot commented Feb 28, 2022

shysank commented Feb 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karthikbalasub Mar 1, 2022 • 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

CecileRobertMichon Mar 2, 2022 • 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

CecileRobertMichon commented Feb 28, 2022

karthikbalasub commented Mar 1, 2022

CecileRobertMichon commented Mar 2, 2022

CecileRobertMichon commented Mar 7, 2022

karthikbalasub commented Mar 7, 2022

jackfrancis commented Mar 7, 2022

CecileRobertMichon commented Mar 8, 2022

k8s-ci-robot commented Mar 8, 2022

CecileRobertMichon commented Mar 8, 2022

k8s-infra-cherrypick-robot commented Mar 8, 2022

jackfrancis left a comment

Choose a reason for hiding this comment

k8s-infra-cherrypick-robot commented Mar 8, 2022

karthikbalasub commented Feb 28, 2022 •

edited by CecileRobertMichon

Loading

linux-foundation-easycla bot commented Feb 28, 2022 •

edited

Loading

karthikbalasub Mar 1, 2022 •

edited

Loading

CecileRobertMichon Mar 2, 2022 •

edited

Loading