Volume size #253

pohly · 2021-03-08T14:48:40Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

More complete storage capacity simulation.

Does this PR introduce a user-facing change?:

The hostpath driver now has a configurable fixed maximum volume size. It reports the minimum of that and the remaining capacity as `GetCapacityResponse.MaximumVolumeSize`. `GetCapacityResponse.MinimumVolumeSize` is always zero.

Fixes: #274

k8s-ci-robot · 2021-04-08T18:28:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [pohly]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pohly · 2021-04-08T18:29:41Z

@avalluri I took some of your commits from PR #269, added some fixes and rewrote the capacity tracking as I mentioned there.

Can you perhaps review this PR?

pohly · 2021-04-08T18:30:26Z

/test pull-kubernetes-csi-csi-driver-host-path-distributed-on-kubernetes-master

This job is relevant because that is where the driver gets deployed with capacity tracking. It's still experimental and therefore not enabled by default.

pohly · 2021-04-08T19:22:46Z

/test pull-kubernetes-csi-csi-driver-host-path-distributed-on-kubernetes-master

avalluri · 2021-04-08T20:25:11Z

/lgtm

k8s-ci-robot · 2021-04-08T20:25:12Z

@avalluri: changing LGTM is restricted to collaborators

In response to this:

/lgtm

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

avalluri · 2021-04-08T20:25:58Z

Can you perhaps review this PR?

Looks good to me.

verult · 2021-04-08T23:23:35Z

/assign

verult

LGTM otherwise! Sorry for the delay

verult · 2021-04-15T05:48:29Z

pkg/hostpath/capacity.go

@@ -50,24 +50,27 @@ func (c Capacity) Set(arg string) error {
 	}

 	// We overwrite any previous value.
-	c[parts[0]] = quantity
+	if *c == nil {
+		*c = Capacity{}


c is still set to hostpath.Capacity{} before flag declaration, so *c is never nil. Did you mean to remove that line?

This is a bit risky: if c is nil then *c == nil results in a panic. It's easy for others working on this code to make a mistake.

Pre-allocating memory for an empty map might be better comparing to opening up risks for panics? Although do you happen to know how much memory it eats up?

Pre-allocating memory for an empty map might be better comparing to opening up risks for panics?

That's what I did earlier in

csi-driver-host-path/cmd/hostpathplugin/main.go

Line 45 in 9734686

c := hostpath.Capacity{}

Then someone else removed that line without realizing that it is needed, leading to a panic when setting the parameter. It would be nicer if a default-initialized Capacity (= nil map instead of map[string]resource.Quantity{}) just worked, which is what I am trying to achieve here.

if c is nil then *c == nil results in a panic. It's easy for others working on this code to make a mistake.

How can c be nil? Someone would have to explicitly declare a pointer to a Capacity and then call Set for that pointer. That is not something that I would expect to work unless explicitly documented for a type, so I think panicking in that case is fine.

Ah ok thanks for the context! nit: add a comment for the rationale in case anyone in the future wonders why this is done.

How can c be nil? Someone would have to explicitly declare a pointer to a Capacity and then call Set for that pointer. That is not something that I would expect to work unless explicitly documented for a type, so I think panicking in that case is fine.
That's true, ack

Added. Please review and LGTM.

verult · 2021-04-15T05:51:27Z

cmd/hostpathplugin/main.go

 	// Set by the build process
 	version = ""
 )

 func main() {
+	var cfg hostpath.Config


Nice refactor!

verult · 2021-04-15T05:58:44Z

cmd/hostpathplugin/main.go

@@ -55,6 +55,7 @@ func main() {
 	// The proxy-endpoint option is intended to used by the Kubernetes E2E test suite
 	// for proxying incoming calls to the embedded mock CSI driver.
 	proxyEndpoint := flag.String("proxy-endpoint", "", "Instead of running the CSI driver code, just proxy connections from csiEndpoint to the given listening socket.")
+	flag.Int64Var(&cfg.MaxVolumeSize, "maxvolumesize", 1024*1024*1024*1024, "maximum size of volumes in bytes")


nit: Keep cfg flags together

nit: name it "max-volume-size" to be consistent with word delineation in other flags

verult · 2021-04-15T06:00:49Z

pkg/hostpath/controllerserver.go

 	if hp.config.Capacity.Enabled() {
 		kind := req.GetParameters()[storageKind]
 		quantity := hp.config.Capacity.Check(kind)
 		available = quantity.Value()
 	}
+	maxVolumeSize := hp.config.MaxVolumeSize
+	if maxVolumeSize < available {
+		maxVolumeSize = available


My understanding was the intention of MaxVolumeSize is that the underlying storage system may not be able to provision a volume as large as all available capacity (for example due to fragmentation). Should we allow for ability to emulate the case where maxVolumeSize < available then?

Did you mean if maxVolumeSize > available? If so, consider logging available and the previous maxVolumeSize to let the user know maxVolumeSize got truncated.

The comparison is indeed backwards. I really wish Go had a min(a,b) template function 😢

Fixed. I did not add a log message for this though, because the result should be obvious also without it.

verult · 2021-04-15T07:23:42Z

pkg/hostpath/capacity.go

-// Alloc reserves a certain amount of bytes. Errors are
-// usable as result of gRPC calls. Empty kind means
-// that any large enough one is fine.
-func (c *Capacity) Alloc(kind string, size int64) (actualKind string, err error) {


This whole commit is pretty great! Much simpler

Yes, I should have done it like that from the start. Oh well.

verult · 2021-04-15T07:57:33Z

/lgtm
/hold

Since this PR is blocking a few other ones, please feel free to cancel the hold and address the remaining comments in a separate PR if needed.

pohly · 2021-04-15T09:25:19Z

@verult please take another look, I pushed the updated commits.

pohly · 2021-04-15T09:25:44Z

/test pull-kubernetes-csi-csi-driver-host-path-distributed-on-kubernetes-master

verult · 2021-04-15T17:42:12Z

/lgtm

Keeping hold in case you want to address the nit

The map had to be allocated in advance for Set(). By using a pointer receiver the Set function itself can allocate the map if (and only if) needed.

Introduced new Config structure to hold driver configuration, this way it is convenient to pass them to NewHostPathDriver().

Less code this way...

The new spec version makes it possible to report maximum volume size.

Maximum volume size is now configurable, with 1TiB as default. Previously, that value was hard-coded. The GetCapacityResponse always includes a value for MaximumVolumeSize which is the minimum of that fixed size and available capacity. MinimumVolumeSize is always set to zero, for the sake of completeness.

The previous approach of adapting remaining capacity while adding or removing volumes was unnecessarily complex. When we accept that we need to sum up allocated space repeatedly, the remaining code becomes simpler.

The introduction of sed had input/output backwards.

pohly · 2021-04-15T18:24:03Z

/hold cancel

verult · 2021-04-15T18:36:24Z

/lgtm

4967685 Merge pull request kubernetes-csi#254 from bells17/add-github-actions d9bd160 Update skip list in codespell GitHub Action adb3af9 Merge pull request kubernetes-csi#252 from bells17/update-go-version f5aebfc Add GitHub Actions workflows b82ee38 Merge pull request kubernetes-csi#253 from bells17/fix-typo c317456 Fix typo 0a78505 Bump to Go 1.22.3 edd89ad Merge pull request kubernetes-csi#251 from jsafrane/add-logcheck 043fd09 Add test-logcheck target d7535ae Merge pull request kubernetes-csi#250 from jsafrane/go-1.22 b52e7ad Update go to 1.22.2 14fdb6f Merge pull request kubernetes-csi#247 from msau42/prow 9b4352e Update release playbook c7bb972 Fix release notes script to use fixed tags 463a0e9 Add script to update specific go modules git-subtree-dir: release-tools git-subtree-split: 4967685

Fix typo

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. labels Mar 8, 2021

k8s-ci-robot requested review from msau42 and vladimirvivien March 8, 2021 14:48

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 8, 2021

pohly force-pushed the volume-size branch 2 times, most recently from 56397e3 to 13f8c54 Compare March 9, 2021 09:50

pohly changed the title ~~Volume size~~ WIP: Volume size Mar 10, 2021

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2021

pohly force-pushed the volume-size branch from 2501c12 to bb5542b Compare March 10, 2021 08:53

pohly changed the title ~~WIP: Volume size~~ Volume size Mar 10, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2021

pohly force-pushed the volume-size branch from bb5542b to d9174c7 Compare March 19, 2021 08:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 19, 2021

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2021

pohly mentioned this pull request Apr 8, 2021

Add more missing features from mock csi driver #269

Merged

pohly force-pushed the volume-size branch from d9174c7 to 110d3d0 Compare April 8, 2021 18:28

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 8, 2021

k8s-ci-robot assigned verult Apr 8, 2021

pohly mentioned this pull request Apr 15, 2021

distributed-on-master failing #274

Closed

verult suggested changes Apr 15, 2021

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Apr 15, 2021

pohly force-pushed the volume-size branch from cd15eab to 79794b6 Compare April 15, 2021 09:24

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2021

pohly and others added 7 commits April 15, 2021 20:22

capacity: support nil map for parameters

efdf20e

The map had to be allocated in advance for Set(). By using a pointer receiver the Set function itself can allocate the map if (and only if) needed.

Move driver flags into config structure

9ab5c19

Introduced new Config structure to hold driver configuration, this way it is convenient to pass them to NewHostPathDriver().

config: initialize VendorVersion directly

7b3e7f9

Less code this way...

update to CSI 1.4.0

acf764a

The new spec version makes it possible to report maximum volume size.

capacity: read-only Capacity struct

6ad3189

The previous approach of adapting remaining capacity while adding or removing volumes was unnecessarily complex. When we accept that we need to sum up allocated space repeatedly, the remaining code becomes simpler.

deploy: fix script for distributed provisioning

6ec1713

The introduction of sed had input/output backwards.

pohly force-pushed the volume-size branch from 79794b6 to 6ec1713 Compare April 15, 2021 18:23

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 15, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 15, 2021

k8s-ci-robot merged commit 85a7174 into kubernetes-csi:master Apr 15, 2021

TerryHowe pushed a commit to TerryHowe/csi-driver-host-path that referenced this pull request Oct 17, 2024

Merge pull request kubernetes-csi#253 from bells17/fix-typo

b82ee38

Fix typo

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Volume size #253

Volume size #253

pohly commented Mar 8, 2021 •

edited

Loading

k8s-ci-robot commented Apr 8, 2021

pohly commented Apr 8, 2021

pohly commented Apr 8, 2021

pohly commented Apr 8, 2021

avalluri commented Apr 8, 2021

k8s-ci-robot commented Apr 8, 2021

avalluri commented Apr 8, 2021

verult commented Apr 8, 2021

verult left a comment

verult Apr 15, 2021

pohly Apr 15, 2021

verult Apr 15, 2021

pohly Apr 15, 2021

verult Apr 15, 2021

verult Apr 15, 2021

pohly Apr 15, 2021

verult Apr 15, 2021

verult Apr 15, 2021

pohly Apr 15, 2021

verult Apr 15, 2021

pohly Apr 15, 2021

verult commented Apr 15, 2021

pohly commented Apr 15, 2021

pohly commented Apr 15, 2021

verult commented Apr 15, 2021

pohly commented Apr 15, 2021

verult commented Apr 15, 2021

Volume size #253

Volume size #253

Conversation

pohly commented Mar 8, 2021 • edited Loading

k8s-ci-robot commented Apr 8, 2021

pohly commented Apr 8, 2021

pohly commented Apr 8, 2021

pohly commented Apr 8, 2021

avalluri commented Apr 8, 2021

k8s-ci-robot commented Apr 8, 2021

avalluri commented Apr 8, 2021

verult commented Apr 8, 2021

verult left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented Apr 15, 2021

pohly commented Apr 15, 2021

pohly commented Apr 15, 2021

verult commented Apr 15, 2021

pohly commented Apr 15, 2021

verult commented Apr 15, 2021

pohly commented Mar 8, 2021 •

edited

Loading