kubernetes-csi: full test matrix for hostpath by pohly · Pull Request #11981 · kubernetes/test-infra

pohly · 2019-03-28T17:58:43Z

We recently clarified that newer sidecars must work with older
Kubernetes and vice versa. Here we add all currently supported
permutations and enable testing on a single configuration for those
repos where it makes no sense to test with different Kubernetes
versions.

/cc @msau42

We recently clarified that newer sidecars must work with older Kubernetes and vice versa. Here we add all currently supported permutations and enable testing on a single configuration for those repos where it makes no sense to test with different Kubernetes versions.

msau42 · 2019-04-01T18:29:06Z

-csi-release-tools
+external-attacher
+external-provisioner
+external-snapshotter


We should leave snapshotter out of version skew testing. It is still alpha and does not offer any compatibility guarantees.

Okay. It's now tested only once, against the Kubernetes version specified in the external-snapshotter repo itself.

msau42 · 2019-04-01T18:31:55Z

+    for deployment in 1.13 1.14; do # must have a deploy/kubernetes-<version> dir in csi-driver-host-path
+        for kubernetes in 1.13.3 1.14.0; do # these versions must have pre-built kind images (see https://hub.docker.com/r/kindest/node/tags)
+            cat >>"$base/$repo/$repo-config.yaml" <<EOF
+  - name: pull-sig-storage-$repo-$(echo "$deployment" | tr . -)-on-kubernetes-$(echo "$kubernetes" | tr . - | sed 's/\([0-9]*\)-\([0-9]*\)-\([0-9]*\)/\1-\2/')


Can the kubernetes version parsing be in a util function so we only need to update it in one place in the future?

msau42 · 2019-04-01T18:40:46Z

+
+presubmits:
+  kubernetes-csi/csi-driver-host-path:
+  - name: pull-sig-storage-csi-driver-host-path-1-13-on-kubernetes-1-13


I don't think we need all the skews in presubmit jobs. (however, CI Jobs should have all the skews).

I think for presubmits, it is sufficient to just have:
canary hostpath driver on K8s master
canary hostpath driver on K8s master - 1

Is that meant to save resources in the Prow test cluster?

We trade less resource usage against the risk that some breaking change slips through.

When you are submitting a PR, you are only modifying the latest version of a sidecar/driver. Your PR changes should not impact already tagged and released deployments.

Testing against master is problematic, because then pre-submit jobs become non-deterministic: some change in K8S master can break the component and there's nothing can be done about it in the PR itself.

I've reduced the matrix to just two jobs: the latest stable release and the release before that.

I'm still confused why we need to run 1.13 and 1.14 versions of the driver in pull jobs? It should just be testing the latest build of the driver and not previous releases.

I think these are the only jobs we need (for pull jobs):

driver-master-kubernetes-1.14

driver-master-kubernetes-1.13

Similar story for the sidecars.

I'm still confused why we need to run 1.13 and 1.14 versions of the driver in pull jobs?

This is still about pull-sig-storage-csi-driver-host-path-1-13-on-kubernetes-1-13, right?

It should just be testing the latest build of the driver and not previous releases.

I think that's were the confusion comes from. That job does use the driver that was built as part of the PR job. If this needs to be more obvious, then I can add CSI_PROW_BUILD_JOB: true explicitly with a comment. It's left out at the moment because that's the default.

"1-13-on-kubernetes-1-13" just means that it then runs tests using the deploy/kubernetes-1.13 deployment on Kubernetes 1.13 with the driver image replaced.

The deployment is relevant because it determines which sidecars to test with. The job then tests that the updated driver is still compatible with that older deployment, which is something that we are promising and thus need to test. I think this is a good candidate for a pre-submit job because it keeps everything else at a stable version.

I've been thinking a bit more about the situation where a new sidecar or driver feature depends on a new API in Kubernetes master. Right now, such changes can be submitted in a PR, but pre-submit tests will not cover them. Once merged, the periodic job with on-kubernetes-master will do that. If we want to enable pre-submit testing of such features, then we could do it like this:

create a pr job which does not override the Kubernetes version

bump the CSI_PROW_KUBERNETES_VERSION in the component to some known-good revision of Kubernetes, potentially also switch to a newer version of the E2E tests

submit the PR

I'm just not sure how important this is. My suggestion is to not add it at this time and wait whether there is a need.

The job then tests that the updated driver is still compatible with that older deployment, which is something that we are promising and thus need to test.

Hostpath driver compatibility is not something we're promising. We are defining a versioning policy for sidecars, not for the hostpath driver. That's why I think once we release a hostpath driver, we should not need to modify older deployments.

msau42 · 2019-04-01T18:42:11Z

+        resources:
+          requests:
+            cpu: 2000m
+  - name: pull-sig-storage-csi-driver-host-path-deployment-1-13-on-kubernetes-1-13


what is the difference between this and pull-sig-storage-csi-driver-host-path-1-13-on-kubernetes-1-13 above?

The difference is explained in

test-infra/config/jobs/kubernetes-csi/gen-jobs.sh

Lines 143 to 148 in 1f2e565

# The pre-merge job above replaces the

# csi-driver-host-path image with a locally built

# one. That can hide issues when the deployments get

# updated (for example, replacing image versions).

# To test the unmodified deployments we define additional jobs

# that don't rebuild the driver.

One tests with updated images, the other the images specified in the deployment.

Why do the jobs that don't modify the images need to run in the pull job?

I'm still unclear why do we need to run jobs that don't modify images in pull jobs?

That is for PRs that modify just the deployment directory. Suppose I submit a PR with image: quay.io/k8scsi/hostpathplugin:v1.1.0 in deploy/kubernetes-1.13 (something we talked about yesterday) and that image doesn't exist yet. If merged, the deployment in master gets broken. This is bad because we point people to the master branch as the source for the latest stable deployments. We have to do that a) because a tagged release of csi-driver-host-path cannot contain a .yaml file with the image for that tagged release (chicken-and-egg problem) b) it's easier to have one link that remains constant (link to master vs. link to latest release).

One could argue that it's okay to break master temporarily, but according to Murphy, this time window will be when someone tries out the deployment and notices the regression. I'd prefer to keep this as pre-submit. We can limit the number of times that it actually runs by adding a path filter. That should also clarify the purpose. How about that?

I would prefer having the tagged version actually reference the correct deployment images. If someone is automatically deploying the driver in some production environment, they should be depending on a tagged version, not master.

I would prefer having the tagged version actually reference the correct deployment images.

Then how do you propose to deal with the chicken-and-egg problem? Allow breaking master temporarily?

If someone is automatically deploying the driver in some production environment, they should be depending on a tagged version, not master.

This is the hostpath driver. It's never going to be deployed in production.

Yes, breaking master temporarily

In that case we can remove these jobs again.

I've removed them the PR.

msau42 · 2019-04-01T18:52:23Z

+
+presubmits:
+  kubernetes-csi/external-attacher:
+  - name: pull-sig-storage-external-attacher-1-13-on-kubernetes-1-13


For sidecar presubmits, I think we only need the following jobs:

canary on K8s master

canary on K8s master - 1

Review feedback pointed out that pre-submits do not need to cover all cases. External-snapshotter is alpha and should be excluded from skew testing.

pohly · 2019-04-01T21:53:28Z

@msau42 updated, please have another look

msau42 · 2019-04-01T22:06:14Z

+        resources:
+          requests:
+            cpu: 2000m
+  - name: pull-sig-storage-csi-driver-host-path-deployment-1-13-on-kubernetes-1-13


I'm still unclear why do we need to run jobs that don't modify images in pull jobs?

msau42 · 2019-04-01T22:10:00Z

+        resources:
+          requests:
+            cpu: 2000m
+  - name: pull-sig-storage-csi-driver-host-path-1-13-on-kubernetes-master


You mentioned that we shouldn't be running pull jobs against K8s master?

The goal was to have the job in Prow, but never enable it by default (in contrast to the other jobs, which will get enabled by default eventually):

# Experimental job, explicitly needs to be started with /test. # This cannot be enabled by default because there's always the risk # that something changes in master which breaks the pre-merge check.

Then if we see a PR which is worthwhile testing on master right away, we can explicitly request a job run with /test.

This is not quite the same as testing against a version of Kubernetes revision specified inside a PR (see my other comment), because here we always use the latest Kubernetes without updating the E2E tests.

msau42 · 2019-04-01T22:18:39Z

+
+presubmits:
+  kubernetes-csi/csi-driver-host-path:
+  - name: pull-sig-storage-csi-driver-host-path-1-13-on-kubernetes-1-13


I'm still confused why we need to run 1.13 and 1.14 versions of the driver in pull jobs? It should just be testing the latest build of the driver and not previous releases.

I think these are the only jobs we need (for pull jobs):

driver-master-kubernetes-1.14

driver-master-kubernetes-1.13

msau42 · 2019-04-01T22:23:34Z

+          requests:
+            cpu: 2000m
+- interval: 6h
+  name: ci-kubernetes-csi-1-13-on-kubernetes-master


To reduce our testing matrix, I think we should only test "-1, current, +1" version skew with K8s.

So we would have the following jobs:

driver-1.13-on-kubernetes-1.13

driver-1.13-on-kubernetes-1.14

driver-1.14-on-kubernetes-1.13

driver-1.14-on-kubernetes-1.14

driver-1.14-on-kubernetes-master

driver-master-on-kubernetes-1.14

driver-master-on-kubernetes-master

But we still promise that "1.13 on Kubernetes 1.15" works, right? How can we promise that if we don't test it?

Ok, I can see the point about 1.13 on k8s 1.15.

But thinking about the newer configs, I think we are misinterpreting the version skew support. Let's say 1.14 adds some new features that were not present in 1.13 and the driver wants to use them. Then the 1.14 deployment enables those new features and cannot be backwards compatible because we only offer backwards compatibility if you don't use the new features. So the deployment should never be used on an older K8s, and the jobs look like:

driver-1.13-on-k8s-1.13

driver-1.13-on-k8s-1.14

driver-1.13-on-k8s-master

driver-1.14-on-k8s-1.14

driver-1.14-on-k8s-master

driver-master-on-k8s-master

Let's say 1.14 adds some new features that were not present in 1.13 and the driver wants to use them. Then the 1.14 deployment enables those new features and cannot be backwards compatible because we only offer backwards compatibility if you don't use the new features.

Correct, if 1.14 enables and depends on new features, then it cannot run on older Kubernetes. But we still may want to test that the new sidecars with the new features disabled work on Kubernetes 1.13. It'll depend on the feature whether that can work and is supported: if it doesn't, we announce a breaking change and release a new major version.

So for the upcoming sidecars we theoretically have two deployments: with topology enabled and disabled. The one where it's off can run on 1.13, and this is something that we have to support and test - at least that was my understanding of the compatibility document, and the new versions (1.1 instead of 2.0) seems to confirm that.

I think having different deployments with different feature sets in the same release is going to be too complex to maintain, and have too many permutations to test.

Let's just start with just like-version testing for now and iron out the details for skew testing later.

I think testing a newer deployment, which might have enabled new features against older Kubernetes is not correct, and is not something we support in our versioning policy.

(a newer sidecar using an older deployment is something we do support)

If you want test newer sidecars on older deployments, you could still use the 1.13 deployment on K8s 1.13 job to do that.

Testing "canary" images with older deployments is something that can be done because it is unambiguous what those images are. It gets more tricky when we have multiple release trains: "stable images" becomes ambiguous in that case. Which ones do we test on 1.13, and in which combination? There has to be a definition of that somewhere. Currently the example deployments are our definition of which sidecars are meant to be used together for a certain situation.

So that would leave us with only 6 jobs in CI:
Hostpath with stable images * K8s 1.13, 1.14, master
Hostpath with canary images * K8s 1.13, 1.14, master

Which "stable images"?

What if we don't have a new hostpath deployment for every release, and only add a new deployment when something in the hostpath deployment actually changes?

What is this "something"? Does adding a new release train count? Because that is what changes now: we support external-provisioner 1.0.x for Kubernetes 1.13 and we add 1.1.x for Kubernetes 1.14. Nothing else changes in the deployment.

If a new release fully replaces the previous one, we can just bump the image version in the existing deployment. But this is not the case right now.

I think testing a newer deployment, which might have enabled new features against older Kubernetes is not correct, and is not something we support in our versioning policy.

I agree, and this is not what I was suggesting. I was talking about a new deployment that does not enable new features.

Let's take the tentative "kubernetes-1.14" deployment as example. Assume that we do the 1.1 releases and then update that deployment with those new image versions. We don't enable any new features. Do you want this deployment to be in the csi-driver-host-path repo? What kind of testing do you want to do with that deployment?

cc @jsafrane

In order to make some progress with this PR and perhaps get it merged, I've taken out the ci-kubernetes-csi-1-14-on-* jobs.

@msau42 is that now something that could be merged?

In order to test older, but still supported release trains, I think we will need to branch every release and have a job off of the release branch that tests the supported versions at the time of release.

There's a lot of detail and logistics that I think we still need to work out, so I am fine with starting with a smaller test matrix until we can document and agree on a plan.

msau42 · 2019-04-01T22:24:26Z

+
+presubmits:
+  kubernetes-csi/csi-driver-host-path:
+  - name: pull-sig-storage-csi-driver-host-path-1-13-on-kubernetes-1-13


Similar story for the sidecars.

It turned out that the case that these jobs were meant to catch ("does the deployment work as specified without changing images") is too strict. The plan is to allow bumping the hostpath image version before actually publishing that image, to ensure that the tagged release has the right image version. This will break the deployment temporarily on the branch.

There was no consensus on how to test the upcoming deployment for 1.14 (see kubernetes#11981 (comment)). It also has not been finalized yet (uses canary images). Therefore the periodic jobs using it get removed. The periodic jobs also unintentionally override the deployment. They should use the default deployment for the Kubernetes version they test against. The "deployment" branch was merged, so we can use the upstream csi-driver-host-path again.

msau42 · 2019-04-03T16:59:17Z

/lgtm
/approve

k8s-ci-robot · 2019-04-03T16:59:20Z

LGTM label has been added.

Details

Git tree hash: d142c36a3f466f909e177306e73bde7d948eb219

msau42 · 2019-04-03T16:59:41Z

/assign @spiffxp

There was no consensus on how to test the upcoming deployment for 1.14 (see kubernetes#11981 (comment)). It also has not been finalized yet (uses canary images). Therefore the periodic jobs using it get removed. The periodic jobs also unintentionally override the deployment. They should use the default deployment for the Kubernetes version they test against. Canary jobs use just the new images, without changing the RBAC rules used by the deployments. The "deployment" branch was merged, so we can use the upstream csi-driver-host-path again.

pohly · 2019-04-03T17:12:29Z

@msau42 I made one last fix for the canary periodic jobs:

+      # Replace images....
+      - name: CSI_PROW_HOSTPATH_CANARY
+        value: "canary"
+      # ... but not the RBAC rules.
+      - name: UPDATE_RBAC_RULES
+        value: "false"

They weren't actually using canary images and if they had, they would have used updated RBAC rules, which is not what we want to test here. Please add LGTM again.

msau42 · 2019-04-03T17:42:20Z

/lgtm

k8s-ci-robot · 2019-04-03T17:42:22Z

LGTM label has been added.

Details

Git tree hash: 96a7ce78ba097ad5e97ae4e4ca42a2580ea3fe71

msau42 · 2019-04-03T23:57:10Z

/assign @michelle192837

michelle192837 · 2019-04-04T00:01:15Z

 #  -- Yaml representation for configuring testgrid.k8s.io
 #
 #  To add a new test:
-#   1. Append a new testgroup under test_groups, specify the name and where to get the log.


Was this supposed to be deleted?

Oops, no. Fixed in the revised commit.

There was no consensus on how to test the upcoming deployment for 1.14 (see kubernetes#11981 (comment)). It also has not been finalized yet (uses canary images). Therefore the periodic jobs using it get removed. The periodic jobs also unintentionally override the deployment. They should use the default deployment for the Kubernetes version they test against. Canary jobs use just the new images, without changing the RBAC rules used by the deployments. The "deployment" branch was merged, so we can use the upstream csi-driver-host-path again.

michelle192837 · 2019-04-04T18:58:19Z

/approve

k8s-ci-robot · 2019-04-04T18:58:28Z

[APPROVALNOTIFIER] This PR is APPROVED

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

~~config/jobs/kubernetes-csi/OWNERS~~ [msau42]
~~testgrid/OWNERS~~ [michelle192837]

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

msau42 · 2019-04-04T19:48:16Z

/lgtm

k8s-ci-robot · 2019-04-04T19:48:17Z

LGTM label has been added.

Details

Git tree hash: f6161201f8527d923f1ad87668bb61052edfc9f1

k8s-ci-robot · 2019-04-04T19:59:21Z

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

key cluster-driver-registrar.yaml using file ``
key csi-driver-host-path-config.yaml using file config/jobs/kubernetes-csi/csi-driver-host-path/csi-driver-host-path-config.yaml
key csi-driver-host-path.yaml using file ``
key csi-lib-utils-config.yaml using file config/jobs/kubernetes-csi/csi-lib-utils/csi-lib-utils-config.yaml
key csi-release-tools-config.yaml using file config/jobs/kubernetes-csi/csi-release-tools/csi-release-tools-config.yaml
key csi-release-tools.yaml using file ``
key csi-test-config.yaml using file config/jobs/kubernetes-csi/csi-test/csi-test-config.yaml
key csi-test.yaml using file ``
key external-attacher-config.yaml using file config/jobs/kubernetes-csi/external-attacher/external-attacher-config.yaml
key external-provisioner-config.yaml using file config/jobs/kubernetes-csi/external-provisioner/external-provisioner-config.yaml
key external-snapshotter-config.yaml using file config/jobs/kubernetes-csi/external-snapshotter/external-snapshotter-config.yaml
key livenessprobe-config.yaml using file config/jobs/kubernetes-csi/livenessprobe/livenessprobe-config.yaml
key node-driver-registrar-config.yaml using file config/jobs/kubernetes-csi/node-driver-registrar/node-driver-registrar-config.yaml

Details

In response to this:

We recently clarified that newer sidecars must work with older
Kubernetes and vice versa. Here we add all currently supported
permutations and enable testing on a single configuration for those
repos where it makes no sense to test with different Kubernetes
versions.

/cc @msau42

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.

There was no consensus on how to test the upcoming deployment for 1.14 (see kubernetes/test-infra#11981 (comment)). It also has not been finalized yet (uses canary images). Therefore the periodic jobs using it get removed. The periodic jobs also unintentionally override the deployment. They should use the default deployment for the Kubernetes version they test against. Canary jobs use just the new images, without changing the RBAC rules used by the deployments. The "deployment" branch was merged, so we can use the upstream csi-driver-host-path again.

k8s-ci-robot requested a review from msau42 March 28, 2019 17:58

pohly force-pushed the kubernetes-csi-prow branch from 87938f8 to 7bd128b Compare March 28, 2019 19:22

pohly mentioned this pull request Mar 29, 2019

deployment enhancements kubernetes-csi/csi-driver-host-path#29

Merged

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

pohly force-pushed the kubernetes-csi-prow branch from 7bd128b to b78b59e Compare April 1, 2019 06:53

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

pohly force-pushed the kubernetes-csi-prow branch from b78b59e to 1f2e565 Compare April 1, 2019 17:29

msau42 reviewed Apr 1, 2019

View reviewed changes

kubernetes-csi: reduce testing a bit again

4d1315a

Review feedback pointed out that pre-submits do not need to cover all cases. External-snapshotter is alpha and should be excluded from skew testing.

msau42 reviewed Apr 1, 2019

View reviewed changes

k8s-ci-robot assigned msau42 Apr 3, 2019

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

k8s-ci-robot assigned spiffxp Apr 3, 2019

pohly force-pushed the kubernetes-csi-prow branch from b8adeb6 to 3f857e8 Compare April 3, 2019 17:02

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

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

k8s-ci-robot assigned michelle192837 Apr 3, 2019

michelle192837 reviewed Apr 4, 2019

View reviewed changes

pohly force-pushed the kubernetes-csi-prow branch from 3f857e8 to 4001776 Compare April 4, 2019 06:13

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

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

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

k8s-ci-robot merged commit 1497021 into kubernetes:master Apr 4, 2019

pohly mentioned this pull request Apr 21, 2019

Update sidecar images to v1.1.0 kubernetes-csi/csi-driver-host-path#44

Merged

	# The pre-merge job above replaces the
	# csi-driver-host-path image with a locally built
	# one. That can hide issues when the deployments get
	# updated (for example, replacing image versions).
	# To test the unmodified deployments we define additional jobs
	# that don't rebuild the driver.

Conversation

pohly commented Mar 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pohly commented Apr 1, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment