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

🧹 Split device discovery from partition discovery in blockdevices.go #4115

Merged
merged 1 commit into from
May 28, 2024

Conversation

preslavgerchev
Copy link
Contributor

Split device discovery from partition discovery entirely. This means that the code is now more predictable and makes less assumptions.

  1. When we have a device name or a LUN arg, we first identify the specific device
  2. We search for partitions only in that device. We don't loop around the other devices
  3. Deprecate the old code. We need to clean it after we upgrade the AWS snapshot connection.

Copy link
Contributor

github-actions bot commented May 28, 2024

Test Results

3 031 tests  +13   3 030 ✅ +13   1m 18s ⏱️ -2s
  337 suites ± 0       1 💤 ± 0 
   24 files   ± 0       0 ❌ ± 0 

Results for commit e3c7f44. ± Comparison against base commit bb16281.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@afiune afiune left a comment

Choose a reason for hiding this comment

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

return nil, err
}
var device snapshot.BlockDevice
var deviceErr error
// if we have exactly one device present at the LUN we can directly point the volume mounter towards it
Copy link
Contributor

Choose a reason for hiding this comment

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

this needs to be update? we, on top, look for the block device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it says that we have exactly one device that is found through the LUN lookup, which is what the if check does

Copy link
Contributor

Choose a reason for hiding this comment

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

This is absolutely a nitpick, my point here was that, we added an extra step here, we are no longer just passing the volume path directly, we now try to find the block.

That's all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, i see your point, ive adjusted it

providers/os/connection/device/linux/device_manager.go Outdated Show resolved Hide resolved
@@ -57,7 +57,7 @@ func filterScsiDevices(scsiDevices scsiDevices, lun int) []scsiDeviceInfo {
// there can be multiple devices mounted at the same LUN.
// the LUN so we need to find all the blocks, mounted at that LUN. then we find the first one
// that has no mounted partitions and use that as the target device. this is a best-effort approach
Copy link
Contributor

Choose a reason for hiding this comment

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

could we please update this comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

what would you like to have updated? it still is relevant to the code it refers to

providers/os/connection/device/linux/lun.go Outdated Show resolved Hide resolved
providers/os/connection/device/linux/lun_test.go Outdated Show resolved Hide resolved
providers/os/connection/snapshot/blockdevices.go Outdated Show resolved Hide resolved
providers/os/connection/snapshot/blockdevices.go Outdated Show resolved Hide resolved
sortPartitionsBySize(partitions)

// return the largest partition. we can extend this to be a parameter in the future
devFsName := "/dev/" + partitions[0].Name
Copy link
Contributor

Choose a reason for hiding this comment

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

If we only sort the partitions to return the largest one, if feels a waste of processing.

I know that we won't have a huge number of partitions so, processing here is low-cost, but if feels that the sorting is unnecessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i can also replace this with iterating over and keeping track of the one with the largest size. this just felt like less code

providers/os/connection/snapshot/volumemounter.go Outdated Show resolved Hide resolved
@preslavgerchev preslavgerchev merged commit c47279a into main May 28, 2024
15 checks passed
@preslavgerchev preslavgerchev deleted the preslav/parts-fix branch May 28, 2024 14:51
@github-actions github-actions bot locked and limited conversation to collaborators May 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants