Review registration log traces for Provisioner by andfasano · Pull Request #757 · metal3-io/baremetal-operator

andfasano · 2021-01-05T14:12:40Z

This PR introduce a debug logger for the provisioner, to avoid printing too many traces in the log during the steady cases.

dhellmann · 2021-01-05T14:20:41Z

How easy/hard is it to change the verbosity level of the log output to debug a problem? I've usually found that it's better to log a little more, and throw info away in the log aggregation tool, but there's not a hard rule about where that line is.

zaneb

What does logging at verbosity level 5 mean in practice?
I see we have a -dev flag to "enable developer logging", but no way to set the verbosity level explicitly AFAICT.

zaneb · 2021-01-05T14:45:11Z


-	info.host.ClearError()
+	// Avoid to clean up other kind of errors
+	if info.host.Status.ErrorType == metal3v1alpha1.RegistrationError {


We should probably also always clear the error if registeredNewCreds is true. WDYT?

That's an interesting point. I assumed that the error was set only here during the registration. In case of credentials change the ValidateManagementAccess can fail in the update, but in such case the actions gets simply re-enqueued (409) or an actionError is returned, so it seems that such scenario the ClearError() shouldn't be required for the host.

Credentials can change at any time. If someone changes the password on the server prior to changing it in the BMH, then I think it's possible to see another kind of error.
It seems reasonable that once we know the creds have changed (and the change worked), we discard any assumptions about what we thought might have been broken before.

Ok, so if IIUC changing the creds could be considered a very "strong" kind of event that somehow resets the current situation (forcing anyhow implicitly a re-evaluation).

andfasano · 2021-01-05T15:11:12Z

How easy/hard is it to change the verbosity level of the log output to debug a problem? I've usually found that it's better to log a little more, and throw info away in the log aggregation tool, but there's not a hard rule about where that line is.

Zap offers a couple of configuration options (see also https://pkg.go.dev/go.uber.org/zap#ex-package--BasicConfiguration) but as @zaneb correctly pointed out the verbosity level wasn't directly available. My original intention was also to have a smaller footprint assuming an optimistic case

andfasano · 2021-01-05T15:16:20Z

What does logging at verbosity level 5 mean in practice?
I see we have a -dev flag to "enable developer logging", but no way to set the verbosity level explicitly AFAICT.

Following the logr docs indication the idea was to have a more technical debug level. I can see if we can use that flag instead

andfasano · 2021-01-05T17:42:42Z

What does logging at verbosity level 5 mean in practice?
I see we have a -dev flag to "enable developer logging", but no way to set the verbosity level explicitly AFAICT.

Following the logr docs indication the idea was to have a more technical debug level. I can see if we can use that flag instead

I checked the current configuration and By default zap is configured to trace level 0 only (Info). -dev flag also enables tracing at level 1 (Debug), so it could be possible to switch to that level to reuse an existing BMO configuration without the need of adding a new one

andfasano · 2021-01-07T09:45:20Z

Changed the implementation to use the -dev flag to print additional log traces

andfasano · 2021-01-07T16:20:38Z

/test-integration

zaneb · 2021-01-11T21:56:50Z

 		switch err.(type) {
 		case nil:
-			p.log.Info("found existing node by ID")
+			p.log.V(1).Info("found existing node by ID")


Let's set this up as a debugLog member in the ironicProvisioner struct.

andfasano · 2021-01-13T09:50:14Z

/test-integration

stbenjam · 2021-01-14T19:14:21Z

 	return hb
 }

+func (hb *hostBuilder) SetStatusError(opStatus metal3v1alpha1.OperationalStatus, errType metal3v1alpha1.ErrorType, errMsg string, errCount int) *hostBuilder {


Nit, does this need to be exported?

zaneb · 2021-01-15T15:52:07Z

I ended up with a patch to fix up the logs in the controller in #761 (some other stuff was dependent on moving it all around, so this will reduce conflicts). Stephen has included the change to limit which errors get reset in #762. So maybe let's reduce the scope of this PR to just the provisioner and get it merged?

andfasano · 2021-03-23T14:19:40Z

/test-integration

zaneb · 2021-03-23T14:25:45Z

/approve

metal3-io-bot · 2021-03-23T14:26:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, zaneb

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

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

hardys · 2021-03-24T09:50:44Z

/lgtm

metal3-io-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 5, 2021

zaneb reviewed Jan 5, 2021

View reviewed changes

andfasano force-pushed the registration-log-and-fix branch from b74251b to 6ce9300 Compare January 7, 2021 09:40

andfasano requested a review from zaneb January 7, 2021 09:45

andfasano force-pushed the registration-log-and-fix branch from 6ce9300 to d67d800 Compare January 11, 2021 10:20

andfasano mentioned this pull request Jan 11, 2021

Do retries with backoff in ValidateManagementAccess, Inspect, and Deprovision #749

Merged

zaneb reviewed Jan 11, 2021

View reviewed changes

andfasano force-pushed the registration-log-and-fix branch from d67d800 to 084037f Compare January 12, 2021 16:44

zaneb mentioned this pull request Jan 12, 2021

Retry when node in "Adopt Failed" state #697

Closed

andfasano mentioned this pull request Jan 14, 2021

Force retry when adoption fails #762

Merged

stbenjam reviewed Jan 14, 2021

View reviewed changes

andfasano force-pushed the registration-log-and-fix branch from 084037f to ef745ac Compare January 15, 2021 09:50

metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 15, 2021

andfasano force-pushed the registration-log-and-fix branch from ef745ac to 15d1330 Compare January 15, 2021 16:35

metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 15, 2021

andfasano changed the title ~~Review registration log traces and clean error fix~~ Review registration log traces for Provisioner Jan 15, 2021

zaneb reviewed Jan 20, 2021

View reviewed changes

Comment thread pkg/provisioner/ironic/ironic.go Outdated

Comment thread pkg/provisioner/ironic/ironic.go Outdated

Comment thread pkg/provisioner/ironic/ironic.go Outdated

Add debug log for provisioner

06584aa

andfasano force-pushed the registration-log-and-fix branch from 15d1330 to 06584aa Compare March 12, 2021 10:50

andfasano requested a review from zaneb March 12, 2021 10:50

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2021

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 24, 2021

metal3-io-bot merged commit 0e6772b into metal3-io:master Mar 24, 2021

Conversation

andfasano commented Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhellmann commented Jan 5, 2021

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

Uh oh!

zaneb Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

andfasano Jan 5, 2021

Choose a reason for hiding this comment

Uh oh!

zaneb Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

andfasano Jan 12, 2021

Choose a reason for hiding this comment

Uh oh!

andfasano commented Jan 5, 2021

Uh oh!

andfasano commented Jan 5, 2021

Uh oh!

andfasano commented Jan 5, 2021

Uh oh!

andfasano commented Jan 7, 2021

Uh oh!

andfasano commented Jan 7, 2021

Uh oh!

zaneb Jan 11, 2021

Choose a reason for hiding this comment

Uh oh!

andfasano commented Jan 13, 2021

Uh oh!

stbenjam Jan 14, 2021

Choose a reason for hiding this comment

Uh oh!

zaneb commented Jan 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andfasano commented Mar 23, 2021

Uh oh!

zaneb commented Mar 23, 2021

Uh oh!

metal3-io-bot commented Mar 23, 2021

Uh oh!

hardys commented Mar 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

andfasano commented Jan 5, 2021 •

edited

Loading