Skip to content

RKE1 Azure node templates have accelerated networking and availability zones#4932

Merged
catherineluse merged 4 commits intorancher:masterfrom
catherineluse:azure-options
Dec 20, 2022
Merged

RKE1 Azure node templates have accelerated networking and availability zones#4932
catherineluse merged 4 commits intorancher:masterfrom
catherineluse:azure-options

Conversation

@catherineluse
Copy link
Contributor

@catherineluse catherineluse commented Dec 19, 2022

Addresses rancher/dashboard#7753 and rancher/dashboard#7752

This PR is functionally the same as rancher/dashboard#7687, but for RKE1 node templates.

New options

  • Can select an availability zone.
  • Can enable accelerated networking.
  • The VM size dropdown is grouped so that you can see which VMs support accelerated networking and which do not.

Refetching

  • VMs are refetched when the region changes because different sizes are available per region.

Error messages

  • Warn the user when the selected VM size is not in the selected region.
  • Warn the user if they have selected availability zones, but that is not supported for the region or VM size.
    • For example, South Africa North has availability zones for some sizes but not others. So Standard_A2_v2 in that region has three AZs and that works, but if you change the size to Standard_A2_v2 you see an error.
  • Warn the user if they wanted availability zones, but availability zones are not supported in the selected region.
  • Warn the user if accelerated networking is enabled but the selected VM size does not support accelerated networking. Show the warning by the vm sizes and by the AN checkbox.

Notes

  • Replaced the hardcoded VM size list with V2 API call to get VM sizes.
  • Replaced the hardcoded region list with the API call to get regions, to show the full list and be consistent with AKS and RKE2/K3s provisioning.
  • I deleted the "environment" choice dropdown because it was last updated five years ago, and while I assume it worked at the time, it doesn't work like that anymore. Now the environment, such as Azure public cloud or the government cloud, is configured in the cloud credentials, and the credentials are used to fetch the regions, so the environment should not be able to be configured in this form. If you want to create nodes in a different environment, you should use a different cloud credential. We may want to call that out in the release notes.
  • Because the form now needs to call the Azure API with the user's credentials, I have changed the form so that you can't see it until valid credentials are selected.
  • I think there is a backend bug where if you first create a node template with an availability zone, then change it to an availability set, the change does not get persisted. Edit: Fixed by clearing out values using empty string instead of null.

@catherineluse catherineluse added this to the 2.7.1 milestone Dec 19, 2022
@catherineluse
Copy link
Contributor Author

I fixed the issue where switching from an AZ to an availability set wasn't being persisted - I don't intend to make any more changes to the PR before it's reviewed

<p>{{vmSizeAvailabilityWarning}}</p>
{{/banner-message}}
{{/if}}
{{#if showVmAvailabilityZoneWarning}}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure it always makes sense to show vmSizeAvailabilityWarning here, since one of the two potential error messages is asking the user to re-configure one of two options in a different section. regionSupportsAzsButNotThisSize certainly makes sense here, but probably not regionDoesNotSupportAzs :

Screen Shot 2022-12-19 at 2 06 00 PM

Copy link
Contributor Author

@catherineluse catherineluse Dec 19, 2022

Choose a reason for hiding this comment

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

removed the vmSizeAvailabilityWarning from below the size select

Copy link
Member

Choose a reason for hiding this comment

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

The one that needs to go is actually vmAvailabilityZoneWarning completely my fault, sorry. Other than that the PR looks good

.concat(vmsWithAn)

if (!get(this, 'selectedVmSizeExistsInSelectedRegion')) {
out.push({
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately I don't see why, but this doesn't seem to work. When I select a VM size then switch to a region in which the size is not available, I see the warning about the size not being supported, but the dropdown value switches to whatever is first in sizeChoices and the invalid size doesn't seem to be in the list (unless I overlooked it, it's a big list)...then when I switch back to the previous region, the dropdown value also switches back to what I had selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored it and it seems fixed:

let out = [{
      kind:     'group',
      name:     noAnLabel,
      value:    noAnLabel,
      disabled: true
    }]
      .concat(vmsWithoutAn)
      .concat({
        kind:     'group',
        name:     withAnLabel,
        value:    withAnLabel,
        disabled: true
      })
      .concat(vmsWithAn)

    out = out.map((vmData) => {
      const { Name } = vmData;

      if (vmData.kind === 'group') {
        return vmData;
      }

      return {
        label:    Name,
        value:    Name,
        disabled: vmData.disabled || false,
      };
    });

    if (!get(this, 'selectedVmSizeExistsInSelectedRegion')) {
      return out.concat({
        label:    get(this, 'config.size'),
        value:    get(this, 'config.size'),
        disabled: true
      });
    }

    return out

Maybe later this week I'll take some time to figure out why the refactor works but not the original.

Copy link
Contributor Author

@catherineluse catherineluse Dec 19, 2022

Choose a reason for hiding this comment

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

I see what happened. About array.push:

It changes the size of the original array and returns a new array (with the new element added) as output.

The way I had it, I conditionally added something to the array, then returned the original array. But when I conditionally added the array item, it created a new array. Whereas the original variable was pointing to the outdated array - the one that didn't have the new item added.

Copy link
Member

Choose a reason for hiding this comment

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

.push appends to the array and returns the new length of the array https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/push

concat otoh does return a new array https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/concat

Maybe we're missing something about how ember computed properties work? It's weird that the contents of the dropdown are updated when a new region is selected, and just that option isn't added.

Copy link
Member

@mantis-toboggan-md mantis-toboggan-md left a comment

Choose a reason for hiding this comment

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

I gave a bad suggestion initially - vmAvailabilityZoneWarning should be removed instead of the size warning. Other than that this all looks good.

@catherineluse
Copy link
Contributor Author

I've updated it to replace vmAvailabilityZoneWarning with showVmSizeAvailabilityWarning

@catherineluse catherineluse merged commit 5dc88c4 into rancher:master Dec 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants