Skip to content
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

test: fix race condition between registry credentials and mirror tests #296

Closed

Conversation

supershal
Copy link
Contributor

Stacked on #292
Fixes flakes in unit tests for image registries: https://github.com/d2iq-labs/capi-runtime-extensions/actions/runs/7564146208/job/20597802432#step:5:68

 Message: failed to apply mutation: failed to apply Image Registry Credentials Secret: server-side apply failed: Operation cannot be fulfilled on secrets "test-kubeadmconfigtemplate-registry-config": object was modified

When a static credentials are provided for registry, the patch generator creates secret that contains static credentials provider configuration with registry-config suffix. This fix is created with server side apply. however Server side apply does not work with the fake client, hack around it by pre-creating empty Secrets. kubernetes-sigs/controller-runtime#2341

When we added more test cases for mirror registry, two parallel running tests were changing the same secrets that resulted in above error intermittently.

Fix:

  • ensure that the test secret name for static credentials provider is unique to avoid updating the same secret in parallel.

@supershal supershal force-pushed the shalin/containerd-mirror-unit-tets branch from f546b8b to 1293e11 Compare January 19, 2024 04:41
Copy link
Contributor

@dkoshkin dkoshkin left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple of nits

return NewKubeadmConfigTemplateRequest(uid, kubeadmConfigTemplateRequestObjectName)
}

func NewKubeadmConfigTemplateRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

How about something like this? I don't think dropping the "Item" is clear what the difference is

Suggested change
func NewKubeadmConfigTemplateRequest(
func NewNamedKubeadmConfigTemplateRequestItem(

@@ -75,8 +84,9 @@ func NewKubeadmConfigTemplateRequestItem(uid types.UID) runtimehooksv1.GenerateP
)
}

func NewKubeadmControlPlaneTemplateRequestItem(
func NewKubeadmControlPlaneTemplateRequest(
Copy link
Contributor

Choose a reason for hiding this comment

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

Also here

@@ -5,6 +5,7 @@ package tests

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we should rename this file to generate_credentials_patches.go to match the new generate_mirror_patches.go?

@supershal
Copy link
Contributor Author

It was difficult to refactor #292 without this changes . cherry-picked commits from this PR to the parent PR. #292
Closing this PR.

@supershal supershal closed this Jan 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants