Skip to content

Conversation

@ryanzhang-oss
Copy link
Contributor

Description of your changes

Need to wait for the snapshot appear before creating the CRP otherwise, it's a race between override and rollout controller

Fixes #

I have:

  • Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

Signed-off-by: Ryan Zhang <zhangryan@microsoft.com>
Signed-off-by: Ryan Zhang <zhangryan@microsoft.com>
@ryanzhang-oss ryanzhang-oss requested a review from Copilot April 22, 2025 22:35
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request addresses the override e2e flakiness by ensuring that the proper snapshot (both for resource overrides and cluster resource overrides) is available before creating the CRP. Key changes include:

  • Adding wait logic (using Eventually blocks) to ensure the snapshot is present.
  • Updating tests in both placement_ro_test.go and placement_cro_test.go to use the computed snapshot names instead of reformatting inline.
  • Standardizing error messages and flow to avoid race conditions between the override and rollout controller.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
test/e2e/placement_ro_test.go Added snapshot wait logic and updated error messages to ensure test stability.
test/e2e/placement_cro_test.go Added similar snapshot wait logic for cluster resource overrides and updated context naming.
Comments suppressed due to low confidence (3)

test/e2e/placement_ro_test.go:248

  • [nitpick] Consider revising the error message to explicitly indicate that the snapshot retrieval failed rather than referencing 'ro' or using 'crpName', which might confuse readers.
}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update ro as expected", crpName)

test/e2e/placement_ro_test.go:481

  • [nitpick] Update the error message to clearly describe that the cluster resource override snapshot retrieval failed, instead of using a generic message that references 'ro'.
}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update ro as expected", crpName)

test/e2e/placement_cro_test.go:77

  • [nitpick] Consider revising this error message to specifically mention that the failure occurred during the cluster resource override snapshot retrieval, ensuring clarity in context.
}, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update ro as expected", crpName)

@ryanzhang-oss ryanzhang-oss merged commit e88d0bf into kubefleet-dev:main Apr 23, 2025
23 of 24 checks passed
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