revise "single Kubernetes repos" jobs #14379
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/hold kubernetes-csi/external-resizer#53 changed external-resizer so that it always tests against CSI_PROW_KUBERNETES_VERSION=latest. As long as that is in place, enabling the testing of external-resizer with Prow jobs for different Kubernetes versions doesn't make sense. @gnufied: can you clarify how external-resizer should be tested, now and once Kubernetes 1.16 is out? |
k8s-ci-robot
added
the
do-not-merge/hold
| @@ -12,10 +12,6 @@ presubmits: | |||
| preset-service-account: "true" | |||
| preset-dind-enabled: "true" | |||
| preset-kind-volume-mounts: "true" | |||
| annotations: | |||
There was a problem hiding this comment.
we should replace this with updated testgrid annotations
There was a problem hiding this comment.
Added. Wasn't that then also missing for the other repos which were tested against multiple Kubernetes releases?
According to https://github.com/kubernetes/test-infra/blob/master/testgrid/config.md#prow-job-configuration, testgrid-tab-name is optional and defaults to the job name. I therefore left it out. Perhaps it can also be removed from other jobs?
However, the config check now fails with "Testgrid group pull-kubernetes-csi-csi-driver-host-path-1-15-on-kubernetes-master does not have a matching jenkins or prow job" and similar messages about the other jobs. pull-kubernetes-csi-csi-driver-host-path-1-15-on-kubernetes-master is only used once as job name, why is it called a "Testgrid group"?
There was a problem hiding this comment.
The old repos were whitelisted as being allowed to have no matching testgrid entry. I think we can remove the whitelist now
There was a problem hiding this comment.
I agree on removing the whitelist. But that won't fix the verification error, will it?
| always_run: true | ||
| decorate: true | ||
| skip_report: false | ||
| skip_branches: ["^(errorhandling|k8s_1.12.0-beta.1|release-0.4|release-1.0|revert-72-pvclister|saad-ali-patch-1|saad-ali-patch-2|test-yang|updateSize)$"] |
There was a problem hiding this comment.
I think all of these private/test branches should be cleaned up by now.
There was a problem hiding this comment.
livenessprobe still has them, and external-provisioner gained a new branch recently (https://github.com/kubernetes-csi/external-provisioner/tree/revert-212-automated-cherry-pick-of-%23209-upstream-release-1.0).
I've removed the extra skip branches anyway, because it only affects pull requests and those branches shouldn't be the target of a pull request.
| # livenessprobe is listed here because the API that it provides | ||
| # towards Kubernetes hasn't changed much. But even that isn't actually | ||
| # getting tested at the moment because the sidecar isn't part of | ||
| # the csi-driver-host-path deployment and thus doesn't get tested |
There was a problem hiding this comment.
I think it's fine to move liveness probe up too
There was a problem hiding this comment.
Oh, I missed that. Yes, then moving liveness probe makes sense.
|
@pohly can we make the change here anyway, and remove the override from external-resizer later? I think we'll want these changes to help us add 1.16 jobs to external-resizer. |
pohly
force-pushed
the
single-kubernetes-repos
branch
from
5829c83 to
c2b3610
Compare
k8s-ci-robot
added
size/XL
pohly
force-pushed
the
single-kubernetes-repos
branch
3 times, most recently
from
a5f8662 to
42b7d0a
Compare
|
/hold cancel
Yes, makes sense. |
k8s-ci-robot
removed
the
do-not-merge/hold
|
I asked on #sig-testing regarding the pull-test-infra-bazel error and @Katharine replied that this looks like a bug in the scripts and that she'll look into it. @msau42: with the new annotations on all pull jobs we'll have 72 jobs that'll appear in https://k8s-testgrid.appspot.com/sig-storage-csi - isn't that a bit much? |
|
Some options for splitting up testgrid tabs:
Or both? Tab per repo has pull jobs, CI has its own tab |
I like that best. It also allows keeping the names of the entries shorter because all entries in "csi-pull-external-attacher" are pull jobs for the same repo and thus that part doesn't need to be repeated. |
Some of these branches have been removed from the repos and those that remain shouldn't be the target of a pull request, so we don't need to list them.
The pull jobs for repos that get tested with multiple Kubernetes versions didn't have dashboard annotations. Those are preferred over explicitly configuring the dashboard.
…etes repos The original intention was to let repos which only need to be tested against a single Kubernetes release choose that release themselves. The comment for `single_kubernetes_repos` still describes that behavior. kubernetes#13434 changed that so that 1.15 is used for all of these repos because of some issues with external-snapshotter. external-snapshotter now has 1.15 in its own prow.sh, so we don't need the override anymore in the test job and can revert to the original behavior.
… Kubernetes versions All are now part of all csi-driver-host-path deployments for Kubernetes 1.13 and later and their functionality may differ depending on the Kubernetes version. Therefore it makes sense to enable the full test matrix instead of testing with just one Kubernetes version.
All of them are in testgrid now.
pohly
force-pushed
the
single-kubernetes-repos
branch
2 times, most recently
from
27c2572 to
658524c
Compare
pohly
force-pushed
the
single-kubernetes-repos
branch
from
658524c to
7a47459
Compare
k8s-ci-robot
added
size/XXL
pohly
force-pushed
the
single-kubernetes-repos
branch
2 times, most recently
from
cb34e8b to
902f166
Compare
|
@msau42: I split up the testgrid and now it also passes validation. |
We can't use that name for a testgrid, the name has to start with "sig" or some other whitelisted prefix. I kept "sig-storage" as common prefix. |
| @@ -3,7 +3,15 @@ dashboard_groups: | |||
| dashboard_names: | |||
| - sig-storage-kubernetes | |||
| - sig-storage-local-static-provisioner | |||
| - sig-storage-csi | |||
| - sig-storage-ci | |||
There was a problem hiding this comment.
Can we have multiple nested dashboard groups? For example, put all of these under a sig-storage-csi group?
Otherwise, would prefer prefixing all of these with "sig-storage-csi"
There was a problem hiding this comment.
Can we have multiple nested dashboard groups? For example, put all of these under a sig-storage-csi group?
I'm not sure whether that is supported by testgrid.
Otherwise, would prefer prefixing all of these with "sig-storage-csi"
So sig-storage-csi-<something> with <something> = ci/other/<repo>?
72 jobs in a single testgrid are too much. We now distinguish between periodic CI jobs and pull jobs. Testgrids with just a single job are avoided, instead those jobs are in "sig-storage-csi-other".
pohly
force-pushed
the
single-kubernetes-repos
branch
from
902f166 to
7ecd68a
Compare
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/assign @krzyzacy |
|
(I'm not on OSS these days, and I probably don't have approve rights now?) |
|
/assign @BenTheElder |
|
happy to handle this but don't generally endorse assigning to oncall, oncall is a voluntary business hours rotation for keeping prow running.. in general config directories should have OWNERS to review / approve |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, 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 |
k8s-ci-robot
added
the
approved
|
@pohly: Updated the
In response to this:
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. |
This restores the original design for these jobs (i.e. letting the repo decide what it tests) and, after reconsidering what those repos currently test, extends the testing of all sidecars such they get tested against the same set of Kubernetes releases.