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

add more events for localdiskclaim #1249

Merged
merged 1 commit into from
Nov 21, 2023

Conversation

hikariwo
Copy link
Member

@hikariwo hikariwo commented Nov 16, 2023

What this PR does / why we need it:

add more event in detail while the localdiskclaim is running

FYI: #1218

Special notes for your reviewer:

Does this PR introduce a user-facing change?


@codecov-commenter
Copy link

codecov-commenter commented Nov 16, 2023

Codecov Report

Attention: 11 lines in your changes are missing coverage. Please review.

Comparison is base (b87638c) 35.77% compared to head (392a089) 36.35%.
Report is 9 commits behind head on main.

Files Patch % Lines
...roller/localdiskclaim/localdiskclaim_controller.go 61.53% 5 Missing ⚠️
pkg/local-disk-manager/filter/filter_disk.go 66.66% 4 Missing ⚠️
.../local-disk-manager/handler/localdisk/localdisk.go 0.00% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1249      +/-   ##
==========================================
+ Coverage   35.77%   36.35%   +0.57%     
==========================================
  Files          28       28              
  Lines        2180     2214      +34     
==========================================
+ Hits          780      805      +25     
- Misses       1297     1306       +9     
  Partials      103      103              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@SSmallMonster SSmallMonster left a comment

Choose a reason for hiding this comment

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

Can you also take a look at https://github.com/hwameistor/hwameistor/blob/c31d8f050dff9efe526006debe202e456bd795d4/pkg/local-disk-manager/controller/localdiskclaim/localdiskclaim_controller.go#L142C31-L142C42

The most common disk allocation error usually occurs here. But the return information here is not enough to show what happened internally, which is very helpful for troubleshooting disk allocation.

@SSmallMonster SSmallMonster added the kind/enhancement New feature or request label Nov 17, 2023
@hikariwo
Copy link
Member Author

Can you also take a look at https://github.com/hwameistor/hwameistor/blob/c31d8f050dff9efe526006debe202e456bd795d4/pkg/local-disk-manager/controller/localdiskclaim/localdiskclaim_controller.go#L142C31-L142C42

The most common disk allocation error usually occurs here. But the return information here is not enough to show what happened internally, which is very helpful for troubleshooting disk allocation.

The problem is produced by FilterDisk. When it filter a not suitable disk,there is no wrong message will return. IMO, adding a message field to ldHandler is a good way to keep the filter error message for troubleshooting disk allocation.

@hikariwo
Copy link
Member Author

Not add the field maybe ok, the EventRecorder is kept at handler

@SSmallMonster
Copy link
Member

Can you also take a look at https://github.com/hwameistor/hwameistor/blob/c31d8f050dff9efe526006debe202e456bd795d4/pkg/local-disk-manager/controller/localdiskclaim/localdiskclaim_controller.go#L142C31-L142C42
The most common disk allocation error usually occurs here. But the return information here is not enough to show what happened internally, which is very helpful for troubleshooting disk allocation.

The problem is produced by FilterDisk. When it filter a not suitable disk,there is no wrong message will return. IMO, adding a message field to ldHandler is a good way to keep the filter error message for troubleshooting disk allocation.

Yes, reason for why disks not match can be shown on LocalDisk or LocalDiskClaim.

@hikariwo
Copy link
Member Author

hikariwo commented Nov 17, 2023

Not add the field maybe ok, the EventRecorder is kept at handler

But I think adding a message field will offer more flexible, otherwise

Can you also take a look at https://github.com/hwameistor/hwameistor/blob/c31d8f050dff9efe526006debe202e456bd795d4/pkg/local-disk-manager/controller/localdiskclaim/localdiskclaim_controller.go#L142C31-L142C42
The most common disk allocation error usually occurs here. But the return information here is not enough to show what happened internally, which is very helpful for troubleshooting disk allocation.

The problem is produced by FilterDisk. When it filter a not suitable disk,there is no wrong message will return. IMO, adding a message field to ldHandler is a good way to keep the filter error message for troubleshooting disk allocation.

Yes, reason for why disks not match can be shown on LocalDisk or LocalDiskClaim.

Yes, but the message is shown on LocalDisk or LocalDiskClaim, the LocalDiskClaim has to keep in Handler to provide the ldc information for not changing a lot of code signature

@hikariwo
Copy link
Member Author

It can directly show in LocalDisk, but the LocalDiskClaim information will be unknown to user. IMO, adding the LocalDiskClaim in Handler, and each one filter method judge the LocalDiskClaim existing and produce the error event to LocalDiskClaim when the error happens.

@SSmallMonster
Copy link
Member

It can directly show in LocalDisk, but the LocalDiskClaim information will be unknown to user. IMO, adding the LocalDiskClaim in Handler, and each one filter method judge the LocalDiskClaim existing and produce the error event to LocalDiskClaim when the error happens.

message should order by type, e.g., nodeName not mach, devType not match..

@hikariwo
Copy link
Member Author

It can directly show in LocalDisk, but the LocalDiskClaim information will be unknown to user. IMO, adding the LocalDiskClaim in Handler, and each one filter method judge the LocalDiskClaim existing and produce the error event to LocalDiskClaim when the error happens.

message should order by type, e.g., nodeName not mach, devType not match..

Adding a message field maybe more suitable instead of adding a LocalDiskClaim to avoid a lot of event is recorded .

@hikariwo hikariwo force-pushed the ldc-more-event branch 3 times, most recently from bb17b91 to 158056e Compare November 17, 2023 15:44
@hikariwo hikariwo force-pushed the ldc-more-event branch 2 times, most recently from eb125a6 to db18c96 Compare November 21, 2023 04:23
@hikariwo
Copy link
Member Author

Done, it outputs like LocalDiskUnAvailable: aaa/LocalDiskHasReserved: bbb/LocalDiskAlreadyHasPartition: aaa,bbb when it assigned failed

@hikariwo
Copy link
Member Author

@SSmallMonster

pkg/local-disk-manager/filter/filter_disk.go Outdated Show resolved Hide resolved
pkg/local-disk-manager/filter/filter_disk.go Outdated Show resolved Hide resolved
pkg/local-disk-manager/filter/filter_disk.go Outdated Show resolved Hide resolved
Signed-off-by: lancerxiu <1377004871@qq.com>
@SSmallMonster
Copy link
Member

LGTM

@SSmallMonster SSmallMonster merged commit 4457356 into hwameistor:main Nov 21, 2023
3 checks passed
@hikariwo hikariwo deleted the ldc-more-event branch November 21, 2023 06:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants