Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test driver config #143

Merged
merged 24 commits into from
Feb 11, 2020
Merged

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Feb 10, 2020

What type of PR is this?
/kind cleanup

What this PR does / why we need it:

We want to get rid of hostpath driver specific code in csi-release-tools, both to simplify it and to make it applicable to other CSI drivers.

Which issue(s) this PR fixes:
Related-to kubernetes-csi/csi-release-tools#32

Special notes for your reviewer:

This needs to be rolled out like this:

  • merge this PR into csi-driver-host-path
  • do a 1.3.0-rc release
  • use that release as CSI_PROW_DRIVER_REVISION in prow.sh and update csi-release-tools
  • merge that change into all repos

Does this PR introduce a user-facing change?:

NONE

msau42 and others added 18 commits December 3, 2019 18:18
Improve README by adding an explicit Kubernetes dependency section
This requires adding one more parallel e2e test run with
a special focus flag because snapshot tests are still guarded
with a "[Feature:VolumeSnapshotDataSource]" tag. The setting that
skips all tests with "[Feature:.*]" has to be removed because it
overrides the focus.

We don't have serial snapshot tests yet. This needs to be modified
again if we add any in the future.
Enable snapshot tests in 1.17 to be run in non-alpha jobs.
…netes_fix

Fix version_gt to work with Kubernetes prefix
Update prow hostpath driver version to 1.3.0-rc2
…rsion_update

Update snapshotter to version v2.0.0
Update hostpath driver version to get fix for connection-timeout
Document the process for releasing a new sidecar
@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note-none Denotes a PR that doesn't merit a release note. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Feb 10, 2020
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 10, 2020
This relies on a slightly different deployment script: a "deploy.sh"
must exist which knows that it has to dump a test driver configurion
into the file pointed to with CSI_PROW_TEST_DRIVER, if that env
variable is set.

That way, we no longer need to know what capabilities the installed
driver has.
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 10, 2020
That this hasn't been done before is an oversight. Apparently it
hasn't been a problem because there never have been feature gates that
mattered?
@pohly pohly changed the title WIP: test driver config test driver config Feb 10, 2020
@k8s-ci-robot 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 Feb 10, 2020
@pohly
Copy link
Contributor Author

pohly commented Feb 10, 2020

/assign @msau42

Copy link
Collaborator

@msau42 msau42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

Name: hostpath.csi.k8s.io
Capabilities:
block: true
controllerExpansion: true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gnufied were resizing tests added in 1.16 or 1.17?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to git diff v1.15.0..v1.16.0 test/e2e/storage/testsuites/testdriver.go, controllerExpansion was added in 1.16:

@@ -126,7 +152,10 @@ const (
        // - NodeStageVolume in the spec
        CapMultiPODs Capability = "multipods"
 
-       CapRWX Capability = "RWX" // support ReadWriteMany access modes
+       CapRWX                 Capability = "RWX"                 // support ReadWriteMany access modes
+       CapControllerExpansion Capability = "controllerExpansion" // support volume expansion for controller
+       CapNodeExpansion       Capability = "nodeExpansion"       // support volume expansion for node
+       CapVolumeLimits                   = "volumeLimits"        // support volume limits (can be *very* slow)
 )

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shall we enable the tests in 1.16 too then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's missing for them in 1.16?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see the capabilities enabled in the 1.16 config

Copy link
Contributor Author

@pohly pohly Feb 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, right. I thought that the change above was for 1.16, but it's 1.17. Let me add it for 1.16 then, too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Revised test-driver.yaml files:

$ diff deploy/kubernetes-1.15/test-driver.yaml deploy/kubernetes-1.16/test-driver.yaml
2,3c2,3
< # using the Kubernetes 1.15 E2E test suite. For details see:
< # https://github.com/kubernetes/kubernetes/tree/v1.15.0/test/e2e/storage/external
---
> # using the Kubernetes 1.16 E2E test suite. For details see:
> # https://github.com/kubernetes/kubernetes/tree/v1.16.0/test/e2e/storage/external
12a13
>     controllerExpansion: true
14d14
<     dataSource: true
15a16
>     nodeExpansion: true
16a18
>     snapshotDataSource: true

$ diff deploy/kubernetes-1.16/test-driver.yaml deploy/kubernetes-1.17/test-driver.yaml
2,3c2,3
< # using the Kubernetes 1.16 E2E test suite. For details see:
< # https://github.com/kubernetes/kubernetes/tree/v1.16.0/test/e2e/storage/external
---
> # using the Kubernetes 1.17 E2E test suite. For details see:
> # https://github.com/kubernetes/kubernetes/tree/v1.17.0/test/e2e/storage/external
17a18
>     singleNodeVolume: true
18a20
>     topology: true

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2020
This pulls in the prow.sh which uses the revised deploy script.
This will enable the removal of hostpath specific code from
csi-release-tools. The deploy script (now using the name "deploy.sh"
because that can be the same for all CSI drivers) is asked to create a
test driver configuration in addition to installing the driver.

The script could do that dynamically. But in the CSI driver hostpath
repo we simply keep one test-driver.yaml per deployment, which works
based on the assumption that the E2E test suite that corresponds to
the Kubernetes version is used for testing, which is the case
nowadays.

These three test-driver.yaml are indeed different because the name of
capabilities changed over time. prow.sh used to have a union of all
those names. Now each file really only uses the name that is
applicable.

"topology" support hadn't been enabled in prow.sh. This looks like an
oversight, so it gets added now.
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 11, 2020
@pohly
Copy link
Contributor Author

pohly commented Feb 11, 2020

@msau42: I also pulled the corresponding release-tools update into this PR, so the test runs really were done with the new test-driver.yaml files.

@msau42
Copy link
Collaborator

msau42 commented Feb 11, 2020

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, 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:

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

@k8s-ci-robot k8s-ci-robot merged commit 9572e1c into kubernetes-csi:master Feb 11, 2020
pohly added a commit to pohly/csi-driver-host-path that referenced this pull request May 5, 2021
6616a6b Merge pull request kubernetes-csi#146 from pohly/kubernetes-1.21
510fb0f prow.sh: support Kubernetes 1.21
c63c61b prow.sh: add CSI_PROW_DEPLOYMENT_SUFFIX
51ac11c Merge pull request kubernetes-csi#144 from pohly/pull-jobs
dd54c92 pull-test.sh: test importing csi-release-tools into other repo
7d2643a Merge pull request kubernetes-csi#143 from pohly/path-setup
6880b0c prow.sh: avoid creating paths unless really running tests
bc0504a Merge pull request kubernetes-csi#140 from jsafrane/remove-unused-k8s-libs
5b1de1a go-get-kubernetes.sh: remove unused k8s libs
49b4269 Merge pull request kubernetes-csi#120 from pohly/add-kubernetes-release
f7e7ee4 docs: steps for adding testing against new Kubernetes release

git-subtree-dir: release-tools
git-subtree-split: 6616a6b
TerryHowe pushed a commit to TerryHowe/csi-driver-host-path that referenced this pull request Oct 17, 2024
prow.sh: avoid creating paths unless really running tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants