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

rbd: allow importing/exporting block devices #49358

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

petrutlucian94
Copy link
Contributor

@petrutlucian94 petrutlucian94 commented Dec 9, 2022

rbd: allow importing/exporting block devices

This change checks if we're exporting to or importing from a block device. If so, we have to:

  • ensure that writes are sector aligned
  • ensure that writes are not skipped in case of zero buffers
  • avoid fstat, use blkdev.get_size to retrieve the disk size (DeviceIoControl on Windows)
  • avoid ftruncate
  • avoid opening with O_CREAT | O_EXCL

This enables to do the following:

set-disk -Number 1 -IsReadOnly $false -IsOffline $true
rbd export test_64m@snap2 \\.\PhysicalDrive1
rbd import \\.\PhysicalDrive1 test_64m_import

Contribution Guidelines

Checklist

  • Tracker (select at least one)
    • References tracker ticket
    • Very recent bug; references commit where it was introduced
    • New feature (ticket optional)
    • Doc update (no ticket needed)
    • Code cleanup (no ticket needed)
  • Component impact
    • Affects Dashboard, opened tracker ticket
    • Affects Orchestrator, opened tracker ticket
    • No impact that needs to be tracked
  • Documentation (select at least one)
    • Updates relevant documentation
    • No doc update is appropriate
  • Tests (select at least one)
Show available Jenkins commands
  • jenkins retest this please
  • jenkins test classic perf
  • jenkins test crimson perf
  • jenkins test signed
  • jenkins test make check
  • jenkins test make check arm64
  • jenkins test submodules
  • jenkins test dashboard
  • jenkins test dashboard cephadm
  • jenkins test api
  • jenkins test docs
  • jenkins render docs
  • jenkins test ceph-volume all
  • jenkins test ceph-volume tox
  • jenkins test windows

@petrutlucian94 petrutlucian94 requested a review from a team as a code owner December 9, 2022 09:52
@petrutlucian94 petrutlucian94 added the win32 Specifix changes for the windows platform label Dec 9, 2022
@petrutlucian94
Copy link
Contributor Author

Wasn't sure whether to mark this as a bug or as a feature. We used to have this debate in Openstack and Nova cores usually said that if a patch enables a feature that never worked before, it's considered a feature and not a bug fix.

@github-actions
Copy link

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

@petrutlucian94
Copy link
Contributor Author

jenkins test windows

src/common/win32/blkdev.cc Show resolved Hide resolved
src/common/win32/blkdev.cc Outdated Show resolved Hide resolved
src/tools/rbd/Utils.cc Outdated Show resolved Hide resolved
src/tools/rbd/Utils.cc Outdated Show resolved Hide resolved
src/tools/rbd/Utils.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Show resolved Hide resolved
src/tools/rbd/action/Import.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Import.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Import.cc Outdated Show resolved Hide resolved
@idryomov
Copy link
Contributor

This enables to do the following:
[...]
rbd export-diff test_64m@snap2 --from-snap snap1 snap2_snap1

I don't see anything special about this command -- it seems to be exporting to a regular file?

@petrutlucian94
Copy link
Contributor Author

petrutlucian94 commented Jan 4, 2023

This enables to do the following:
[...]
rbd export-diff test_64m@snap2 --from-snap snap1 snap2_snap1

I don't see anything special about this command -- it seems to be exporting to a regular file?

Indeed, initially I was thinking about showcasing a block device but it probably doesn't really make sense to export a diff to a raw block device.

I'll address the remaining comments ASAP.

@petrutlucian94 petrutlucian94 marked this pull request as draft January 4, 2023 14:29
@github-actions github-actions bot added the tests label Jan 5, 2023
@petrutlucian94 petrutlucian94 marked this pull request as ready for review January 5, 2023 11:42
@petrutlucian94 petrutlucian94 changed the title rbd: allow importing/exporting Windows physical disks rbd: allow importing/exporting block devices Jan 5, 2023
@petrutlucian94
Copy link
Contributor Author

jenkins test make check

@petrutlucian94
Copy link
Contributor Author

jenkins test windows

@petrutlucian94
Copy link
Contributor Author

jenkins test api

@rzarzynski
Copy link
Contributor

From core PR scrub: the change in common isn't interesting from the core's PoV.

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Aug 26, 2023
@idryomov idryomov removed the stale label Aug 27, 2023
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@idryomov idryomov removed the stale label Oct 26, 2023
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@github-actions github-actions bot added the stale label Dec 25, 2023
@idryomov idryomov removed the stale label Dec 26, 2023
Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@petrutlucian94
Copy link
Contributor Author

jenkins test make check

1 similar comment
@petrutlucian94
Copy link
Contributor Author

jenkins test make check

@petrutlucian94
Copy link
Contributor Author

@idryomov this PR already went through a round of reviews, do you think we can get it in?

@idryomov
Copy link
Contributor

idryomov commented Mar 5, 2024

@idryomov this PR already went through a round of reviews, do you think we can get it in?

The workunit isn't wired up (i.e. wouldn't actually run), so that would need to be fixed at the very least. I'll try to do another pass sometime this week or early next week.

Copy link
Contributor

@idryomov idryomov left a comment

Choose a reason for hiding this comment

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

I'll have more comments on the workunit, but wanted to get clarity on the use cases for exporting and some semantics first.

src/tools/rbd/Utils.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
@petrutlucian94 petrutlucian94 force-pushed the rbd_import branch 2 times, most recently from f68ff0a to 6d8174b Compare March 13, 2024 09:40
@petrutlucian94
Copy link
Contributor Author

jenkins test make check

@petrutlucian94
Copy link
Contributor Author

@idryomov Thanks for reviewing this patch, please let me know if there's anything else that needs to be updated.

src/tools/rbd/Utils.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Import.cc Show resolved Hide resolved
src/tools/rbd/action/Import.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Show resolved Hide resolved
src/tools/rbd/action/Export.cc Outdated Show resolved Hide resolved
src/tools/rbd/action/Export.cc Show resolved Hide resolved
src/tools/rbd/action/Import.cc Outdated Show resolved Hide resolved
qa/workunits/rbd/krbd_export_import.sh Outdated Show resolved Hide resolved
@petrutlucian94 petrutlucian94 force-pushed the rbd_import branch 2 times, most recently from 8cd6122 to 9e8d6f6 Compare March 21, 2024 13:44
@petrutlucian94
Copy link
Contributor Author

I've prepared some Windows tests as well:

petrutlucian94@62d9178

That commit is based on another PR, which breaks the Windows Python tests into separate modules and makes the code reusable: #52560. I think I'll wait for this PR to merge and then I'll submit the Windows export tests there.

This change checks if we're exporting to or importing from a block
device. If so, we have to:

* ensure that writes are sector aligned (Windows)
* ensure that writes are not skipped in case of zero buffers
* avoid fstat, use blkdev.get_size to retrieve the disk size
  (DeviceIoControl on Windows)
* avoid ftruncate
* avoid opening with O_CREAT | O_EXCL

The import/export test is updated to cover block devices as well.

Signed-off-by: Lucian Petrut <lpetrut@cloudbasesolutions.com>
@petrutlucian94
Copy link
Contributor Author

jenkins test make check arm64

1 similar comment
@petrutlucian94
Copy link
Contributor Author

jenkins test make check arm64

@petrutlucian94
Copy link
Contributor Author

@idryomov I've addressed the comments and squashed the commits, I think it's ready to merge. Thanks again for the review.

@petrutlucian94
Copy link
Contributor Author

jenkins test make check arm64

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common feature rbd tests win32 Specifix changes for the windows platform
Projects
None yet
3 participants