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

scanner: add more logs #100

Merged
merged 2 commits into from
May 9, 2024
Merged

Conversation

Vicente-Cheng
Copy link
Collaborator

Problem:
We encounter some weird issue that the ID is changed after reboot.

Solution:
Add these logs would help us to verify easily.

Related Issue:

Test plan:
None

Copy link
Contributor

@tserong tserong left a comment

Choose a reason for hiding this comment

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

LGTM (just one tiny suggestion)

pkg/controller/blockdevice/scanner.go Outdated Show resolved Hide resolved
    We encounter some weird issue that the ID is changed after reboot.
    Add these logs would help us to verify easily.

Signed-off-by: Vicente Cheng <[email protected]>
Copy link
Member

@bk201 bk201 left a comment

Choose a reason for hiding this comment

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

lgtm

@Vicente-Cheng Vicente-Cheng merged commit afaec3d into harvester:master May 9, 2024
8 checks passed
@@ -98,6 +98,11 @@ func (s *Scanner) collectAllDevices() []*deviceWithAutoProvision {
logrus.Infof("Skip adding non-identifiable block device /dev/%s", disk.Name)
continue
}
logrus.Infof("Adding the disk with block device /dev/%s, id(Name): %s on node %s", disk.Name, bd.Name, s.NodeName)
Copy link
Member

Choose a reason for hiding this comment

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

It's already too late now that the PR has been merged, but this question nevertheless.

Does it not make sense to write this information in one line? The output in the log does not always have to look like in the code. In the meantime in which these output lines are written to the log, other messages can also be written. This can make the output quite confusing. Shouldn't logrus.WithFields be used here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I agree that.
We will use WithFields after introducing the log format.

I thought we had a similar issue with this (for the whole harvester components).

Copy link
Member

Choose a reason for hiding this comment

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

I thought we had a similar issue with this (for the whole harvester components).

Yes, see here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exactly this one. I would like to refine all the logs when processing this.

@Vicente-Cheng
Copy link
Collaborator Author

@Mergifyio backport v0.5.x v0.6.x

Copy link

mergify bot commented May 23, 2024

backport v0.5.x v0.6.x

✅ Backports have been created

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants