fix: (azure)machinepools: stop continuous reconciliation of resources by sorting ProviderIDs

[damdo]

What type of PR is this?

/kind bug

What this PR does / why we need it:

Fixes continuous reconciliation of AzureMachinePool and MachinePool resources that causes the E2E clusterctl upgrade test to fail with: resourceVersions didn't stay stable

The ProviderIDList field in AzureMachinePool.Spec was being populated from results of client.List() calls, which do not guarantee a deterministic order. When the same set of machines is returned in a different order across reconciliations, the ProviderIDList slice changes, causing a spec update and bumping the resourceVersion—even though the underlying set of provider IDs is identical.

Why did this start failing now ?
This issue only started showing up after the CAPI v1.11 bump.
This is because CAPI v1.11 introduced a new validation step in the clusterctl upgrade E2E test framework via kubernetes-sigs/cluster-api#12546 that checks resourceVersions remain stable after upgrade completion:

"If this is the last step of the upgrade sequence check that the resourceVersions are stable, i.e. it verifies there are no continuous reconciles when everything should be stable."

The underlying bug has likely always existed, but was never detected before because we didn't have this stability check in the upgrade test.

How does this PR solve it?
Sorts providerIDs slices before assigning them to ProviderIDList fields, ensuring deterministic ordering regardless of the order returned by client.List() or Azure API calls.

This PR stacks on top of #6010 so:

/hold

until #6010 is merged

Release note:

fix: (azure)machinepools: stop continuous reconciliation of resources by sorting ProviderIDs

[damdo]

Depends on #6010

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 44.41%. Comparing base (01f24d8) to head (39b272d).
⚠️ Report is 6 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6011   +/-   ##
=======================================
  Coverage   44.41%   44.41%           
=======================================
  Files         280      280           
  Lines       25377    25379    +2     
=======================================
+ Hits        11271    11273    +2     
  Misses      13293    13293           
  Partials      813      813           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[damdo]

/assign @mboersma @nojnhuh @jackfrancis @willie-yao @bryan-cox

[nojnhuh]

I'd like to see this test pass a few times in a row to make sure we're confident this is fixing what we think it is.

/test pull-cluster-api-provider-azure-apiversion-upgrade

[jackfrancis]

@damdo anything interesting from the last test failure

(thank you so much for looking into this! 🍻)

[mboersma]

/retest

[mboersma]

/test pull-cluster-api-provider-azure-apiversion-upgrade

1 passed.

[damdo]

/test pull-cluster-api-provider-azure-apiversion-upgrade

[damdo]

/test pull-cluster-api-provider-azure-apiversion-upgrade

[nojnhuh]

It looks like this test should be working better now.

/test pull-cluster-api-provider-azure-apiversion-upgrade

[nojnhuh]

/test pull-cluster-api-provider-azure-apiversion-upgrade

[damdo]

Timedout for unrelated reasons

/test pull-cluster-api-provider-azure-apiversion-upgrade

[damdo]

Timedout for unrelated reasons

/test pull-cluster-api-provider-azure-apiversion-upgrade

[nojnhuh]

I think it's safe to say this isn't the cause of the upgrade test flakes since the AzureMachinePools created in those tests only have one replica (so one provider ID).

The more likely cause is from CAPZ updating the cloud-init data for the VMSS when CAPI periodically updates the KubeadmConfig's bootstrap data and storing the hash in an annotation on the AzureMachinePool. If that update every 10 minutes happens within the 2-minute window that the framework expects resourceVersions to remain stable, then the test fails.

I think this change is still worth it, but we shouldn't expect it to fix the upgrade test.

[jackfrancis]

I think it's safe to say this isn't the cause of the upgrade test flakes since the AzureMachinePools created in those tests only have one replica (so one provider ID).

The more likely cause is from CAPZ updating the cloud-init data for the VMSS when CAPI periodically updates the KubeadmConfig's bootstrap data and storing the hash in an annotation on the AzureMachinePool. If that update every 10 minutes happens within the 2-minute window that the framework expects resourceVersions to remain stable, then the test fails.

I think this change is still worth it, but we shouldn't expect it to fix the upgrade test.

For the purposes of this test should we manually update the bootstrap data refresh time to be greater than the P50 total time duration of the test run (plus some extra overhead) so that we can get the test outcome that this test is actually looking for?

[nojnhuh]

For the purposes of this test should we manually update the bootstrap data refresh time to be greater than the P50 total time duration of the test run (plus some extra overhead) so that we can get the test outcome that this test is actually looking for?

SGTM

[nojnhuh]

For the purposes of this test should we manually update the bootstrap data refresh time to be greater than the P50 total time duration of the test run (plus some extra overhead) so that we can get the test outcome that this test is actually looking for?

SGTM

Testing this out in #6031

[damdo]

Thanks @nojnhuh @jackfrancis

Would it make sense to base #6031 on top of this PR to make sure we cover both issues?
So that if we do that and see 6031 consistently go green we can merge both, TY.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jackfrancis. For more information see the Code Review Process.

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

Details

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

[damdo]

@mboersma @nojnhuh @jackfrancis even though the main cause of the upgrade issue has been identified and fixed, I think this could still be a good QoL improvement.
Are we up for merging it? TY

[nojnhuh]

Change looks good overall. Can we update the unit tests?

[damdo]

@nojnhuh Done thanks!

[damdo]

/test pull-cluster-api-provider-azure-apiversion-upgrade

[k8s-ci-robot]

@damdo: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cluster-api-provider-azure-apiversion-upgrade	39b272d	link	true	/test pull-cluster-api-provider-azure-apiversion-upgrade

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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-sigs/prow repository. I understand the commands that are listed here.