test: Add client side install method for restoring Config Sync #1834

tiffanny29631 · 2025-08-19T23:43:58Z

To guarantee the e2e client claims ownership of all fields for objects that might have drifted, use client-side apply when reinstalling Config Sync after ConfigManagement was previously installed and removed.

gemini-code-assist

Summary of Changes

Hello @tiffanny29631, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request enhances the reliability of end-to-end tests, specifically those related to nomos migrate and ACM uninstallation. It addresses test flakiness by ensuring that legacy reconciler-manager and resource-group-controller deployments are explicitly deleted during test teardown. This guarantees a clean slate for Config Sync installations in subsequent test runs, preventing issues caused by lingering states from previous tests.

Highlights

Improved Test Reliability: Enhances the stability of nomos migrate and ACM uninstall end-to-end tests by preventing state leakage between test runs.
Explicit Deployment Deletion: Introduces logic to explicitly delete reconciler-manager and resource-group-controller deployments during test cleanup.
Ensuring Clean State: Guarantees a fresh Config Sync installation for each test, avoiding reliance on patching or implicit cleanup of legacy components.
Targeted Cleanup: The cleanup logic is applied to the Cleanup functions of TestNomosMigrate, TestNomosMigrateMonoRepo, and TestACMUninstallScript.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

gemini-code-assist

Code Review

This pull request adds cleanup logic to delete legacy reconciler-manager and resource-group-controller deployments in several e2e tests. This improves test reliability by ensuring a clean state. The implementation is correct, but the same cleanup logic is duplicated across three test functions. I've suggested refactoring this duplicated code into a helper function to improve maintainability.

e2e/testcases/cli_test.go

tiffanny29631 · 2025-08-28T20:07:01Z

Note: should only merge if #1791 has stable success presubmit result.

/hold

To guarantee the e2e client claims ownership of all fields for objects that might have drifted, use client-side apply when reinstalling Config Sync and Webhook fter ConfigManagement was previously installed and removed.

Copilot

Pull Request Overview

This PR adds a client-side install method for restoring Config Sync in e2e tests to ensure proper field ownership when reinstalling after ConfigManagement removal. The key changes include replacing server-side apply with client-side apply and updating watcher methods to check for current status rather than absence.

Introduces InstallConfigSyncFromManifest function using client-side kubectl apply
Updates test cleanup to use the new manifest-based installation method
Changes watcher behavior from checking for resource absence to checking current status

Reviewed Changes

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

File	Description
e2e/testcases/cli_test.go	Updates three test functions to use new installation method and modified watcher behavior
e2e/nomostest/config_sync.go	Adds new client-side manifest installation function with direct kubectl apply

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

e2e/nomostest/config_sync.go

tiffanny29631 · 2025-09-05T16:09:47Z

/retest

tiffanny29631 · 2025-09-08T22:34:05Z

/unhold

Camila-B

/lgtm

sdowell · 2025-09-09T20:48:17Z

e2e/nomostest/config_sync.go


