Skip to content

🐛 Fix race condition in InMemoryMachine controller tests #12347

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

Merged
merged 1 commit into from
Jul 8, 2025

Conversation

alimaazamat
Copy link
Contributor

@alimaazamat alimaazamat commented Jun 11, 2025

What this PR does / why we need it:
Error:
The controller returned ctrl.Result{} (no requeue) too early.
The test expects a requeue ctrl.Result{RequeueAfter: ...} until the provisioning duration has finished.

If res is equal to 0 then reconciliation will not be requeued, but it should be requeued and equal to 0.
if now < until then res != 0 and we requeue
if now >= until then res == 0 and we do not requeue (provisioned)

Problem:
Test setup took longer than provisioning duration causing the controller to think provisioning is completed when the test expected it to still be incomplete. Controller uses LastTransitionTime of NodeProvisionedCondition to determine if provisioning time is completed but this timestamp is out of date by the time test setup finishes.

Fix:
Added a helper function that updates the LastTransitionTime to current time right before calling the reconciler. Note that the helper function handles both current and deprecated status structures because we are migrating in between API versions until approximately August 2026.

Verification:
@mboersma and I ran this fix with a test script for multiple days and didn't get the error.

Which issue(s) this PR fixes:
Fixes #12301

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 11, 2025
@k8s-ci-robot k8s-ci-robot added do-not-merge/needs-area PR is missing an area label size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 11, 2025
@alimaazamat alimaazamat force-pushed the race-conditions branch 2 times, most recently from 9dcf6df to 21b1255 Compare June 24, 2025 22:00
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 25, 2025
@alimaazamat
Copy link
Contributor Author

test result of d290d64:

I0625 07:08:03.191676   34585 inmemorymachine_backend.go:672] APIServer check: start=2025-06-25 07:08:00.844856 -0700 PDT m=+4.659283959, now=2025-06-25 07:08:03.191632 -0700 PDT m=+7.006113626, provisioningDuration=2s, until=2025-06-25 07:08:02.844856 -0700 PDT m=+6.659283959
I0625 07:08:03.201794   34585 mux.go:406] "APIServer instance added to workloadClusterListener" listenerName="foo" address="https://127.0.0.1:21100" podName="kube-apiserver-bar"
I0625 07:08:03.235570   34585 mux.go:494] "WorkloadClusterListener successfully started" listenerName="foo" address="https://127.0.0.1:21100"
--- FAIL: TestReconcileNormalApiServer (2.39s)
    --- FAIL: TestReconcileNormalApiServer/create_pod_if_Node_is_ready (2.39s)
        inmemorymachine_controller_test.go:626: 
            Expected
                <bool>: true
            to be false
FAIL
FAIL    sigs.k8s.io/cluster-api/test/infrastructure/docker/internal/controllers/backends/inmemory     7.717s
FAIL

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 25, 2025
@alimaazamat alimaazamat changed the title [WIP] Race Condition where controller marks provisioned Race Condition where controller marks provisioned Jun 30, 2025
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jun 30, 2025
@alimaazamat alimaazamat changed the title Race Condition where controller marks provisioned 🐛: Race Condition where controller marks provisioned Jun 30, 2025
@alimaazamat alimaazamat changed the title 🐛: Race Condition where controller marks provisioned 🐛Race Condition where controller marks provisioned Jun 30, 2025
@sbueringer
Copy link
Member

/assign

@mboersma
Copy link
Contributor

mboersma commented Jul 7, 2025

/area testing

@k8s-ci-robot k8s-ci-robot added area/testing Issues or PRs related to testing and removed do-not-merge/needs-area PR is missing an area label labels Jul 7, 2025
Copy link
Member

@sbueringer sbueringer left a comment

Choose a reason for hiding this comment

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

Thank you very much!!

Sorry for the delay. Two nits, then happy to merge :)

@sbueringer sbueringer changed the title 🐛Race Condition where controller marks provisioned 🐛 Fix race condition in InMemoryMachine controller tests Jul 7, 2025
@sbueringer
Copy link
Member

Thanks again! That one was really annoying :)

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2025
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 542efc17fdf9753194cf86cfd9af01dde64d315a

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2025
@k8s-ci-robot k8s-ci-robot merged commit 24a4184 into kubernetes-sigs:main Jul 8, 2025
18 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Jul 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/testing Issues or PRs related to testing cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[unit test] Reconcilliation should be incomplete
4 participants