Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add image building and tagged testing to CI #350

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
96bbc24
build containers in runtests script
HenryL27 Apr 11, 2024
42683d3
fix buildscript wc weirdness
HenryL27 Apr 11, 2024
36facda
use docker registry caching for image builds
HenryL27 Apr 11, 2024
79cb6d6
remove post-build exit
HenryL27 Apr 12, 2024
f9d266f
put test logs in s3
HenryL27 Apr 15, 2024
6e5122d
specify arch in s3 path for results
HenryL27 Apr 15, 2024
49536db
address pr comments. only build images for machines arch, and push af…
HenryL27 Apr 18, 2024
bb69c94
allow untracked files when deciding whether to checkout
HenryL27 Apr 18, 2024
db82883
use exit code for checkout_main_if_new
HenryL27 Apr 18, 2024
fe3ad44
make git status checks more robust with --porcelain
HenryL27 Apr 29, 2024
1783048
Merge branch 'main' into it-automation
HenryL27 Apr 29, 2024
6122bde
address style comments (mostly)
HenryL27 May 3, 2024
2ed3ffa
Merge branch 'it-automation' of github.com:aryn-ai/sycamore into it-a…
HenryL27 May 3, 2024
82fb8e6
add args to specify which parts of integration script to run. also re…
HenryL27 May 3, 2024
1666e46
address more pr comments
HenryL27 May 7, 2024
fdd90b9
change -r to --recursive because aws cli is verbose
HenryL27 May 8, 2024
bbbb1c1
check that test-output exists before trying to move it
HenryL27 May 8, 2024
f1247c7
aws cli doesnt do globs either
HenryL27 May 8, 2024
67f958f
add ssh orchestration for building and testing on multiple arches
HenryL27 May 10, 2024
cab5ebb
syntax
HenryL27 May 10, 2024
bc6d094
test flags correctly
HenryL27 May 10, 2024
4b34844
fix regex match
HenryL27 May 10, 2024
2cb85e1
tell docker to build for both platforms
HenryL27 May 10, 2024
6091289
kill the right port forward process
HenryL27 May 10, 2024
fed8b44
run remote tests asynchronously
HenryL27 May 10, 2024
3e0c027
Merge branch 'it-automation' of github.com:aryn-ai/sycamore into it-a…
HenryL27 May 10, 2024
a9eba5c
dont prompt about whether to prune networks, just prune them
HenryL27 May 11, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -34,3 +34,6 @@ apps/jupyter/bind_dir/redirect.html
.venv
lib/remote-processors/remote_processors/*pb2*
poetry.toml
apps/integration/runs/
notebooks/data/
test-output.log
202 changes: 202 additions & 0 deletions apps/integration/integration/automation/integrate
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
#!/bin/bash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why name this integration/integration from the start?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


# Parse args
SKIP_BUILD=0
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

SKIP_TESTS=0
SKIP_PUSH=0
TAG="integration_tests"
while [[ $# -gt 0 ]]; do
case "$1" in
--help)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

--help|-h)

echo "Utility script for building containers, running integration tests, and pushing images"
echo "Make sure to run this from the sycamore root directory."
echo "-------------------------------------------------------"
echo "Arguments:"
echo " --help Display this message"
echo " --build Build images"
echo " --tests Run integration tests"
echo " --push Push images"
echo " --tag [TAG] When building, running, and/or pushing, use this docker tag."
echo " Default is 'integration_tests'"
exit 0
;;
--build)
SKIP_BUILD=1
echo "Will build images"
shift
;;
--tests)
SKIP_TESTS=1
echo "Will run integration tests"
shift
;;
--push)
SKIP_PUSH=1
echo "Will push images"
shift
;;
--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 --"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

TAG="$2"
echo "Using tag ${TAG}"
shift
shift
;;
esac
done

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

RUNDIR="apps/integration/runs/${NOW}"
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"

main() {
[[ ! -d ".git" ]] && die "Please run this script from sycamore root!"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

mkdir -p "${RUNDIR}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest having a --clean option that will delete RUNDIR before recreating it. That way you know everything is clean. I usually end up defaulting options like that to off for performance.

echo "Building/testing tag ${TAG}" >&2
echo "Get the newest git commits" >&2
if checkout_main_if_new; then
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; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

local passed_tests=$?
[[ $SKIP_PUSH ]] || push_images >> "${DOCKER_LOGFILE}" 2>&1
handle_outputs $passed_tests
else
echo "No changes detected. Skipping integration tests" >&2
fi
}

error() {
echo "ERROR: $@" >&2
}

die() {
error "$1"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

error "$@"
(you want to capture everything passed to die)

exit 1
}

checkout_main_if_new() {
old_sha="$(git rev-parse HEAD)"
git fetch origin main > "${GIT_LOGFILE}" 2>&1
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; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this needs to be die "Working tree not clean"
Otherwise it will continue and skip tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yeah. My initial thought was that hiving it continue and skip all the stuff would be fine but I guess you probably do want this case to outright kill itself

git pull --rebase origin main >> "${GIT_LOGFILE}" 2>&1
return 0
else
return 1
fi
}

build_images() {
echo "Building all images" >&2
docker-build-hub apps/crawler/crawler/http/Dockerfile \
&& docker-build-hub apps/crawler/crawler/s3/Dockerfile \
&& docker-build-hub apps/importer/Dockerfile.buildx \
&& docker-build-hub apps/opensearch/Dockerfile \
&& docker-build-hub apps/jupyter/Dockerfile.buildx --build-arg=TAG="${TAG}" \
&& docker-build-hub apps/demo-ui/Dockerfile.buildx \
&& docker-build-hub apps/remote-processor-service/Dockerfile.buildx \
&& return 0
return 1
}

handle_outputs() {
echo "Handling test outputs" >&2
local passed_tests="$1"
mv test-output.log "${QUERY_LOGFILE}"
if [[ ${passed_tests} = 0 ]]; then
touch "${RUNDIR}/passed"
else
touch "${RUNDIR}/failed"
fi
aws s3 cp -r "${RUNDIR}" "s3://sycamore-ci/${ARCH}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having arch be the top level thing to me feels weird. I'd suggest top level is the git version and below that is arch since I'd expect amd64 & arm64 builds for some rev have more in common.

}

push_images() {
echo "Pushing tested images to dockerhub" >&2
docker-push-hub apps/crawler/crawler/http/Dockerfile \
&& docker-push-hub apps/crawler/crawler/s3/Dockerfile \
&& docker-push-hub apps/importer/Dockerfile.buildx \
&& docker-push-hub apps/opensearch/Dockerfile \
&& docker-push-hub apps/jupyter/Dockerfile.buildx \
&& docker-push-hub apps/demo-ui/Dockerfile.buildx \
&& docker-push-hub apps/remote-processor-service/Dockerfile.buildx \
&& return 0
return 1
}

runtests() {
docker system prune -f --volumes
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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*

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

docker compose up reset
poetry run pytest apps/integration/ -p integration.conftest --noconftest --docker-tag "${TAG}"
# this is a complicated command, so:
# -p integration.conftest - load conftest with plugins, to capture the custom command line arg (--docker-tag)
# --noconftest - don't load conftest at pytest runtime; it's already loaded
# --docker-tag - specify tag of containers to test
return $?
}

docker-build-hub() {
local docker_file="$1"
[[ -n "${docker_file}" ]] || { error "missing ${docker_file}" && return 1;}
local repo_name="$(_docker-repo-name "${docker_file}")"
[[ -n "${repo_name}" ]] || { error "empty repo name" && return 1;}
shift

echo
echo "Building in sycamore and pushing to docker hub with repo name '${repo_name}'"
docker buildx build "$(_docker-build-args)" -t "${repo_name}:${TAG}" -f "${docker_file}" \
--cache-to type=registry,ref="${repo_name}:build-cache",mode=max \
--cache-from type=registry,ref="${repo_name}:build-cache" \
"$@" --load . || { error "buildx failed" && return 1;}
echo "Successfully built using docker file $docker_file"
}

docker-push-hub() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

local docker_file="$1"
[[ -n "${docker_file}" ]] || { error "missing ${docker_file}" && return 1;}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

local repo_name="$(_docker-repo-name "${docker_file}")"
[[ -n "${repo_name}" ]] || { error "empty repo name" && return 1;}

echo
echo "Pushing image to docker hub for repo '${repo_name}"
docker push "${repo_name}:${TAG}" || { error "docker push failed" && return 1;}
echo "Successfully pushed image previously built from dockerfile ${docker_file}"
}

_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}')"
if (( $(wc -w <<< ${repo_name}) != 1 )); then
echo "Unable to find repo name in ${docker_file}" 1>&2
exit 1
fi
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we expect this? but ok

echo "${repo_name}"
}

_docker-build-args() {
local branch="$(git branch --show-current)"
local rev="$(git rev-parse --short HEAD)"
local date="$(git show -s --format=%ci HEAD | sed -e 's/ /_/g')"
local diff=unknown
if [[ -z $(git status --porcelain) ]]; then
diff=clean
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}"
}

main
52 changes: 0 additions & 52 deletions apps/integration/integration/automation/runtests.sh

This file was deleted.

Loading