MON-3216: Add ownership labels to kube resources #1986

machine424 · 2023-06-01T09:12:36Z

Generalize the use of recommended "app.kubernetes.io/managed-by" and "app.kubernetes.io/part-of" labels on Kube resources created and managed by CMO and CVO.

See https://issues.redhat.com/browse/MON-3216 for the whys.

We chose this "inject at the end" approach as it requires less changes, but
we're aware that:

We need to label resources managed by CVO individually (see utils/add-labels.libsonnet)
Items of a XXXList (RoleList in role-specific-namespaces.yaml for example) are missed
Resources created by some CR are missed (as we don't tell their operators to add the labels)

A draft of the other approach (Add the labels explicitly to each resource) can be found at
machine424@5386735#diff-3f0ac462e0855b3c5693e93fd73febeec9422783681a4a15b2a82f9f40efec89, with this approcah we can get better coverage, we have better control
but it's harder to maintain and is more intrusive.

I added CHANGELOG entry for this change.
No user facing changes, so no entry in CHANGELOG was needed.

jsonnet/components/node-exporter.libsonnet

jsonnet/components/telemeter-client.libsonnet

machine424 · 2023-06-08T12:37:52Z

/retitle MON-3216: Add ownership labels to kube resources

openshift-ci-robot · 2023-06-08T12:43:22Z

@machine424: This pull request references MON-3216 which is a valid jira issue.

In response to this:

I added CHANGELOG entry for this change.

No user facing changes, so no entry in CHANGELOG was needed.

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.

machine424 · 2023-06-08T12:43:23Z

I'll be simulating an upgrade on a cluster, this shouldn't break controllers as I made sure to only ADD labels.

simonpasquier

I think that we can accomplish the same with less modifications to the jsonnet files and a 100% coverage:

8954acf

openshift-ci-robot · 2023-06-23T11:54:06Z

@machine424: This pull request references MON-3216 which is a valid jira issue.

In response to this:

Generalize the use of recommended "app.kubernetes.io/managed-by" and "app.kubernetes.io/part-of" labels on Kube resources created and managed by CMO and CVO.

See https://issues.redhat.com/browse/MON-3216 for the whys.

We chose this "inject at the end" approach as it requires less changes, but
we're aware that:

We need to label resources managed by CVO individually (see utils/add-labels.libsonnet)

Items of a XXXList (RoleList in role-specific-namespaces.yaml for example) are missed

Resources created by some CR are missed (as we don't tell their operators to add the labels)

A draft of the other approach (Add the labels explicitly to each resource) can be found at
machine424@5386735#diff-3f0ac462e0855b3c5693e93fd73febeec9422783681a4a15b2a82f9f40efec89, with this approcah we can get better coverage, we have better control
but it's harder to maintain and is more intrusive.

I added CHANGELOG entry for this change.

No user facing changes, so no entry in CHANGELOG was needed.

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.

simonpasquier · 2023-06-23T12:07:05Z

We need to label resources managed by CVO individually (see utils/add-labels.libsonnet)

sgtm

Items of a XXXList (RoleList in role-specific-namespaces.yaml for example) are missed

is it possible to extend jsonnet/utils/add-labels.libsonnet and support *List objects?

Resources created by some CR are missed (as we don't tell their operators to add the labels)

Are you talking about the resources managed by the Prometheus operator (e.g. Prometheus, Alertmanager & ThanosRuler)? We could pass the --labels argument to inject labels in the child resources.

machine424 · 2023-06-23T12:53:28Z

is it possible to extend jsonnet/utils/add-labels.libsonnet and support *List objects?

Yep, we do sth like that in add-annotations.libsonnet, but as I see it it'll make the util complicated, so maybe in another PR (or the next time someone asks for those resources)

Are you talking about the resources managed by the Prometheus operator (e.g. Prometheus, Alertmanager & ThanosRuler)? We could pass the --labels argument to inject labels in the child resources.

Yep, (and maybe others managed by other operators), it'd be great to have app.kubernetes.io/part-of: openshift-monitoring on them (normally they're passed via podMetadata.metadata) I didn't know about --labels, the labels setting will no longer be centralized in jsonnet + we'll have another "implicit" injection, but I'll add it.

simonpasquier · 2023-06-23T13:27:51Z

(or the next time someone asks for those resources)

I'm afraid it will be never then 😝

machine424 · 2023-06-23T14:01:37Z

or maybe in another PR ;) I'll be thinking about a simpler/cleaner solution. (now, I'll overdose on jsonnet)
(I added --labels to prom-op)

jsonnet/utils/add-labels.libsonnet

machine424 · 2023-06-27T06:53:36Z

/jira refresh

openshift-ci-robot · 2023-06-27T06:53:39Z

@machine424: This pull request references MON-3216 which is a valid jira issue.

In response to this:

/jira refresh

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.

jan--f

One nit, otherwise this looks good.

jan--f · 2023-07-17T11:08:12Z

jsonnet/utils/add-labels.libsonnet

+    else
+      'cluster-monitoring-operator',
+
+  addLabels(o, labels): {


Suggested change

addLabels(o, labels): {

addLabels(labels,o ): {

Nit but what do you think of swapping the arguments here. I think that would improve readability in the main file. Other wise we have

addLabels( ...32 lines..., $labelsToAdd)

and "app.kubernetes.io/part-of" labels on Kube resources created and managed by CMO and CVO. See https://issues.redhat.com/browse/MON-3216 for the whys. We chose this "inject at the end" approach as it requires less changes and is future-proof, but we're aware that: - We need to label resources managed by CVO individually (see utils/add-labels.libsonnet) - Items of a XXXList (RoleList in role-specific-namespaces.yaml for example) are missed - Resources created by some CR MAY be missed (as we don't tell their operators to add the labels) A draft of the other approach (Add the labels explicitly to each resource) can be found at 5386735#diff-3f0ac462e0855b3c5693e93fd73febeec9422783681a4a15b2a82f9f40efec89, with this approcah we can get better coverage, we have better control but it's harder to maintain and is more intrusive. Signed-off-by: Ayoub Mrini <amrini@redhat.com>

jan--f

/lgtm

openshift-ci · 2023-07-18T08:32:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jan--f, machine424

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:

~~OWNERS~~ [jan--f]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2023-07-18T10:57:08Z

@machine424: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

…-operator, during 4.13->4.14 upgrade The option --labels was set to the prometheus-operator in openshift#1986 to make it add the label "app.kubernetes.io/part-of: openshift-monitoring" to resources it creates. prometheus-operator adds the labels to some controllers matchLabels as well (to the prometheus statefulset e.g.) which makes the prometheus-operator recreate the statefulset as matchLabels is an immutable field. Recreating the statefulset leads to downtime as the prometheus-operator run the deletion in a foreground mode, check prometheus-operator/prometheus-operator#3875 for the whys. We consider that adding the ownership labels is not worth the downtime, and we'll be thinking of a way to avoid it.

…-operator, during 4.13->4.14 upgrade The option --labels was set to the prometheus-operator in openshift#1986 to make it add the label "app.kubernetes.io/part-of: openshift-monitoring" to resources it creates. prometheus-operator adds the labels to some controllers matchLabels as well (to the prometheus statefulset e.g.) which makes the prometheus-operator recreate the statefulset as matchLabels is an immutable field. Recreating the statefulset leads to downtime as the prometheus-operator run the deletion in a foreground mode, check prometheus-operator/prometheus-operator#3875 for the whys. We consider that adding the ownership labels is not worth the downtime, and we'll be thinking of a way to avoid it. Of course, this will lead to the recreation of these resources if openshift#1986 is alredy deployed.

machine424 marked this pull request as draft June 1, 2023 09:12

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 1, 2023

openshift-ci bot requested review from raptorsun and sthaha June 1, 2023 09:14

machine424 force-pushed the labels branch 2 times, most recently from 4a768b1 to 86e44d8 Compare June 8, 2023 10:02

machine424 commented Jun 8, 2023

View reviewed changes

jsonnet/components/node-exporter.libsonnet Outdated Show resolved Hide resolved

machine424 force-pushed the labels branch from 86e44d8 to 41bdbea Compare June 8, 2023 12:35

machine424 commented Jun 8, 2023

View reviewed changes

jsonnet/components/telemeter-client.libsonnet Outdated Show resolved Hide resolved

machine424 marked this pull request as ready for review June 8, 2023 12:38

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 8, 2023

openshift-ci bot requested a review from simonpasquier June 8, 2023 12:41

machine424 changed the title ~~Labels~~ MON-3216: Add ownership labels to kube resources Jun 8, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Jun 8, 2023

machine424 force-pushed the labels branch from 41bdbea to 5386735 Compare June 8, 2023 12:51

simonpasquier reviewed Jun 8, 2023

View reviewed changes

machine424 marked this pull request as draft June 12, 2023 07:37

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 12, 2023

jsonnet: add common labels to all resources

c7a4627

machine424 force-pushed the labels branch 3 times, most recently from 6106f35 to f8aaf5e Compare June 23, 2023 11:51

machine424 marked this pull request as ready for review June 23, 2023 11:55

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2023

openshift-ci bot requested a review from simonpasquier June 23, 2023 11:56

machine424 force-pushed the labels branch from f8aaf5e to 8da6912 Compare June 23, 2023 12:08

machine424 force-pushed the labels branch from 8da6912 to eb61f1e Compare June 23, 2023 13:56

simonpasquier reviewed Jun 23, 2023

View reviewed changes

jsonnet/utils/add-labels.libsonnet Outdated Show resolved Hide resolved

jsonnet/utils/add-labels.libsonnet Outdated Show resolved Hide resolved

machine424 force-pushed the labels branch from eb61f1e to d5e1434 Compare June 23, 2023 15:41

jan--f reviewed Jul 17, 2023

View reviewed changes

machine424 force-pushed the labels branch from d5e1434 to 6795f50 Compare July 18, 2023 08:10

jan--f approved these changes Jul 18, 2023

View reviewed changes

openshift-ci bot assigned jan--f Jul 18, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 18, 2023

openshift-merge-robot merged commit 4e8efd9 into openshift:master Jul 18, 2023
16 checks passed

machine424 mentioned this pull request Aug 7, 2023

OCPBUGS-17346: Avoid recreating some resources, created by prometheus-operator, during 4.13->4.14 upgrade #2066

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MON-3216: Add ownership labels to kube resources #1986

MON-3216: Add ownership labels to kube resources #1986

machine424 commented Jun 1, 2023 •

edited

machine424 commented Jun 8, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jun 8, 2023 •

edited by openshift-ci bot

machine424 commented Jun 8, 2023

simonpasquier left a comment

openshift-ci-robot commented Jun 23, 2023 •

edited by openshift-ci bot

simonpasquier commented Jun 23, 2023

machine424 commented Jun 23, 2023

simonpasquier commented Jun 23, 2023

machine424 commented Jun 23, 2023 •

edited

machine424 commented Jun 27, 2023

openshift-ci-robot commented Jun 27, 2023 •

edited by openshift-ci bot

jan--f left a comment

jan--f Jul 17, 2023

machine424 Jul 18, 2023

jan--f left a comment

openshift-ci bot commented Jul 18, 2023

openshift-ci bot commented Jul 18, 2023

MON-3216: Add ownership labels to kube resources #1986

MON-3216: Add ownership labels to kube resources #1986

Conversation

machine424 commented Jun 1, 2023 • edited

machine424 commented Jun 8, 2023 • edited by openshift-ci bot

openshift-ci-robot commented Jun 8, 2023 • edited by openshift-ci bot

machine424 commented Jun 8, 2023

simonpasquier left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Jun 23, 2023 • edited by openshift-ci bot

simonpasquier commented Jun 23, 2023

machine424 commented Jun 23, 2023

simonpasquier commented Jun 23, 2023

machine424 commented Jun 23, 2023 • edited

machine424 commented Jun 27, 2023

openshift-ci-robot commented Jun 27, 2023 • edited by openshift-ci bot

jan--f left a comment

Choose a reason for hiding this comment

jan--f Jul 17, 2023

Choose a reason for hiding this comment

machine424 Jul 18, 2023

Choose a reason for hiding this comment

jan--f left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jul 18, 2023

openshift-ci bot commented Jul 18, 2023

machine424 commented Jun 1, 2023 •

edited

machine424 commented Jun 8, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jun 8, 2023 •

edited by openshift-ci bot

openshift-ci-robot commented Jun 23, 2023 •

edited by openshift-ci bot

machine424 commented Jun 23, 2023 •

edited

openshift-ci-robot commented Jun 27, 2023 •

edited by openshift-ci bot