Executors Native SSBC by Piszmog · Pull Request #50079 · sourcegraph/sourcegraph-public-snapshot

Piszmog · 2023-03-28T20:44:56Z

This PR updates the batcheshelper image (Native SSBC) to be feature parity with src. This will allow us to remove the dependency executors have on src.

Changes

Added a Native SSBC Doc page
- Linked as a Batch Change requirement for K8s runtime
Added a README.md to the batcheshelper
Changes to pre mode
Changes to post mode
Updated command.go to check the logs if the step is skipped
- This bubbles up to avoid running the actual step and the post step
Added an Index to the step to understand what "group" of commands belong together
- Helps with skipping
Updated batchSpecWorkspaceStepV2Resolver to handle env resolution by parsing the logs and adding a stepInfo to it

Created Issues

Single K8s Job Pod #50601 to investigate the slowness of K8s runtime (this could also be a local problem)

Test plan

Added unit tests.

Scenarios

Skipped Steps

Hardcoded

Batch spec

name: conditional-hardcoded-test
description: Conditional hardcoded test
on:
  - repositoriesMatchingQuery: file:README.md repo:^github.com/sourcegraph-testing/zap$
steps:
  - run: echo Hello1
    container: alpine:latest
  - if: false
    run: echo Hello2
    container: alpine:latest
  - run: echo Hello3
    container: alpine:latest
changesetTemplate:
  title: Hello from a file
  body: Updated using a file
  branch: file-mount-test
  commit:
    message: Append Hello World to all README.md files

Docker Clip

Screen.Recording.2023-04-13.at.09.22.58.mov

K8s Clip

Screen.Recording.2023-04-13.at.10.06.20.mov

Dynamic

Batch spec

name: conditional-dynamic-test
description: Conditional dynamic test
on:
  - repositoriesMatchingQuery: file:README.md repo:^github.com/sourcegraph-testing/zap$
steps:
  - run: echo Hello1
    container: alpine:latest
  - if: ${{ eq "hello" previous_step.stdout }}
    run: echo Hello2
    container: alpine:latest
  - run: echo Hello3
    container: alpine:latest
changesetTemplate:
  title: Hello from a file
  body: Updated using a file
  branch: file-mount-test
  commit:
    message: Append Hello World to all README.md files

Docker Clip

Screen.Recording.2023-04-13.at.09.23.30.mov

K8s Clip

Screen.Recording.2023-04-13.at.10.22.00.mov

Workspace Files

File

Batch spec

name: python-test
description: Use a python script
on:
  - repositoriesMatchingQuery: file:README.md repo:^github.com/sourcegraph-testing/zap$
steps:
  - run: python /tmp/updater.py
    container: python:latest
    mount:
      - path: ./updater.py
        mountpoint: /tmp/updater.py
changesetTemplate:
  title: Hello from Python
  body: Updated using a Python Script
  branch: python-script-test
  commit:
    message: Append Hello World to all README.md files

Docker Clip

Screen.Recording.2023-04-13.at.09.32.01.mov

K8s Clip

Screen.Recording.2023-04-13.at.10.24.45.mov

File Mounts

Batch spec

name: file-mount-test
description: Mount a file
on:
  - repositoriesMatchingQuery: file:README.md repo:^github.com/sourcegraph-testing/zap$
steps:
  - run: /tmp/my-script.sh
    container: alpine:latest
    files:
      "/tmp/my-script.sh": |
        #!/bin/sh
        IFS=$'\n'; echo Hello World | tee -a $(find -name README.md)
changesetTemplate:
  title: Hello from a file
  body: Updated using a file
  branch: file-mount-test
  commit:
    message: Append Hello World to all README.md files

Docker Clip

Screen.Recording.2023-04-13.at.09.48.05.mov

K8s Clip

Screen.Recording.2023-04-13.at.10.26.56.mov

Env Var Rendering

Batch spec

name: output-env-test
description: Output ENV test
on:
  - repositoriesMatchingQuery: file:README.md repo:^github.com/sourcegraph-testing/zap$
steps:
  - run: echo $MESSAGE1
    container: alpine:latest
    env:
      - MESSAGE1: hello 3
      - PASSWORD: mypass
      - MY_SUPER_PASS
  - run: echo $MESSAGE2
    container: alpine:latest
    env:
      MESSAGE2: ${{ previous_step.stdout }}
changesetTemplate:
  title: Hello from a file
  body: Updated using a file
  branch: file-mount-test
  commit:
    message: Append Hello World to all README.md files

Docker Clip

Screen.Recording.2023-04-13.at.09.51.06.mov

K8s Clip

Screen.Recording.2023-04-13.at.10.28.05.mov

