lvmd: unify VG and ThinPool behind storagePool interface #1057
Conversation
67c1364 to
deffae4
Compare
|
@peng225 The timeout appeared to be just a flaky failure, but I have retried the test. |
internal/lvmd/command/lvm.go
Outdated
| @@ -287,6 +288,17 @@ type ThinPoolUsage struct { | |||
| SizeBytes uint64 | |||
| } | |||
|
|
|||
| func (t ThinPoolUsage) Free(overprovisionRatio float64) (uint64, error) { | |||
There was a problem hiding this comment.
How about renaming this function like this?
| func (t ThinPoolUsage) Free(overprovisionRatio float64) (uint64, error) { | |
| func (t ThinPoolUsage) FreeBytes(overprovisionRatio float64) (uint64, error) { |
It's to clarify that this function is to return the remaining space in bytes. Free means deallocate something in many cases and it might confuse developers.
There was a problem hiding this comment.
Agreed, that sounds better!
deffae4 to
9f03694
Compare
| @@ -79,7 +79,7 @@ func ValidateDeviceClasses(deviceClasses []*lvmdTypes.DeviceClass) error { | |||
| return fmt.Errorf("thinpool name should not be empty: %s", dc.Name) | |||
| } | |||
|
|
|||
| if dc.ThinPoolConfig.OverprovisionRatio < 1.0 { | |||
| if dc.ThinPoolConfig.OverprovisionRatio <= 1.0 { | |||
There was a problem hiding this comment.
@llamerada-jp I have one question. The proposal of thinpool support describes "allowed values are greater than 1.0". On the other hands, this code allows 1.0 for this parameter. Furthermore, L83 describes "should be greater than 1.0". Could you tell me which is the original intention?
There was a problem hiding this comment.
This is a difficult question.
Looking at the discussion at the time, it seems that 1.0 was finally accepted. However, since the comments and specifications have not been updated, it is likely that this difference has not been taken into consideration.
https://github.com/topolvm/topolvm/pull/442/files#r830792492
Since the current implementation allows values of 1.0, if there are sites that already have 1.0 set, an error will occur. If there is no technical reason to reject 1.0, I think it is better to avoid taking the risk of changing the check condition in this PR.
There was a problem hiding this comment.
I won't modify the check condition in this PR.
9f03694 to
af6579d
Compare
This cleans up duplicated branch logic, improves abstraction, and makes future extensions easier. - Introduce storagePool interface and storagePoolForDeviceClass factory - Add volumeGroupAdapter and thinPoolAdapter (which embed VG/ThinPool) to implement storagePool - Use storagePool methods in lvservice/vgservice, eliminating dc.Type switches - Rename ThinPool.Free to Usage; move overprovision logic into ThinPoolUsage.Free - Remove calcThinPoolFreeBytes utility Signed-off-by: Kyori Sakao <sakaok@cybozu.co.jp>
af6579d to
9fe4a53
Compare
This cleans up duplicated branch logic, improves abstraction, and makes future extensions easier.