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

fix finalizer removal #31

Merged
merged 2 commits into from
May 28, 2024
Merged

fix finalizer removal #31

merged 2 commits into from
May 28, 2024

Conversation

QuentinBisson
Copy link
Contributor

@QuentinBisson QuentinBisson commented May 27, 2024

What this PR does / why we need it

Towards https://github.com/giantswarm/giantswarm/issues/30887

This PR fixes the finalizer removal. This has been tested on grizzly

Checklist

  • Update changelog in CHANGELOG.md.

@QuentinBisson QuentinBisson self-assigned this May 27, 2024
@QuentinBisson QuentinBisson marked this pull request as ready for review May 27, 2024 13:07
@QuentinBisson QuentinBisson requested a review from a team as a code owner May 27, 2024 13:07
@@ -189,14 +189,13 @@ func (pas PrometheusAgentService) deleteConfigMap(ctx context.Context, cluster *
}

// Delete the finalizer
desired := current.DeepCopy()
Copy link
Member

Choose a reason for hiding this comment

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

How does removing the DeepCopy fixes the finalizer removal /

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deepcopy is useless if we are updating instead of patching

Copy link
Member

Choose a reason for hiding this comment

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

In both case DeepCopy should not be needed, since the object is local to the function.

But how does moving from patch to update fixes the issue then ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes DeepCopy is needed in case of a controller runtime patch because that's how the client.MergeFrom function works but still, I'm not sure why it fixes it, but I've seen it does

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was advised by upstream to inverse the order of the delete and the patch, that's worth trying

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Testing here #32

@QuentinBisson QuentinBisson mentioned this pull request May 27, 2024
1 task
Copy link
Contributor

@QuantumEnigmaa QuantumEnigmaa left a comment

Choose a reason for hiding this comment

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

LGTM if tested

@QuentinBisson
Copy link
Contributor Author

After some discussions, we decided to remove the finalizers from the resource that this operator manages itself without relying on any other operator.

Copy link
Contributor

@QuantumEnigmaa QuantumEnigmaa left a comment

Choose a reason for hiding this comment

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

LGTM

@QuentinBisson
Copy link
Contributor Author

Tested on grizzly. @TheoBrigitte do you want to take a look?

@QuentinBisson
Copy link
Contributor Author

No more issues on grizzly

@QuentinBisson QuentinBisson merged commit 68b8532 into main May 28, 2024
6 checks passed
@QuentinBisson QuentinBisson deleted the fix-finalizer-resource-cleanup branch May 28, 2024 11:28
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.

3 participants