Create phaino to run prowjobs locally #12163

fejta · 2019-04-11T04:54:55Z

/assign @BenTheElder @stevekuznetsov @Katharine

stevekuznetsov · 2019-04-11T20:22:45Z

I would prefer this to be a separate tool, as mkpj does one thing and does it well

fejta · 2019-04-11T21:14:36Z

oikos it is then

stevekuznetsov · 2019-04-11T22:35:41Z

From Wikipedia ....

The ancient Greek word oikos refers to three related but distinct concepts: the family, the family's property, and the house. Its meaning shifts even within texts, which can lead to confusion.

Also may cause confusion when used as a name for Prow components 🤦‍♂️

fejta · 2019-04-11T22:58:40Z

may cause confusion

Horologium will be proud

fejta · 2019-04-11T23:00:51Z

And let me know if you have an alternate and equally incomprehensible name (need to correct for my error in creating prow/cmd/build)

fejta · 2019-04-12T21:29:27Z

Can someone review?

/assign @amwat

fejta · 2019-04-15T22:47:53Z

Ping!

Katharine · 2019-04-15T23:34:11Z

prow/cmd/phaino/local.go

+		defer cancel()
+	}
+	args, err := convertToLocal(log, pj, priv)
+	if err != nil { // see local.go


Stale comment? (albeit technically true)

Katharine · 2019-04-15T23:39:28Z

prow/cmd/phaino/local.go

+	case err := <-ch:
+		return err
+	case <-ctx.Done():
+		if err := cmd.Process.Kill(); err != nil {


Is this actually going to stop the job running, or just detach the client from it?

Added a TODO. Assuming killing a docker run --rm=true process doesn't also kill the container (quite likely) it will be mildly annoying to get the container id first.

Katharine · 2019-04-15T23:44:39Z

prow/cmd/phaino/local.go

+	fmt.Fprint(os.Stderr, "Privileged jobs are unsafe. Remove from local run? [yes]: ")
+	var out string
+	fmt.Scanln(&out)
+	if out == "" || out[0] == 'y' || out[0] == 'Y' {


"yes" (or other case variations thereof?)

Possibly: require an understandable "no" to not remove, rather than an understandable "yes" to remove, because "no" is the unsafe choice.

Done

(yes will match out[0] == 'y', right?)

(evidently I cannot read.)

Demo: bazel run //prow/cmd/phaino -- https://prow.k8s.io/rerun?prowjob=d08f1ca5-5d63-11e9-ab62-0a580a6c1281

Katharine

/hold

Katharine · 2019-04-16T16:41:50Z

prow/cmd/phaino/local.go

+	case err := <-ch:
+		return err
+	case <-ctx.Done():
+		if err := cmd.Process.Kill(); err != nil { // TODO(fejta): docker rm


Ideally, I'd like to see a warning here that despite timing out the job will keep running, but I'm not terribly hung up about it.

I don't want to leave it this way :)

k8s-ci-robot · 2019-04-16T16:54:18Z

LGTM label has been added.

Git tree hash: 62e9507bb1a60b4c6da47a2c8c581f0f077580ab

k8s-ci-robot · 2019-04-16T16:54:32Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, Katharine

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:

~~prow/OWNERS~~ [Katharine,fejta]

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

fejta · 2019-04-16T17:48:41Z

/hold cancel

cjwagner

I'm excited to try this out! Sorry for the late review.

cjwagner · 2019-04-16T17:14:14Z

prow/cmd/mkpj/main.go

@@ -262,6 +268,9 @@ func main() {
 		logrus.WithError(err).Fatal("Error marshalling YAML.")
 	}
 	fmt.Print(string(b))
+	if o.local {
+		logrus.Info("Use 'bazel run //prow/cmd/oikos' to run this job locally in docker")


Suggested change

logrus.Info("Use 'bazel run //prow/cmd/oikos' to run this job locally in docker")

logrus.Info("Use 'bazel run //prow/cmd/phaino' to run this job locally in docker")

cjwagner · 2019-04-16T17:22:30Z

prow/cmd/phaino/README.md

+  - For prow.k8s.io jobs use `//config:mkpj`
+      * `bazel run //config:mkpj -- --job=pull-test-infra-bazel | bazel run //prow/cmd/bazel`
+  - Other deployments will need to clone that rule and/or pass in extra flags:
+      * `bazel run //prow/cmd/mkpj -- --config=/my/config.yaml --job=my-job | bazel run //prow/cmd/bazel`


Suggested change

* `bazel run //prow/cmd/mkpj -- --config=/my/config.yaml --job=my-job | bazel run //prow/cmd/bazel`

* `bazel run //prow/cmd/mkpj -- --config-path=/my/config.yaml --job=my-job | bazel run //prow/cmd/phaino`

cjwagner · 2019-04-16T17:22:51Z

prow/cmd/phaino/README.md

+
+* Use `mkpj` to create the job and pipe this to `phaino`
+  - For prow.k8s.io jobs use `//config:mkpj`
+      * `bazel run //config:mkpj -- --job=pull-test-infra-bazel | bazel run //prow/cmd/bazel`


Suggested change

* `bazel run //config:mkpj -- --job=pull-test-infra-bazel | bazel run //prow/cmd/bazel`

* `bazel run //config:mkpj -- --job=pull-test-infra-bazel | bazel run //prow/cmd/phaino`

cjwagner · 2019-04-16T17:58:22Z

prow/cmd/phaino/main.go

+	return readPJ(resp.Body)
+}
+
+func readPJs(ctx context.Context, jobs []string) (<-chan prowapi.ProwJob, <-chan error) {


ctx is unused.

cjwagner · 2019-04-16T20:55:24Z

prow/cmd/phaino/README.md

@@ -0,0 +1,43 @@
+# Phaino
+
+Run prowjobs on your local workstation with `phaino`.


Mention what "Phaino" means.

cjwagner · 2019-04-16T21:20:23Z

prow/cmd/phaino/local.go

+			if workingDir == "" {
+				workingDir = dest
+			}
+			localArgs = append(localArgs, "-v", repo+":"+dest)


This ignores BaseRef, BaseSHA, and the Pulls fields of the Refs. Ideally we would respect these, but at a minimum we should warn that they are not respected if they are specified.

Don't think this is necessary as we explicitly ask the user to provide the source for this ref.

Agree that long-term we should support a mode where we check out the repos for you (probably after we add knative-build job support, which will also have to deal with multiple containers).

cjwagner · 2019-04-16T21:23:39Z

prow/cmd/phaino/local.go

+		}
+	}
+	if workingDir == "" {
+		workingDir = "/random-location"


The pod utilities will use the existing value for spec.Containers[0].WorkingDir if there are no refs specified:

test-infra/prow/pod-utils/decorate/podspec.go

Lines 551 to 552 in 4ce54b1

if len(refs) > 0 {

spec.Containers[0].WorkingDir = clone.PathForRefs(codeMount.MountPath, refs[0])

We should probably keep the behavior identical.

cjwagner · 2019-04-16T21:29:25Z

prow/cmd/phaino/local.go

+		return err
+	case <-ctx.Done():
+		if err := cmd.Process.Kill(); err != nil { // TODO(fejta): docker rm
+			log.WithError(err).Warn("Kill error")


nit: error level

IMO warning is correct here. We'll log an error line later

cjwagner · 2019-04-16T21:39:21Z

prow/cmd/phaino/main.go

+	fs.BoolVar(&o.priv, "privileged", false, "Allow privileged local runs")
+	fs.DurationVar(&o.timeout, "timeout", 10*time.Minute, "Maximum duration for each job (0 for unlimited)")
+	fs.DurationVar(&o.totalTimeout, "total-timeout", 0, "Maximum duration for all jobs (0 for unlimited)")
+	fs.DurationVar(&o.grace, "grace", 10*time.Second, "Terminate timed out jobs after this grace period (1s minimum)")


Can we consume the timeout and grace period from the decoration_config if available and if the flags are unspecified?

Done (TODO)

cjwagner · 2019-04-16T21:42:12Z

prow/cmd/phaino/main.go

+	}
+	if pj.Kind != "ProwJob" {
+		return nil, fmt.Errorf("bad kind: %q", pj.Kind)
+	}


We also need to require agent: kubernetes and that the podspec is non-nil.

Once we have a ProwJob admission controller we can share the ProwJob validation logic here.

cjwagner · 2019-04-16T22:44:57Z

We should also mention Phaino and link to its README here: https://github.com/kubernetes/test-infra/blob/master/prow/build_test_update.md#how-to-test-a-prowjob

fejta · 2019-04-18T03:27:02Z

ref #6590

k8s-ci-robot assigned BenTheElder, Katharine and stevekuznetsov Apr 11, 2019

k8s-ci-robot requested review from cjwagner and stevekuznetsov April 11, 2019 04:55

fejta force-pushed the local branch from c66bb43 to b54a491 Compare April 12, 2019 00:30

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

fejta force-pushed the local branch from b54a491 to 4a8bae1 Compare April 12, 2019 21:28

fejta changed the title ~~Add --local flag to mkpj.~~ Create phaino to run prowjobs locally Apr 12, 2019

k8s-ci-robot assigned amwat Apr 12, 2019

fejta force-pushed the local branch from 4a8bae1 to 3a9e1f0 Compare April 12, 2019 22:44

Katharine reviewed Apr 15, 2019

View reviewed changes

Create phaino to run prowjobs locally.

e519d4d

Demo: bazel run //prow/cmd/phaino -- https://prow.k8s.io/rerun?prowjob=d08f1ca5-5d63-11e9-ab62-0a580a6c1281

fejta force-pushed the local branch from 3a9e1f0 to e519d4d Compare April 16, 2019 00:11

Katharine approved these changes Apr 16, 2019

View reviewed changes

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

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 16, 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 Apr 16, 2019

k8s-ci-robot merged commit 94d8a27 into kubernetes:master Apr 16, 2019

cjwagner reviewed Apr 16, 2019

View reviewed changes

fejta deleted the local branch April 16, 2019 23:06

fejta mentioned this pull request Apr 16, 2019

Phaino UX improvements #12231

Merged

	logrus.Info("Use 'bazel run //prow/cmd/oikos' to run this job locally in docker")
	logrus.Info("Use 'bazel run //prow/cmd/phaino' to run this job locally in docker")

	* `bazel run //prow/cmd/mkpj -- --config=/my/config.yaml --job=my-job \| bazel run //prow/cmd/bazel`
	* `bazel run //prow/cmd/mkpj -- --config-path=/my/config.yaml --job=my-job \| bazel run //prow/cmd/phaino`

	* `bazel run //config:mkpj -- --job=pull-test-infra-bazel \| bazel run //prow/cmd/bazel`
	* `bazel run //config:mkpj -- --job=pull-test-infra-bazel \| bazel run //prow/cmd/phaino`

		@@ -0,0 +1,43 @@
		# Phaino

		Run prowjobs on your local workstation with `phaino`.

	if len(refs) > 0 {
	spec.Containers[0].WorkingDir = clone.PathForRefs(codeMount.MountPath, refs[0])

Create phaino to run prowjobs locally #12163

Create phaino to run prowjobs locally #12163

Conversation

fejta commented Apr 11, 2019 • edited Loading

stevekuznetsov commented Apr 11, 2019

fejta commented Apr 11, 2019

stevekuznetsov commented Apr 11, 2019

fejta commented Apr 11, 2019

fejta commented Apr 11, 2019 • edited Loading

fejta commented Apr 12, 2019

fejta commented Apr 15, 2019

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

Katharine left a 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 Apr 16, 2019

k8s-ci-robot commented Apr 16, 2019

fejta commented Apr 16, 2019

cjwagner 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

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

cjwagner commented Apr 16, 2019

fejta commented Apr 18, 2019

fejta commented Apr 11, 2019 •

edited

Loading

fejta commented Apr 11, 2019 •

edited

Loading