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

kubernetes-csi: test 1.14 deployment periodically #12482

Merged
merged 3 commits into from
May 8, 2019

Conversation

pohly
Copy link
Contributor

@pohly pohly commented May 6, 2019

With the kubernetes-1.14 deployment finalized in the
csi-driver-host-path master branch (no more canary images!) we can
start testing it periodically against all Kubernetes versions for
which it works. As for kubernetes-1.13, alpha testing is limited to
the matching Kubernetes release.

Also tweaks the on-master periodic jobs such that the snapshotter alpha tests
work again.

Fixes: kubernetes-csi/external-snapshotter#120

pohly added 2 commits May 6, 2019 08:55
This image gets updated in the generated jobs via global
search/replace by others without also updating the generator
script. Storing the image name in one place simplifies the task of
keeping up with those changes.
With the kubernetes-1.14 deployment finalized in the
csi-driver-host-path master branch (no more canary images!) we can
start testing it periodically against all Kubernetes versions for
which it works. As for kubernetes-1.13, alpha testing is limited to
the matching Kubernetes release.
@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 6, 2019
@k8s-ci-robot k8s-ci-robot requested review from saad-ali and spiffxp May 6, 2019 07:10
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label May 6, 2019
@pohly
Copy link
Contributor Author

pohly commented May 6, 2019

/assign @msau42

@k8s-ci-robot k8s-ci-robot added area/config Issues or PRs related to code in /config area/testgrid sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels May 6, 2019
@spiffxp
Copy link
Member

spiffxp commented May 6, 2019

/approve
For the testgrid changes
I leave the /lgtm to @msau42

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 6, 2019
@@ -355,6 +355,111 @@ periodics:
resources:
requests:
cpu: 2000m
- interval: 6h
name: ci-kubernetes-csi-1-14-on-kubernetes-1-13
Copy link
Member

Choose a reason for hiding this comment

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

I'm unsure if this configuration is valid. A driver deployment for 1.14 could have enabled features that will only work on Kubernetes 1.14 (like the new Leases leader election type).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, it may or may not be compatible. But we know that it will work for the 1.14 deployment, therefore this combination is valid and worth testing. For example, https://kubernetes-csi.github.io/docs/external-attacher.html says that the minimum Kubernetes version is 1.13. This job covers that promise.

Copy link
Member

Choose a reason for hiding this comment

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

Once we add leader election, topology or any of the CSI driver features, then it will no longer be the case.

I'm thinking in the future if we want to better automate setting up these jobs, then it's simpler to have rules that work across all scenarios

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I consider the kubernetes-1.14 deployment mostly frozen at this point. It enables a certain set of features and those happen to be compatible with 1.13 (when ignoring alpha). IMHO it is useful to keep it this way, to increase test coverage of the stable images also with Kubernetes 1.13.

When we added leader election, I would do it in a separate deployment because we need cover both scenarios (with and without leader election). That new deployment then cannot run on 1.13.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having said that, if you don't care about testing this particular aspect, then I won't argue further in favor of keeping it. It's not a combination that I personally use.

Copy link
Member

Choose a reason for hiding this comment

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

I mainly don't want this to be misinterpreted as a general statement that we support running these sidecars against 1.13 in all scenarios. It's very dependent per driver and what features they use.

If we eventually add these 1.14+ features to the hostpath-driver, our deployment management and version skew testing will become very complex if we want to be able to test with all theoretical combinations of new/old features and versions.

The latest external-snapshotter fails in the alpha-canary-on-master
job because it needs modified RBAC rules. For on-master jobs it makes
sense to use also the latest RBAC rules, while for other canary jobs
we want to keep the RBAC rules unmodified to detect when an upcoming
release breaks compatibility.
@spiffxp
Copy link
Member

spiffxp commented May 7, 2019

/uncc

@k8s-ci-robot k8s-ci-robot removed the request for review from spiffxp May 7, 2019 14:59
@msau42
Copy link
Member

msau42 commented May 8, 2019

/lgtm
/approve

For now this is fine. But we'll need to revisit this once we start enabling new features in the hostpath driver.

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

LGTM label has been added.

Git tree hash: e2956841e1aafef022c1ee2776c1bebdf70b6781

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, pohly, spiffxp

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 4ea8f65 into kubernetes:master May 8, 2019
@k8s-ci-robot
Copy link
Contributor

@pohly: Updated the job-config configmap in namespace default using the following files:

  • key csi-driver-host-path-config.yaml using file config/jobs/kubernetes-csi/csi-driver-host-path/csi-driver-host-path-config.yaml

In response to this:

With the kubernetes-1.14 deployment finalized in the
csi-driver-host-path master branch (no more canary images!) we can
start testing it periodically against all Kubernetes versions for
which it works. As for kubernetes-1.13, alpha testing is limited to
the matching Kubernetes release.

Also tweaks the on-master periodic jobs such that the snapshotter alpha tests
work again.

Fixes: kubernetes-csi/external-snapshotter#120

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.

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. area/config Issues or PRs related to code in /config area/testgrid cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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.

snapshot tests are failing on master
4 participants