-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Cluster API should not use controller refs for resources that it does not create #4014
Comments
IIRC (although this was long ago) we've only used controller-type owner references where the lifecycle of that resource is managed by a Cluster API controller, while on the other resources we add the Cluster owner reference (or the parent) to make sure we have a way to tie things up together. Would you be able to give more details on what the error is? Owner references are also used for clusterctl move as of today, and it seems that semantically that's the right place to understand ownership. Regarding the Crossplane integration, I'm curious to understand how that would work 😄 given that we've had some similar ideas in mind for the infrastructure providers, is that something you could share? |
@vincepri I can give some more insight here, and would also love to attend a community meeting to present some ideas / demos. CAPI and Crossplane can interact at two main levels:
The I think a broader discussion about how CAPI and Crossplane can integrate would be beneficial here, and I am more than happy to create some content that can be presented. In many ways, the components of CAPI are similar to many Crossplane providers:
|
/milestone v0.4.0 |
@hasheddan we are trying to close API changes for the v1alpha4 version. /priority important-soon |
@fabriziopandini yes, there has been significant work around using crossplane as a drop in replacement for types in cluster-api-provider-gcp. The work is being tracked in crossplane-contrib/provider-gcp#303 and most of it has taken place on livestreams that have been recorded. I would love to give a demo to CAPI community soon (potentially next week) -- what would you recommend as best forum for that? Note that I believe that any crossplane integration will not need to block /cc @vincepri |
I think for now we can delay a change in references until we find a suitable approach that works across all the proposed integrations, for now we can possibly use what's in #4135 to have a crossplane integration /milestone Next |
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
Stale issues rot after 30d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-contributor-experience at kubernetes/community. |
The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs. This bot triages issues and PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /close |
@k8s-triage-robot: Closing this issue. In response to this:
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. |
/reopen i'd like to reopen this issue as originally titled; it had deviated into a discussion about a crossplane integration but i think it's more general. above @vincepri wrote:
from looking at the code i think this is almost correct (granted it was a few years ago!) except for one thing: when adding the Cluster owner reference to it is actually a controller type ref that's being added. see here: https://github.com/kubernetes-sigs/cluster-api/blob/main/internal/controllers/cluster/cluster_controller_phases.go#L95-L98 - this is when reconciling objects such as i would like to see cluster-api only create controller refs for resources it creates (e.g. the AWS provider creating machines from a machine deployment), but use only owner refs for everything else where it needs to tie "things up together". my use case, for context, is very similar to the OP except instead of crossplane it's metacontroller. it's best-practice to take a controller ref of resources your controller creates and that's what metacontroller and crossplane are doing. in my case i am creating the following resources from a metacontroller operator: cluster, aws cluster, machine template, machine deployment, k3s controlplane, k3s config. metacontroller takes a controller ref which means that capi can't take one and the AWS provider won't start the infra work. i forked metacontroller to make setting controller: true in the ownerReference configurable and it works, but i think the problem is actually cluster-api side and i don't expect them to take that PR. |
@lukebond: You can't reopen an issue/PR unless you authored it or you are a collaborator. In response to this:
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. |
/reopen |
@CecileRobertMichon: Reopened this issue. In response to this:
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. |
/remove-lifecycle rotten |
Sounds reasonable to me at a first glance. I think we would have to differentiate between "standalone" clusters and clusters using clusterclass. With clusterclass the CAPI controller is actually creating infra cluster, control plane and MachineDeployments as well |
cc @killianmuldoon given recent work on ownerRefs |
Revisiting this seems reasonable to me. IIRC The rationale behind using controller: true originally was mainly to prevent multiple Clusters from fighting over the "descendent" resources e.g MachineDep. Also pending having better docs #5487 |
Just another data point on this; the code base is full of assumptions about OwnerReferences to be in place, and unfortunately, there is not good documentation linking owners with those assumptions (AFAIK the only exception is https://cluster-api.sigs.k8s.io/clusterctl/provider-contract.html#ownerreferences-chain that explicitly documents why it is relying on this info). As a consequence changing owner reference should not be taken lightly, because it can lead to unexpected problems all around. We should figure out how to prevent regressions and how to communicate the change to all the Cluster API consumers that could have similar assumptions on their code |
Agree. I think we first should "lock down" and document the current behavior (which is what Killian was and is working on, e.g. also with: #7606, not sure if the ownerRef being a controller ref or not is part of that) Then we can evolve it from there. |
/triage accepted |
/assign I'll come back to this, organise documentation and see if there's places where we can move controller-references to non-exclusive OwnerRefs |
We've now merged documentation and tests to cover the current state of ownerReferences: #9153 I probably don't have time to follow up on the second part of this issue - i.e. deciding which ownerReferences should remain controllers and which do not, so unassigning myself for now. /unassign |
/help |
@killianmuldoon: GuidelinesPlease ensure that the issue body includes answers to the following questions:
For more details on the requirements of such an issue, please see here and ensure that they are met. If this request no longer meets these requirements, the label can be removed In response to this:
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. |
What steps did you take and what happened:
Attempting to create a Cluster using an external tool such as Crossplane causes issues with both CAPI and Crossplane wanting to have the singular controller reference for resources that are created from Crossplane.
We've used controller refs in the past to avoid the possibility of multiple Clusters owning the same resources, but siince this hampers interoperability, the use of non-controller ownerReferences should be used and the verification that a resource isn't "owned" by the same cluster should be handled separately.
What did you expect to happen:
Crossplane should be able to create a Cluster API Cluster
Anything else you would like to add:
Anywhere that Cluster API controllers are creating resources should still continue to use controller references.
/kind bug
The text was updated successfully, but these errors were encountered: