Bug 2019532: Logger object in LSO does not log source location accurately by dobsonj · Pull Request #299 · openshift/local-storage-operator

dobsonj · 2021-12-03T00:36:32Z

See https://bugzilla.redhat.com/show_bug.cgi?id=2019532
/cc @openshift/storage

openshift-ci · 2021-12-03T00:36:35Z

@dobsonj: This pull request references Bugzilla bug 2019532, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 2019532: Logger object in LSO does not log source location accurately

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.

openshift-ci · 2021-12-03T00:37:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dobsonj

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dobsonj]

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

dobsonj · 2021-12-03T00:37:33Z

/bugzilla refresh
/assign @gnufied

openshift-ci · 2021-12-03T00:37:39Z

@dobsonj: This pull request references Bugzilla bug 2019532, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @chao007

Details

In response to this:

/bugzilla refresh
/assign @gnufied

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.

dobsonj · 2021-12-03T00:40:32Z

sample log file from these changes after adding a couple of LV's and enabling discovery:
local-storage-operator-74b94cc95f-ssbjs.20211202.log

dobsonj · 2021-12-03T01:07:18Z

I ended up stashing the NewDelegatingLogger implementation on a separate branch, if that seems better:
master...dobsonj:bug2019532
Still, it's just a workaround, and klog may be more reliable / less likely to break, and it would be more consistent with our other controllers.

openshift-bot · 2022-01-28T18:24:39Z

/bugzilla refresh

The requirements for Bugzilla bugs have changed (BZs linked to PRs on master branch need to target OCP 4.11), recalculating validity.

openshift-ci · 2022-01-28T18:25:44Z

@openshift-bot: This pull request references Bugzilla bug 2019532, which is invalid:

expected the bug to target the "4.11.0" release, but it targets "4.10.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed (BZs linked to PRs on master branch need to target OCP 4.11), recalculating validity.

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.

…tely

dobsonj · 2022-02-03T19:49:10Z

/hold
for manual testing

dobsonj · 2022-02-03T23:04:49Z

/unhold
logs after the rebase:
20220203_2019532-diskmaker-discovery-lx94c.log
20220203_2019532-diskmaker-manager-k76dl.log
20220203_2019532-local-storage-operator-6cf5b648d4-nvwrh.log

dobsonj · 2022-02-03T23:05:12Z

/cc @gnufied

gnufied · 2022-02-03T23:11:47Z

 	})
 	if opRes != controllerutil.OperationResultNone {
-		pvLogger.Info("pv changed", "operation", opRes)
+		klog.Infof("PV changed, name = %s, status = %s", pvName, opRes)


Could we use InfoS in these places?

klog doesn't have any structured logging functions that I could find... at least not this version of it. Am I missing something obvious?

Ah, that was added in klog v2 it looks like, and LSO is still on v1:

$ grep klog go.mod k8s.io/klog v1.0.0

hmm okay. Lets handle this later.

Hold on, I made the changes, but need to retest.

dobsonj · 2022-02-04T01:37:29Z

Using structured logging:
20220203_2019532-diskmaker-discovery-gzcnf.log
20220203_2019532-diskmaker-manager-6cghw.log
20220203_2019532-local-storage-operator-6cf5b648d4-md5nz.log

dobsonj · 2022-02-04T01:40:25Z

-		logger.Error(err, " error creating local-storage directory ", "symLinkLocation", r.symlinkLocation)
-		os.Exit(-1)
+		klog.ErrorS(err, "error creating local-storage directory", "symLinkLocation", r.symlinkLocation)
+		return ctrl.Result{}, err


This is buried in the logging changes, so I just want to point out the os.Exit call I removed here--that seemed very wrong to me.

I distinctly remember spotting this during review, but at that time my thinking was - if we can't create local-storage root directory, something is indeed wrong with the configuration/node and exiting here will cause pod to crash loop and throw easy to spot errors that might be helpful (basically complain as loudly as we can, if things aren't really working).

dobsonj · 2022-02-04T17:28:29Z

/bugzilla refresh

openshift-ci · 2022-02-04T17:28:36Z

@dobsonj: This pull request references Bugzilla bug 2019532, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.11.0) matches configured target release for branch (4.11.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @chao007

Details

In response to this:

/bugzilla 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.

gnufied · 2022-02-04T19:19:40Z

/lgtm

openshift-ci · 2022-02-04T20:27:13Z

@dobsonj: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

openshift-ci · 2022-02-04T20:27:30Z

@dobsonj: All pull requests linked via external trackers have merged:

openshift/local-storage-operator#299

Bugzilla bug 2019532 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2019532: Logger object in LSO does not log source location accurately

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.

dobsonj · 2022-02-04T20:44:19Z

/cherry-pick release-4.10

openshift-cherrypick-robot · 2022-02-04T20:44:59Z

@dobsonj: new pull request created: #317

Details

In response to this:

/cherry-pick release-4.10

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.

openshift-ci Bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Dec 3, 2021

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 3, 2021

openshift-ci Bot assigned gnufied Dec 3, 2021

openshift-ci Bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Dec 3, 2021

openshift-ci Bot requested a review from chao007 December 3, 2021 00:37

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2021

openshift-ci Bot removed the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 28, 2022

openshift-ci Bot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jan 28, 2022

dobsonj added 2 commits February 3, 2022 12:43

Bug 2019532: Logger object in LSO does not log source location accura…

e1de62c

…tely

Use ISO8601TimeEncoder for controller-runtime logging

9566291

dobsonj force-pushed the bug2019532-klog branch from 3dc36ac to 9566291 Compare February 3, 2022 19:48

openshift-ci Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 3, 2022

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2022

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 3, 2022

openshift-ci Bot requested a review from gnufied February 3, 2022 23:05

gnufied reviewed Feb 3, 2022

View reviewed changes

Use klog v2 structured logging

89809b8

dobsonj commented Feb 4, 2022

View reviewed changes

openshift-ci Bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Feb 4, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Feb 4, 2022

openshift-merge-robot merged commit 843d3ca into openshift:master Feb 4, 2022

openshift-cherrypick-robot mentioned this pull request Feb 4, 2022

[release-4.10] Bug 2050883: Logger object in LSO does not log source location accurately #317

Merged

Conversation

dobsonj commented Dec 3, 2021

Uh oh!

openshift-ci Bot commented Dec 3, 2021

Uh oh!

openshift-ci Bot commented Dec 3, 2021

Uh oh!

dobsonj commented Dec 3, 2021

Uh oh!

openshift-ci Bot commented Dec 3, 2021

Uh oh!

dobsonj commented Dec 3, 2021

Uh oh!

dobsonj commented Dec 3, 2021

Uh oh!

openshift-bot commented Jan 28, 2022

Uh oh!

openshift-ci Bot commented Jan 28, 2022

Uh oh!

dobsonj commented Feb 3, 2022

Uh oh!

dobsonj commented Feb 3, 2022

Uh oh!

dobsonj commented Feb 3, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobsonj commented Feb 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dobsonj commented Feb 4, 2022

Uh oh!

openshift-ci Bot commented Feb 4, 2022

Uh oh!

gnufied commented Feb 4, 2022

Uh oh!

openshift-ci Bot commented Feb 4, 2022

Uh oh!

openshift-ci Bot commented Feb 4, 2022

Uh oh!

dobsonj commented Feb 4, 2022

Uh oh!

openshift-cherrypick-robot commented Feb 4, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants