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

Enable driver features to be toggled off/on #32

Open
gnufied opened this issue Sep 16, 2019 · 6 comments
Open

Enable driver features to be toggled off/on #32

gnufied opened this issue Sep 16, 2019 · 6 comments
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.

Comments

@gnufied
Copy link
Contributor

gnufied commented Sep 16, 2019

Rather than defaulting to always on features, I think we should allow tests suites to toggle them. Something like -

CSI_ENABLED_FEATURES="controllerExpansion=true,nodeExpansion=true"

This will enable tests like https://github.com/kubernetes-csi/external-resizer/pull/53/files to enable certain features without having to update csi-release-tools. Also because features can differ between versions it will allow .prow.sh file in individual branches to have their own configuration. I am not sure if this is preferable to having if-elses built into prow.sh though.

@gnufied
Copy link
Contributor Author

gnufied commented Sep 16, 2019

cc @pohly @msau42

@pohly
Copy link
Contributor

pohly commented Sep 17, 2019

This is about the test driver config, right?

csi-release-tools/prow.sh

Lines 742 to 765 in 0affdf9

# The default implementation of this function generates a external
# driver test configuration for the hostpath driver.
#
# The content depends on both what the E2E suite expects and what the
# installed hostpath driver supports. Generating it here seems prone
# to breakage, but it is uncertain where a better place might be.
generate_test_driver () {
cat <<EOF
ShortName: csiprow
StorageClass:
FromName: true
SnapshotClass:
FromName: true
DriverInfo:
Name: ${CSI_PROW_HOSTPATH_DRIVER_NAME}
Capabilities:
block: $(hostpath_supports_block)
persistence: true
dataSource: true
multipods: true
nodeExpansion: true
controllerExpansion: true
EOF
}

The content of that file must match the deployment of the host path driver, which is determined by CSI_PROW_HOSTPATH_VERSION (controlled by prow.sh) and CSI_PROW_KUBERNETES_VERSION (sometimes controlled by the Prow job config), and what the E2E suite expects, which is determined by CSI_PROW_E2E_VERSION (again controlled by prow.sh). That this isn't entirely controlled by prow.sh is unfortunate, but any other solution that I could think of had the same issue.

What I don't understand is why you want to enable certain features without having to update csi-release-tools. Is it because that is more work? It is more work, but it has the advantage that testing is consistent.

If we want testing that is different for different releases, then csi-release-tools needs to be forked.

@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 16, 2019
@msau42
Copy link
Collaborator

msau42 commented Dec 24, 2019

I think the issue @gnufied is referring to the issue of new features and maybe sidecars are introduced in newer releases. But the test config is the same across all releases. We currently add a lot of release-specific logic into prow.sh to toggle on and off focus/skip flags because of this, which I don't really like because it adds a lot of toil every new release to update these settings and then sync it across all repos.

What if we add the test config to the per-release hostpath deployments? That should solve some of the difficulties because the test will be configured to run based on the hostpath deployment version, which indicates what the driver supports.

It could also potentially help us move towards a model where prow.sh is not so hardcoded with hostpath driver, and could be used for other drivers too.

/lifecycle-frozen

@msau42
Copy link
Collaborator

msau42 commented Dec 24, 2019

/lifecycle frozen

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 24, 2019
@pohly
Copy link
Contributor

pohly commented Jan 6, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness.
Projects
None yet
Development

No branches or pull requests

5 participants