Add image building and tagged testing to CI #350

HenryL27 · 2024-04-12T17:24:31Z

now runtests.sh also builds images for a tag specified at runtime and pushes them to dockerhub, using a registry cache (also in dockerhub)
also push test results to s3 (s3://sycamore-ci/<arch>/<datetime>)

Signed-off-by: Henry Lindeman <[email protected]>

alexaryn

A bunch of comments, but overall like it.

alexaryn · 2024-04-16T23:19:10Z

apps/integration/integration/automation/runtests.sh

+TAG="$1"
+[[ -z "${TAG}" ]] && TAG="latest_rc"
+
+NOW="$(date +"%Y-%m-%d_%H_%M")"


No seconds? At the very least it could be good for debugging.

alexaryn · 2024-04-16T23:20:53Z

apps/integration/integration/automation/runtests.sh

+
+NOW="$(date +"%Y-%m-%d_%H_%M")"
+ARCH="amd64"
+[[ "$(uname -m)" = "arm64" ]] && ARCH="arm64"


Is it bad to just do ARCH=$(uname -m)

I want amd64 instead of X86_64 which is what I get on those machines. I don't anticipate trying to run this on android or raspberry pi or (god forbid) powerpc

alexaryn · 2024-04-16T23:23:40Z

apps/integration/integration/automation/runtests.sh

+GIT_LOGFILE="${RUNDIR}/git.log"
+DOCKER_LOGFILE="${RUNDIR}/docker.log"
+POETRY_LOGFILE="${RUNDIR}/poetry.log"
+PYTEST_LOGFILE="${RUNDIR}/pytest.log"
+QUERY_LOGFILE="${RUNDIR}/test_queries.log"


I'd just rename these to _LOG for brevity. It's no less inaccurate. These are really paths, but nobody wants to read DOCKER_LOGFILE_PATH. Anyway, a log is a file by default.

alexaryn · 2024-04-16T23:26:56Z

apps/integration/integration/automation/runtests.sh

+    build_containers > "${DOCKER_LOGFILE}" 2>&1
+    runtests > "${PYTEST_LOGFILE}" 2>&1
+    local passed_tests=$?
+    handle_outputs passed_tests


Missing a $?

you know it! (exit codes don't do anything weird when they become variables, right?)

I believe the number just carries across the assignment faithfully. It's when used as a conditional that zero becomes true.

alexaryn · 2024-04-16T23:28:54Z

apps/integration/integration/automation/runtests.sh

  else
    echo "No changes detected. Skipping integration tests" >&2
  fi
 }

+error() {


We should call this die for accuracy and consistency with our other scripts and general convention.

leaving as error but changing so it doesn't exit. I basically never want this script to exit early

If it's just cleanup you're worried about, you could also look into trap with EXIT.

alexaryn · 2024-04-17T00:14:13Z

apps/integration/integration/automation/runtests.sh

+}
+
+_docker-build-args() {
+  local branch="$(git status | head -1 | grep 'On branch ' | awk '{print $3}')"


head -n1 is better (fewer warnings). I'd also use grep -i just in case (get it?).

git branch --show-current ?

alexaryn · 2024-04-17T00:15:36Z

apps/integration/integration/automation/runtests.sh

+_docker-build-args() {
+  local branch="$(git status | head -1 | grep 'On branch ' | awk '{print $3}')"
+  local rev="$(git rev-parse --short HEAD)"
+  local date="$(git show -s --format=%ci HEAD | sed 's/ /_/g')"


It's preferred to have sed -e <program>.

Mostly hygiene. If the program happens to have something that might get interpreted as a command-line option, -e protects it. Also, you can do multi-expression scripts with multiple -e directives.

alexaryn · 2024-04-17T00:20:49Z

apps/integration/integration/automation/runtests.sh

+  local rev="$(git rev-parse --short HEAD)"
+  local date="$(git show -s --format=%ci HEAD | sed 's/ /_/g')"
+  local diff=unknown
+  if [[ $(git status | grep -c 'nothing to commit, working tree clean') = 1 ]]; then


Maybe grep -i but even that seems too dependent on the exact wording of the message. Also, you might be able to use the exit code of grep directly, like:

if git status | grep -iq 'nothing to commit'; then

We want to check for the specific message to guarantee the tree is clean (no unexpected files also).

The definition of clean is zero modified files and zero untracked files. The future-proof way to do this is:

if [[ -z $(git status --porcelain) ]]; then

alexaryn · 2024-04-17T00:24:17Z

apps/integration/integration/automation/runtests.sh

+  if [[ $(git status | grep -c 'nothing to commit, working tree clean') = 1 ]]; then
+    diff=clean
+  else
+    diff="pending_changes_$(git diff HEAD | shasum | awk '{print $1}')"


Is the SHA important? If so, SHA-1 is considered obsolete; so, you'd want shasum -a 256, but they're longer. For a shorter checksum that's OK, try md5sum.

alexaryn · 2024-04-17T00:25:01Z

apps/integration/integration/automation/runtests.sh

+  else
+    diff="pending_changes_$(git diff HEAD | shasum | awk '{print $1}')"
+  fi
+  echo "--build-arg=GIT_BRANCH=${branch} --build-arg=GIT_COMMIT=${rev}--${date} --build-arg=GIT_DIFF=${diff}"


Why double-delimiters between rev and date?

Because date has -'s in it; and -- makes it easier to read.

There are better formats for date available, for instance ISO 8601 basic.

eric-anderson · 2024-04-17T17:41:03Z

apps/integration/integration/automation/runtests.sh

 main() {
  if [[ ! -d ".git" ]]; then
    echo "Error: please run this script from sycamore root!" >&2
    exit 1
  fi
+  mkdir -p "${RUNDIR}"
+  echo "Building/testing tag ${TAG}" >&2
  echo "Get the newest git commits" >&2
  checkout_main_if_new
  local should_run=$?
  if [[ $should_run ]]; then


You can shorten this to if [[ checkout_main_if_new ]]; then

You can ditch the square brackets entirely.

$ mytrue() { > return 0 > } $ if mytrue; then > echo yes > fi yes

eric-anderson · 2024-04-17T17:54:02Z

apps/integration/integration/automation/runtests.sh

-    build_containers
-    runtests
-    handle_outputs
+    poetry install > "${POETRY_LOGFILE}" 2>&1


You're going to need || die "poetry install failed"
and similar on all the remaining bits.

well, I always want to get to the handle_outputs, so I don't think I want to die. I think I can && all these things together to get the correct behavior though.

poetry install > "${POETRY_LOGFILE}" 2>&1 \ && build_containers > "${DOCKER_LOGFILE}" 2>&1 \ && runtests > "${PYTEST_LOGFILE}" 2>&1

eric-anderson · 2024-04-17T17:59:03Z

apps/integration/integration/automation/runtests.sh

  new_sha="$(git rev-parse FETCH_HEAD)"
  if [[ "${old_sha}" != "${new_sha}" ]]; then
-    git pull origin main >&2
+    git pull --rebase origin main >> "${GIT_LOGFILE}"


Check that git status is clean.

[[ $(git status | grep -c 'nothing to commit, working tree clean') = 1 ]] || die "Working tree not clean"

since I dont want to die, does
{ echo "Working tree not clean" > "${GIT_LOGFILE}" && return 1; }
do the right thing after the ||?

Hmm, I need it to be okay with untracked files, bc I create them in apps/integration/runs.
will make this a grep -c -e 'nothing to commit, working tree clean' -e 'nothing added to commit but untracked files present'

Let's not rely on specific English-language messages. How about running git status --porcelain | grep -vF '??' and using -z to check for no output.

alternatively I can just add apps/integration/runs to the gitignore and then take the simpler -z git status --procelain you gave me earlier

eric-anderson · 2024-04-17T18:03:56Z

apps/integration/integration/automation/runtests.sh

@@ -1,30 +1,52 @@
 #!/bin/bash

+TAG="$1"
+[[ -z "${TAG}" ]] && TAG="latest_rc"


I would do TAG="integration_tests" by default.

eric-anderson · 2024-04-17T18:07:23Z

apps/integration/integration/automation/runtests.sh

+  [[ -n "${repo_name}" ]] || error "empty repo name"
+  shift
+
+  local platform=linux/amd64,linux/arm64


Do we want this? I would think we would build/test for the local platform.

eric-anderson · 2024-04-17T18:17:06Z

apps/integration/integration/automation/runtests.sh

+  local rev="$(git rev-parse --short HEAD)"
+  local date="$(git show -s --format=%ci HEAD | sed 's/ /_/g')"
+  local diff=unknown
+  if [[ $(git status | grep -c 'nothing to commit, working tree clean') = 1 ]]; then


We want to check for the specific message to guarantee the tree is clean (no unexpected files also).

eric-anderson · 2024-04-17T18:18:03Z

apps/integration/integration/automation/runtests.sh

+  else
+    diff="pending_changes_$(git diff HEAD | shasum | awk '{print $1}')"
+  fi
+  echo "--build-arg=GIT_BRANCH=${branch} --build-arg=GIT_COMMIT=${rev}--${date} --build-arg=GIT_DIFF=${diff}"


Because date has -'s in it; and -- makes it easier to read.

eric-anderson · 2024-04-17T18:26:14Z

apps/integration/integration/automation/runtests.sh

+  mv test-output.log "${QUERY_LOGFILE}"
+  [[ ${passed_tests} = 0 ]] && touch "${RUNDIR}/passed"
+  [[ ${passed_tests} != 0 ]] && touch "${RUNDIR}/failed"
+  aws s3 cp -r "${RUNDIR}" "s3://sycamore-ci/${ARCH}"
 }

 runtests() {
  docker system prune -f --volumes


This seems risky since if someone runs it it will prune other stuff they might want; It also means that we get rid of the old volumes; it's also not guaranteed to clean things up if it's still in use.

I'd instead suggest that we generate a unique run id (use the date stamp?). and do the docker compose -p up. If we tag the images in the same way, then if there's a problem someone could go back and debug against the exact set of images. This would imply we don't want to push to docker hub since it's a lot of stuff.
That guarantees you're getting

eric-anderson · 2024-04-17T18:27:01Z

apps/integration/integration/automation/runtests.sh

@@ -33,20 +55,73 @@ checkout_main_if_new() {

 build_containers() {
  echo "Yep, definitely building containers. That's what this function does" >&2
+  docker-build-hub apps/crawler/crawler/http/Dockerfile


Do we want to build and push to docker hub or only build to local? This seems like it's going to generate a lot of transit data. Moreover, given the way you're doing the testing, if we want to test on arm it would be a separate build/test run and then the two builds would clash.

Personally, I'd find it convenient that reasonably up-to-date images are easily available to test with. Having them in DockerHub seems like the lowest-friction way.

eric-anderson · 2024-04-17T18:28:28Z

apps/integration/integration/automation/runtests.sh

+_docker-repo-name() {
+  local docker_file="$1"
+  echo "Finding repo name in: ${docker_file}" >&2
+  local repo_name="$(grep '^# Repo name: ' "${docker_file}" | awk '{print $4}')"


We don't need to be very robust here. We control the input file.

…ter testing Signed-off-by: Henry Lindeman <[email protected]>

Signed-off-by: Henry Lindeman <[email protected]>

alexaryn · 2024-04-29T15:53:51Z

Is it time to rename runtests.sh to something more reflective of it's current effects? Also, if we drop the suffix, we'd be free to rewrite this in Python without identifying and changing its callers.

Signed-off-by: Henry Lindeman <[email protected]>

HenryL27 · 2024-04-29T16:54:31Z

you know I love rewriting bash into python!

alexaryn

Looking better. I'll let Eric look, too.

I'm still not super-happy calling it runtests when it does so much more. Would things improve if it were split into different scripts (or verb arguments) for the various steps: building, pushing, etc.

alexaryn · 2024-05-02T23:53:23Z

apps/integration/integration/automation/runtests

+  mkdir -p "${RUNDIR}"
+  echo "Building/testing tag ${TAG}" >&2
+  echo "Get the newest git commits" >&2
+  if [[ checkout_main_if_new ]]; then


The square brackets here are superfluous. Nowadays, this stuff is built-in for speed, but check out /bin on a unixy box and you'll see an executable called [. That's a pretty big clue about how this stuff works. It turns out that [ is just like /bin/test; they share a man page, which is worth skimming. The other important thing to note is the ; which terminates the conditional command, which can be a pipeline. Blah blah blah...

alexaryn · 2024-05-02T23:54:10Z

apps/integration/integration/automation/runtests

+    && build_images > "${DOCKER_LOGFILE}" 2>&1 \
+    && runtests > "${PYTEST_LOGFILE}" 2>&1


I'd indent these one more level to reduce confusion.

alexaryn · 2024-05-02T23:57:50Z

apps/integration/integration/automation/runtests

+  [[ ${passed_tests} = 0 ]] && touch "${RUNDIR}/passed"
+  [[ ${passed_tests} != 0 ]] && touch "${RUNDIR}/failed"


if...else would seem more clear here.

alexaryn · 2024-05-02T23:59:53Z

apps/integration/integration/automation/runtests

+  new_sha="$(git rev-parse FETCH_HEAD)"
+  if [[ "${old_sha}" != "${new_sha}" ]]; then
+    [[ -z $(git status --porcelain) ]] \
+      || { echo "Working tree not clean" > "${GIT_LOGFILE}" && return 1; }


What does the semicolon do at the end? I see it a lot in this file.

Not sure, I copied it from the stack overflow I found on grouping. Although I guess I could clean this up a lot by turning the -z into a -n and the || into && and ungrouping it. echo > file will never fail, right?

Signed-off-by: Henry Lindeman <[email protected]>

…utomation

…name it Signed-off-by: Henry Lindeman <[email protected]>

HenryL27 · 2024-05-03T17:29:06Z

I guess git mv and edit a file in one commit doesn't do the right thing w.r.t. comments. ugh, sorry

eric-anderson · 2024-05-06T20:16:15Z

apps/integration/integration/automation/integrate

@@ -0,0 +1,202 @@
+#!/bin/bash


why name this integration/integration from the start?

wdym?
The apps/integration/integration comes from the python project name being equal to the directory name - that pattern is all over this repo.

eric-anderson · 2024-05-06T20:16:31Z

apps/integration/integration/automation/integrate

+TAG="integration_tests"
+while [[ $# -gt 0 ]]; do
+  case "$1" in
+    --help)


eric-anderson · 2024-05-06T20:17:22Z

apps/integration/integration/automation/integrate

+#!/bin/bash
+
+# Parse args
+SKIP_BUILD=0


generally prefer positive conditions, so
RUN_BUILD=1, etc.

I think it needs to be negative conditions. the logic I need is (if I want to be able to chain each step with &&'s) is:

if (action_condition) -> (action) else -> true

with, e.g. DO_BUILD, I have

{ [[ $DO_BUILD ]] && build; } \ && { [[ $DO_TESTS ]] && tests; } \ && etc...

but if DO_BUILD is false this breaks my pipeline. With negative conditions I can do

{ [[ $SKIP_BUILD ]] || build; } \ && { [[ $SKIP_TESTS ]] || tests; } \ && etc...

which has the intended selectivity behavior. To get the right behavior with positive variable I think I need to negate them in the conditions, so SKIP seems cleaner to me

eric-anderson · 2024-05-06T20:45:55Z

apps/integration/integration/automation/integrate

+      ;;
+    --tag)
+      [[ -z $2 ]] && die "A tag must be speicified when using the --tag arg; e.g. --tag my-tag"
+      [[ $2 == "--*" ]] && die "Detected tag was $2. Tags should not begin with --"


I'd suggest tags should always start with lowercase, which is a stronger test. That would prevent --tag -h

eric-anderson · 2024-05-06T20:49:43Z

apps/integration/integration/automation/integrate

+QUERY_LOGFILE="${RUNDIR}/test_queries.log"
+
+main() {
+  [[ ! -d ".git" ]] && die "Please run this script from sycamore root!"


prefer positive tests, so [[ -d ".git" ]] || die

eric-anderson · 2024-05-06T21:09:02Z

apps/integration/integration/automation/integrate

+    echo "Changes detected. Running Tests" >&2
+    poetry install --no-root > "${POETRY_LOGFILE}" 2>&1 \
+        && { [[ $SKIP_BUILD ]] || build_images > "${DOCKER_LOGFILE}" 2>&1; } \
+        && { [[ $SKIP_TESTS ]] || runtests > "${PYTEST_LOGFILE}" 2>&1; }


I'd do:

... && touch "${RUNDIR}/passed_tests" [[ -f "${RUNDIR}/passed_tests" ]] || touch "${RUNDIR}/failed_tests"

If someone put an echo "done with tests" above the local passed_tests=$? line, it would make all the tests appear to pass in the current form.

eric-anderson · 2024-05-06T21:15:18Z

apps/integration/integration/automation/integrate

+}
+
+runtests() {
+  docker system prune -f --volumes


This is a pretty big hammer; my guess is the purpose is to clean up integration test volumes, but it will also clean up build caches and other people's stuff.
I'd suggest:

docker volume rm integration_crawl_data integration_jupyter_data integration_opensearch_data docker compose -p integration up reset

That will clean up integration specific volumes but leave everything else.
Optionally clean them after successful testing and verify there are no volumes named *integration*

There's not, as far as I can tell, a good way to get testcontainers to do project names...

eric-anderson · 2024-05-06T21:16:49Z

apps/integration/integration/automation/integrate

+  echo "Successfully built using docker file $docker_file"
+}
+
+docker-push-hub() {


If you run this on both arm & amd, what shows up in dockerhub? I was building both architectures in a single go because I thought that was necessary for it to show up cleanly (may not matter for integration testing)

eric-anderson · 2024-05-06T21:17:53Z

apps/integration/integration/automation/integrate

+
+docker-push-hub() {
+  local docker_file="$1"
+  [[ -n "${docker_file}" ]] || { error "missing ${docker_file}" && return 1;}


slightly safer to write { error "..."; return 1 }
That way if error returns an error you don't accidentally continue.

eric-anderson · 2024-05-06T21:19:26Z

apps/integration/integration/automation/integrate

+  if (( $(wc -w <<< ${repo_name}) != 1 )); then
+    echo "Unable to find repo name in ${docker_file}" 1>&2
+    exit 1
+  fi


[[ "${repo_name}" = *private* ]] && die "Private repo ${repo_name} disallowed"

do we expect this? but ok

Signed-off-by: Henry Lindeman <[email protected]>

…utomation

Signed-off-by: Henry Lindeman <[email protected]>

HenryL27 added 4 commits April 11, 2024 12:59

build containers in runtests script

96bbc24

Signed-off-by: Henry Lindeman <[email protected]>

fix buildscript wc weirdness

42683d3

Signed-off-by: Henry Lindeman <[email protected]>

use docker registry caching for image builds

36facda

Signed-off-by: Henry Lindeman <[email protected]>

remove post-build exit

79cb6d6

Signed-off-by: Henry Lindeman <[email protected]>

HenryL27 requested a review from eric-anderson April 12, 2024 17:25

HenryL27 changed the title ~~Add image building and tagged testing to CO~~ Add image building and tagged testing to CI Apr 12, 2024

HenryL27 added 2 commits April 15, 2024 10:18

put test logs in s3

f9d266f

Signed-off-by: Henry Lindeman <[email protected]>

specify arch in s3 path for results

6e5122d

Signed-off-by: Henry Lindeman <[email protected]>

alexaryn reviewed Apr 17, 2024

View reviewed changes

eric-anderson reviewed Apr 17, 2024

View reviewed changes

HenryL27 added 3 commits April 18, 2024 09:31

address pr comments. only build images for machines arch, and push af…

49536db

…ter testing Signed-off-by: Henry Lindeman <[email protected]>

allow untracked files when deciding whether to checkout

bb69c94

Signed-off-by: Henry Lindeman <[email protected]>

use exit code for checkout_main_if_new

db82883

Signed-off-by: Henry Lindeman <[email protected]>

HenryL27 added 2 commits April 29, 2024 09:41

make git status checks more robust with --porcelain

fe3ad44

Signed-off-by: Henry Lindeman <[email protected]>

Merge branch 'main' into it-automation

1783048

alexaryn reviewed May 3, 2024

View reviewed changes

HenryL27 added 3 commits May 3, 2024 09:09

address style comments (mostly)

6122bde

Signed-off-by: Henry Lindeman <[email protected]>

Merge branch 'it-automation' of github.com:aryn-ai/sycamore into it-a…

2ed3ffa

…utomation

add args to specify which parts of integration script to run. also re…

82fb8e6

…name it Signed-off-by: Henry Lindeman <[email protected]>

eric-anderson reviewed May 6, 2024

View reviewed changes

HenryL27 added 7 commits May 7, 2024 10:25

address more pr comments

1666e46

Signed-off-by: Henry Lindeman <[email protected]>

change -r to --recursive because aws cli is verbose

fdd90b9

Signed-off-by: Henry Lindeman <[email protected]>

check that test-output exists before trying to move it

bbbb1c1

Signed-off-by: Henry Lindeman <[email protected]>

aws cli doesnt do globs either

f1247c7

Signed-off-by: Henry Lindeman <[email protected]>

add ssh orchestration for building and testing on multiple arches

67f958f

Signed-off-by: Henry Lindeman <[email protected]>

syntax

cab5ebb

Signed-off-by: Henry Lindeman <[email protected]>

test flags correctly

bc6d094

Signed-off-by: Henry Lindeman <[email protected]>

HenryL27 added 6 commits May 10, 2024 11:28

fix regex match

4b34844

Signed-off-by: Henry Lindeman <[email protected]>

tell docker to build for both platforms

2cb85e1

Signed-off-by: Henry Lindeman <[email protected]>

kill the right port forward process

6091289

Signed-off-by: Henry Lindeman <[email protected]>

run remote tests asynchronously

fed8b44

Signed-off-by: Henry Lindeman <[email protected]>

Merge branch 'it-automation' of github.com:aryn-ai/sycamore into it-a…

3e0c027

…utomation

dont prompt about whether to prune networks, just prune them

a9eba5c

Signed-off-by: Henry Lindeman <[email protected]>

		&& build_images > "${DOCKER_LOGFILE}" 2>&1 \
		&& runtests > "${PYTEST_LOGFILE}" 2>&1

		[[ ${passed_tests} = 0 ]] && touch "${RUNDIR}/passed"
		[[ ${passed_tests} != 0 ]] && touch "${RUNDIR}/failed"

Add image building and tagged testing to CI #350

Are you sure you want to change the base?

Add image building and tagged testing to CI #350

Conversation

HenryL27 commented Apr 12, 2024 • edited Loading

alexaryn 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

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

alexaryn commented Apr 29, 2024

HenryL27 commented Apr 29, 2024

alexaryn 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

HenryL27 commented May 3, 2024

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

HenryL27 commented Apr 12, 2024 •

edited

Loading