Caching

Batch spec

name: python-test
description: Use a python script
on:
  - repositoriesMatchingQuery: file:README.md repo:^github.com/sourcegraph-testing/zap$
steps:
  - run: python /tmp/updater.py
    container: python:latest
    mount:
      - path: ./updater.py
        mountpoint: /tmp/updater.py
changesetTemplate:
  title: Hello from Python
  body: Updated using a Python Script
  branch: python-script-test
  commit:
    message: Append Hello World to all README.md files

Docker Clip

Screen.Recording.2023-04-13.at.09.54.21.mov

K8s Clip

Screen.Recording.2023-04-13.at.10.29.25.mov

# Conflicts: # enterprise/cmd/executor/internal/worker/runtime/docker.go # enterprise/cmd/executor/internal/worker/runtime/runtime.go

# Conflicts: # doc/admin/executors/deploy_executors_kubernetes.md # enterprise/cmd/executor/internal/worker/runtime/runtime.go

eseliger · 2023-04-18T21:39:01Z

+	skipped, err := batcheslib.SkippedStepsForRepo(batchSpec.Spec, string(repo.Name), workspace.FileMatches)
+	if err != nil {
+		return apiclient.Job{}, err
+	}
+	executionInput.SkippedSteps = skipped


since this simply uses a library method, would it be easier to just let the batcheshelper recompute this vs putting it in the payload? once we go with dynamic skipping, too, we have to recompute it anyways on the client side.

Actually, this is quite the lift to move into batcheshelper. In order for batcheshelper to call this lib function, it needs

BatchSpecWorkspace

Repo name (I believe batcheshelper gets this in the WorkspacesExecutionInput)

BatchSpec

We could update the WorkspacesExecutionInput to have these fields (then have transform.go put these fields in it since it already has all the data), but that's quite a change from how it looks today.

Also, SkippedSteps is static. So it only needs to be computed once. It wont change between steps.

ahh I see 🤔 those are not required for dynamic step skipping?

That's correct. This is just pure static skipping. For dynamic skipping, we use the stepContext with the if to determine if the step should be skipped - this is eval in batcheshelper

stepContext is based on the previous result and WorkspacesExecutionInput.

eseliger · 2023-04-18T22:08:15Z

 		if err := runtimeRunner.Run(ctx, spec); err != nil {
 			return errors.Wrapf(err, "running command %q", spec.CommandSpec.Key)
 		}
+		if strings.HasSuffix(spec.CommandSpec.Key, "pre") {


this again is coupling the batcheshelper and the executor implementation, can we simply skip to the read next step name from the skipfile and jump there? A bit scared that the "pre" word is in here - also because we already talked about that we might be able to consolidate pre and post into one and .. might rename it then :)

The problem I am trying to solve with the pre check is I only want to check for a skip file when a pre step has been ran. I don't want to wait the cycles checking for the skip file on every step - especially when we are not doing a batches job.

So the following is the flow (for batches),

Run the step (step.docker.step.0.pre)

Check for skip file

No skip file

Run step (step.docker.step.0.run)

Run step (step.docker.step.0.post)

Run step (step.docker.step.1.pre)

Check for skip file

There is a skip file - get the "next" key (step.docker.step.3.pre)

Check the next step (step.docker.step.1.run) against the skip key - does not match

Check next step (step.docker.step.1.post) against the skip key - does not match

Check next step (step.docker.step.2.pre) against the skip key - does not match

Check next step (step.docker.step.2.run) against the skip key - does not match

Check next step (step.docker.step.2.post) against the skip key - does not match

Check next step (step.docker.step.3.pre) against the skip key - does match

Run step (step.docker.step.3.pre)

Check for skip file

No skip file

Rinse and repeat

I can't really think of a nice way to not decouple batcheshelper and executor. The skip file is contextual to batcheshelper. I could at least move this check to a lib so all this pre, post, run stuff are all consolidated in a single place versus scatter all over the code base

…xtract out building of the pre, post, and run step keys. Extract the Skip file struct and name

eseliger

Great work. I think I'm fairly comfortable with the state of this. I'd say let's just get this out and see what works and what doesn't and get integration tests written against it :) We can always follow up more and fixup things, or reduce the coupling if we get better ideas.

eseliger · 2023-04-19T16:46:05Z

 )
+
+// CommandKey returns the fully formatted key for the command.
+func CommandKey(name Name, rawStepKey string, index int) string {


should we prevent jumping back? Otherwise, it might mean that one can create a loop.

Could you tell me more on what you mean by jumping back?

step0
step1
step2

are meant to be executed sequentially - if step1 now says "go to step 0", should we allow that? That could cause loops

I don't think we should allow that. However going based on the Key value, there isn't a way to know if we are going backwards without parsing out the index from the Key string.

But we wont run into loops since we are not implementing a "go to" system, but a "skip to" system.

So say step1 says "go to step0", it will skip step2 (since it is not step0) and the Job will complete.

eseliger · 2023-04-19T16:51:20Z

+	skipped, err := batcheslib.SkippedStepsForRepo(batchSpec.Spec, string(repo.Name), workspace.FileMatches)
+	if err != nil {
+		return apiclient.Job{}, err
+	}
+	executionInput.SkippedSteps = skipped


ahh I see 🤔 those are not required for dynamic step skipping?

sanderginn

Another high quality PR 🙌

A lot of unit tests write to or read from disk. It seems to me that disk operations should be mocked out, but I dunno if that's a feasible thing.
There's a bunch of unnecessary line breaks in the markdown files, probably from your IDE formatter. Not sure if those are still rendered incorrectly but probably worth fixing.

sanderginn · 2023-04-20T10:39:35Z

+Native Server-Side Batch Changes is an image that runs Batch Changes without
+requiring [`src-cli`](https://github.com/sourcegraph/src-cli) to be installed on the Executor machine.


Let's include a short motivation for why a customer would benefit from this

I wonder if we should even brand this as "native execution", or simply call it the "new execution infrastructure" or execution model or something. This feels like unnecessary terms for a customer to learn, especially if the migration works well, 99.9% of users should never worry about it. Just my 2ct though.

I like Native Execution

sanderginn · 2023-04-20T10:43:37Z

+The Docker Images that execute the actual Batch Change step require `tee` to be available on the image. Without `tee`,
+the output of the step cannot be captured properly for template variable rendering.


Is it feasible to validate that tee is present on the image somewhere? Maybe in run.Pre()?

I created #51049 to follow up on this.

sanderginn · 2023-04-20T13:39:03Z

+## Build
+
+When running on Kubernetes, the image can be built with the following command.
+
+```shell
+IMAGE=sourcegraph/batcheshelper:insiders ./build.sh
+```
+
+When running the following `sg` commands, `batcheshelper` will automatically be built on any changes to the binary.
+
+- `batcheshelper-builder`
+- `batches`


Why does it need to be built manually?

We don't yet have a profile/command yet in the sg run. So any changes to the source code wont automatically build the images.

sanderginn · 2023-04-20T14:59:19Z

+	// KubernetesMountDir is the path where the Kubernetes volume is mounted in the container.
+	KubernetesMountDir = "/data"
+	// KubernetesMountPath is the path where the Kubernetes volume is mounted in the container.
+	KubernetesMountPath = "/data/"


These are named in a confusing way: I would reflect the mountPath and subPath names in these constants, especially because the KubernetesMountDir is now used as the MountPath in the volume mount.

sanderginn · 2023-04-20T15:29:35Z

+									MountPath: KubernetesMountDir,
+									SubPath:   strings.TrimPrefix(path, KubernetesMountPath),


Looking at what you're doing here, I'm assuming that path contains the workspace (and maybe step) directory, and the files that the job should have access to live in the /data/ directory in this location?

I would suggest to maybe rename KubernetesMountDir to something like ContainerMountPath. KubernetesMountPath could be HostVolumeSubPath or something.

We do want the Kubernetes prefix on these since we have docker and firecracker in the same package. How about KubernetesMountPath for KubernetesMountDir (since it is in additional places than just the volume mount), and KubernetesVolumeMountSubPath for KubernetesMountPath?

sanderginn · 2023-04-20T15:33:27Z

+	if err = os.Remove(path); err != nil {
+		return "", errors.Wrap(err, "removing skip file")
+	}


What happens if os.Remove(path) errors due to something like an I/O failure in the middle of a series of steps? Is the entire job marked as failed?

Yea, you're right. And it is not end of the world if it can't be removed

sanderginn · 2023-04-20T15:36:13Z

+  - `miniube image load executor-kubernetes:latest`
+  - `miniube image load sourcegraph/batcheshelper:insiders`


Suggested change

- `miniube image load executor-kubernetes:latest`

- `miniube image load sourcegraph/batcheshelper:insiders`

- `minikube image load executor-kubernetes:latest`

- `minikube image load sourcegraph/batcheshelper:insiders`

⌨️ 🔨

sanderginn · 2023-04-20T15:47:57Z

-			si.ExitCode = entry.ExitCode
-			si.FinishedAt = entry.StartTime.Add(time.Duration(*entry.DurationMs) * time.Millisecond)
+// SetSafeFunc is a function that can be used to set a value on a StepInfo.
+type SetSafeFunc func(step int, cb func(*StepInfo))


Why is it called a SafeFunc?

Good question.. Seems like it prevents any panics from happening 🤷 - by having checks... ?

I'll rename to just SetFunc

Piszmog added 30 commits March 22, 2023 14:51

Use shell runtime if buildtag set

5b93250

Remove previous version of runners

ce0ef3a

Add shell runtime and runner

789170f

Add tests for shell runner

5dd5863

Add tests for shell runner

218a83f

Merge branch 'rc/executor-runtime-interface' into rc/firecracker-runtime

215fa8c

# Conflicts: # enterprise/cmd/executor/internal/worker/runtime/docker.go # enterprise/cmd/executor/internal/worker/runtime/runtime.go

Merge main and add a test for shell runtime

6d9332b

Add tests for shell runtime

dce6311

Merge branch 'rc/firecracker-runtime' into rc/k8s-runtime

8798f2d

# Conflicts: # doc/admin/executors/deploy_executors_kubernetes.md # enterprise/cmd/executor/internal/worker/runtime/runtime.go

Enable runtime so we can build an image for testing

612be35

Revert change

18ff6fd

Stub a README.md

6181feb

Cleanup main

c33c015

Add additional information to README.md

3aca423

Start splitting out the different modes

eb04f50

Call out that Firecracker is not supported in Kubernetes

e77d925

Fix node selector check

7092bbb

Add validation for EXECUTOR_KUBERNETES_NODE_SELECTOR

0c30419

Merge validations when not in k8s

864f25a

Add additional check of k8s env variables

61a321f

Remove unused function

fc17755

Update constants to have sg-executor prefix

06ae898

Allow the retry to be configurable2

09888c3

Update constants to have sg-executor prefix

ffa61a4

Fix misspelling

b2c016c

Remove docker comment

ba5ad80

Add sg-prefix

affb44b

Remove comment

22cf564

Change the example yaml to just a link to the development yaml

2aad7e0

Remove helm and fix typo

5aa232d

Piszmog added 2 commits April 18, 2023 14:55

Add docs

b7eae49

Format

f72b981

eseliger reviewed Apr 18, 2023

View reviewed changes

Piszmog added 8 commits April 19, 2023 09:11

Update to write a key instead of a numbered index to the skip file. E…

097bd3c

…xtract out building of the pre, post, and run step keys. Extract the Skip file struct and name

Remove redundant tests

6f5469f

Simplify error checking

b6603c6

Simplify code

dfaaa7f

Update bazel

3d16642

Update bazel

63c3c5a

Move context of pre step to the executor util lib

cd803af

Update tests

7f4adb3

eseliger approved these changes Apr 19, 2023

View reviewed changes

Add additional help for minikube

80e011c

sanderginn approved these changes Apr 20, 2023

View reviewed changes

Piszmog added 13 commits April 24, 2023 10:53

Fix misspelling

ce00fe5

Fix naming

c1cc5c3

Fix naming

a5fd04c

Rename to match functionality

adfc18e

Do not error if the file could not be removed

2c8d426

Add additional context about the image

6ddbe81

Remove port

f45b1e6

Merge remote-tracking branch 'origin/main' into rc/native-ssbc

635b932

Debug e2e failure

60cd02d

Debug e2e failure

a726874

Revert

f2c6722

Fix e2e failures

385fedf

Rebrand to native execution

d938540

Piszmog merged commit 3eaa7d4 into main Apr 26, 2023

Piszmog deleted the rc/native-ssbc branch April 26, 2023 20:02

		Native Server-Side Batch Changes is an image that runs Batch Changes without
		requiring [`src-cli`](https://github.com/sourcegraph/src-cli) to be installed on the Executor machine.

		The Docker Images that execute the actual Batch Change step require `tee` to be available on the image. Without `tee`,
		the output of the step cannot be captured properly for template variable rendering.

		MountPath: KubernetesMountDir,
		SubPath: strings.TrimPrefix(path, KubernetesMountPath),

		- `miniube image load executor-kubernetes:latest`
		- `miniube image load sourcegraph/batcheshelper:insiders`

Conversation

Piszmog commented Mar 28, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Created Issues

Test plan

Scenarios

Skipped Steps

Hardcoded

Dynamic

Workspace Files

File

Directory

File Mounts

Env Var Rendering

Caching

Uh oh!

Uh oh!

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!

Piszmog Apr 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Uh oh!

Uh oh!

eseliger left a comment

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!

sanderginn left a comment

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!

Piszmog Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Piszmog commented Mar 28, 2023 •

edited

Loading

Piszmog Apr 19, 2023 •

edited

Loading

Piszmog Apr 24, 2023 •

edited

Loading