image pushing: configure jobs for all Kubernetes-CSI repos #17741

pohly · 2020-05-28T09:10:18Z

This is part of rolling out multi-architecture image building for GCR,
see kubernetes-csi/csi-release-tools#86

pohly · 2020-05-28T09:11:01Z

@msau42: do we want to set up jobs before or after updating all repos? Right now, most of the jobs will still fail.

pohly · 2020-05-28T09:11:25Z

/hold

This is part of rolling out multi-architecture image building for GCR, see kubernetes-csi/csi-release-tools#86

pohly · 2020-05-28T13:29:35Z

@msau42: there's no dedicated testgrid for csi-test. Should we use this instead?
https://testgrid.k8s.io/sig-storage-csi-other

msau42

I'm fine creating a new testgrid tab for csi-test, or putting all these jobs into a "image_build" tab.

msau42 · 2020-05-28T16:33:40Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+
+output="$(dirname $0)/k8s-staging-csi.yaml"
+repos="
+csi-driver-host-path


We also have csi-driver-nfs, csi-driver-smb, csi-driver-iscsi, external-health-monitor, csi-proxy

Will add those.

msau42 · 2020-05-28T16:37:50Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+        testgrid-dashboards: sig-storage-csi-$repo
+      decorate: true
+      branches:
+        # For publishing canary images. Publishing canary images for release branches can


Does it hurt to enable it on branches that don't have the build files? It will cause the job to fail, there is not much activity happening in release branches.

Besides failed jobs there shouldn't be any negative effect. If you aren't concerned about that, then I can enable also release-.* branches.

msau42 · 2020-05-28T16:40:37Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+        # This is a regex for semver, from https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string.
+        # This is okay for upcoming releases, but old releases will not have the necessary cloud build
+        # files and thus the job will fail.
+        - ^v(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$


Do we need the branches to be specific, or can we enable it on everything?

We can enable it for all tags. Doing it for all branches makes less sense, because an arbitrary branch name (say, "my-experimental-feature-branch") will then be used as-is in the image tag, which means the image will be built once and then all future branch updates will trigger this error during image pushing:
https://github.com/kubernetes-csi/node-driver-registrar/blob/da1e2a455271eb7c693dbd9835f6419c82e2ee02/release-tools/build.make#L154-L160

So consider this limitation for branches, we somehow need to distinguish here between "real" branches and tags. I kept the semver check because it seemed reasonable enforce that tags on images have that format.

pohly · 2020-05-29T07:25:49Z

I'm fine creating a new testgrid tab for csi-test, or putting all these jobs into a "image_build" tab.

I don't use test grid much (read: not at all) and thus I don't have an opinion on what is better for its users. A separate "image_build" looks okay to me and is easier to handle, so I am going with that.

…oard As discussed in kubernetes#17741 it's fine to enable jobs that will not succeed. A separate dashboard gets added because it's easier than trying to determine per repo where the job is meant to be shown and because it might be useful to check all of those jobs at one glance.

msau42 · 2020-05-29T16:46:33Z

/lgtm
/approve

dims · 2020-06-02T10:25:01Z

/assign @bartsmykla @spiffxp

bartsmykla

The only thing I think would be really nice to address are

#!/usr/bin/env bash

and usage of array instead of multiline string and others you can decide.

/lgtm
/hold unhold if you'll decide to now address my suggestions

bartsmykla · 2020-06-02T10:46:47Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+output="$(dirname $0)/k8s-staging-csi.yaml"


I would suggest using upper case as and marking this as readonly (ref. https://google.github.io/styleguide/shellguide.html#constants-and-environment-variable-names)

Suggested change

output="$(dirname $0)/k8s-staging-csi.yaml"

readonly OUTPUT="$(dirname "$0")/k8s-staging-csi.yaml"

bartsmykla · 2020-06-02T10:49:24Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+repos="
+csi-driver-host-path
+csi-driver-iscsi
+csi-driver-nfs
+csi-driver-smb
+csi-proxy
+csi-test
+external-attacher
+external-health-monitor
+external-provisioner
+external-resizer
+external-snapshotter
+livenessprobe
+node-driver-registrar
+"


Same commetn about readonly and upper case + I would suggest using array's instead of strings (ref. https://google.github.io/styleguide/shellguide.html#arrays):

Suggested change

repos="

csi-driver-host-path

csi-driver-iscsi

csi-driver-nfs

csi-driver-smb

csi-proxy

csi-test

external-attacher

external-health-monitor

external-provisioner

external-resizer

external-snapshotter

livenessprobe

node-driver-registrar

"

readonly REPOS=(

csi-driver-host-path

csi-driver-iscsi

csi-driver-nfs

csi-driver-smb

csi-proxy

csi-test

external-attacher

external-health-monitor

external-provisioner

external-resizer

external-snapshotter

livenessprobe

node-driver-registrar

)

bartsmykla · 2020-06-02T10:50:50Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+postsubmits:
+EOF
+
+for repo in $repos; do


REPOS is now an array (per last suggestion)

Suggested change

for repo in $repos; do

for repo in "${REPOS[@]}"; do

bartsmykla · 2020-06-02T10:54:13Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+node-driver-registrar
+"
+
+cat >"$output" <<EOF


For consistency purposes I would suggest to enclose the variables in braces (ref. https://google.github.io/styleguide/shellguide.html#variable-expansion):

Suggested change

cat >"$output" <<EOF

cat > "${OUTPUT}" <<EOF

bartsmykla · 2020-06-02T10:54:39Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+EOF
+
+for repo in $repos; do
+    cat >>"$output" <<EOF


For consistency purposes I would suggest to enclose the variables in braces (ref. https://google.github.io/styleguide/shellguide.html#variable-expansion):

Suggested change

cat >>"$output" <<EOF

cat >> "${OUTPUT}" <<EOF

bartsmykla · 2020-06-02T10:55:53Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+  kubernetes-csi/$repo:
+    - name: post-$repo-push-images


For consistency purposes I would suggest to enclose the variables in braces (ref. https://google.github.io/styleguide/shellguide.html#variable-expansion):

Suggested change

kubernetes-csi/$repo:

- name: post-$repo-push-images

kubernetes-csi/${repo}:

- name: post-${repo}-push-images

bartsmykla · 2020-06-02T11:04:43Z

config/jobs/image-pushing/k8s-staging-csi-gen.sh

+#! /bin/bash -e
+# Copyright 2020 The Kubernetes Authors.
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.


In our infra-related scripts at k/k8s.io we are trying to be consistent with Google's shell styleguide so some of my comments will be related to this styleguide, but I'm not gonna block the PR because of this and leave you the decission if you would like to consider the suggestions.

As setting flags directly in shebangs can be broken when calling te script like:

bash script_name.sh

I would suggest

Suggested change

#! /bin/bash -e

# Copyright 2020 The Kubernetes Authors.

#

# Licensed under the Apache License, Version 2.0 (the "License");

# you may not use this file except in compliance with the License.

# You may obtain a copy of the License at

#

# http://www.apache.org/licenses/LICENSE-2.0

#

# Unless required by applicable law or agreed to in writing, software

# distributed under the License is distributed on an "AS IS" BASIS,

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

# See the License for the specific language governing permissions and

# limitations under the License.

#!/usr/bin/env bash

# Copyright 2020 The Kubernetes Authors.

#

# Licensed under the Apache License, Version 2.0 (the "License");

# you may not use this file except in compliance with the License.

# You may obtain a copy of the License at

#

# http://www.apache.org/licenses/LICENSE-2.0

#

# Unless required by applicable law or agreed to in writing, software

# distributed under the License is distributed on an "AS IS" BASIS,

# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

# See the License for the specific language governing permissions and

# limitations under the License.

set -o errexit

In our infra-related scripts at k/k8s.io we are trying to be consistent with Google's shell styleguide so some of my comments will be related to this styleguide, but I'm not gonna block the PR because of this and leave you the decission if you would like to consider the suggestions.

The suggestions make sense, but I'd prefer to get the PR merged as-is first because I want the jobs in place before updating the individual repos.

Sure, I don't have problems with doing it in the following up PR, but unfortunately I don't have approving powers so we'll have to wait for @spiffxp or others to approve it

Script enhancements are in #17808.

pohly · 2020-06-02T12:04:43Z

/assign @spiffxp

pohly · 2020-06-02T12:05:29Z

/hold cancel

spiffxp

I agree with @bartsmykla's comments. I'm also not sure how sustainable it is to have job generation scripts littered around the /config/jobs dir. I'll /approve for now to unblock but please address in a followup

spiffxp · 2020-06-02T15:50:57Z

/approve

k8s-ci-robot · 2020-06-02T15:51:34Z

[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:

~~config/jobs/image-pushing/OWNERS~~ [spiffxp]
~~config/testgrids/kubernetes/sig-storage/OWNERS~~ [msau42,spiffxp]

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

bartsmykla · 2020-06-02T16:00:12Z

/test pull-test-infra-bazel

This applies the https://google.github.io/styleguide/shellguide.html also to the generator script (review feedback from kubernetes#17741).

pohly · 2020-06-02T16:05:41Z

I'm also not sure how sustainable it is to have job generation scripts littered around the /config/jobs dir.

Should those rather be under hack? But let's discuss that in #17808.

k8s-ci-robot · 2020-06-02T16:10:20Z

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

key k8s-staging-csi.yaml using file config/jobs/image-pushing/k8s-staging-csi.yaml

In response to this:

This is part of rolling out multi-architecture image building for GCR,
see kubernetes-csi/csi-release-tools#86

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 applies the https://google.github.io/styleguide/shellguide.html also to the generator script (review feedback from kubernetes#17741).

…oard As discussed in kubernetes#17741 it's fine to enable jobs that will not succeed. A separate dashboard gets added because it's easier than trying to determine per repo where the job is meant to be shown and because it might be useful to check all of those jobs at one glance.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 28, 2020

k8s-ci-robot requested review from chases2 and spiffxp May 28, 2020 09:10

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 28, 2020

pohly force-pushed the kubernetes-csi-cloud-build branch from fa84bf0 to c0935b7 Compare May 28, 2020 11:12

image pushing: configure jobs for all Kubernetes-CSI repos

2e547f8

This is part of rolling out multi-architecture image building for GCR, see kubernetes-csi/csi-release-tools#86

pohly force-pushed the kubernetes-csi-cloud-build branch from c0935b7 to 2e547f8 Compare May 28, 2020 12:41

msau42 reviewed May 28, 2020

View reviewed changes

msau42 mentioned this pull request May 28, 2020

Add prow job to publishing csi-proxy binary #17683

Closed

pohly mentioned this pull request May 29, 2020

Moving images away from quay.io? kubernetes-csi/csi-release-tools#86

Closed

pohly force-pushed the kubernetes-csi-cloud-build branch from ed4c9a0 to 6af5a71 Compare May 29, 2020 08:12

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 29, 2020

k8s-ci-robot assigned msau42 May 29, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 29, 2020

k8s-ci-robot assigned bartsmykla Jun 2, 2020

k8s-ci-robot assigned spiffxp Jun 2, 2020

bartsmykla reviewed Jun 2, 2020

View reviewed changes

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 2, 2020

spiffxp approved these changes Jun 2, 2020

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 2, 2020

pohly added a commit to pohly/test-infra that referenced this pull request Jun 2, 2020

kubernetes-csi cloud build: update shell script

97f0239

This applies the https://google.github.io/styleguide/shellguide.html also to the generator script (review feedback from kubernetes#17741).

pohly mentioned this pull request Jun 2, 2020

image pushing: enhance Kubernetes-CSI generator script #17808

Merged

k8s-ci-robot merged commit c417fac into kubernetes:master Jun 2, 2020

k8s-ci-robot added this to the v1.19 milestone Jun 2, 2020

pohly added a commit to pohly/test-infra that referenced this pull request Jun 2, 2020

kubernetes-csi cloud build: update shell script

3cd0126

This applies the https://google.github.io/styleguide/shellguide.html also to the generator script (review feedback from kubernetes#17741).

spiffxp mentioned this pull request Nov 10, 2020

Create image-pushing job per image for test/images kubernetes/kubernetes#96432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

image pushing: configure jobs for all Kubernetes-CSI repos #17741

image pushing: configure jobs for all Kubernetes-CSI repos #17741

pohly commented May 28, 2020

pohly commented May 28, 2020

pohly commented May 28, 2020

pohly commented May 28, 2020

msau42 left a comment

msau42 May 28, 2020

pohly May 29, 2020

msau42 May 28, 2020

pohly May 29, 2020

msau42 May 28, 2020

pohly May 29, 2020

pohly May 29, 2020

pohly commented May 29, 2020

msau42 commented May 29, 2020

dims commented Jun 2, 2020

bartsmykla left a comment

bartsmykla Jun 2, 2020

bartsmykla Jun 2, 2020

bartsmykla Jun 2, 2020

bartsmykla Jun 2, 2020 •

edited

Loading

bartsmykla Jun 2, 2020 •

edited

Loading

bartsmykla Jun 2, 2020

bartsmykla Jun 2, 2020

pohly Jun 2, 2020

bartsmykla Jun 2, 2020 •

edited

Loading

pohly Jun 2, 2020

pohly commented Jun 2, 2020

pohly commented Jun 2, 2020

spiffxp left a comment

spiffxp commented Jun 2, 2020

k8s-ci-robot commented Jun 2, 2020

bartsmykla commented Jun 2, 2020

pohly commented Jun 2, 2020

k8s-ci-robot commented Jun 2, 2020

	output="$(dirname $0)/k8s-staging-csi.yaml"
	readonly OUTPUT="$(dirname "$0")/k8s-staging-csi.yaml"

image pushing: configure jobs for all Kubernetes-CSI repos #17741

image pushing: configure jobs for all Kubernetes-CSI repos #17741

Conversation

pohly commented May 28, 2020

pohly commented May 28, 2020

pohly commented May 28, 2020

pohly commented May 28, 2020

msau42 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented May 29, 2020

msau42 commented May 29, 2020

dims commented Jun 2, 2020

bartsmykla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartsmykla Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

bartsmykla Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartsmykla Jun 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Jun 2, 2020

pohly commented Jun 2, 2020

spiffxp left a comment

Choose a reason for hiding this comment

spiffxp commented Jun 2, 2020

k8s-ci-robot commented Jun 2, 2020

bartsmykla commented Jun 2, 2020

pohly commented Jun 2, 2020

k8s-ci-robot commented Jun 2, 2020

bartsmykla Jun 2, 2020 •

edited

Loading

bartsmykla Jun 2, 2020 •

edited

Loading

bartsmykla Jun 2, 2020 •

edited

Loading