+// InstallConfigSyncFromManifest installs ConfigSync on the test cluster by directly
+// applying the manifest file using kubectl client-side apply
+func InstallConfigSyncFromManifest(nt *NT) error {


This will revert some of the modifications made by the usual InstallConfigSync function used by the test scaffolding. Can this be consolidated to use the same function? I expect you could add an option to use Update instead of Patch/Apply

Could you elaborate on the 'same function'? I assume you were not suggesting merging the two install functions, the KubeClient.Apply does not achieve necessary field management or removes any legacy fields. Is it now kubectl update instead of kubectl apply that you suggest?

I'm suggesting it would use the client-go Update (KubeClient.Update) method instead of the client-go Apply method (KubeClient.Apply)

The go-client update has a few edge cases too.

When working on the configsync manifest, CRD like clusters.clusterregistry.k8s.io disallows update when there's no resourceversion attached. Something like

cli_test.go:1477: 2025-09-10 05:42:53.626931518 +0000 UTC ERROR: customresourcedefinitions.apiextensions.k8s.io "clusters.clusterregistry.k8s.io" is invalid: metadata.resourceVersion: Invalid value: 0x0: must be specified for an update

The operator also removes the admission webhook, which will need a special handle when restoring the shared test env, a create instead of update. We could also use preventDrift: true when configuring the configmanagement?

Anyways not sure if the effort is worthy for a deprecated use case test.

OK added the resourceVersion, hopefully it solves the issue

Yes you have to set the resourceVersion when using update. Are you missing context on working with k8s client libraries? If so I can take over the PR - although I don't think this deliverable is really needed until we solve the upgrade issue

Good chance to learn. The latest change seemed to be working.

google-oss-prow · 2025-09-09T22:51:57Z

New changes are detected. LGTM label has been removed.

google-oss-prow · 2025-09-09T22:52:00Z

[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 ask for approval from camila-b. For more information see the Kubernetes Code Review Process.

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

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

Copilot

Pull Request Overview

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

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-11T17:52:10Z

e2e/nomostest/config_sync.go

+				return err
+			}
+		case InstallMethodUpdate:
+			currentObj := o.DeepCopyObject().(client.Object)


The type assertion .(client.Object) could panic if DeepCopyObject() returns an object that doesn't implement client.Object. Consider using a type switch or checking the assertion result to handle this more safely.

Suggested change

currentObj := o.DeepCopyObject().(client.Object)

currentObjObj := o.DeepCopyObject()

currentObj, ok := currentObjObj.(client.Object)

if !ok {

return fmt.Errorf("object %v does not implement client.Object", core.GKNN(o))

}

Copilot · 2025-09-11T17:52:11Z

e2e/nomostest/config_sync.go

+				// Attach existing resourceVersion to the object
+				o.SetResourceVersion(currentObj.GetResourceVersion())
+				if err := nt.KubeClient.Update(o); err != nil {
+					return err
+				}


Setting the resource version on the object being updated could cause conflicts if the object was modified between the Get and Update operations. Consider using a retry mechanism or server-side apply to handle concurrent modifications more reliably.

Suggested change

// Attach existing resourceVersion to the object

o.SetResourceVersion(currentObj.GetResourceVersion())

if err := nt.KubeClient.Update(o); err != nil {

return err

}

// Attach existing resourceVersion to the object and retry on conflict

const maxUpdateAttempts = 5

var lastErr error

for attempt := 1; attempt <= maxUpdateAttempts; attempt++ {

o.SetResourceVersion(currentObj.GetResourceVersion())

if err := nt.KubeClient.Update(o); err != nil {

if apierrors.IsConflict(err) {

// Re-fetch the latest object and retry

if err := nt.KubeClient.Get(currentObj.GetName(), currentObj.GetNamespace(), currentObj); err != nil {

lastErr = err

break

}

lastErr = err

continue

} else {

lastErr = err

break

}

} else {

lastErr = nil

break

}

}

if lastErr != nil {

return lastErr

}

sdowell · 2025-09-11T17:55:32Z

e2e/nomostest/config_sync.go

+type InstallMethod string
+
+const (
+	// InstallMethodApply uses server-side apply (default)


It doesn't look like this is currently the default. If the intent is to retain default behavior, I'd suggest updating the function signature to accept a variadic list of options (for example func InstallConfigSync(nt *NT, opts ...InstallConfigSyncOpts) error {)

sdowell · 2025-09-11T17:58:42Z

e2e/nomostest/config_sync.go

+				return err
+			}
+		case InstallMethodUpdate:
+			currentObj := o.DeepCopyObject().(client.Object)


A DeepCopy should not be necessary here for every object, can this be switched to populating an empty object (e.g. Unstructured)?

sdowell · 2025-09-11T18:04:22Z

e2e/nomostest/config_sync.go

+					return err
+				}
+			} else {
+				// Attach existing resourceVersion to the object


The comment doesn't add much context, I'd suggest explaining "why" it's attaching the resourceVersion to the object.

In this instance, the current state of the object on the cluster does not matter and the intent is to fully update to the original configuration declared in the manifest

google-oss-prow bot requested review from mikebz and sdowell August 19, 2025 23:44

google-oss-prow bot added the size/M label Aug 19, 2025

gemini-code-assist bot reviewed Aug 19, 2025

View reviewed changes

e2e/testcases/cli_test.go Outdated Show resolved Hide resolved

sdowell reviewed Aug 19, 2025

View reviewed changes

e2e/testcases/cli_test.go Outdated Show resolved Hide resolved

tiffanny29631 mentioned this pull request Aug 20, 2025

Remove readiness probe otel-agent sidecars #1791

Open

tiffanny29631 force-pushed the migrate-test branch from ab3731e to 7dd2508 Compare August 27, 2025 22:21

google-oss-prow bot added size/S and removed size/M labels Aug 27, 2025

tiffanny29631 changed the title ~~Cleanup legacy controller deployments in test teardown~~ test: Add client side install method for restoring Config Sync Aug 27, 2025

google-oss-prow bot added the do-not-merge/hold label Aug 28, 2025

tiffanny29631 force-pushed the migrate-test branch from 7dd2508 to b93791b Compare September 3, 2025 20:31

google-oss-prow bot added size/M and removed size/S labels Sep 3, 2025

tiffanny29631 force-pushed the migrate-test branch 8 times, most recently from 851c901 to 6cbe85e Compare September 5, 2025 01:14

test: Add client side install method for restoring Config Sync

313a440

To guarantee the e2e client claims ownership of all fields for objects that might have drifted, use client-side apply when reinstalling Config Sync and Webhook fter ConfigManagement was previously installed and removed.

tiffanny29631 force-pushed the migrate-test branch from 6cbe85e to 313a440 Compare September 5, 2025 01:16

mikebz requested a review from Copilot September 5, 2025 03:18

Copilot AI reviewed Sep 5, 2025

View reviewed changes

e2e/nomostest/config_sync.go Outdated Show resolved Hide resolved

e2e/nomostest/config_sync.go Outdated Show resolved Hide resolved

e2e/nomostest/config_sync.go Outdated Show resolved Hide resolved

Add server-side false flag

77a9062

tiffanny29631 force-pushed the migrate-test branch from f13e1ae to 77a9062 Compare September 5, 2025 04:36

Add server-side false flag

9e508cc

google-oss-prow bot removed the do-not-merge/hold label Sep 8, 2025

Camila-B reviewed Sep 9, 2025

View reviewed changes

google-oss-prow bot assigned Camila-B Sep 9, 2025

google-oss-prow bot added the lgtm label Sep 9, 2025

sdowell reviewed Sep 9, 2025

View reviewed changes

Add go-client update as re-install option

7f771ee

google-oss-prow bot removed the lgtm label Sep 9, 2025

tiffanny29631 added 3 commits September 9, 2025 23:55

Also re-install webhook after test done

0c6dcc9

Create object if not found when updating

9b56f3b

Attach resourceVersion for updating existing objects

e8cc294

Camila-B removed their assignment Sep 10, 2025

mikebz requested a review from Copilot September 11, 2025 17:51

Copilot AI reviewed Sep 11, 2025

View reviewed changes

sdowell reviewed Sep 11, 2025

View reviewed changes

-			currentObj := o.DeepCopyObject().(client.Object)
+			currentObjObj := o.DeepCopyObject()
+			currentObj, ok := currentObjObj.(client.Object)
+			if !ok {
+				return fmt.Errorf("object %v does not implement client.Object", core.GKNN(o))
+			}

-				// Attach existing resourceVersion to the object
-				o.SetResourceVersion(currentObj.GetResourceVersion())
-				if err := nt.KubeClient.Update(o); err != nil {
-					return err
-				}
+				// Attach existing resourceVersion to the object and retry on conflict
+				const maxUpdateAttempts = 5
+				var lastErr error
+				for attempt := 1; attempt <= maxUpdateAttempts; attempt++ {
+					o.SetResourceVersion(currentObj.GetResourceVersion())
+					if err := nt.KubeClient.Update(o); err != nil {
+						if apierrors.IsConflict(err) {
+							// Re-fetch the latest object and retry
+							if err := nt.KubeClient.Get(currentObj.GetName(), currentObj.GetNamespace(), currentObj); err != nil {
+								lastErr = err
+								break
+							}
+							lastErr = err
+							continue
+						} else {
+							lastErr = err
+							break
+						}
+					} else {
+						lastErr = nil
+						break
+					}
+				}
+				if lastErr != nil {
+					return lastErr
+				}

test: Add client side install method for restoring Config Sync #1834

Are you sure you want to change the base?

test: Add client side install method for restoring Config Sync #1834

Uh oh!

Conversation

tiffanny29631 commented Aug 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

tiffanny29631 commented Aug 28, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tiffanny29631 commented Sep 5, 2025

Uh oh!

tiffanny29631 commented Sep 8, 2025

Uh oh!

Camila-B left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-prow bot commented Sep 9, 2025

Uh oh!

google-oss-prow bot commented Sep 9, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 11, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tiffanny29631 commented Aug 19, 2025 •

edited

Loading