Skip to content
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

Support structured logging #330

Merged

Conversation

saku3
Copy link
Contributor

@saku3 saku3 commented Sep 7, 2023

What type of PR is this?

/kind feature

What this PR does / why we need it:

In this PR I have added JSON logging.
With this change, it is now possible to output the logs of the external-resizer container in JSON format.
Running the container with the --logging-format=json option will output the logs in JSON format.

In addition, I've modified the log messages based on the following guideline:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md

I’ve update the klog functions in use according to the guidelines provided below, and I've confirmed that they pass the
logcheck tests:https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/migration-to-structured-logging.md#change-log-functions

logcheck cmd/csi-resizer/main.go 
logcheck ./pkg/util/
logcheck ./pkg/resizer/ 
logcheck ./pkg/csi/    
logcheck ./pkg/controller/
logcheck ./pkg/features/ 

related PR:kubernetes-csi/livenessprobe#202

Special notes for your reviewer:

# The logs below were outputted because the program was running on MacOS.
$ go run cmd/csi-resizer/main.go --logging-format=json
{"ts":1694049594780.344,"caller":"csi-resizer/main.go:105","msg":"Version","v":0,"version":"unknown"}
{"ts":1694049594780.369,"caller":"csi-resizer/main.go:128","msg":"Failed to create cluster config","err":"unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"}
...

$ go run cmd/csi-resizer/main.go 
I0907 10:19:16.001980   70980 main.go:105] "Version" version="unknown"
E0907 10:19:16.002069   70980 main.go:128] "Failed to create cluster config" err="unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"
...

Does this PR introduce a user-facing change?:

Added support structured logging

@k8s-ci-robot k8s-ci-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Sep 7, 2023
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 7, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: saku3 / name: saku (1e9b0ad)

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Sep 7, 2023
@k8s-ci-robot
Copy link
Contributor

Welcome @saku3!

It looks like this is your first PR to kubernetes-csi/external-resizer 🎉. 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-csi/external-resizer 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 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 Sep 7, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @saku3. Thanks for your PR.

I'm waiting for a kubernetes-csi 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.

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Sep 7, 2023
@saku3 saku3 force-pushed the support-structured-logging branch from dbdf92c to 5a28102 Compare September 9, 2023 04:03
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 9, 2023
@bells17
Copy link

bells17 commented Sep 12, 2023

/ok-to-test

@k8s-ci-robot 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 Sep 12, 2023
@saku3 saku3 force-pushed the support-structured-logging branch from 5a28102 to a6a1794 Compare September 12, 2023 10:52
@@ -112,15 +125,16 @@ func main() {
config, err = rest.InClusterConfig()
}
if err != nil {
klog.Fatal(err.Error())
klog.ErrorS(err, "Failed to create cluster config")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exit is missing here, klog.Fatal would exit but ErrorS does not.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've add klog.FlushAndExit. Could you please review it again?

@saku3 saku3 force-pushed the support-structured-logging branch from a6a1794 to d1c4427 Compare September 16, 2023 11:47
@jsafrane
Copy link
Contributor

/lgtm
@gnufied can you please take a look?

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 18, 2023
@saku3 saku3 force-pushed the support-structured-logging branch from d1c4427 to 3078e4e Compare September 23, 2023 01:43
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 23, 2023
@saku3
Copy link
Contributor Author

saku3 commented Sep 27, 2023

@jsafrane

I've resolved the conflicts and pushed the changes according to the guidelines provided below. Could you please give it another look and approve if everything is fine?

https://github.com/kubernetes/community/blob/master/contributors/guide/github-workflow.md#4-keep-your-branch-in-sync

@gnufied
Copy link
Contributor

gnufied commented Oct 2, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 2, 2023
@jsafrane
Copy link
Contributor

jsafrane commented Oct 3, 2023

/retest

@jsafrane
Copy link
Contributor

jsafrane commented Oct 4, 2023

/retest
It looks unrelated to this PR...

@saku3
Copy link
Contributor Author

saku3 commented Oct 4, 2023

I'm getting the same error on multiple PRs in other repositories.

  • external-snapshotter
  • external-attacher
  • external-provisioner

The failing tests are as follows
https://github.com/kubernetes/kubernetes/blob/56f330493cf592ac0c9c9b9d35e8c9f69c45ab33/test/e2e/storage/testsuites/provisioning.go#L452

The error is identical.

{ failed [FAILED] Filesystem resize failed when restoring from snapshot to PVC with larger size. Restored fs size: 299853492224 bytes is not larger than Restored fs size: 299853492224 bytes is not larger than origin fs size: 299853492224 bytes.
HINT: Your driver needs to check the volume in NodeStageVolume and resize fs if needed.
HINT: For an example patch see: https://github.com/kubernetes/cloud-provider-openstack/pull/1563/files
Expected
    <int>: 299853492224
to be >
    <int>: 299853492224
In [It] at: test/e2e/storage/testsuites/provisioning.go:491 @ 10/02/23 18:08:27.298
}

@saku3
Copy link
Contributor Author

saku3 commented Oct 6, 2023

Related to OSS issue: kubernetes/kubernetes#120997

@saku3 saku3 force-pushed the support-structured-logging branch from 3078e4e to 1e9b0ad Compare October 18, 2023 14:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 18, 2023
@saku3
Copy link
Contributor Author

saku3 commented Oct 24, 2023

@jsafrane @gnufied

Sorry to bother you again. I've resolved the conflicts and pushed the changes. Could you please give it another look and approve if everything is fine?

@jsafrane
Copy link
Contributor

jsafrane commented Nov 1, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gnufied, jsafrane, saku3

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants