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

Document CRs ownership #5487

Closed
enxebre opened this issue Oct 25, 2021 · 19 comments · Fixed by #9150
Closed

Document CRs ownership #5487

enxebre opened this issue Oct 25, 2021 · 19 comments · Fixed by #9150
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@enxebre
Copy link
Member

enxebre commented Oct 25, 2021

User Story

As a cluster operator/developer I would like comprehensive documentation for the CRs ownership relationship. This comes in particularly handy as the number of CRDs of the project keeps growing.

Detailed Description

In order to make it easier for devs to quickly understand CAPI internals, we could include a section with some CRs ownership diagram and implications in the developer guide https://cluster-api.sigs.k8s.io/developer/architecture/controllers.html

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind feature
/kind documentation
/help
/good-first-issue

@k8s-ci-robot
Copy link
Contributor

@enxebre:
This request has been marked as suitable for new contributors.

Guidelines

Please ensure that the issue body includes answers to the following questions:

  • Why are we solving this issue?
  • To address this issue, are there any code changes? If there are code changes, what needs to be done in the code and what places can the assignee treat as reference points?
  • Does this issue have zero to low barrier of entry?
  • How can the assignee reach out to you for help?

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
by commenting with the /remove-good-first-issue command.

In response to this:

User Story

As a cluster operator/developer I would like comprehensive documentation for the CRs ownership relationship. This comes in particularly handy as the number of CRDs of the project keeps growing.

Detailed Description

In order to make it easier for devs to quickly understand CAPI internals, we could include a section with some CRs ownership diagram and implications in the developer guide https://cluster-api.sigs.k8s.io/developer/architecture/controllers.html

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

/kind feature
/kind documentation
/help
/good-first-issue

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.

@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/documentation Categorizes issue or PR as related to documentation. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Oct 25, 2021
@codablock
Copy link
Contributor

Is there any generic k8s documentation available that documents when and who is recommended to take ownership of already existing objects? The only thing I can find is https://kubernetes.io/docs/concepts/overview/working-with-objects/owners-dependents/ and it does not explicitly document this.

My understanding so far was that ownership is taken for objects that are created as a result of another object, for example when a controller creates PODs from a POD template in Deployment/ReplicaSet. This means, Deployments create and own ReplicaSets, ReplicaSets create and own PODs and so on. When I started integrating CAPI into the tooling that I use/develop, I assumed that ownership is handled in a comparable way with MachineDeployments, meaning that we get a chain of MachineDeployment -> MachineSet -> Machine -> AWSMachine (for AWS as an example).

I assumed that MachineDeployment would NOT get claimed by Cluster, as it was clearly not created by any CAPI controller as a result of of the cluster spec. It's a different story when it comes to cluster topologies, where it would be clear that MachineDeployments created from the machineDeployments templates found in the ClusterClass are owned by the cluster.

The reason why this is important for me to understand is, that I'm partly basing the pruning strategy in kluctl (a deployment tool that I work on) on object ownership. kluctl does not try to prune objects that have an owner, because it must assume that it did NOT create that object by itself (even if labels match, as some controllers copy labels from source->dependent objects). This result in MachineDeployments never being pruned even if I remove them from my deployment project. If my understanding of ownership is wrong, I need to reconsider how to handle pruning.

I also assume that other tools (e.g. ArgoCD) might run into the same issue.

@enxebre
Copy link
Member Author

enxebre commented Oct 25, 2021

Related discussion #4014

@sbueringer
Copy link
Member

@codablock I think the cluster deletion code could also be relevant for you:

func (r *ClusterReconciler) reconcileDelete(ctx context.Context, cluster *clusterv1.Cluster) (reconcile.Result, error) {

@codablock
Copy link
Contributor

Thanks @enxebre. The linked discussion basically describes the same issue. I wonder why ownership has to be taken in the first place and if the underlying requirement can't be simply solved by labels and/or annotations that CAPI attaches to the objects? Thanks to server-side-apply and managed fields, this is something that all tools should understand in the future (mine already works with managed fields and would respect it.

@sbueringer Yeah I've noticed that cluster deletion also leads to MD deletion, so I already assumed that cleanup is one of the requirements that are solved via owner references. This however, could also be solved with the help of labels/annotations (in addition to owner reference based deletion, because the topology case would/should still set owner references)

@sbueringer
Copy link
Member

MachineSet and Machine deletion is also triggered via ownerRefs.

@codablock
Copy link
Contributor

@sbueringer Which is something that imho is expected and fine, because these objects are created by the controller, on behalf of the parent objects (MachineDeployment/MachineSet). The MachineDeployment is however not created on behalf of the Cluster object, so it should not become owned by the Cluster object. The only exception is when MachineDeployments are created via cluster topologies, where the cluster object defines MachineDeployments indirectly via topology.

@enxebre
Copy link
Member Author

enxebre commented Oct 25, 2021

More relevant discussion #5483

@enxebre
Copy link
Member Author

enxebre commented Nov 8, 2021

/milestone v1.1

@k8s-ci-robot
Copy link
Contributor

@enxebre: You must be a member of the kubernetes-sigs/cluster-api-maintainers GitHub team to set the milestone. If you believe you should be able to issue the /milestone command, please contact your Cluster API Maintainers and have them propose you as an additional delegate for this responsibility.

In response to this:

/milestone v1.1

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.

@fabriziopandini
Copy link
Member

/milestone v1.1

NOTE: this issue originally was about documenting current owner chain, which is something useful per se.

I would suggest to move discussion about changing current owner chain on another issue; mostly because CAPI relies heavily on it for deletion and move on top of my mind but most probably in other places as well, and this requires proper investigation.

@k8s-ci-robot k8s-ci-robot added this to the v1.1 milestone Nov 8, 2021
@fabriziopandini fabriziopandini modified the milestones: v1.1, v1.2 Feb 3, 2022
@Jont828
Copy link
Contributor

Jont828 commented Feb 25, 2022

@enxebre Did you have any ideas in mind for how to document ownership of cluster resources? This is a bit off topic, but I've recently worked on a CAPI GUI app that visualizes resources. It currently does not show the ownership relations, but what do you think about using a graph or tree to document resource ownership?

@fabriziopandini
Copy link
Member

FYI there is already a kubctl tree plugin showing document ownership

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label May 29, 2022
@fabriziopandini
Copy link
Member

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 31, 2022
@fabriziopandini fabriziopandini added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 29, 2022
@fabriziopandini fabriziopandini removed this from the v1.2 milestone Jul 29, 2022
@fabriziopandini fabriziopandini removed the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Jul 29, 2022
@fabriziopandini
Copy link
Member

/triage accepted

@k8s-ci-robot k8s-ci-robot added the triage/accepted Indicates an issue or PR is ready to be actively worked on. label Sep 30, 2022
@ashutosh887
Copy link

@enxebre @codablock I would like to work on this Issue
/assign

@sbueringer
Copy link
Member

sbueringer commented Mar 20, 2023

cc @killianmuldoon
IIRC we have a way to automatically generate a dot file with a lot of the ownership information based on ownerRefs. This could probably be leveraged for this issue.

@killianmuldoon
Copy link
Contributor

/assign

I'll take a go at this.

Linked to #4014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/documentation Categorizes issue or PR as related to documentation. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants