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

feat(upgrade): support engine suspension and target switchover #94

Merged
merged 8 commits into from
Jul 6, 2024

Conversation

derekbit
Copy link
Member

@derekbit derekbit commented Apr 9, 2024

Which issue(s) this PR fixes:

Issue longhorn/longhorn#6001

What this PR does / why we need it:

Special notes for your reviewer:

Additional documentation or context

@derekbit derekbit self-assigned this Apr 9, 2024
@derekbit derekbit force-pushed the v2-volume-upgrade branch 3 times, most recently from a44f841 to 007817c Compare April 16, 2024 09:51
@derekbit derekbit force-pushed the v2-volume-upgrade branch 5 times, most recently from b3a2588 to cc8ae61 Compare April 26, 2024 15:06
@derekbit derekbit force-pushed the v2-volume-upgrade branch 4 times, most recently from bede3af to 3829ce5 Compare May 16, 2024 08:44
@derekbit derekbit force-pushed the v2-volume-upgrade branch 4 times, most recently from 59db3ee to 8f31293 Compare June 4, 2024 03:20
Copy link

codecov bot commented Jun 6, 2024

Codecov Report

Attention: Patch coverage is 23.12775% with 349 lines in your changes missing coverage. Please review.

Project coverage is 23.95%. Comparing base (35e00d1) to head (e236459).

Files Patch % Lines
app/cmd/dmsetup/dmsetup.go 0.00% 184 Missing ⚠️
pkg/nvme/initiator.go 32.66% 93 Missing and 8 partials ⚠️
app/cmd/nvmecli/nvmecli.go 0.00% 30 Missing ⚠️
pkg/util/device.go 37.50% 14 Missing and 1 partial ⚠️
pkg/nvme/nvmecli.go 20.00% 12 Missing ⚠️
pkg/util/dmsetup.go 89.58% 3 Missing and 2 partials ⚠️
pkg/nvme/nvme.go 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main      #94      +/-   ##
==========================================
+ Coverage   22.78%   23.95%   +1.17%     
==========================================
  Files          33       34       +1     
  Lines        4240     4621     +381     
==========================================
+ Hits          966     1107     +141     
- Misses       3115     3340     +225     
- Partials      159      174      +15     
Flag Coverage Δ
unittests 23.95% <23.12%> (+1.17%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@derekbit derekbit force-pushed the v2-volume-upgrade branch 10 times, most recently from 7f5c3fa to 2b83180 Compare June 20, 2024 02:12
@derekbit derekbit force-pushed the v2-volume-upgrade branch 3 times, most recently from 032381a to 28d108f Compare June 26, 2024 23:58
@derekbit derekbit changed the title Support engine suspension and target switchover feat(upgrade): support engine suspension and target switchover Jun 27, 2024
@derekbit derekbit force-pushed the v2-volume-upgrade branch 2 times, most recently from 33001b3 to adcba02 Compare June 27, 2024 07:35
Copy link
Contributor

@DamiaSan DamiaSan left a comment

Choose a reason for hiding this comment

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

I see you have implemented the flush command over nvme, but actually lvol bdev doesn't support that. I had implemented it, but this development caused a memory leak in the set external parent functionality, which we use in online rebuilding. I understood that we didn't need the flush over nvme and that we only used it at device mapper layer, so I discarded that commit in the new SPDK branch longhorn-v24.05. I thought to restart working over it, to remove the leak, after 1.7.0.

@derekbit
Copy link
Member Author

derekbit commented Jun 30, 2024

in online rebuilding. I understood that we didn't need the flush over nvme and that we only used it at device mapper layer, so I discarded that commit in the new SPDK branch longhorn-v24.05. I thought to restart working over it, to remove the leak, after 1.7.0.

Got it. I think the flush subcommand is fine because I don't use it in the suspension and switchover functionalities. The subcommand is just for making the nvme command more complete. WDYT?

@DamiaSan
Copy link
Contributor

DamiaSan commented Jul 1, 2024

in online rebuilding. I understood that we didn't need the flush over nvme and that we only used it at device mapper layer, so I discarded that commit in the new SPDK branch longhorn-v24.05. I thought to restart working over it, to remove the leak, after 1.7.0.

Got it. I think the flush subcommand is fine because I didn't use in the suspension and switchover functionalities. The subcommand is just for making the nvme command more complete. WDYT?

Ok, this is fine for now. In the future I will work again on the flush implementation so we will have it again.

DamiaSan
DamiaSan previously approved these changes Jul 1, 2024
Copy link
Contributor

@DamiaSan DamiaSan left a comment

Choose a reason for hiding this comment

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

Just a couple of questions, but globally it LGTM

pkg/nvme/initiator.go Show resolved Hide resolved
pkg/nvme/initiator.go Show resolved Hide resolved
Longhorn 6001

Signed-off-by: Derek Su <derek.su@suse.com>
Longhorn 6001

Signed-off-by: Derek Su <derek.su@suse.com>
Longhorn 6001

Signed-off-by: Derek Su <derek.su@suse.com>
- Rename KernelDevice to LonghornBlockDevice
- Refactor DetectDevice()

Longhorn 6001

Signed-off-by: Derek Su <derek.su@suse.com>
Longhorn 6001

Signed-off-by: Derek Su <derek.su@suse.com>
@derekbit derekbit force-pushed the v2-volume-upgrade branch 2 times, most recently from f8a9517 to 7195ae0 Compare July 4, 2024 15:46
Longhorn 6001

Signed-off-by: Derek Su <derek.su@suse.com>
@derekbit
Copy link
Member Author

derekbit commented Jul 5, 2024

@DamiaSan I appended an extra commit for resolving build environment error. Can you review it again? Thank you.

@derekbit derekbit requested a review from DamiaSan July 5, 2024 03:15
The check function is commonly used in go-spdk-helper and longhorn-spdk-helper.

Longhorn 6001

Signed-off-by: Derek Su <derek.su@suse.com>
DamiaSan
DamiaSan previously approved these changes Jul 5, 2024
The change make LoadNVMeDeviceInfo be able to load NVMe device with different ip, port and nqn.

Longhorn 6001

Signed-off-by: Derek Su <derek.su@suse.com>
@derekbit
Copy link
Member Author

derekbit commented Jul 5, 2024

@DamiaSan Can you help review the latest commit? Thank you.

@derekbit derekbit merged commit 9f66e2c into longhorn:main Jul 6, 2024
7 checks passed
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.

2 participants