Bug fixes and address integration test flakes by gagan16k · Pull Request #1154 · gardener/machine-controller-manager

gagan16k · 2026-09-16T11:52:50Z

/kind flake

What this PR does / why we need it:
This PR addresses some bugs in MCM surfaced by integration tests on kwok:

AutoPreserveFailedMachineMax reduction tests result in deletion of both machines
- Instances: Run 1, Run 2, Run 3
- Observed Behaviour: When AutoPreserveFailedMachineMax was reduced (2 -> 1), the controller unpreserved both machines instead of one, and both got deleted.
- Root Cause: Two reconciles run: the first un-preserved one machine, updated Status.AutoPreserveFailedMachineCount and wrote it to the API server but it hadn't yet been persisted in the lister cache . The second still read the stale count (2), thought it needed to remove one more, and unpreserved the second machine too.
- Fix: Count the currently auto-preserved machines from their annotations instead of the stale Status.AutoPreserveFailedMachineCount. A second reconcile then sees the reduced count and does nothing. The oldest machine is un-preserved first. Added unit tests in machineset_test.go to cover these cases.

Other changes to address a set of flakes in the existing integration tests.

Reducing MCD replicas to 2 in Manual Preservation tests (from 3 in auto-preservation tests) terminated a machine that got picked up by a test spec.
- Instances: Run
- Fix: Keep the replica count consistent throughout the test body.
BeforeEach node readiness check stalls on a preserved node
- Files changed: nodes.go, kwok-config.yaml
- Root Cause: The BeforeEach check waits until every node is Ready (GetNumberOfReadyNodes==GetNumberOfNodes). Kwok sometimes does not update the leases until a node update event forces it to do so. Additionally the Get->Update way can sometimes not persist due to conflict errors. Nodes recovering from an earlier spec go through the kwok node-recover stage, which kept them NotReady for a long time. The next spec hangs waiting for nodes to be ready.
- Fix: Removed this check as we wait for running machines in every preservation test, and added these checks where necessary in the other tests. Wherever the spec waits for preserved nodes to come back alive, add the kwok "kwok/fail-condition":"Recover","kwok/recovery-attempts":"%d" annotations on every poll interval to trigger an update edge on the node for the kwok controller. Also tighten the kwok node-recover stage (immediateNextStage: true) so recovering nodes come back faster. Renamed addNodeRecoverAnnotation to attemptNodeRecovery, and changed it to use a strategic-merge Patch to avoid update conflicts while the node is recovering.
Freeze/Unfreeze test captures more events than assertion requires when run against a real cluster multiple times.
- Instances: Run
- Root Cause: Across multiple runs, the machineset name is the same and the events can persist in the cluster across runs.
- Fix: Track events by machineset UID instead of name

Other changes/improvements:
- Added more detailed logging in framework.go which now tells exactly which machine a test is being executed on.
- Extracted functions with reuse value as helpers from changes done for Bug #1
- Make target to easily export kubeconfig of a simulated cluster.
- Changed constants in kwok-config.yaml to more tighter values to make the average test runtime faster.
- Requeue preserved machines immediately after their PreserveExpiryTime to avoid them potentially waiting a full reconcile (10mins).
- Delete MCDs before running the orphaned resources test, as a test failure can leave the mcd still on the cluster causing this test to hang waiting for non-terminating VMs.
- Upload mc and mcm process logs to the actions run in the event of a failure, for easy diagnosis.

Special notes for Reviewers
Current runtime statistics

Date         : Fri Sep 18 06:28:35 IST 2026
Executed     : 20 run(s)
Passed       : 20
Failed       : 0
Pass rate    : 100% (20/20)

Runtime per run (all executed):
  best       : 11m 08s
  worst      : 16m 33s
  average    : 13m 56s
  median     : 13m 40s

Release note:

Fix bugs and integration test flakes

gardener-prow · 2026-09-16T11:52:56Z

[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 assign takoverflow for approval. 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

gagan16k · 2026-09-18T04:46:58Z

/kind regression

takoverflow

Thanks for looking in detail at the IT test failures and improving the flakiness.

I've added some comments, PTAL

takoverflow · 2026-09-23T08:49:10Z

  delay:
-    durationMilliseconds: 60000
-    jitterDurationMilliseconds: 61000
+    durationMilliseconds: 2500


Why has this been reduced to such a small value?
Was the 60s delay too long for some operation?

takoverflow · 2026-09-23T09:05:06Z

      - key: '.metadata.annotations["kwok/fail-condition"]'
        operator: 'In'
        values:
        - 'Recover'


As discussed offline, the trigger for running this stage can now be changed where the annotation key can be used to denote recovery and the value can be the timestamp when the annotation is added.

This is to allow for subsequent update events to be triggered via modification of a single annotation.

takoverflow · 2026-09-23T09:16:05Z


+// isNodePreserved reports whether the node is preserved, by checking if it carries the
+// preserve taint that MCM applies when a Failed machine's node is preserved.
+func isNodePreserved(node *corev1.Node) bool {


There's a NodeCondition with type NodePreserved that's added. Checking for its presence and state would be better IMO compared to looking for the taint.

nodeops.GetCondition(node, v1alpha1.NodePreserved)

takoverflow · 2026-09-23T09:33:30Z

 	}

 	pollingInterval := 2 * time.Second
+	if isSimulatedProvider {


This value is set in SetupBeforeSuite, which is invoked in simulatedprovider/test/integration/controller/controller_test.go after the call to NewIntegrationTestFramework so I'm not sure if its even set at this point, can you check locally if things are working as expected. I'm guessing even with the simulated provider it might still be using 2s polling period.

takoverflow · 2026-09-23T09:34:47Z

 		gomega.Expect(mcsession.ExitCode()).Should(gomega.Equal(-1))
 		ginkgo.By("Checking machineControllerManager process is running")
 		gomega.Expect(mcmsession.ExitCode()).Should(gomega.Equal(-1))
-		ginkgo.By("Checking nodes in target cluster are healthy")


Are you sure getting rid of this check cannot cause problems when running IT with infra?

takoverflow · 2026-09-23T09:38:01Z

 }

-// AreMachinesRunning returns boolean value indicating whether all the machines names passed to it are in the running state or not
-func (c *Cluster) AreMachinesRunning(ctx context.Context, machineNames []string, namespace string) bool {


There's a PR now that relies on AreMachinesRunning without any preservation in context.
#1158
It might be worthwhile to have two methods now.

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

- Add artifact upload for logs on failure in CI - Introduce KUBE_API_QPS and KUBE_API_BURST parameters for simulated provider - Update polling interval for simulated provider tests - Modify machine status checks to improve recovery logic, add new annotation - Modify kwok config to work quicker Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

gagan16k requested a review from a team as a code owner September 16, 2026 11:52

gardener-prow Bot added the kind/flake Tracking or fixing a flaky test label Sep 16, 2026

gardener-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Sep 16, 2026

gagan16k marked this pull request as draft September 16, 2026 18:35

gardener-prow Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 16, 2026

gagan16k changed the title ~~Fix flaky integration tests on GHA~~ Bug fixes and address integration test flakes Sep 17, 2026

gagan16k marked this pull request as ready for review September 18, 2026 04:40

gardener-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 18, 2026

gardener-prow Bot added the kind/regression Bug that hit us already in the past and that is reappearing/requires a proper solution label Sep 18, 2026

elankath reviewed Sep 18, 2026

View reviewed changes

Comment thread pkg/controller/deployment_machineset_util.go Outdated

gagan16k removed the kind/regression Bug that hit us already in the past and that is reappearing/requires a proper solution label Sep 21, 2026

gardener-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 21, 2026

gagan16k force-pushed the Itest branch from 2a3dc9d to 3a38589 Compare September 21, 2026 10:50

gardener-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 21, 2026

r4mek reviewed Sep 22, 2026

View reviewed changes

Comment thread pkg/util/provider/machineutils/utils.go Outdated

r4mek mentioned this pull request Sep 22, 2026

Preservation bugfix to put PreserveExpiryTime whenever machine transitions to Failed phase #1147

Open

gardener-prow Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 22, 2026

takoverflow reviewed Sep 23, 2026

View reviewed changes

gardener-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2026

gagan16k added 20 commits September 23, 2026 18:22

add more logs + increase replica count + harden intra spec stability

79f3549

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

fix race in auto-preservation logic and update related tests

b41b99e

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

change getTestMachineSets to return UID instead of name

2b729e5

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

remove extra echo

8c72a8a

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

reduce machine health timeout from 10m to 5m

e78a874

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

revert missed test change

41c191b

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Extract helpers

3c244c2

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Add #nosec comment

92761e6

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Requeue preserved machines at their PreserveExpiryTime

a0c768f

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Linter fix

f05807c

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Update comments for KUBE_API_QPS and KUBE_API_BURST in Makefile

525b3d5

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Address review

bd22353

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Remove health check for nodes in BeforeEachCheck

80ddac2

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

machine deployment replica count to use constant

a851fdb

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Rename functions

ee2079d

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

update docstring

9310d38

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

attemptNodeRecovery only for simulated clusters

d699301

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Review comments

0a5618b

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

Address review - 1

5912ffb

Signed-off-by: Gagan163264 <gagan.surathkal@gmail.com>

gagan16k force-pushed the Itest branch from 56d7ffc to 5912ffb Compare September 23, 2026 12:53

gardener-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2026

Navigation Menu

Bug fixes and address integration test flakes - #1154

Bug fixes and address integration test flakes#1154
gagan16k wants to merge 20 commits into
gardener:masterfrom
gagan16k:Itest

gagan16k commented Sep 16, 2026 •

edited

Loading

Uh oh!

gardener-prow Bot commented Sep 16, 2026

Uh oh!

gagan16k commented Sep 18, 2026

Uh oh!

Uh oh!

Uh oh!

takoverflow left a comment

Uh oh!

Uh oh!

takoverflow Sep 23, 2026

Uh oh!

Uh oh!

takoverflow Sep 23, 2026

Uh oh!

takoverflow Sep 23, 2026

Uh oh!

Uh oh!

Uh oh!

takoverflow Sep 23, 2026

Uh oh!

takoverflow Sep 23, 2026

Uh oh!

takoverflow Sep 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

gagan16k commented Sep 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gardener-prow Bot commented Sep 16, 2026

Uh oh!

gagan16k commented Sep 18, 2026

Uh oh!

Uh oh!

Uh oh!

takoverflow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

takoverflow Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

takoverflow Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

takoverflow Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

takoverflow Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

takoverflow Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

takoverflow Sep 23, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gagan16k commented Sep 16, 2026 •

edited

Loading