feat: Change AppliedWork to used foregroundDeletion #60

britaniar · 2025-05-08T16:53:09Z

Description of your changes

Fixes #

I have:

now track work that is deleting by using Foreground deletion mode on applied work
update the owner reference to not block applied work deletion when there is co-ownership.
Run make reviewable to ensure this PR is ready for review.

How has this code been tested

Special notes for your reviewer

References:

codecov · 2025-05-08T17:14:55Z

Codecov Report

Attention: Patch coverage is 71.59091% with 25 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/controllers/workapplier/controller.go	66.66%	21 Missing and 4 partials ⚠️

📢 Thoughts on this report? Let us know!

pkg/controllers/workgenerator/controller.go

pkg/controllers/workgenerator/controller_integration_test.go

pkg/controllers/workapplier/controller.go

test/e2e/rollout_test.go

pkg/controllers/workapplier/controller.go

test/e2e/placement_apply_strategy_test.go

test/e2e/placement_pickfixed_test.go

pkg/controllers/workapplier/controller.go

michaelawyu · 2025-06-02T22:57:41Z

Hi Britania! A side note: though the case we troubleshot was quite special, we probably still need to handle it as a corner case to avoid system being stuck forever -> it could be sent in a separate PR though.

michaelawyu · 2025-06-02T22:57:57Z

Added some comments, PTAL.

pkg/controllers/workapplier/controller.go

pkg/controllers/workapplier/controller_integration_test.go

test/e2e/placement_apply_strategy_test.go

Copilot

Pull Request Overview

This PR updates the deletion mechanism for applied work by switching to foreground deletion and adjusting owner reference behavior accordingly. Key changes include updating tests to verify work resource deletion, modifying BlockOwnerDeletion settings in owner references, and changing the deletion propagation policy in garbage collection.

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/e2e/utils_test.go	Added waiting for work resource deletion and reset ResourceVersion in tests
test/e2e/placement_pickfixed_test.go	Added scenarios to simulate stuck deletions and verify proper finalizer management
test/e2e/placement_apply_strategy_test.go	Replaced inline owner reference definitions with a reusable variable and improved error messages
test/e2e/enveloped_object_placement_test.go	Minor adjustment to delete a second envelope resource
pkg/controllers/workapplier/preprocess.go	Removed unused import and updated owner reference equality checks in removal logic
pkg/controllers/workapplier/controller_test.go	Added tests for ClusterRole resources
pkg/controllers/workapplier/controller.go	Changed deletion propagation to foreground and updated owner reference settings

pkg/controllers/workapplier/preprocess.go

…reference when co-ownership is allowed Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

michaelawyu

Added some comments, PTAL. Let me know if you'd like to discuss it a bit further.

pkg/controllers/workapplier/controller.go

michaelawyu · 2025-06-17T23:03:24Z

pkg/controllers/workapplier/controller.go

+		UID:        appliedWork.UID,
+	}
+
+	for _, res := range appliedWork.Status.AppliedResources {


Hi Britania! This part might have some potential consistency issues; the AppliedResources on AppliedWork objects are updated only at the end of the reconciliation loop on a best effort basis; it might not incude all the applied manifests if, say, the status update is skipped/interruptted. The work object status is, instead, designed to be always consistent.

can we make appliedResources accurate? I thought we rely on this to remove resources from the member cluster

pkg/controllers/workapplier/controller.go

pkg/controllers/workapplier/process.go

pkg/controllers/workapplier/controller.go

ryanzhang-oss · 2025-06-18T20:28:50Z

pkg/controllers/workapplier/controller.go

+		UID:        appliedWork.UID,
+	}
+
+	for _, res := range appliedWork.Status.AppliedResources {


can we make appliedResources accurate? I thought we rely on this to remove resources from the member cluster

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

pkg/controllers/workapplier/controller.go

pkg/controllers/workapplier/preprocess.go

pkg/controllers/workapplier/process.go

test/e2e/placement_apply_strategy_test.go

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

britaniar marked this pull request as ready for review May 8, 2025 20:52

zhiying-lin reviewed May 12, 2025

View reviewed changes

britaniar force-pushed the deletedWork branch from 06fd750 to d1f199f Compare May 14, 2025 00:02

britaniar force-pushed the deletedWork branch from d1f199f to bb6ad2f Compare May 22, 2025 23:01

jwtty reviewed May 27, 2025

View reviewed changes

pkg/controllers/workapplier/controller.go Show resolved Hide resolved

britaniar force-pushed the deletedWork branch from 7d61007 to 939a3f9 Compare May 28, 2025 17:42

ryanzhang-oss reviewed Jun 2, 2025

View reviewed changes

test/e2e/rollout_test.go Outdated Show resolved Hide resolved

michaelawyu reviewed Jun 2, 2025

View reviewed changes

britaniar force-pushed the deletedWork branch 4 times, most recently from 6bc8746 to 82bd3ad Compare June 5, 2025 23:36

britaniar commented Jun 6, 2025

View reviewed changes

pkg/controllers/workapplier/controller.go Outdated Show resolved Hide resolved

ryanzhang-oss reviewed Jun 6, 2025

View reviewed changes

zhiying-lin reviewed Jun 10, 2025

View reviewed changes

pkg/controllers/workapplier/controller.go Outdated Show resolved Hide resolved

pkg/controllers/workapplier/controller.go Outdated Show resolved Hide resolved

pkg/controllers/workapplier/controller.go Outdated Show resolved Hide resolved

britaniar changed the title ~~feat: track work deletion~~ feat: Change AppliedWork to used foregroundDeletion Jun 10, 2025

britaniar force-pushed the deletedWork branch 3 times, most recently from 8332560 to 10311a3 Compare June 16, 2025 18:30

jwtty reviewed Jun 17, 2025

View reviewed changes

britaniar requested a review from Copilot June 17, 2025 16:45

Copilot AI reviewed Jun 17, 2025

View reviewed changes

pkg/controllers/workapplier/preprocess.go Outdated Show resolved Hide resolved

britaniar force-pushed the deletedWork branch from 3021bc8 to 5723a3d Compare June 17, 2025 16:50

britaniar added 2 commits June 17, 2025 12:55

update to workapplier to use foreground deletion and to update owner …

3e62cf4

…reference when co-ownership is allowed Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

address comments (fix requeue, formatting, etc.)

c0c3243

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

britaniar force-pushed the deletedWork branch 2 times, most recently from dca21c5 to c0c3243 Compare June 17, 2025 19:57

britaniar added 5 commits June 17, 2025 13:30

minor fix

584429c

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

remove whitespace

3b69b3b

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

update check in process.go

94c748e

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

update owner check

4f03345

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

update owner check in apply.go

9d4979c

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

michaelawyu reviewed Jun 17, 2025

View reviewed changes

ryanzhang-oss reviewed Jun 23, 2025

View reviewed changes

address comments & update implementation

92b0e24

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

britaniar force-pushed the deletedWork branch from b7b20ab to 92b0e24 Compare June 23, 2025 23:11

britaniar added 3 commits June 23, 2025 18:11

update integration tests

f142dc6

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

update e2e owner reference

d8b4fc2

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

fix e2e tests

b111d7d

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

ryanzhang-oss reviewed Jun 24, 2025

View reviewed changes

address comments

48e15e3

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

ryanzhang-oss approved these changes Jun 25, 2025

View reviewed changes

ryanzhang-oss merged commit dcd82cb into kubefleet-dev:main Jun 27, 2025
19 of 21 checks passed

britaniar deleted the deletedWork branch June 27, 2025 17:25

britaniar added a commit to britaniar/kubefleet that referenced this pull request Jun 28, 2025

feat: Change AppliedWork to used foregroundDeletion (kubefleet-dev#60)

050ee94

Signed-off-by: Britania Rodriguez Reyes <britaniar@microsoft.com>

audrastump pushed a commit to audrastump/kubefleet that referenced this pull request Aug 20, 2025

feat: Change AppliedWork to used foregroundDeletion (kubefleet-dev#60)

999b521

feat: Change AppliedWork to used foregroundDeletion #60

feat: Change AppliedWork to used foregroundDeletion #60

Uh oh!

Conversation

britaniar commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of your changes

How has this code been tested

Special notes for your reviewer

Uh oh!

codecov bot commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michaelawyu commented Jun 2, 2025

Uh oh!

michaelawyu commented Jun 2, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

michaelawyu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelawyu Jun 17, 2025

Choose a reason for hiding this comment

Uh oh!

ryanzhang-oss Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ryanzhang-oss Jun 18, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

britaniar commented May 8, 2025 •

edited

Loading

codecov bot commented May 8, 2025 •

edited

Loading