Add more missing features from mock csi driver by avalluri · Pull Request #269 · kubernetes-csi/csi-driver-host-path

avalluri · 2021-03-29T07:45:08Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
With the combination of #260, this enables in replacing the Mock CSI driver with host-path driver in Kubernetes E2E testing as described in #247.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
This PR is based on changes in #260.

Does this PR introduce a user-facing change?:


* Added support for configuring the maximum number of volumes that could be attached on a node using `--attach-limit`.
* New command-line option `--enable-topology` for enabling/disabling driver topology.
* New command-line option `--node-expand-required` for enabling/disabling volume expansion feature.

k8s-ci-robot · 2021-03-29T07:45:16Z

Hi @avalluri. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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.

pohly · 2021-03-29T21:23:38Z

/ok-to-test

pohly · 2021-04-07T16:20:52Z

-	mutex        sync.Mutex
-	volumes      map[string]hostPathVolume
-	snapshots    map[string]hostPathSnapshot
-	capacity     Capacity


Capacity must be protected by the mutex. That is no longer obvious.

Perhaps a better solution for it would be to have a read-only copy of the total capacity in config and dynamically calculate the remaining capacity by substracting the size of all current volumes.

I have another, older PR which also adds another parameter and partly fixed the capacity check: #253

Here's a proposal:

I change how capacity is tracked in that PR.

I add your config struct commit.

I add my new value to it.

Once that is merged, you can rebase your PR.

Sounds good,, Once your PR is ready, I will rebase mine.

pohly

The rest looks okay to me.

pohly · 2021-04-08T18:17:47Z

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

avalluri · 2021-04-08T19:24:58Z

@avalluri: The following test failed, say /retest to rerun all failed tests:

I will check what caused the failure.

pohly · 2021-04-08T20:08:17Z

I will check what caused the failure.

I have a fix for the install issue in #253. The code itself probably also needed some of the changes there.

avalluri · 2021-04-09T10:17:16Z

/retest

pohly · 2021-04-09T11:45:57Z

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

pohly · 2021-04-09T11:48:03Z

-	maxVolumesPerNode = flag.Int64("maxvolumespernode", 0, "limit of volumes per node")
-	showVersion       = flag.Bool("version", false, "Show version.")
-	capacity          = func() hostpath.Capacity {
-		c := hostpath.Capacity{}


This initialization here must not get dropped. Dropping it causes a nil-map access when a parameter gets set.

#253 changes hostpath.Capacity so that a nil pointer is okay.

avalluri · 2021-04-09T16:34:19Z

Rebased on #253

avalluri · 2021-04-09T16:35:21Z

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

This is _on_ by default. If disabled, the driver does not add VOLUME_ACCESSIBILITY_CONSTRAINTS to its plugin capabilities and also does not add AccessibiilityTopology information for the volumes. This makes it possible to test some more code paths in external-provisioner.

It is _on_ default. When it is disabled, driver does not add RPC_EXPAND_VOLUME to its capabilities list. This is the feature supported by mock CSI driver.

avalluri · 2021-04-16T11:28:50Z

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

k8s-ci-robot · 2021-04-16T12:05:20Z

@avalluri: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-kubernetes-csi-csi-driver-host-path-distributed-on-kubernetes-master	`6fe4041`	link	`/test pull-kubernetes-csi-csi-driver-host-path-distributed-on-kubernetes-master`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

pohly · 2021-04-16T13:07:01Z

External Storage [Driver: hostpath.csi.k8s.io] [Testpattern: Dynamic PV (immediate binding)] topology should provision a volume and schedule a pod with AllowedTopologies is a new failure that also occurs without this PR (#274). We can ignore it for this PR.

pohly

The code is fine, but the release notes entry only reflects one of the four feature commits. Please also mention the other new options.

avalluri · 2021-04-19T06:59:08Z

The code is fine, but the release notes entry only reflects one of the four feature commits. Please also mention the other new options.

Updated the release notes with two other command-line options.

pohly

/lgtm
/approve

k8s-ci-robot · 2021-04-19T08:44:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: avalluri, pohly

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

The pull request process is described here

Details

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

gnufied · 2022-01-14T14:59:47Z

 	flag.Int64Var(&cfg.MaxVolumeSize, "max-volume-size", 1024*1024*1024*1024, "maximum size of volumes in bytes (inclusive)")
-
+	flag.BoolVar(&cfg.EnableTopology, "enable-topology", true, "Enables PluginCapability_Service_VOLUME_ACCESSIBILITY_CONSTRAINTS capability.")
+	flag.BoolVar(&cfg.EnableVolumeExpansion, "node-expand-required", true, "Enables NodeServiceCapability_RPC_EXPAND_VOLUME capacity.")


This variable and configuration option is strangely named. It says "node-expand-required" but in truth, it controls entire volume expansion feature. I would like us to rename this.

I haven't looked into it, but that sounds reasonable. Can you submit a PR?

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 29, 2021

k8s-ci-robot requested review from saad-ali and xing-yang March 29, 2021 07:45

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 29, 2021

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Mar 29, 2021

avalluri mentioned this pull request Mar 29, 2021

merge mock and hostpath driver #247

Closed

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 29, 2021

avalluri force-pushed the replace-mock-driver-2 branch 2 times, most recently from 68887e1 to 5fcc26d Compare April 7, 2021 14:24

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 7, 2021

pohly reviewed Apr 7, 2021

View reviewed changes

pohly mentioned this pull request Apr 8, 2021

Volume size #253

Merged

pohly reviewed Apr 9, 2021

View reviewed changes

avalluri force-pushed the replace-mock-driver-2 branch from fbaa9fb to 2d4c721 Compare April 9, 2021 16:33

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 9, 2021

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

avalluri added 4 commits April 16, 2021 10:43

simplify the boolean condition check

10e4bc1

Made volume expansion optional

97fa8f7

It is _on_ default. When it is disabled, driver does not add RPC_EXPAND_VOLUME to its capabilities list. This is the feature supported by mock CSI driver.

Support for configuring volume attachable limit on a node.

6fe4041

avalluri force-pushed the replace-mock-driver-2 branch from 2d4c721 to 6fe4041 Compare April 16, 2021 07:53

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 16, 2021

pohly requested changes Apr 16, 2021

View reviewed changes

pohly approved these changes Apr 19, 2021

View reviewed changes

k8s-ci-robot assigned pohly Apr 19, 2021

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

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

k8s-ci-robot merged commit e4d72e3 into kubernetes-csi:master Apr 19, 2021

gnufied reviewed Jan 14, 2022

View reviewed changes

Conversation

avalluri commented Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 29, 2021

Uh oh!

pohly commented Mar 29, 2021

Uh oh!

pohly Apr 7, 2021

Choose a reason for hiding this comment

Uh oh!

pohly Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

avalluri Apr 8, 2021

Choose a reason for hiding this comment

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

pohly commented Apr 8, 2021

Uh oh!

avalluri commented Apr 8, 2021

Uh oh!

pohly commented Apr 8, 2021

Uh oh!

avalluri commented Apr 9, 2021

Uh oh!

pohly commented Apr 9, 2021

Uh oh!

pohly Apr 9, 2021

Choose a reason for hiding this comment

Uh oh!

avalluri commented Apr 9, 2021

Uh oh!

avalluri commented Apr 9, 2021

Uh oh!

avalluri commented Apr 16, 2021

Uh oh!

k8s-ci-robot commented Apr 16, 2021

Uh oh!

pohly commented Apr 16, 2021

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

avalluri commented Apr 19, 2021

Uh oh!

pohly left a comment

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 19, 2021

Uh oh!

gnufied Jan 14, 2022

Choose a reason for hiding this comment

Uh oh!

pohly Jan 15, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

avalluri commented Mar 29, 2021 •

edited

Loading