Improvements and docs for pj-on-kind.sh. #14355

cjwagner · 2019-09-17T02:02:52Z

clarketm · 2019-09-17T04:35:30Z

config/pj-on-kind.sh


-../prow/pj-on-kind.sh "$@"
+$(dirname "${BASH_SOURCE[0]}")/../prow/pj-on-kind.sh "$@"


Looks good! To promote readability you could tweak to something like:

dir="$(realpath "$(dirname "${BASH_SOURCE[0]}")")" export CONFIG_PATH="$dir/../prow/config.yaml" export JOB_CONFIG_PATH="$dir/jobs" "$dir/../prow/pj-on-kind.sh" "$@"

clarketm

Documentation and code looks good! I made one suggestion that you can choose to take. Unhold at your leisure.
/lgtm
/hold

k8s-ci-robot · 2019-09-17T04:37:10Z

LGTM label has been added.

Git tree hash: daff4f9f6bca3b87ba5ae2e025b46f8d8ef83abe

BenTheElder · 2019-09-17T04:30:12Z

config/pj-on-kind.sh

-export CONFIG_PATH="$(readlink -f $(dirname "${BASH_SOURCE[0]}")/../prow/config.yaml)"
-export JOB_CONFIG_PATH="$(readlink -f $(dirname "${BASH_SOURCE[0]}")/jobs)"
+export CONFIG_PATH="$(realpath $(dirname "${BASH_SOURCE[0]}")/../prow/config.yaml)"
+export JOB_CONFIG_PATH="$(realpath $(dirname "${BASH_SOURCE[0]}")/jobs)"


realpath is not natively installed on mac FWIW.

instead this is more portable:

Suggested change

export JOB_CONFIG_PATH="$(realpath $(dirname "${BASH_SOURCE[0]}")/jobs)"

export JOB_CONFIG_PATH="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)/jobs"

Thanks. I'm surprised there isn't a portable command to canonicalize paths.

Yeah, cd "${path}" && pwd -P is pretty much the only method doable on POSIX, unless you write your own binary against the C stdlib.

pwd -P (-P == "physical" will follow symlinks for the current working directory and is in all POSIX versions IIRC. -L == logical will not)

BenTheElder · 2019-09-17T04:33:32Z

prow/pj-on-kind.sh

@@ -77,7 +82,7 @@ function ensureInstall() {
  # Install kind and set up cluster if not already done.
  if ! command -v kind >/dev/null 2>&1; then
    echo "Installing kind..."
-    GO111MODULE="on" go get sigs.k8s.io/kind@v0.4.0
+    GO111MODULE="on" go get sigs.k8s.io/kind@v0.5.0


v0.5.1 has minor fixes (windows mainly but...) and is otherwise compatible

BenTheElder · 2019-09-17T04:36:22Z

prow/pj-on-kind.sh

+  job_config="${JOB_CONFIG_PATH:-}"
+  out_dir="${OUT_DIR:-/etc/prowjob-out/${job}}"
+  kind_config="${KIND_CONFIG:-}"
+  node_dir="${NODE_DIR:-/etc/kind-node}"  # Any pod hostPath mounts should be under this dir to reach the true host via the kind node.


nit: this is sort of a mis-use of /etc, this should probably under /var/, /run or /mnt?
https://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard
./shrug

I was going off of https://cloud.google.com/container-optimized-os/docs/concepts/disks-and-filesystem

Looks like I can use /mnt/disks/kind-node instead. Should I change /etc/prowjob-out to /mnt/disks/prowjob-out as well?

So inside the kind node you should be able to do any path, I would do one of the listed ones or invent your own /prow just because these are fairly standardized.

On the host machine I'd think you'd write to something in Home owned by the user. /etc is for system config.

Why is this running on COS directly?

Why is this running on COS directly?

I want the generated pod.yaml files to be suitable for use in a real cluster in addition to the kind cluster. I tried to use a path like /prow, but COS doesn't allow that.

BenTheElder · 2019-09-17T04:36:56Z

prow/pj-on-kind.sh

@@ -25,29 +25,34 @@ function main() {

  # Generate PJ and Pod.
  mkpj "--config-path=${config}" "${job_config}" "--job=${job}" > "${PWD}/pj.yaml"
-  mkpod --build-id=snowflake "--prow-job=${PWD}/pj.yaml" --local "--out-dir=/output/${job}" > "${PWD}/pod.yaml"
+  mkpod --build-id=snowflake "--prow-job=${PWD}/pj.yaml" --local "--out-dir=${out_dir}" > "${PWD}/pod.yaml"


might be better to write to a tempdir instead of PWD?

I used a tempdir initially, but I switched to using PWD so that the pj.yaml and pod.yaml can be easily inspected and modified. WDYT?

Hmm yeah. I tend to avoid writing to PWD in tools but...

You could also tee them to the shell?

You could also tee them to the shell?

Yeah that would work, but seems a little less convenient. pod.yaml and pj.yaml are in the .gitignore for config/ and prow/ at least so I think this is probably ok for now.

I think that tee would result in too much stdout clutter.

I would prefer it if it wrote to a tempdir and echoed something like Created foo/dir.yaml. It's a nit, but this way I don't have to delete them to get them off my git status readout.

I fixed the .gitignore so that the files will be ignored anywhere in the repo. Echoing the temp dir could work, but switching directories every time pj-on-kind.sh is run would be annoying when trying to make iterative changes to the pod.yaml and pj.yaml. It would also make it harder to modify the files (currently you can just comment out the file generation to use your own pj.yaml and pod.yaml, but using a temp dir would make this harder)

BenTheElder · 2019-09-17T04:51:49Z

prow/pod-utils/gcs/upload.go

@@ -174,6 +174,11 @@ type localFileWriter struct {

 func (w *localFileWriter) Write(b []byte) (int, error) {
 	if w.file == nil {
+		if dir := path.Dir(w.filePath); dir != "." {


this check should be unecessary?

package main import ( "os" "fmt" ) func main() { fmt.Println(os.Mkdir(".", os.ModePerm)) fmt.Println(os.MkdirAll(".", os.ModePerm)) }

mkdir .: File exists <nil>

https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_13

chases2

This doc is incredibly helpful, especially as someone who started using this script to test things.

chases2 · 2019-09-17T20:44:30Z

prow/build_test_update.md

+The script does the following:
+1. Installs [mkpj], [mkpod], and [Kind] if they are not found in the path. A [Kind]
+cluster named `mkpod` is created if one does not already exist.
+1. Uses [mkpj] to generate a YAML ProwJob CRD given job name, config, and git refs (if applicable).


What is a CRD?
(CRD is mentioned earlier in the doc, but it would make the most sense to explain the acronym here)

chases2 · 2019-09-17T20:53:55Z

prow/build_test_update.md

+for [prow.istio.io](https://prow.istio.io).
+To test ProwJobs for the [prow.k8s.io] instance use [`config/pj-on-kind.sh`](/config/pj-on-kind.sh).
+
+##### Example


Suggested change

##### Example

##### Example

This command runs the Prow job [`pull-test-infra-yamllint`](https://github.com/kubernetes/test-infra/blob/170921984a34ca40f2763f9e71d6ce6e033dec03/config/jobs/kubernetes/test-infra/test-infra-presubmits.yaml#L94-L107) locally

This explicitly calls out that the command is a Prow Job name, and that it's identical to the name of the configuration objects in config/jobs/...

chases2 · 2019-09-17T20:58:00Z

prow/pj-on-kind.sh

@@ -25,29 +25,34 @@ function main() {

  # Generate PJ and Pod.
  mkpj "--config-path=${config}" "${job_config}" "--job=${job}" > "${PWD}/pj.yaml"
-  mkpod --build-id=snowflake "--prow-job=${PWD}/pj.yaml" --local "--out-dir=/output/${job}" > "${PWD}/pod.yaml"
+  mkpod --build-id=snowflake "--prow-job=${PWD}/pj.yaml" --local "--out-dir=${out_dir}" > "${PWD}/pod.yaml"


I think that tee would result in too much stdout clutter.

I would prefer it if it wrote to a tempdir and echoed something like Created foo/dir.yaml. It's a nit, but this way I don't have to delete them to get them off my git status readout.

k8s-ci-robot · 2019-09-17T21:13:22Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2019-09-17T21:13:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cjwagner, clarketm

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:

~~OWNERS~~ [cjwagner]

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

cjwagner · 2019-09-17T21:22:25Z

I think I've addressed everything. PTAL.

cjwagner · 2019-09-17T21:22:37Z

/hold cancel

chases2 · 2019-09-17T22:15:17Z

prow/build_test_update.md

+and become visible to users by posting results to GitHub (if desired). Changes
+to existing jobs can be trialed on canary jobs.
+
+ProwJobs can also be manually triggered by generating a YAML ProwJob [CRD](https://kubernetes.io/docs/concepts/extend-kubernetes/api-extension/custom-resources/) resource


nit: "Custom Resource Definition resource"

BenTheElder · 2019-09-17T23:18:08Z

prow/pj-on-kind.sh


  # Deploy pod and watch.
  echo "Applying pod to the mkpod cluster. Configure kubectl for the mkpod cluster with:"
-  echo '>  export KUBECONFIG="$(kind get kubeconfig-path --name="mkpod")"'
+  echo '>  export KUBECONFIG="$(kind get kubeconfig-path --name=mkpod)"'


slightly off topic but kubernetes-sigs/kind#850 for future direction there

BenTheElder · 2019-09-17T23:19:49Z

code lgtm

k8s-ci-robot requested review from BenTheElder, chases2 and clarketm September 17, 2019 02:02

cjwagner force-pushed the pj-on-kind branch 2 times, most recently from 220c867 to cc7b95e Compare September 17, 2019 02:34

clarketm reviewed Sep 17, 2019

View reviewed changes

k8s-ci-robot assigned clarketm Sep 17, 2019

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

clarketm reviewed Sep 17, 2019

View reviewed changes

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

clarketm approved these changes Sep 17, 2019

View reviewed changes

BenTheElder reviewed Sep 17, 2019

View reviewed changes

chases2 reviewed Sep 17, 2019

View reviewed changes

cjwagner force-pushed the pj-on-kind branch from cc7b95e to d5d9154 Compare September 17, 2019 21:13

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels Sep 17, 2019

cjwagner force-pushed the pj-on-kind branch from d5d9154 to 96aff6c Compare September 17, 2019 21:17

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

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

cjwagner force-pushed the pj-on-kind branch from 96aff6c to 8a6f613 Compare September 17, 2019 22:59

chases2 reviewed Sep 17, 2019

View reviewed changes

Improvements and docs for pj-on-kind.sh.

8fad953

cjwagner force-pushed the pj-on-kind branch from 8a6f613 to 8fad953 Compare September 17, 2019 23:16

BenTheElder reviewed Sep 17, 2019

View reviewed changes

cjwagner added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 17, 2019

k8s-ci-robot merged commit 1b781b8 into kubernetes:master Sep 17, 2019

k8s-ci-robot added this to the v1.16 milestone Sep 17, 2019

cjwagner deleted the pj-on-kind branch September 18, 2019 00:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements and docs for pj-on-kind.sh. #14355

Improvements and docs for pj-on-kind.sh. #14355

cjwagner commented Sep 17, 2019

clarketm Sep 17, 2019 •

edited

Loading

clarketm left a comment •

edited

Loading

k8s-ci-robot commented Sep 17, 2019

BenTheElder Sep 17, 2019

cjwagner Sep 17, 2019

BenTheElder Sep 17, 2019 •

edited

Loading

BenTheElder Sep 17, 2019

BenTheElder Sep 17, 2019

BenTheElder Sep 17, 2019

cjwagner Sep 17, 2019

BenTheElder Sep 17, 2019

cjwagner Sep 17, 2019

BenTheElder Sep 17, 2019

cjwagner Sep 17, 2019

BenTheElder Sep 17, 2019

cjwagner Sep 17, 2019

chases2 Sep 17, 2019

cjwagner Sep 17, 2019

BenTheElder Sep 17, 2019

chases2 left a comment

chases2 Sep 17, 2019

chases2 Sep 17, 2019

chases2 Sep 17, 2019

k8s-ci-robot commented Sep 17, 2019

k8s-ci-robot commented Sep 17, 2019

cjwagner commented Sep 17, 2019

cjwagner commented Sep 17, 2019

chases2 Sep 17, 2019

BenTheElder Sep 17, 2019

BenTheElder commented Sep 17, 2019


		../prow/pj-on-kind.sh "$@"
		$(dirname "${BASH_SOURCE[0]}")/../prow/pj-on-kind.sh "$@"

	export JOB_CONFIG_PATH="$(realpath $(dirname "${BASH_SOURCE[0]}")/jobs)"
	export JOB_CONFIG_PATH="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)/jobs"

	##### Example
	##### Example
	This command runs the Prow job [`pull-test-infra-yamllint`](https://github.com/kubernetes/test-infra/blob/170921984a34ca40f2763f9e71d6ce6e033dec03/config/jobs/kubernetes/test-infra/test-infra-presubmits.yaml#L94-L107) locally

Improvements and docs for pj-on-kind.sh. #14355

Improvements and docs for pj-on-kind.sh. #14355

Conversation

cjwagner commented Sep 17, 2019

clarketm Sep 17, 2019 • edited Loading

Choose a reason for hiding this comment

clarketm left a comment • edited Loading

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder Sep 17, 2019 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chases2 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

k8s-ci-robot commented Sep 17, 2019

k8s-ci-robot commented Sep 17, 2019

cjwagner commented Sep 17, 2019

cjwagner commented Sep 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenTheElder commented Sep 17, 2019

clarketm Sep 17, 2019 •

edited

Loading

clarketm left a comment •

edited

Loading

BenTheElder Sep 17, 2019 •

edited

Loading