Introduce 4096 byte Sector Size Minimum for Rounding LV Sizes on Creation and Resizing

[jakobmoellerdev]

This PR adds a 4096 Minimum Size limitation for all Logical Volumes that are created. The reason is that if one decides to use byte level precision on a PVC, that byte amount is now passed through to lvcreate. However if this byte-amount is not a multiple of the sector size (for most machines either 512, 1024 or 4096) then LVM will throw an error like

Size is not a multiple of 512. Try using 221920768 or 221921280.
  Invalid argument for --virtualsize: 221920847b 

In this case, we should round down in order to allow for a PVC creation with slightly different Size than the one requested as this is much better than getting rejected the PVC scheduling.

The code is now changed to automatically round down or up (based on wether the limit or the request bytes are used and which one is larger). For edge cases during rounding we introduce new error cases.

Note that even though the 4KiB limit here is important for validation, the size might in the end still be rounded up to the nearest physical extent size (4MiB by default)

[daichimukai]

Hi @jakobmoellerdev,

Thank you for your PR! Looks like a reasonable change to me. Before we start a review of the implementation, let me discuss the implementation strategy.

I agree that we introduce LV size limitation to fit into the 4096 byte alignment. But I'm not sure it would be reasonable to implement this logic in lvmd.

The lvmd creates LVs with the size decided by the topolvm-controller based on the capacity_range field of the CSI's CreateVolume RPC request. According to the spec, capacity_range must be honored if specified. I'm worried that we will be violating this spec if lvmd rounds down the requested size. (For example, consider the case where the request in capacity_range is 4MiB+1B.)

// This field is OPTIONAL. This allows the CO to specify the capacity
// requirement of the volume to be provisioned. If not specified, the
// Plugin MAY choose an implementation-defined capacity range. If
// specified it MUST always be honored, even when creating volumes
// from a source; which MAY force some backends to internally extend
// the volume after creating it.
CapacityRange capacity_range = 2;

https://github.com/container-storage-interface/spec/blob/886cd4877954568f9722cbe14d46b9fc16379d62/spec.md#createvolume

So, I'll suggest the following strategy:

1. The topolvm-controller creates LogicalVolume CR with the size which is the request of capacity_range rounded up to fit into the 4096 byte alignment.
2. (optional) The lvmd checks if the requested LV size is aligned with the 4096 byte alignment.

Additionally, we might need to round up the size when the topolvm-controller adds requests to Pods.

[jakobmoellerdev]

Thanks for the input. Agreed that we can change the approach to make it better fit into the CSI Spec.

I just checked CapacityRange and it seems like there are two components to it: RequiredBytes and LimitBytes.

I suggest we use your approach then and do the following logic:

1. CreateVolume verifies req.GetCapacityRange().GetRequiredBytes() % 4096, if it does not solve to 0, round up to nearest 4096 sector. If that new rounded value is larger than req.GetCapacityRange().GetLimitBytes(), throw status.Error(codes.InvalidArgument, "capacity range is invalid, request was rounded up to nearest 4096 sector but cannot fit into capacity limit")
2. LVMD verifies that size % 4096 != 0 and if it is, we should throw an error

Towards your point of

Additionally, we might need to round up the size when the topolvm-controller adds requests to Pods.

Could you tell me what needs to change and why here? Then I will implement accordingly. Not sure what this means right now (maybe you mean the resource extensions on the Pod)

[daichimukai]

Additionally, we might need to round up the size when the topolvm-controller adds requests to Pods.

Could you tell me what needs to change and why here? Then I will implement accordingly. Not sure what this means right now (maybe you mean the resource extensions on the Pod)

I changed my mind. Let's leave it as is. When the scheduler decides a node to which allocate Pods, it would be better to know the accurate size of the LVs to create as much as possible. However, since the discrepancy is at most 4096 bytes, we can leave it as it is until it becomes a problem.

[daichimukai]

topolvm/driver/controller.go

Lines 433 to 442 in 976abaf

	// if requestBytes is 0, default to max(1Gi, limitBytes>0)
	// 1. if limitBytes is 0, default to 1Gi
	// 2. if limitBytes is greater than 1Gi, default to 1Gi
	// 3. if limitBytes is less than 1Gi, default to limitBytes
	if requestBytes == 0 {
	if limitBytes > 0 && topolvm.DefaultSize > limitBytes {
	return limitBytes, nil
	}
	return topolvm.DefaultSize, nil
	}

Take care of the case where requestByte == 0 and limitByte < 4096. It is not allowed now. It might be better not to use early returns and force return values always pass the "round up logic".

topolvm/driver/controller_test.go

Lines 32 to 38 in 976abaf

	v, err := convertRequestCapacityBytes(0, 10)
	if err != nil {
	t.Error("should not be error")
	}
	if v != 10 {
	t.Errorf("should be the limit capacity by default if 0 is supplied and limit is smaller than 1Gi: %d", v)
	}

Then this test case needs to be fixed.

[daichimukai]

@jakobmoellerdev
Thank you, LGTM. Please squash commits into one commit.

[daichimukai]

LGTM

[jakobmoellerdev]

@pluser mind taking a look over this one now that first review has passed?