fix: add missing labels to resources #2519

cwrau · 2024-01-10T09:23:54Z

What this PR does / why we need it:
Currently the chart doesn't contain the required labels on the applied resources, this makes it impossible to update the chart without manual intervention

Which issue this PR fixes(if applicable):

Special notes for reviewers:
I introduced the common chart from bitnami to standardly handle labels. The chart could also be used to handle resource names, secrets, matchlabels, ... . But I wanted to have this change be backwards-compatible and non-intrusive

Release note:

- adds missing labels to enable helm upgrades

k8s-ci-robot · 2024-01-10T09:24:03Z

Hi @cwrau. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

cwrau · 2024-01-10T09:24:40Z

Also, I'd like to backport this to at least 1.27 and up, do I still just open a PR with a cherry pick?

dulek · 2024-01-10T17:41:36Z

charts/openstack-cloud-controller-manager/templates/daemonset.yaml

-  labels:
-    {{- include "occm.labels" . | nindent 4 }}


Will this give use the same labels here?

Yes, this will result in the same labels

I'm looking at CI results and I only see this label added to the DaemonSet:

Labels: k8s-app=openstack-cloud-controller-manager

See for yourself in the "Describe failed openstack-cloud-controller-manager" step: https://storage.googleapis.com/kubernetes-jenkins/pr-logs/pull/cloud-provider-openstack/2519/openstack-cloud-controller-manager-e2e-test/1749846521752326144/build-log.txt

I don't know how CI comes up with that, but I would say it's technically impossible for this label to exist, neither in the old version nor my new one 😅

Okay, CI gets that from https://github.com/kubernetes/cloud-provider-openstack/blob/f7abe101ed041c8867d5fd1faa8351044f40a9cb/manifests/controller-manager/openstack-cloud-controller-manager-ds.yaml, so I guess we can disregard this.

dulek · 2024-01-10T17:44:47Z

charts/openstack-cloud-controller-manager/Chart.yaml

+dependencies:
+  - name: common
+    version: 2.14.1
+    repository: https://charts.bitnami.com/bitnami


I think I'd rather prefer to just copy this single function here.

Uff, I'm highly against copying code, that's the whole point of using a central, standard library for this; to remove code duplication.

Then you'd have to keep track of the upstream changes and copy them as well.

Sure, but how "standard" is this standard library? I see it's used extensively in charts around GitHub, but it still seems to be from a third party?

Maybe it's not a "standard" by definition, but as you said it's extremely widely used across helm charts and bitnami, even though they're a third party, has made themselves a recognizable helm chart writer and supporter known for their quality, at least in my knowledge circle.

charts/openstack-cloud-controller-manager/Chart.yaml

jichenjc · 2024-01-11T06:33:49Z

/ok-to-test

jichenjc · 2024-01-11T06:34:41Z

I introduced the common chart from bitnami to standardly handle labels. The chart could also be used to handle resource names, secrets, matchlabels, ... . But I wanted to have this change be backwards-compatible and non-intrusive

not sure ,is it a bug or label ? usually we only backport bugs

cwrau · 2024-01-11T07:57:59Z

I introduced the common chart from bitnami to standardly handle labels. The chart could also be used to handle resource names, secrets, matchlabels, ... . But I wanted to have this change be backwards-compatible and non-intrusive

not sure ,is it a bug or label ? usually we only backport bugs

It's both, the missing labels are a major bug. To be honest; I'm really surprised that noone noticed this before

dulek · 2024-01-11T16:13:58Z

If this is to be backported it'll need an issue reported.

jichenjc · 2024-01-17T06:11:06Z

charts/openstack-cloud-controller-manager/Chart.yaml

@@ -1,11 +1,15 @@
-apiVersion: v1
+apiVersion: v2


I think it's not something we want to change

This is needed for dependencies

And, why not? 🤔🤷

ok, I thought it's a dependency upgrade then no need to update the api version ..

jichenjc · 2024-01-17T06:11:21Z

charts/openstack-cloud-controller-manager/Chart.yaml

 appVersion: v1.28.0
 description: Openstack Cloud Controller Manager Helm Chart
 icon: https://object-storage-ca-ymq-1.vexxhost.net/swift/v1/6e4619c416ff4bd19e1c087f27a43eea/www-images-prod/openstack-logo/OpenStack-Logo-Vertical.png
 home: https://github.com/kubernetes/cloud-provider-openstack
 name: openstack-cloud-controller-manager
-version: 2.29.0-alpha.5
+version: 2.29.1-alpha.5


should this be .6 instead ?

otherwise upgrades fail with `resource already exists, add missing labels`

cwrau · 2024-01-22T11:14:17Z

charts/openstack-cloud-controller-manager/Chart.yaml

 appVersion: v1.29.0
 description: Openstack Cloud Controller Manager Helm Chart
 icon: https://object-storage-ca-ymq-1.vexxhost.net/swift/v1/6e4619c416ff4bd19e1c087f27a43eea/www-images-prod/openstack-logo/OpenStack-Logo-Vertical.png
 home: https://github.com/kubernetes/cloud-provider-openstack
 name: openstack-cloud-controller-manager
-version: 2.29.0
+version: 2.29.0-1


Is this correct?

I guess not, so maybe now? 2.29.1?

dulek · 2024-01-23T16:44:17Z

/test openstack-cloud-controller-manager-e2e-test
/test openstack-cloud-csi-cinder-e2e-test
/test openstack-cloud-csi-cinder-e2e-test

dulek · 2024-01-23T16:46:33Z

/test openstack-cloud-csi-manila-e2e-test

dulek · 2024-01-23T17:28:13Z

These CI errors look unrelated, I see them on #2316 too.

/retest

jichenjc · 2024-01-25T07:27:45Z

e2e report this error

Error from server (BadRequest): container "openstack-cloud-controller-manager" in pod "openstack-cloud-controller-manager-ghc27" is waiting to start: ImageInspectError

I googled this but seems the related report is pointing to image has some problem ,may need check more

dulek · 2024-01-25T10:06:05Z

e2e report this error
Error from server (BadRequest): container "openstack-cloud-controller-manager" in pod "openstack-cloud-controller-manager-ghc27" is waiting to start: ImageInspectError
I googled this but seems the related report is pointing to image has some problem ,may need check more

Yes, I'm on this.

dulek · 2024-01-30T09:01:38Z

/retest

dulek · 2024-02-08T16:57:40Z

/lgtm

@jichenjc, @kayrus, @zetaab, could you take a look here? I'm not that experienced with Helm to be confident to approve this myself.

jichenjc · 2024-02-19T03:15:04Z

/approve
/hold

also ok to me and leave unhold to someone might have further experience on helm than me :)

k8s-ci-robot · 2024-02-19T03:15:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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~~ [jichenjc]

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

zetaab · 2024-02-20T20:03:36Z

imo the change looks ok, the labels should stay as is

zetaab · 2024-02-20T20:03:42Z

/hold cancel

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 10, 2024

k8s-ci-robot requested review from dulek and kayrus January 10, 2024 09:24

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 10, 2024

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 10, 2024

dulek reviewed Jan 10, 2024

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 11, 2024

cwrau force-pushed the fix/missing-labels branch from 45b4a16 to 7a35e94 Compare January 11, 2024 10:04

jichenjc reviewed Jan 17, 2024

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2024

cwrau force-pushed the fix/missing-labels branch from 7a35e94 to 7ceb454 Compare January 22, 2024 11:11

fix: add missing labels to resources

5739f5c

otherwise upgrades fail with `resource already exists, add missing labels`

cwrau force-pushed the fix/missing-labels branch from 7ceb454 to 5739f5c Compare January 22, 2024 11:13

cwrau commented Jan 22, 2024

View reviewed changes

chore: bump version

ffe6abb

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2024

k8s-ci-robot assigned dulek Feb 8, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2024

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 19, 2024

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2024

k8s-ci-robot merged commit d9106b3 into kubernetes:master Feb 20, 2024
8 checks passed

This was referenced Jun 14, 2024

OCPBUGS-35462: rebase CPO master on upstream 1.30 openshift/cloud-provider-openstack#286

Closed

OCPBUGS-34137, OCPBUGS-34460, OCPBUGS-34113, OCPBUGS-35462: sync upstream on release-1.30/go1.22 openshift/cloud-provider-openstack#287

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add missing labels to resources #2519

fix: add missing labels to resources #2519

cwrau commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

cwrau commented Jan 10, 2024

dulek Jan 10, 2024

cwrau Jan 11, 2024

dulek Jan 24, 2024

cwrau Jan 24, 2024

dulek Feb 8, 2024

dulek Jan 10, 2024

cwrau Jan 11, 2024

dulek Jan 11, 2024

cwrau Jan 11, 2024

jichenjc commented Jan 11, 2024

jichenjc commented Jan 11, 2024

cwrau commented Jan 11, 2024 •

edited

Loading

dulek commented Jan 11, 2024

jichenjc Jan 17, 2024

cwrau Jan 17, 2024

jichenjc Jan 17, 2024

jichenjc Jan 17, 2024

cwrau Jan 22, 2024

cwrau Jan 22, 2024

dulek commented Jan 23, 2024

dulek commented Jan 23, 2024

dulek commented Jan 23, 2024

jichenjc commented Jan 25, 2024

dulek commented Jan 25, 2024

dulek commented Jan 30, 2024

dulek commented Feb 8, 2024

jichenjc commented Feb 19, 2024

k8s-ci-robot commented Feb 19, 2024

zetaab commented Feb 20, 2024

zetaab commented Feb 20, 2024

		@@ -1,11 +1,15 @@
		apiVersion: v1
		apiVersion: v2

fix: add missing labels to resources #2519

fix: add missing labels to resources #2519

Conversation

cwrau commented Jan 10, 2024

k8s-ci-robot commented Jan 10, 2024

cwrau commented Jan 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jichenjc commented Jan 11, 2024

jichenjc commented Jan 11, 2024

cwrau commented Jan 11, 2024 • edited Loading

dulek commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dulek commented Jan 23, 2024

dulek commented Jan 23, 2024

dulek commented Jan 23, 2024

jichenjc commented Jan 25, 2024

dulek commented Jan 25, 2024

dulek commented Jan 30, 2024

dulek commented Feb 8, 2024

jichenjc commented Feb 19, 2024

k8s-ci-robot commented Feb 19, 2024

zetaab commented Feb 20, 2024

zetaab commented Feb 20, 2024

cwrau commented Jan 11, 2024 •

edited

Loading