[Charts] Add logVerbosityLevel variable to all Helm charts #1679

gman0 · 2021-10-29T08:36:31Z

What this PR does / why we need it:

This PR adds logVerbosityLevel variable to all Helm charts in this repository. Operators can now choose how verbose they want their logs. Default levels are set to 2.

Which issue this PR fixes(if applicable):
fixes #

Special notes for reviewers:

Release note:

[Charts] Added logVerbosityLevel variable to cinder-csi-plugin, manila-csi-plugin and openstack-controller-manager Helm charts

gman0 · 2021-10-29T08:39:20Z

 ✖︎ openstack-cinder-csi => (version: "2.0.0", path: "charts/cinder-csi-plugin") > Chart version not ok. Needs a version bump!
34
 ✖︎ openstack-manila-csi => (version: "1.3.2", path: "charts/manila-csi-plugin") > Chart version not ok. Needs a version bump!
35
 ✖︎ openstack-cloud-controller-manager => (version: "1.1.1", path: "charts/openstack-cloud-controller-manager") > Chart version not ok. Needs a version bump!

#1654 ?

gman0 · 2021-10-29T08:39:52Z

PTAL @lingxiankong @jichenjc @ramineni

jichenjc · 2021-11-01T03:28:31Z

not sure the lint issue is related?

gman0 · 2021-11-01T17:05:49Z

@jichenjc it is, no? I haven't incremented chart versions, and it's complaining about it.

jichenjc · 2021-11-02T03:31:31Z

@gman0 yes,so please update the version :)

gman0 · 2021-11-02T09:09:50Z

@jichenjc ok I thought it was a pipeline bug that it was still asking for version bump.

gman0 · 2021-11-02T09:13:42Z

Should be ok now. All jobs passed.

gman0 · 2021-11-02T09:16:22Z

I've updated the release note so that it mentions new variable in helm chart values.

jichenjc · 2021-11-02T09:16:25Z

thanks ~
/lgtm

gman0 · 2021-11-08T08:51:01Z

Any updates on this pls? @ramineni ?

ramineni · 2021-11-08T09:45:50Z

charts/cinder-csi-plugin/values.yaml

+# Log verbosity level.
+# See https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md
+# for description of individual verbosity levels.
+logVerbosityLevel: 0


if nothing is passed in argument , isnt default log level would be 2?

@ramineni do you have some reference for this where it says it really defaults to 2? If not I'll go check it myself later today or in the coming days.

Because AFAIK it indeed defaults to zero:

-v=0
Enable V-leveled logging at the specified level.

https://pkg.go.dev/github.com/kubernetes/klog

But that's just what I implied when I saw -v=0. I couldn't find anything explicitly stating what's the default level though, so that's why I'm asking if you saw it somewhere else.

Some more references:

Whether an individual call to V generates a log record depends on the setting of the -v and --vmodule flags; both are off by default.

https://pkg.go.dev/github.com/kubernetes/klog#V

@gman0 Its there in same link you have mentioned in comment above :)

If that's the case, I think it's sensible to set all logVerbosityLevel values to 2. What do you and others think?

Yeah, I agree

@ramineni I think https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md just summarizes the recommendations rather than actual defaults that klog is configured with. Either way I'll set the level to 2 as we just agreed.

ramineni · 2021-11-08T09:49:57Z

@gman0 @jichenjc Yes, according to this ,#1654 , we have decided not to bump version as it causes confusion . We need to fix that not to cause error if there is no version bump.
/cc @lingxiankong

lingxiankong · 2021-11-09T10:20:56Z

@gman0 @jichenjc Yes, according to this ,#1654 , we have decided not to bump version as it causes confusion . We need to fix that not to cause error if there is no version bump. /cc @lingxiankong

This is weird, according to https://github.com/helm/chart-testing/blob/main/doc/ct_lint.md, the job shouldn't check chart version.

ramineni · 2021-11-09T10:38:33Z

/override openstack-cloud-keystone-authentication-authorization-test

k8s-ci-robot · 2021-11-09T10:38:35Z

