cloud-provider: add log options, allow setting logging-format via CLI option #108984

LittleFox94 · 2022-03-24T15:21:15Z

What type of PR is this?

/kind feature

What this PR does / why we need it:

This PR allows users to configure logging format for CCMs based on k8s.io/cloud-provider.

Does this PR introduce a user-facing change?

* add logging-format option to CCMs based on k8s.io/cloud-provider

k8s-ci-robot · 2022-03-24T15:23:11Z

Welcome @LittleFox94!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-03-24T15:23:12Z

Hi @LittleFox94. 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.

andrewsykim

/ok-to-test

LittleFox94 · 2022-03-24T21:09:16Z

whoops - sorry for the failed CI run and noise

LittleFox94 · 2022-03-25T08:41:32Z

hm.. that failed test looks weird, like not a problem of me?

Strange log entry

Terminated (Error - Program process in pidfile '/var/run/docker-ssd.pid', 1 process(es), refused to die. ================================================================================ Done cleaning up after docker in docker. {"component":"entrypoint","error":"wrapped process failed: exit status 2","file":"k8s.io/test-infra/prow/entrypoint/run.go:80","func":"k8s.io/test-infra/prow/entrypoint.Options.Run","level":"error","msg":"Error executing test process","severity":"error","time":"2022-03-24T21:54:43Z"} ) at 2022-03-24 21:54:43 +0000 UTC with exit code 2
--

/retest

LittleFox94 · 2022-03-25T11:34:11Z

@andrewsykim can you please take a look at the failing test, if there is something I have to do?

LittleFox94 · 2022-03-28T13:37:22Z

/retest

LittleFox94 · 2022-04-06T10:45:02Z

Please take another look @andrewsykim, @deads2k or @wlan0

Tests ran successfully, now only needs another look at the code and hopefully merge :)

Thanks :)

LittleFox94 · 2023-01-19T12:32:06Z

This PR is slowly approaching an age of one year, with a multitude of rebases in between and, as I learnt now, rebases will reset the lgtm label..

Would be really great to get this merged, seems like most required review is done already anyway? What's blocking this?

pohly · 2023-01-19T14:00:02Z

Someone who has approval powers for the code has to approve it. It's unfortunate that people ignore assigned PRs, but it happens. In that case, bring it to the SIG which owns the code and ask for approval.

pohly

/lgtm

k8s-ci-robot · 2023-01-19T14:01:24Z

LGTM label has been added.

Git tree hash: 90d9738091b998974527ccccb19176fd9db378d3

andrewsykim

/approve

Thanks and sorry for the delay

andrewsykim · 2023-01-19T14:26:06Z

staging/src/k8s.io/cloud-provider/app/controllermanager.go

