Update Volume API #138
Merged
Update Volume API #138
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
k8s-ci-robot
added
the
size/XXL
mauriciopoppe
force-pushed
the
volume-api-changes
branch
2 times, most recently
from
8d99202 to
fbb215a
Compare
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
|
/uncc @kkmsft @andyzhangx |
k8s-ci-robot
requested review from
ddebroy,
jingxu97 and
msau42
and removed request for
andyzhangx and
kkmsft
mauriciopoppe
commented
May 12, 2021
jingxu97
reviewed
May 13, 2021
jingxu97
reviewed
May 13, 2021
jingxu97
reviewed
May 13, 2021
jingxu97
reviewed
May 13, 2021
mauriciopoppe
force-pushed
the
volume-api-changes
branch
from
48a80a6 to
fa9e209
Compare
|
I need to change disk number to |
k8s-ci-robot
added
the
do-not-merge/hold
|
/unhold |
k8s-ci-robot
removed
the
do-not-merge/hold
mauriciopoppe
force-pushed
the
volume-api-changes
branch
2 times, most recently
from
fcfd54c to
9409e62
Compare
pohly
added a commit
to pohly/csi-proxy
that referenced
this pull request
May 21, 2021
a1e1127 Merge pull request kubernetes-csi#139 from pohly/kind-for-kubernetes-latest 1c0fb09 prow.sh: use KinD main for latest Kubernetes 1d77cfc Merge pull request kubernetes-csi#138 from pohly/kind-update-0.10 bff2fb7 prow.sh: KinD 0.10.0 git-subtree-dir: release-tools git-subtree-split: a1e11275b5a4febd6ad21beeac730e22c579825b
mauriciopoppe
force-pushed
the
volume-api-changes
branch
2 times, most recently
from
b4244fd to
ddb4140
Compare
mauriciopoppe
force-pushed
the
volume-api-changes
branch
from
ddb4140 to
be08a70
Compare
|
/unhold I've run the volume e2e tests in a GCE windows instance with Hyper-V enabled, these are the results: |
k8s-ci-robot
removed
the
do-not-merge/hold
mauriciopoppe
force-pushed
the
volume-api-changes
branch
from
7b9d340 to
eda65f3
Compare
|
@jingxu97 @ddebroy I've added tests for v1alpha1, v1beta1 and v1beta2, the output is: |
jingxu97
reviewed
May 25, 2021
jingxu97
reviewed
May 26, 2021
| } | ||
| // For a volume formatted with 1GB it should be around 1GB, in practice it was 1056947712 bytes or 0.9844GB | ||
| // let's compare with a range of - 20MB | ||
| if !sizeIsAround(t, volumeStatsResponse.VolumeSize, vhd.InitialSize) { |
There was a problem hiding this comment.
how it was handled before your change?
There was a problem hiding this comment.
this wasn't tested before, I added this part
jingxu97
reviewed
May 26, 2021
| // (considers the fact that some bytes were lost) | ||
| func sizeIsAround(t *testing.T, actualSize, expectedSize int64) bool { | ||
| // An upper bound on the number of bytes that are lost when creating or resizing a partition | ||
| var volumeSizeBytesLoss int64 = (20 * 1024 * 1024) |
There was a problem hiding this comment.
how this volumeSizeBytesLoss is obtained? Is there some doc related to this?
There was a problem hiding this comment.
I got this value from looking at the GetStats output after you format a partition, if you format it to 1GB you'r getting actually ~0.9844GB, probably @ddebroy might help us understanding where are those bytes going :)
Anyways if you do 1GB - 20MB that's around 0.98GB which I thought it was a good lower bound
|
/lgtm |
k8s-ci-robot
added
the
lgtm
mauriciopoppe
commented
Aug 21, 2021
| @@ -177,36 +213,36 @@ func (VolAPIImplementor) VolumeStats(volumeID string) (int64, int64, error) { | |||
| volumeSizeRemaining = getVolume["SizeRemaining"] | |||
|
|
|||
| volumeUsedSize := volumeSize - volumeSizeRemaining | |||
| return volumeSizeRemaining, volumeUsedSize, nil | |||
| return volumeSize, volumeUsedSize, nil | |||
There was a problem hiding this comment.
This issue was raised during one of our meetings:
totalBytes, usedBytes, err := s.hostAPI.GetVolumeStats(volumeID)
volumeSize = getVolume["Size"]
volumeSizeRemaining = getVolume["SizeRemaining"]
volumeUsedSize := volumeSize - volumeSizeRemaining
return volumeSizeRemaining, volumeUsedSize, nil
// proposed
return volumeSize, volumeUsedSize, nil
Have we ever attempted to do a resize? I tried running a resize from 1GB to 2GB and I get:
PS C:\Users\mauriciopoppe\go\src\github.com\kubernetes-csi\csi-proxy> Get-Volume -UniqueId "\\?\Volume{1b6810f3-20a3-4a2f-bd7b-3b36f05265d4}\" | Get-partition
DiskPath: \\?\scsi#disk&ven_msft&prod_virtual_disk#2&1f4adffe&0&000006#{53f56307-b6bf-11d0-94f2-00a0c91efb8b}
PartitionNumber DriveLetter Offset Size Type
--------------- ----------- ------ ---- ----
2 16777216 1007.94 MB Basic
PS C:\Users\mauriciopoppe\go\src\github.com\kubernetes-csi\csi-proxy> Get-Volume -UniqueId "\\?\Volume{1b6810f3-20a3-4a2f-bd7b-3b36f05265d4}\" | Get-partition | Resize-Partition -Size 2147483648
Resize-Partition : Not enough available capacity
Activity ID: {229be40a-070d-41ff-b027-d71015bacf61}
At line:1 char:92
+ ... b-3b36f05265d4}\" | Get-partition | Resize-Partition -Size 2147483648
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ CategoryInfo : NotSpecified: (StorageWMI:ROOT/Microsoft/.../MSFT_Partition) [Resize-Partition], CimException
+ FullyQualified
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
/kind feature
What this PR does / why we need it:
Updates the Volume v1beta3 API according to the API review in #130, in addition to that:
client/groups.volume/v1beta3/client_generated.go, I synced these changes back to the vendor folder too so that the server can use themAPItointernal/os/volume/api.gobecause it belongs here, previously the owner was the wrapper, this fixed an issue where the host API wasn't fulfilling any interface and generated problems while renaming thingsintegrationtests/volume_test.goto usediskNumber(uint32) instead of diskID (string)Regenerated theinternal/server/volume/internal/v1Xfiles, the older APIs are incompatible with the new parameters in the v1beta3 proto file, I didn't want to touch the autogenerated for older APIs but unfortunately I can't compile the code if I don't make these changesinternal/server/volume/internal/v1Xfiles, for pre v1beta3 versions I added more conversion fields toconversion.go, that way the internal server representation of the requests from any API is the same (no special cases for pre v1beta3 APIs aside from the pre v1beta3 methods)diskNumberI removed this test and instead I'm checking for negative valuesWhich issue(s) this PR fixes:
Fixes #130
Does this PR introduce a user-facing change?: