Controller-runtime: EnqueueRequestForOwner creates reconcile.Request with wrong data

[vasrem]

I might be wrong but I find the following behaviour strange.

Lets define the following:

• Object1 -> Is created during the reconcile of Object2
• Object2 -> Owner of Object1

I'm watching for Object2 using EnqueueRequestForObject and Object1 with EnqueueRequestForOwner. Object1 is created during the reconcile of Object2. There is a reconcile function for objects of type Object2.

In this line it is defined that the Namespace of the Object that should be reconciled is the Namespace of Object1. This means that if the Object2 is in a different Namespace than Object1 or Object1 has no Namespace at all (Object1 Type is i.e. Namespace) the Reconciler will not find the Object2.

In my case, I'm building an operator that will create Namespaces. If I delete the namespace, then the reconciler cannot find the CR which is the owner of the Namespace and the Namespace will never be recreated.

I could tweak the code to check for the CR in a specific Namespace if the reconcile.Request doesn't contain any Namespace but I would like to know if the behaviour that I'm facing is the correct one.

[DirectXMan12]

/kind bug

So, we definitely need to handle non-namespaced objects, and namespaces themselves (we can use discovery info for that)

[DirectXMan12]

/priority important-soon

[ironcladlou]

Owner references in Kubernetes are:

1. Namespaced
2. Cannot cross namespaces

This is an intentional policy decision upstream. There's some prior conversation about it in the context of garbage collection.

Bottom line is I think EnqueueRequestForOwner is behaving as expected since, per the upstream design, it's safe/necessary to assume that the owner and owned object are both namespace-scoped and in the same namespace.

[grantr]

Intentional policy decisions notwithstanding, didn't https://github.com/kubernetes/kubernetes/pull/57211 add support for garbage collecting dependents of cluster-scoped objects? That seems like practical acknowledgment that the cluster-scoped owner scenario is valid, or at least common enough that it should be supported.

[ironcladlou]

@grantr

Intentional policy decisions notwithstanding, didn't kubernetes/kubernetes#57211 add support for garbage collecting dependents of cluster-scoped objects?

I think there are two orthogonal things to consider:

1. The Kubernetes OwnerReference primitive on which other components are layered, e.g. the garbage collector.

OwnerReference is versioned API, and its semantics are clearly documented:

OwnerReference contains enough information to let you identify an owning object. Currently, an owning object must be in the same namespace, so there is no namespace field.

1. The Kubernetes garbage collector, which implements a GC design^1,2 based on OwnerReference — the OwnerReference type was actually introduced to support this GC design.

It's possible to get the garbage collector to process cluster scoped resources because of behavior that's considered buggy. However, the semantics of an OwnerReference involving a cluster scoped resource is (by current API definition) undefined.

The garbage collector's behavior around OwnerReferences involving a cluster scoped resource is, by extension, undefined. Today the garbage collector happens to deal with these invalid relationships in a potentially useful way, but I wouldn't recommend any production system rely on that behavior, which is subject to change. Only OwnerReference has versioning guarantees.

@grantr

That seems like practical acknowledgment that the cluster-scoped owner scenario is valid, or at least common enough that it should be supported.

I think this attributes a motive to people who made the GC changes, but as one of those people, I think I can say it wasn't because of either widespread demand or a desire to promote the behavior.

That the garbage collector currently does something useful with undefined OwnerReferences doesn't seem like a strong reason to propagate the same undefined behavior to a generic SDK which operates on the same OwnerReference primitive.

I think the only way it makes sense to propagate the garbage collector's behavior around cluster scoped relationships is to change OwnerReference so that these relationships are actually specified in the versioned API. That would validate the garbage collector's behavior and impose new requirements on higher level abstractions (like controller-runtime) to conform to new semantics.

[liggitt]

However, the semantics of an OwnerReference involving a cluster scoped resource is (by current API definition) undefined.

ownerRefs to cluster-scoped objects are allowed:

https://github.com/kubernetes/kubernetes/blob/6e5176b48df8719ab969a233a0411ad6ba35d222/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L288-L290

ownerRefs from cluster-scoped objects to namespaced objects are not supported.

[ironcladlou]

@liggitt @caesarxuchao

Oh, hi there! 😀 👋

ownerRefs to cluster-scoped objects are allowed:

https://github.com/kubernetes/kubernetes/blob/6e5176b48df8719ab969a233a0411ad6ba35d222/staging/src/k8s.io/apimachinery/pkg/apis/meta/v1/types.go#L288-L290

ownerRefs from cluster-scoped objects to namespaced objects are not supported.

Hey, that's new! How does Kubernetes versioning apply with a purely semantic change like this?

All this said, I think the original request in this PR was about inter-namespace dependencies, which still aren't supported (right?)

[vasrem]

@ironcladlou

I think the original request in this PR was about inter-namespace dependencies

Yes! The request to "fix" the behaviour would be:

• Inter-namespace dependencies
• Cluster-scoped objects to be owned by namespaced objects

[ironcladlou]

@vasrem

I think the original request in this PR was about inter-namespace dependencies
Yes! The request to "fix" the behaviour would be:

Inter-namespace dependencies
Cluster-scoped objects to be owned by namespaced objects

I think my earlier diatribe accidentally applies more clearly to your request — those two types of relationships are explicitly disallowed and so I still think the change would have to originate with OwnerReference itself (which doesn't even have a Namespace field to express such relationships). The runtime simply doesn't have the information to do what you want.

[vasrem]

@ironcladlou

The runtime simply doesn't have the information to do what you want.

That's true. I was just curious about the behaviour of the EnqueueRequestForOwner function. If it is correct or not. I faced this behaviour because I didn't follow the internal policy.

Do you know if there are plans to implement the Namespace field in the OwnerReference definition? I can understand that, among others, this will add much more complexity since the inter-namespace dependencies will weaken the "isolation" which the Namespaces offer and force the devs to be more cautious with the dependencies that they will create.

[grantr]

All this said, I think the original request in this PR was about inter-namespace dependencies, which still aren't supported (right?)

To avoid hijacking this thread with a different request, I opened #228 for the specific case of cluster-scoped owners.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[DirectXMan12]

I think this can be closed -- the sub-issue was fixed, and the other issue here is not possible with owner refs.

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.