-
Notifications
You must be signed in to change notification settings - Fork 265
Adding a propsal for changing the KRO instance and resource labels #639
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
base: main
Are you sure you want to change the base?
Conversation
jakobmoellerdev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly LGTM. Just thinking we should really start unifying everything and not keep the old labels.
fabianburth
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
b2f9271 to
c6397ab
Compare
- Based on proposal: kubernetes-sigs#639 - Move to new labels and remove old labels - Add new testcase for nested RGD and update existing testcase labels - Have separate instance labellers for different reconcile paths. This is needed when we have RGD2 instance in RGD1 resources. Today both paths use the same label resulting in conflict. path 1: RGD2.instance created as part of an RGD1 instance reconciliation path 2: RGD2.instance reconciled by RGD2 reconciler
- Based on proposal: kubernetes-sigs#639 - Move to new labels and remove old labels - Add new testcase for nested RGD and update existing testcase labels - Have separate instance labellers for different reconcile paths. This is needed when we have RGD2 instance in RGD1 resources. Today both paths use the same label resulting in conflict. path 1: RGD2.instance created as part of an RGD1 instance reconciliation path 2: RGD2.instance reconciled by RGD2 reconciler
- Based on proposal: kubernetes-sigs#639 - Move to new labels - Add deprecate TODO for old labels - Add new testcase for nested RGD and update existing testcase labels - Have separate instance labellers for different reconcile paths. This is needed when we have RGD2 instance in RGD1 resources. Today both paths use the same label resulting in conflict. path 1: RGD2.instance created as part of an RGD1 instance reconciliation path 2: RGD2.instance reconciled by RGD2 reconciler
- Based on proposal: kubernetes-sigs#639 - Move to new labels - Add deprecate TODO for old labels - Add new testcase for nested RGD and update existing testcase labels - Have separate instance labellers for different reconcile paths. This is needed when we have RGD2 instance in RGD1 resources. Today both paths use the same label resulting in conflict. path 1: RGD2.instance created as part of an RGD1 instance reconciliation path 2: RGD2.instance reconciled by RGD2 reconciler
- Based on proposal: kubernetes-sigs#639 - Move to new labels - Add deprecate TODO for old labels - Add new testcase for nested RGD and update existing testcase labels - Have separate instance labellers for different reconcile paths. This is needed when we have RGD2 instance in RGD1 resources. Today both paths use the same label resulting in conflict. path 1: RGD2.instance created as part of an RGD1 instance reconciliation path 2: RGD2.instance reconciled by RGD2 reconciler
- Based on proposal: kubernetes-sigs#639 - Move to new labels - Add deprecate TODO for old labels - Add new testcase for nested RGD and update existing testcase labels - Add docs - Have separate instance labellers for different reconcile paths. This is needed when we have RGD2 instance in RGD1 resources. Today both paths use the same label resulting in conflict. path 1: RGD2.instance created as part of an RGD1 instance reconciliation path 2: RGD2.instance reconciled by RGD2 reconciler
docs/design/proposals/labels.md
Outdated
| - **Purpose:** To identify which RGD was used as a template to create a resource during the reconciliation of an instance. This label will be applied to all resources created by the instance controller. | ||
| - **Alternative Names:** | ||
| - kro.run/created-by-rgd | ||
| - kro.run/managed-by-rgd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think managed-by probably fits best since we continuously work on it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if we have kro.run/managed-by-rgd it could be reused for the instance too. Is there a specific need to have two labels here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah thats the whole point of the proposal.
I guess the problem statement did not do a good job. Let me know how i can update the problem statement to make the need for 2 labels more apparent.
docs/design/proposals/labels.md
Outdated
| - kro.run/instance-namespace: default | ||
| - **Proposed Labels:** | ||
| - `kro.run/managed-by-instance-group`: The API group of the instance (e.g., `mygroup.example.com`). | ||
| - `kro.run/managed-by-instance-kind`: The kind of the instance (e.g., `MyKind`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you define a group+kind, don't you also need to define a version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
kro.run/instance: production/my-web-app # namespace/name format
kro.run/instance-gvk: myapp.io/v1/WebService # optional for additional context ?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another approach would be to just include an RGD ref?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you mean by RGD Ref ? in the ownerrefs ?
|
Unknown CLA label state. Rechecking for CLA labels. Send feedback to sig-contributor-experience at kubernetes/community. /check-cla |
a-hilaly
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting this together @barney-s ! you've identified an important issue - the current labeling scheme is definitely confusing with both resource-graph-definition-id and resource-graph-definition-name without clear semantics.
docs/design/proposals/labels.md
Outdated
| - kro.run/instance-namespace: default | ||
| - **Proposed Labels:** | ||
| - `kro.run/managed-by-instance-group`: The API group of the instance (e.g., `mygroup.example.com`). | ||
| - `kro.run/managed-by-instance-kind`: The kind of the instance (e.g., `MyKind`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about:
kro.run/instance: production/my-web-app # namespace/name format
kro.run/instance-gvk: myapp.io/v1/WebService # optional for additional context ?
docs/design/proposals/labels.md
Outdated
| - kro.run/instance-namespace: default | ||
| - **Proposed Labels:** | ||
| - `kro.run/managed-by-instance-group`: The API group of the instance (e.g., `mygroup.example.com`). | ||
| - `kro.run/managed-by-instance-kind`: The kind of the instance (e.g., `MyKind`). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another approach would be to just include an RGD ref?
- Based on proposal: kubernetes-sigs#639 - Move to new labels - Add deprecate TODO for old labels - Add new testcase for nested RGD and update existing testcase labels - Add docs - Have separate instance labellers for different reconcile paths. This is needed when we have RGD2 instance in RGD1 resources. Today both paths use the same label resulting in conflict. path 1: RGD2.instance created as part of an RGD1 instance reconciliation path 2: RGD2.instance reconciled by RGD2 reconciler
- Based on proposal: kubernetes-sigs#639 - Move to new labels - Add deprecate TODO for old labels - Add new testcase for nested RGD and update existing testcase labels - Add docs - Have separate instance labellers for different reconcile paths. This is needed when we have RGD2 instance in RGD1 resources. Today both paths use the same label resulting in conflict. path 1: RGD2.instance created as part of an RGD1 instance reconciliation path 2: RGD2.instance reconciled by RGD2 reconciler
c6397ab to
08244bf
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: barney-s The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
08244bf to
3a20f8c
Compare
3a20f8c to
4ae5396
Compare
4ae5396 to
16f7be0
Compare
|
@jakobmoellerdev @a-hilaly @matthchr - Please Take a look at this updated proposal. |
95048ee to
9c1b2a3
Compare
9c1b2a3 to
642aef4
Compare
jakobmoellerdev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me this is looking good to go. Thanks a lot for the investment @barney-s !
|
|
||
| ## Problem statement | ||
|
|
||
| The current labels in KRO are used to track ownership and metadata of resources managed by the orchestrator. While these labels provide essential information, they lack a clear and standardized way to represent the relationships between different KRO components, such as ResourceGraphDefinitions (RGDs), instances, and the resources they generate. This ambiguity can make it difficult to query for related objects and understand the provenance of resources within a cluster. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be interesting to remind about the current set of annotations/labels to better understand the problem and how it gets fixed
| - `kro.run/instance-gvk: mygroup.example.com/v1/MyKind`: The Group Version Kind (GVK) of the instance | ||
| - `kro.run/instance: default/my-instance`: The namespace and name of the instance. | ||
| - `app.kubernetes.io/instance: MyKind/default/my-instance`: Kind namespace and name of the instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Labels are extremely useful to select resources kubectl -n my-namespace get X -l my=label and identify them in a human readable manner.
Given that we refer to 2 owning objects, would it make sense to rather use dns notation:
instance.kro.run/apiVersion: mygroup.example.com/v1
instance.kro.run/kind: MyKind
instance.kro.run/name: my-instance
instance.kro.run/namespace: my-namespace
rgd.kro.run/name: some-rgd
rgd.kro.run/apiVersion: kro.run/v1alpha1
app.kubernetes.io/instance: MyKind/default/my-instance
that way we can easily perform different queries kubectl -n my-namespace get X -l instance.kro.run/kind to get all namespaced resources deployed by a certain kind, kubectl -n my-namespace get -A X -l instance.kro.run/name=my-instance
| - To identify which RGD was used as a template to create a resource during the reconciliation of an instance. This label will be applied to all resources created by the instance controller. | ||
| - **Proposed Labels:** | ||
| - `kro.run/part-of: my-rgd-name` - The name of the ResourceGraphDefinition | ||
| - `app.kubernetes.io/part-of: my-rgd-name` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need both?
| - Currently we dont differentiate, it results in a conflict (https://github.com/kro-run/kro/pull/631) | ||
| - To indicate that an instance is being actively reconciled against a specific RGD. This label will be applied to the instance itself. | ||
| - **Proposed Labels:** | ||
| - `kro.run/managed-by-rgd: my-rgd-name` - name of the ResourceGraphDefinition that reconciles the instance. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about
instance.kro.run/managed-by: my-rgd-name
Could it help reveal the hierarchy for humans?
| ## Other solutions considered | ||
|
|
||
| 1. We could continue using the existing labels, but their purpose is not as explicit for relationship tracking, which can lead to confusion for users and client tools. | ||
| 2. We could also use annotations, but labels are better suited for this purpose as they are queryable via the Kubernetes API, which is a key requirement for observability and tooling. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest 1 or 2 annotations for programmatic access that include IDs
for example
instance.kro.run/reference: {"apiVersion": "kro.run/v1alpha1", "kind": "MyKind", "resource": "mykinds", "namespace": "default", "name": "my-instance", "id": "$id"}
rgd.kro.run/reference: {"apiVersion": "kro.run/v1", "name": "my-rgd", "id": "${id}"}
ID would help reconcilers detect that the instance (or RGD) was fully replaced between 2 reconciliation loops
| - Updating the instance controller to apply these labels to instances and created resources during reconciliation. | ||
| - Updating the official KRO documentation to reflect the new labels and their usage. | ||
| - Adding docs reflecting the new labels. Old labels would not be documented. | ||
| - Deprecate old labels in next minor release or the one after that. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't 1-2 versions a bit short?
Are we confident most users will upgrade to migrate all their labels?
| #### Test plan | ||
|
|
||
| - **Unit tests:** Add unit tests for any new labeler functions in `pkg/metadata/labels.go` to ensure they construct the correct labels. | ||
| - **Integration tests:** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we may want a migration test, to ensure we don't mess up deployed objects with the new version.
We may also want to ensure that in case users jumps directly from the current labels layout to the proposed one (i.e. skip the transition versions) kro keeps running as expected
tjamet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall looks good. i left some comments for discussion
|
@barney-s: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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-sigs/prow repository. I understand the commands that are listed here. |
Related PR that triggered this doc: #631