@ramineni: Overrode contexts on behalf of ramineni: openstack-cloud-keystone-authentication-authorization-test

In response to this:

/override openstack-cloud-keystone-authentication-authorization-test

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.

lingxiankong · 2021-11-09T10:56:24Z

Chart version check fix: #1691

@gman0 Please remove the version change.

gman0 · 2021-11-09T11:18:27Z

Will do, @lingxiankong. Maybe we can get your fix merged and then I'll rebase this PR?

ramineni · 2021-11-09T11:35:07Z

Will do, @lingxiankong. Maybe we can get your fix merged and then I'll rebase this PR?

its merged

gman0 · 2021-11-09T12:07:36Z

All CI jobs passed. PTAL again.

ramineni · 2021-11-09T12:13:43Z

charts/cinder-csi-plugin/templates/controllerplugin-deployment.yaml

@@ -27,6 +27,7 @@ spec:
          image: "{{ .Values.csi.attacher.image.repository }}:{{ .Values.csi.attacher.image.tag }}"
          imagePullPolicy: {{ .Values.csi.attacher.image.pullPolicy }}
          args:
+            - "-v={{ .Values.logVerbosityLevel }}"


it should be --v= .. right? does one dash also work?

@ramineni both forms are allowed.

$ ./openstack-cloud-controller-manager --help ... -v, --v Level number for the log level verbosity $ ./cinder-csi-plugin --help ... -v, --v Level number for the log level verbosity $ ./manila-csi-plugin --help ... -v, --v Level number for the log level verbosity

Usually single-character flags are denoted with a single dash.

ok thanks, then should be ok

ramineni

one comment inline, else looks good

ramineni · 2021-11-09T13:25:58Z

/lgtm

lingxiankong · 2021-11-09T22:29:30Z

/approve

Thanks!

k8s-ci-robot · 2021-11-09T22:29:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lingxiankong

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

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

…s#1679) * added logVerbosityLevel variable to all Helm charts * set logVerbosityLevel to 2

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 29, 2021

k8s-ci-robot requested review from jichenjc and zetaab October 29, 2021 08:36

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Nov 2, 2021

k8s-ci-robot assigned jichenjc Nov 2, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 2, 2021

ramineni reviewed Nov 8, 2021

View reviewed changes

k8s-ci-robot requested a review from lingxiankong November 8, 2021 09:49

gman0 added 2 commits November 9, 2021 12:58

added logVerbosityLevel variable to all Helm charts

45769af

set logVerbosityLevel to 2

495b441

gman0 force-pushed the helm-log-verbosity branch from c8b7ac7 to 495b441 Compare November 9, 2021 12:03

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2021

ramineni reviewed Nov 9, 2021

View reviewed changes

k8s-ci-robot assigned ramineni Nov 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2021

k8s-ci-robot merged commit 8fb469b into kubernetes:master Nov 9, 2021

[Charts] Add logVerbosityLevel variable to all Helm charts #1679

[Charts] Add logVerbosityLevel variable to all Helm charts #1679

Conversation

gman0 commented Oct 29, 2021 • edited

gman0 commented Oct 29, 2021

gman0 commented Oct 29, 2021

jichenjc commented Nov 1, 2021

gman0 commented Nov 1, 2021

jichenjc commented Nov 2, 2021

gman0 commented Nov 2, 2021

gman0 commented Nov 2, 2021

gman0 commented Nov 2, 2021

jichenjc commented Nov 2, 2021

gman0 commented Nov 8, 2021

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

gman0 Nov 9, 2021 • edited

Choose a reason for hiding this comment

ramineni commented Nov 8, 2021

lingxiankong commented Nov 9, 2021

ramineni commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021

lingxiankong commented Nov 9, 2021

gman0 commented Nov 9, 2021

ramineni commented Nov 9, 2021

gman0 commented Nov 9, 2021

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

ramineni left a comment

Choose a reason for hiding this comment

ramineni commented Nov 9, 2021

lingxiankong commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021

gman0 commented Oct 29, 2021 •

edited

gman0 Nov 9, 2021 •

edited