@@ -85,6 +89,11 @@ the cloud specific control loops shipped with Kubernetes.`,
 			verflag.PrintAndExitIfRequested()
 			cliflag.PrintFlags(cmd.Flags())

+			if err := logsapi.ValidateAndApply(logOptions, nil); err != nil {


What's the reason for nil feature gate here?

docs say "If nil, the default for these features is used", which sounds like a good fit here?

e.g. metrics-server does the same: https://github.com/kubernetes-sigs/metrics-server/blob/master/cmd/metrics-server/app/options/options.go#L59

metrics-server doesn't inherit and default feature gates from Kubernetes though -- I think at the very least we want to use the default feature gate we register here: https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/cloud-provider/app/controllermanager.go#L66

utilfeature.DefaultFeatureGate is actually better because it doesn't have to be mutable.

@pohly added it to the DefaultMutableFeatureGate, same as component-base/metrics/features is added already

Before I just rebase this PR again, is the way I implemented the feature gate ok?

Yes, looks good to me.

thanks :)

so another version prepared, freshly rebased and ready for your eyes @andrewsykim :)

another ping @andrewsykim

andrewsykim · 2023-01-19T14:28:18Z

staging/src/k8s.io/cloud-provider/app/controllermanager.go

+
+	globalFlagSet := namedFlagSets.FlagSet("global")
+	verflag.AddFlags(globalFlagSet)
+	logsapi.AddFlags(logOptions, globalFlagSet)


In general, I would like the logging options here to be consistent with kube-controller-manager and this seems to deviate from this: https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/controllermanager.go#L155-L162

Ah, actually we are still doing this here: https://github.com/kubernetes/kubernetes/blob/master/cmd/kube-controller-manager/app/options/options.go#L188

so this can stay as-is or should I change something?

I think it can stay as-is

andrewsykim · 2023-01-19T14:31:35Z

/hold

(holding for the question on feature gates)

Change k8s.io/cloud-provider/app.NewCloudControllerManagerCommand to create a k8s.io/component-base/logs.Options, add it to the flags for the command and apply them on startup. This adds the logging-format command line option.

Extend the cloud-provider sample to register the JSON log format.

andrewsykim

/approve
/lgtm
/hold cancel

k8s-ci-robot · 2023-02-06T17:06:02Z

LGTM label has been added.

Git tree hash: f31d383e5beeedf2a651ff8c4a2be417bbccc325

k8s-ci-robot · 2023-02-06T17:06:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, LittleFox94, pohly

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:

~~staging/src/k8s.io/cloud-provider/OWNERS~~ [andrewsykim]

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

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 Mar 24, 2022

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/cloudprovider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. labels Mar 24, 2022

k8s-ci-robot requested review from deads2k and wlan0 March 24, 2022 15:24

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Mar 24, 2022

andrewsykim reviewed Mar 24, 2022

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 Mar 24, 2022

LittleFox94 force-pushed the cloud-provider/logging-format branch from afc0bc3 to e916ff9 Compare March 24, 2022 21:07

k8s-ci-robot added the area/dependency Issues or PRs related to dependency changes label Mar 24, 2022

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

LittleFox94 force-pushed the cloud-provider/logging-format branch from e916ff9 to c37723a Compare May 5, 2022 13:47

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

LittleFox94 force-pushed the cloud-provider/logging-format branch 2 times, most recently from afe88c0 to 82edb64 Compare May 6, 2022 08:30

k8s-ci-robot requested review from nckturner and pohly January 19, 2023 12:30

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

pohly approved these changes Jan 19, 2023

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2023

andrewsykim reviewed Jan 19, 2023

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 19, 2023

LittleFox94 force-pushed the cloud-provider/logging-format branch from 862096c to a420cb8 Compare January 19, 2023 16:31

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 19, 2023

k8s-ci-robot requested review from andrewsykim and pohly January 19, 2023 16:31

LittleFox94 force-pushed the cloud-provider/logging-format branch from a420cb8 to 51bcaac Compare January 20, 2023 10:04

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

LittleFox94 added 2 commits January 23, 2023 14:40

cloud-provider: add more log options

e9379b3

Change k8s.io/cloud-provider/app.NewCloudControllerManagerCommand to create a k8s.io/component-base/logs.Options, add it to the flags for the command and apply them on startup. This adds the logging-format command line option.

cloud-provider: extend sample for JSON log format

7601347

Extend the cloud-provider sample to register the JSON log format.

LittleFox94 force-pushed the cloud-provider/logging-format branch from 51bcaac to 7601347 Compare January 23, 2023 13:41

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

andrewsykim reviewed Feb 6, 2023

View reviewed changes

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 6, 2023

k8s-ci-robot merged commit 31d6d9a into kubernetes:master Feb 6, 2023

k8s-ci-robot added this to the v1.27 milestone Feb 6, 2023

LittleFox94 deleted the cloud-provider/logging-format branch February 6, 2023 18:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cloud-provider: add log options, allow setting logging-format via CLI option #108984

cloud-provider: add log options, allow setting logging-format via CLI option #108984

LittleFox94 commented Mar 24, 2022 •

edited

k8s-ci-robot commented Mar 24, 2022

k8s-ci-robot commented Mar 24, 2022

andrewsykim left a comment

LittleFox94 commented Mar 24, 2022

LittleFox94 commented Mar 25, 2022

LittleFox94 commented Mar 25, 2022

LittleFox94 commented Mar 28, 2022

LittleFox94 commented Apr 6, 2022

LittleFox94 commented Jan 19, 2023

pohly commented Jan 19, 2023

pohly left a comment

k8s-ci-robot commented Jan 19, 2023

andrewsykim left a comment

andrewsykim Jan 19, 2023

LittleFox94 Jan 19, 2023

LittleFox94 Jan 19, 2023

andrewsykim Jan 19, 2023

andrewsykim Jan 19, 2023

LittleFox94 Jan 20, 2023

LittleFox94 Jan 21, 2023

pohly Jan 23, 2023

LittleFox94 Jan 23, 2023

LittleFox94 Jan 27, 2023

andrewsykim Jan 19, 2023

andrewsykim Jan 19, 2023

LittleFox94 Jan 19, 2023

andrewsykim Jan 19, 2023

andrewsykim commented Jan 19, 2023

andrewsykim left a comment

k8s-ci-robot commented Feb 6, 2023

k8s-ci-robot commented Feb 6, 2023

cloud-provider: add log options, allow setting logging-format via CLI option #108984

cloud-provider: add log options, allow setting logging-format via CLI option #108984

Conversation

LittleFox94 commented Mar 24, 2022 • edited

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 24, 2022

k8s-ci-robot commented Mar 24, 2022

andrewsykim left a comment

Choose a reason for hiding this comment

LittleFox94 commented Mar 24, 2022

LittleFox94 commented Mar 25, 2022

LittleFox94 commented Mar 25, 2022

LittleFox94 commented Mar 28, 2022

LittleFox94 commented Apr 6, 2022

LittleFox94 commented Jan 19, 2023

pohly commented Jan 19, 2023

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jan 19, 2023

andrewsykim left a 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

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

andrewsykim commented Jan 19, 2023

andrewsykim left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 6, 2023

k8s-ci-robot commented Feb 6, 2023

LittleFox94 commented Mar 24, 2022 •

edited