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

operator: log op. config cm setting when changed #12679

Merged
merged 1 commit into from
Aug 16, 2023

Conversation

subhamkrai
Copy link
Contributor

@subhamkrai subhamkrai commented Aug 7, 2023

Description of your changes:

earlier, we're always logging when GetValue method is
called to get operator config values which sometimes
spamming the logs for example ROOK_WATCH_FOR_NODE_FAILURE
keep logging every time 5 min.

Now, we'll store the settings in the map and log only when
map key value is changed.

Which issue is resolved by this Pull Request:
Resolves #12543

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

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

But why does this get logged every 5 minutes? Is there node handling code that is triggered every 5 minutes? I don't see where that is. I thought the node handling code was only triggered on node events, not on a timer.

@subhamkrai
Copy link
Contributor Author

But why does this get logged every 5 minutes? Is there node handling code that is triggered every 5 minutes? I don't see where that is. I thought the node handling code was only triggered on node events, not on a timer.

every time, there is any change in nodes or somehow node watcher is called in nodeLoss scenario is also triggered and the first thing we check is if the configMap setting for nodeLoss is true or not. That's why it keeps logging.

here

watchForNodeLoss, err := k8sutil.GetOperatorSetting(ctx, c.context.Clientset, opcontroller.OperatorSettingConfigMapName, "ROOK_WATCH_FOR_NODE_FAILURE", "true")
if err != nil {
return pkgerror.Wrapf(err, "failed to get configmap value `ROOK_WATCH_FOR_NODE_FAILURE`.")
}
I can try moving that code block around L170 or L180 but that will not be right order to check for NodeLoss case.

pkg/operator/k8sutil/configmap.go Outdated Show resolved Hide resolved
@subhamkrai subhamkrai force-pushed the fix-nodeLoss-logging branch 4 times, most recently from a512643 to d367985 Compare August 10, 2023 07:31
@subhamkrai subhamkrai changed the title operator: suppress log of nodeLoss configmap setting operator: log op. config cm setting when changed Aug 10, 2023
pkg/operator/k8sutil/configmap.go Outdated Show resolved Hide resolved
pkg/operator/k8sutil/configmap.go Outdated Show resolved Hide resolved
@subhamkrai subhamkrai force-pushed the fix-nodeLoss-logging branch 3 times, most recently from 291bf14 to 60f7e44 Compare August 16, 2023 06:09
Copy link
Member

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

LGTM, small suggestion on unit test

pkg/operator/k8sutil/configmap_test.go Outdated Show resolved Hide resolved
pkg/operator/k8sutil/configmap_test.go Outdated Show resolved Hide resolved
earlier, we're always logging when `GetValue` method is
called to get operator config  values which somettime
spamming the logs for example `ROOK_WATCH_FOR_NODE_FAILURE`
keep logging every time 5 min.

Now, we'll store the settings in the map and log only when
map key value is changed.

Signed-off-by: subhamkrai <srai@redhat.com>
@travisn travisn merged commit bffd2e0 into rook:master Aug 16, 2023
43 of 49 checks passed
travisn added a commit that referenced this pull request Aug 16, 2023
operator: log op. config cm setting when changed (backport #12679)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Operator log filled with watch for node failure logs every 5m
4 participants