Skip to content

🌱 Try to fix test flake in which secret is not yet available #5563

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+6 −0
Open

🌱 Try to fix test flake in which secret is not yet available #5563

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions exp/controllers/awsmachinepool_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -913,6 +913,12 @@ func TestAWSMachinePoolReconciler(t *testing.T) {
},
}
g.Expect(testEnv.Create(ctx, newBootstrapSecret)).To(Succeed())
g.Eventually(func(gomega Gomega) {
gomega.Expect(testEnv.Client.Get(ctx, client.ObjectKey{
Copy link
Contributor

@dlipovetsky dlipovetsky Jun 24, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I can't look right now, but I'm wondering if this is a cached client? 🤔

That would explain the failure to get the Secret just after it has been created.

We should use a "direct" or "uncached" client in tests.

kubernetes-sigs/controller-runtime#343 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm indeed we use the manager client

	return &TestEnvironment{
		Manager: mgr,
		Client:  mgr.GetClient(), // <----------
		Config:  mgr.GetConfig(),
		env:     t.env,
	}, nil

which may be a cache:

GetClient returns a client configured with the Config. This client may not be a fully "direct" client -- it may read from a cache, for instance.

But we're testing the reconciler here which uses its caching client. Should we override that to a direct client in tests? I'm not even sure that's possible (we'd need a SetClient or some option to avoid caching?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we want to copy-paste CAPI's CreateAndWait to denote the intent

Copy link
Contributor

Choose a reason for hiding this comment

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

But we're testing the reconciler here which uses its caching client.

Yes, but in production, if the Secret is unavailable, the request would be requeued. That doesn't happen under test: we fail immediately. So one way to ensure that the reconcile is ready to run is to use a direct client.

I think switching to a direct client is something we want to do across all tests, so that's out of scope for this PR.

Maybe we want to copy-paste CAPI's CreateAndWait to denote the intent

That's a good idea.

Name: newBootstrapSecret.Name,
Namespace: newBootstrapSecret.Namespace,
}, newBootstrapSecret)).To(Succeed())
}).Should(Succeed())
ms.MachinePool.Spec.Template.Spec.Bootstrap.DataSecretName = ptr.To[string](newBootstrapSecret.Name)

// Since `AWSMachinePool.status.launchTemplateVersion` isn't set yet,
Expand Down