Updating Azure Plugin to Support Flexible VMSS

[benjamin-lykins]

Fixes: #1096

• Reviewed contributor guide.
  I will note, this might not be a small first PR to put in. Ended up being a little bit more noisy since I did migrate off the legacy Azure SDK.

Changes

1. Updated from deprecated to current Azure Compute SDK.

"github.com/Azure/azure-sdk-for-go/services/compute/mgmt/2020-06-01/compute"
"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute"

2. Updated status checks

Supports both flexible and uniform scale sets. Queries the VMSS to get the type and then conditionally runs the respective process status/scale in and out processes.

• Flexible scale sets are a shift from uniform and were unable to query the instance view of the instance. Essentially, grab the list of instances from the VMSS, then use that against the VM (not VMSS) API to get instance status.

• When getting instance states, since Flexible VMSS can have up to 2000 instances, I used a goroutine to iterate through them more effectively. Hardcoded at 5 concurrent, might be worth allowing the end user to provide a limit on this, but from my testing with 5 concurrent against 100 instances, it took ~1 second. When I was sequentially pulling instance state, it took ~30 seconds for the same count. I did test limiting to 10 concurrent, but it took it from ~1 second to a little less than a second, so not much gained.

3. The only existing function specifically with Uniform I updated was processInstanceView.

I can double check the why, but I believe one of the attributes was not available in the current api.

4. Testing

Admittedly Test_processInstanceView used copilot to refactor for the new api.

Testing

Flexible VMSS

• Scale In
• Scale Out

Uniform VMSS

• Scale In
• Scale Out

[tgross]

This is great work @benjamin-lykins. I've left a bunch of comments -- some on small details and some on design. For the smaller comments like contents of debug logging, try to take a pass over the whole PR with those in mind. (Edit: it looks like GitHub is unhelpfully doing the "hidden conversations" thing so make sure you expand that as that includes some of the major design items)

We've added a lot of new code here and it doesn't seem like there's a lot of test coverage. Unfortunately we don't have integration tests so that's challenging, but maybe as you break up some of those large methods I pointed out there'd be a way to extract some of the logic into testable chunks.

[tgross]

Making good progress!

[tgross]

This log line seems wrong... we're not skipping the instance here, we're iterating to the next InstanceView.Status and will print this log line for each status we find.

[benjamin-lykins]

Ran through this a bit more in my head. This was carry over from what it was previously. I had tried to simplify this since it had multiple nested if statement. I ended up reverting back to what Luiz had previously.

if vm.Properties != nil && vm.Properties.InstanceView != nil && vm.Properties.InstanceView.Statuses != nil {
		for _, s := range vm.Properties.InstanceView.Statuses {
			if s.Code != nil && strings.HasPrefix(*s.Code, "PowerState/") {
				if *s.Code == "PowerState/running" {
					t.logger.Debug("found healthy instance", "name", *vm.Name, "instance_id", *vm.InstanceID)
					vmNames = append(vmNames, *vm.Name)
					break
				} else {
					t.logger.Debug("skipping instance - power state is not running", "name", *vm.Name, "instance_id", *vm.InstanceID, "code", *s.Code)
				}
			}
		}

[tgross]

I think this still has the same problem. Suppose I have the following set of InstanceViewStatus:

 []InstanceViewStatus{
	{ Code: pointer.Of("ProvisioningState/succeeded"), ...}
	{ Code: pointer.Of("Whatever/foo"), ...}
	{ Code: pointer.Of("PowerState/running"), ...}
}

In this case, we'll log that we're skipping the instance twice and then not skip it, because we eventually find a "PowerState/running" code. The number of times we log ends up being dependent on the order that the statuses get returned (which I can't find anywhere in the Azure docs). So we only want to log if we never find that "PowerState/running" code. Similar to how you've got isFlexibleVMReady working.

Or we can drop this else block entirely, which I think would also be fine.

[benjamin-lykins]

@tgross Replied back to your latest reviews and put in a series of commits to address the recommendations.

I'm working to clean up the functions which get both status and IDs for Flexible VMSS. I may try to consolidate those two functions, or generalize some of it that I can.

[benjamin-lykins]

Azure API rate limiting no longer an issue, now bumping into Nomad rate limiting issue. The other plugins have a retry function which I'll look to incorporate. Probably not in this PR, but in a future I will add better handling if the node drain occurs on some nodes, but not all to re-enable them for allocations.

[tgross]

now bumping into Nomad rate limiting issue.

Once #1134 has been merged, this should reduce the impact of this and make it easier to wrap this up.

[benjamin-lykins]

now bumping into Nomad rate limiting issue.

Once #1134 has been merged, this should reduce the impact of this and make it easier to wrap this up.

Ahh great! I'll hold off, since I had been trying to work at what was available to handle that. Thanks for the heads up @tgross.

[tgross]

@benjamin-lykins this is really getting there. I've left a few small comments but nothing that's a major design issue at this point. We should also add a changelog entry to the Unreleased section, so that when we release this we have it noted.

[tgross]

I think this still has the same problem. Suppose I have the following set of InstanceViewStatus:

 []InstanceViewStatus{
	{ Code: pointer.Of("ProvisioningState/succeeded"), ...}
	{ Code: pointer.Of("Whatever/foo"), ...}
	{ Code: pointer.Of("PowerState/running"), ...}
}

In this case, we'll log that we're skipping the instance twice and then not skip it, because we eventually find a "PowerState/running" code. The number of times we log ends up being dependent on the order that the statuses get returned (which I can't find anywhere in the Azure docs). So we only want to log if we never find that "PowerState/running" code. Similar to how you've got isFlexibleVMReady working.

Or we can drop this else block entirely, which I think would also be fine.

[benjamin-lykins]

@tgross made a series of changes off of your last set of reviews and ready for rereview.

[benjamin-lykins]

@tgross I'm going to rework this section as well. It has always pulled what the total number of instances can be in either an orchestrated or flexible vmss. I think it would be better to grab the actual count.

[benjamin-lykins]

Updated and tested with the following successfully:

	// Recommended removing this since it only gets the requested capacity and not the actual count.
	// capacity := *currVMSS.SKU.Capacity

	vms, err := t.getVMSSVMs(ctx, resourceGroup, orchestrationMode, vmScaleSet)

	if err != nil {
		return fmt.Errorf("failed to get VMSS VMs: %w", err)
	}

	capacity := int64(len(vms))

I'll find some time to run this against a uniform set and confirm scaling actions perform without issue as well for all the changes. Last time I did, there were no issues, but wanting to double check since it has been a bit.

[tgross]

One last item and I think that'll do it, @benjamin-lykins!

[tgross]

LGTM! Nice work!