Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AKS - fix editing imported clusters #11120

Merged
merged 3 commits into from
Jun 13, 2024

Conversation

mantis-toboggan-md
Copy link
Member

@mantis-toboggan-md mantis-toboggan-md commented May 24, 2024

Summary

Fixes #10966
Fixes #3872
Fixes #8501

Occurred changes and/or fixed issues

This PR corrects editing of imported clusters by fixing the upstream syncing logic. Hosted clusters (aks eks gke) track both the configuration managed through Rancher and the configuration managed through their respective cloud provider's platform. You can read more about syncing between the two in the Rancher docs here.

Areas or cases that should be tested

When testing this PR, it should be noted that once a cluster has been updated through Rancher, it should only be updated through Rancher.
To test issues with the node pool upgrade checkbox/banner stuff:

  1. In the azure portal, create an AKS cluster that does not use the latest k8s version available.
  2. Import the cluster into Rancher and 'edit config' - the form should be populated and save button not disabled - do not edit the cluster at this stage
  3. Go back to the azure portal and upgrade the k8s version of the cluster control plane only (depending on how you made your cluster you may need to disable auto upgrades to do cp-only upgrading)
  4. Wait ~5 minutes for the upstream spec to sync
  5. Edit the imported cluster through the Rancher UI: there should be an option to upgrade the node pool version
  6. Select a new cluster-level k8s version instead: there should now be a banner telling the user they can upgrade the node pool when the cluster upgrade is done
  7. Add a new node pool: it should have the same k8s version as the cluster k8s version
  8. Save the cluster, let it update, and edit again: there should once again be an option to upgrade the node pool

Other stuff to check:

  • Edit an imported cluster and change nothing: the cluster should not go into updating state
  • The same edit options should be available for imported and rancher-provisioned AKS clusters

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't Import an AKS cluster to test this

  • I created a new AKS cluster via porta.azure.com in a new resource group (think i read somewhere only one cluster is allowed by group)
  • When using cloud cred from a secret from an app in my azure account, the /meta/aksClusters did not return the cluster
  • Question - Did you use the same resource group as a previously created rancher cluster?

I did manage to test create AKS via rancher and edit

  • Changed Description - worked fine
  • Changed Kube version AND added new node. Cluster state did not change for a few minutes before finally going into Updating.
    • http put request contained correct aksConfig.kubernetesVersion and aksConfig.nodePools values
    • putting this down to a backend issue
  • On a broken instance (incorrect resource group) some settings didn't seem to stick
    • Updated cluster resource group and location
    • http put request contained correct aksConfig.resoourceGroup and aksConfig.resourceLocation
    • refreshing the page afterwards showed old values
    • Question - have you seen anything like this? It'll probably be a backend issue again


if (!isEmpty(upstreamConfig)) {
Object.keys(upstreamConfig).forEach((key) => {
if (isEmpty(rancherConfig[key]) && !isEmpty(upstreamConfig.key)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be !isEmpty(upstreamConfig[key])?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call - updated now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i still see this as the old version.

i made the tweak locally and i think it fixed an issue i was seeing (deployed in ukwest, availability zones in edit screen incorrectly shown)

@mantis-toboggan-md
Copy link
Member Author

I couldn't Import an AKS cluster to test this

I created a new AKS cluster via porta.azure.com in a new resource group (think i read somewhere only one cluster is allowed by group)
When using cloud cred from a secret from an app in my azure account, the /meta/aksClusters did not return the cluster
Question - Did you use the same resource group as a previously created rancher cluster?

I haven't hit this issue before myself. I do typically reuse the same resource group, so I made one in a new group and I do see that cluster available to import, as well as one you created. Was the cluster fully provisioned when you tried to import it?

@mantis-toboggan-md
Copy link
Member Author

Changed Kube version AND added new node. Cluster state did not change for a few minutes before finally going into Updating.
http put request contained correct aksConfig.kubernetesVersion and aksConfig.nodePools values
putting this down to a backend issue

I see this as well - it sounds like a backend issue to me. The 5 minute timing to me suggests it may be related to the aks-operator upstream syncing job https://ranchermanager.docs.rancher.com/reference-guides/cluster-configuration/rancher-server-configuration/sync-clusters

On a broken instance (incorrect resource group) some settings didn't seem to stick
Updated cluster resource group and location
http put request contained correct aksConfig.resoourceGroup and aksConfig.resourceLocation
refreshing the page afterwards showed old values
Question - have you seen anything like this? It'll probably be a backend issue again

Showing the old values in the form is more concerning to me (the user could possibly save again and overwrite the changes they just made) but I'm having trouble reproducing. What do you mean by an incorrect resource group?

Copy link
Member

@richard-cox richard-cox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worked out my issue with seeing the existing cluster to import in our UI, had to explicitly give my app contributor permissions to the subscription.

Edit: To confirm the open comment is the only remaining item. I've tested upgrade and it looks good. The other issues ...

  • Changed Kube version AND added new node - slow update of cluster state. i saw this this again, but with right creds cluster reached active in the end
  • Stale values on broken cluster - lets leave this one, if i can find a way to reproduce i'll create a separate issue


if (!isEmpty(upstreamConfig)) {
Object.keys(upstreamConfig).forEach((key) => {
if (isEmpty(rancherConfig[key]) && !isEmpty(upstreamConfig.key)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i still see this as the old version.

i made the tweak locally and i think it fixed an issue i was seeing (deployed in ukwest, availability zones in edit screen incorrectly shown)

@mantis-toboggan-md mantis-toboggan-md merged commit a069ca2 into rancher:master Jun 13, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment