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

Ignore Owner References #11972

Open
spjmurray opened this issue Jan 12, 2023 · 10 comments
Open

Ignore Owner References #11972

spjmurray opened this issue Jan 12, 2023 · 10 comments
Labels
enhancement New feature or request

Comments

@spjmurray
Copy link

Summary

Provide a way to ignore owner references, and force ArgoCD to delete things.

Motivation

Related to kubernetes-sigs/cluster-api#7913. What we are seeing here is cluster-api adding its own owner references to resources managed by Argo. When it comes to deletion, it's not generated by the Helm template any more, but Argo thinks it's an implicitly created by cluster-api because of the owner references CAPI has added. Basically I cannot delete things unless I do it manually.

Proposal

One idea to handle naughty apps is to have Argo spot the tracking ID, and the fact there is no requested resource any more, and just ignore owners, it created it and has all the power here.

@spjmurray spjmurray added the enhancement New feature or request label Jan 12, 2023
@spjmurray
Copy link
Author

Another proposal now I consider it, is an explicit annotation that we can attach to a resource that says "you own this, delete it if not backed by helm template". Probably a better solution than just considering the tracking ID implicitly as I did see something historically about why tracking IDs came about in the first place due to inheritance.

spjmurray added a commit to spjmurray/gitops-engine that referenced this issue Jan 13, 2023
It's possible that a controller can adopt managed resources by attaching
an owner reference to it.  This owner reference has the effect of
keeping the resource alive, even though our intention is to delete it.
This adds in a new annotation that inhibits the consideration of owner
references.

Fixes argoproj/argo-cd#11972
@spjmurray
Copy link
Author

Added a potential--and remarkably small--PoC fix for consideration.

@jannfis
Copy link
Member

jannfis commented Jan 15, 2023

Thanks for bringing this up.

Probably we need to discuss general pruning behavior and how Argo CD handles owner references. I think the initial assumption was, that a resource would never have an owner outside of what the Argo CD application manages (think Deployment -> ReplicaSet, etc).

While it most likely contradicts with how Argo CD views things right now (e.g. that it "owns" a resource it has reconciled from Git to a cluster), I think it's perfectly fine for some other controller to claim ownership over a resource - I guess that's why there can be more than one owner references for any resource in the first place. I'd assume once CAPI puts an owner reference on the resource, Argo CD would also ignore the resource for changes originating on the cluster, and not enter an OutOfSync state when the resource change on the cluster?

Maybe a sync option on the resource, as implemented in your PoC, is a proper way to tell Argo CD to still let it handle a resource despite it having one or more owner references.

However, it sounds like it would need a little more discussion amongst contributors and maintainers on how we can support this use case. I'd like to bring this to the next contributor's meeting, happening weekly each Thursday at 4:15pm UTC, so that we'll find a good solution. You are of course invited to join, see https://github.com/argoproj/argo-cd#contribution-discussion-and-support for more details.

@spjmurray
Copy link
Author

Yeah, my train doesn't leave London until 7, so I should be able to hop on. Thanks for having a look

@spjmurray
Copy link
Author

Note this looks like a dupe of #4764 so linking

Did you come to any conclusions @jannfis I missed the important bit of that meeting due to Zoom (and broken library deps in dpkg!)

@jaideepr97
Copy link
Contributor

There's another potential approach suggested to this problem in #12210

@spjmurray
Copy link
Author

Ah yes, I'm aware of Matt's approach, that's also good as far as I'm concerned. While this is less dangerous, the other way does force people to write their controllers correctly (which is a win for the whole world), lest their stuff gets deleted :)

@jannfis
Copy link
Member

jannfis commented Feb 2, 2023

@spjmurray Sorry, I forgot to update this issue eventually. So we agreed that we should support the use case when another controller sets an owner reference to a resource synced with Argo CD. I think there was also consensus that the solution you propose in your PoC looks good, and there's an item on my todo list to take it for a test drive. Obviously, I have not come around yet to do it.

@spjmurray
Copy link
Author

Cool. If there's anything you want me to do in the mean time just ask e.g. unit tests, technical documentation, etc.

@mkjpryor
Copy link

mkjpryor commented Feb 3, 2023

This is where I eventually landed as a solution: argoproj/gitops-engine#503

I’m running this in a custom build now, and it works perfectly for my use case.

It is very similar to @spjmurray’s solution, except that it respects controller references at all times - the annotation only changes behaviour r.e. non-controller references. I think this is probably what we want as conflicting with another controller could be dangerous and cause a sync race/loop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants