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

ci(framework:skip) Add e2e test to test Docker images #4218

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
34 changes: 34 additions & 0 deletions .github/actions/format-docker-build-args/action.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
name: "format-docker-build-args"
description: "Format the build args to work with the wretry.action"
inputs:
build-args:
description: "List of build-time variables."
outputs:
build-args:
description: "Build args formatted to work with the wretry.action"
value: ${{ steps.build-args.outputs.build-args }}
runs:
using: "composite"
steps:
- id: build-args
shell: python
run: |
import os

# Adds two spaces to the line breaks to ensure proper indentation
# when passing the multi-line string to the wretry.action.
# Without it, the multi-line string is passed like this:
#
# build-args: |
# ARG1=
# ARG2=
# ARG3=
#
# This causes the Docker action to interpret ARG2 and ARG3 as keys instead
# of values ​​of the multi-line string.
build_args = '''${{ inputs.build-args }}'''.replace("\n", "\n ")

with open(os.environ['GITHUB_OUTPUT'], 'a') as fh:
print("build-args<<EOF", file=fh)
print(build_args, file=fh)
print("EOF", file=fh)
23 changes: 7 additions & 16 deletions .github/workflows/_docker-build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,24 +58,15 @@ jobs:
hash = hashlib.sha256('''${{ inputs.namespace-repository }}
${{ inputs.file-dir }}
${{ inputs.build-args }}'''.encode())
# Adds two spaces to the line breaks to ensure proper indentation
# when passing the multi-line string to the wretry.action.
# Without it, the multi-line string is passed like this:
#
# build-args: |
# ARG1=
# ARG2=
# ARG3=
#
# This causes the Docker action to interpret ARG2 and ARG3 as keys instead
# of values ​​of the multi-line string.
build_args = '''${{ inputs.build-args }}'''.replace("\n", "\n ")

with open(os.environ['GITHUB_OUTPUT'], 'a') as fh:
print(f"id={hash.hexdigest()}", file=fh)
print("build-args<<EOF", file=fh)
print(build_args, file=fh)
print("EOF", file=fh)

- name: Format build args
uses: ./.github/actions/format-docker-build-args
id: build-args
with:
build-args: ${{ inputs.build-args }}

- name: Extract metadata (tags, labels) for Docker
id: meta
Expand Down Expand Up @@ -105,7 +96,7 @@ jobs:
context: "{{defaultContext}}:${{ inputs.file-dir }}"
outputs: type=image,name=${{ inputs.namespace-repository }},push-by-digest=true,name-canonical=true,push=true
build-args: |
${{ steps.build-id.outputs.build-args }}
${{ steps.build-args.outputs.build-args }}

- name: Export digest
run: |
Expand Down
100 changes: 100 additions & 0 deletions .github/workflows/_docker-test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
name: Reusable docker image test workflow
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
name: Reusable docker image test workflow
name: Reusable Docker image test workflow


on:
workflow_call:
inputs:
build-args:
description: "List of build-time variables."
required: false
type: string
ref:
required: false
type: string
secrets:
dockerhub-user:
required: true
dockerhub-token:
required: true

permissions:
contents: read

jobs:
test:
name: Test image
runs-on: "ubuntu-22.04"
timeout-minutes: 10
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the benefit of using a hash instead of the version (as we've previously used in other CI steps)? Is it so that we can use a minor+patched version?

Copy link
Contributor

Choose a reason for hiding this comment

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

I also see this repeated in other steps like for docker@setup-buildx-action and Wandalen/wretry.action.

with:
ref: ${{ inputs.ref }}

- name: Format build args
uses: ./.github/actions/format-docker-build-args
id: build-args
with:
build-args: ${{ inputs.build-args }}

- id: matrix
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's describe this step:

Suggested change
- id: matrix
- name: Create matrix for base image
- id: matrix

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like we actually don't use a matrix strategy here. Is this planned for another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right, we're not directly using the matrix strategy, but instead we generate the matrix using the dev/build-docker-image-matrix.py script, which then feeds into subsequent jobs and steps. This script-based approach simplifies maintenance and updates by centralizing the matrix generation, making it easier to add, remove, or modify images as needed.

run: |
python dev/build-docker-image-matrix.py --flwr-version e2e-test --simple > matrix.json
echo "matrix=$(cat matrix.json)" >> $GITHUB_OUTPUT
echo "base-file-dir=$(jq -r .base.images[0].file_dir matrix.json)" >> $GITHUB_OUTPUT
echo "base-namespace-repository=$(jq -r .base.images[0].namespace_repository matrix.json)" >> $GITHUB_OUTPUT

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@988b5a0280414f521da01fcc63a27aeeb4b104db # v3.6.1
with:
# https://github.com/docker/build-push-action/issues/1116#issuecomment-2132899905
driver: docker

- name: Build base image
uses: Wandalen/wretry.action@6feedb7dedadeb826de0f45ff482b53b379a7844 # v3.5.0
with:
action: docker/build-push-action@5cd11c3a4ced054e52742c5fd54dca954e0edd85 # v6.7.0
attempt_limit: 60 # 60 attempts * (9 secs delay + 1 sec retry) = ~10 mins
attempt_delay: 9000 # 9 secs
with: |
pull: true
context: ${{ steps.matrix.outputs.base-file-dir }}
load: true
tags: ${{ steps.matrix.outputs.base-namespace-repository }}:e2e-test
build-args: |
${{ steps.build-args.outputs.build-args }}

- name: Build binary images
run: |
set -e -o pipefail

jq -c '.binary.images[]' <<< '${{ steps.matrix.outputs.matrix }}' | while read -r i; do
FILE_DIR=$(echo "$i" | jq -r .file_dir)
NAMESPACE_REPOSITORY=$(echo "$i" | jq -r .namespace_repository)
docker build --build-arg BASE_IMAGE="e2e-test" -t "${NAMESPACE_REPOSITORY}:e2e-test" "${FILE_DIR}"
done

- uses: ./.github/actions/bootstrap
- name: Install Flower
run: python -m pip install .

- name: Test images
env:
FLWR_VERSION: e2e-test
PROJECT_DIR: e2e-test
working-directory: src/docker/complete
run: |
flwr new ${PROJECT_DIR} --framework NumPy --username flower

docker compose -f certs.yml up --build
sudo chown -R 49999:49999 superexec-certificates/* superlink-certificates/*

cat <<- EOT >> "${PROJECT_DIR}/pyproject.toml"
[tool.flwr.federations.local-deployment-tls]
address = "127.0.0.1:9093"
root-certificates = "../superexec-certificates/ca.crt"
EOT
Comment on lines +91 to +98
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we include a matrix of tests for secure/insecure connections and client authentication later?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we can include those tests when we in the Reinstate E2E tests for Docker RFC.


docker compose -f compose.yml -f with-tls.yml up --build -d
docker compose logs -f &

timeout 30 flwr run ${PROJECT_DIR} local-deployment-tls
timeout 120 docker compose logs -f superexec | grep -q 'Run finished'
Comment on lines +103 to +104
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this test is very straightforward. Is it still worth for us to run it with pytest like in #3477? Since your approach can be easily parameterized in a matrix, maybe it makes sense to follow and extend your approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good question. With pytest, we can run all tests locally, but if we use a matrix, we'll likely need to run a bash loop to execute all tests. I prefer to use pytest because it makes it easier to run tests locally.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, does it makes sense to do this in 2 steps?

  1. Resolve and merge this PR.
  2. Migrate the tests to pytest.

18 changes: 7 additions & 11 deletions .github/workflows/docker-build-main.yml
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ jobs:
pip-version: ${{ steps.versions.outputs.pip-version }}
setuptools-version: ${{ steps.versions.outputs.setuptools-version }}
flwr-version-ref: ${{ steps.versions.outputs.flwr-version-ref }}
matrix: ${{ steps.versions.outputs.matrix }}
steps:
- uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1

Expand All @@ -26,15 +27,17 @@ jobs:
echo "pip-version=${{ steps.bootstrap.outputs.pip-version }}" >> "$GITHUB_OUTPUT"
echo "setuptools-version=${{ steps.bootstrap.outputs.setuptools-version }}" >> "$GITHUB_OUTPUT"
echo "flwr-version-ref=git+${{ github.server_url }}/${{ github.repository }}.git@${{ github.sha }}" >> "$GITHUB_OUTPUT"
python dev/build-docker-image-matrix.py --flwr-version unstable --simple > matrix.json
echo "matrix=$(cat matrix.json)" >> $GITHUB_OUTPUT

build-docker-base-images:
name: Build base images
if: github.repository == 'adap/flower'
uses: ./.github/workflows/_docker-build.yml
needs: parameters
with:
namespace-repository: flwr/base
file-dir: src/docker/base/ubuntu
namespace-repository: ${{ fromJson(needs.parameters.outputs.matrix).base[0].namespace_repository }}
file-dir: ${{ fromJson(needs.parameters.outputs.matrix).base[0].file_dir }}
build-args: |
PIP_VERSION=${{ needs.parameters.outputs.pip-version }}
SETUPTOOLS_VERSION=${{ needs.parameters.outputs.setuptools-version }}
Expand All @@ -48,17 +51,10 @@ jobs:
name: Build binary images
if: github.repository == 'adap/flower'
uses: ./.github/workflows/_docker-build.yml
needs: build-docker-base-images
needs: [parameters, build-docker-base-images]
strategy:
fail-fast: false
matrix:
images: [
{ repository: "flwr/superlink", file_dir: "src/docker/superlink" },
{ repository: "flwr/supernode", file_dir: "src/docker/supernode" },
{ repository: "flwr/serverapp", file_dir: "src/docker/serverapp" },
{ repository: "flwr/superexec", file_dir: "src/docker/superexec" },
{ repository: "flwr/clientapp", file_dir: "src/docker/clientapp" }
]
matrix: ${{ fromJson(needs.parameters.outputs.matrix).binary }}
with:
namespace-repository: ${{ matrix.images.repository }}
file-dir: ${{ matrix.images.file_dir }}
Expand Down
43 changes: 43 additions & 0 deletions .github/workflows/docker-test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: Test Docker

on:
push:
Comment on lines +3 to +4
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is quite resource intensive. Maybe we can limit the workflow to push and pull_request to main?

Suggested change
on:
push:
on:
push:
branches:
- main
pull_request:
branches:
- main

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be removed. This workflow is just used to run the Docker e2e test workflow

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I'm missing something, can the workflow be triggered if we don't specify on:?


jobs:
# parameters:
# if: github.repository == 'adap/flower'
# name: Collect docker build parameters
# runs-on: ubuntu-22.04
# timeout-minutes: 10
# outputs:
# pip-version: ${{ steps.versions.outputs.pip-version }}
# setuptools-version: ${{ steps.versions.outputs.setuptools-version }}
# flwr-version-ref: ${{ steps.versions.outputs.flwr-version-ref }}
# matrix: ${{ steps.versions.outputs.matrix }}
# steps:
# - uses: actions/checkout@b4ffde65f46336ab88eb53be808477a3936bae11 # v4.1.1

# - uses: ./.github/actions/bootstrap
# id: bootstrap

# - id: versions
# run: |
# echo "pip-version=${{ steps.bootstrap.outputs.pip-version }}" >> "$GITHUB_OUTPUT"
# echo "setuptools-version=${{ steps.bootstrap.outputs.setuptools-version }}" >> "$GITHUB_OUTPUT"
# echo "flwr-version-ref=git+${{ github.server_url }}/${{ github.repository }}.git@${{ github.sha }}" >> "$GITHUB_OUTPUT"
# python dev/build-docker-image-matrix.py --flwr-version unstable --simple > matrix.json
# echo "matrix=$(cat matrix.json)" >> $GITHUB_OUTPUT
Comment on lines +7 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these lines be uncommented once the parameters are fully defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be removed. This workflow is just used to run the Docker e2e test workflow

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I've the same response as above.


build-docker-base-images:
name: Build base images
if: github.repository == 'adap/flower'
uses: ./.github/workflows/_docker-test.yml
# needs: parameters
with:
build-args: |
PIP_VERSION=24.1
SETUPTOOLS_VERSION=70
FLWR_VERSION=1.11.0
secrets:
dockerhub-user: ${{ secrets.DOCKERHUB_USERNAME }}
dockerhub-token: ${{ secrets.DOCKERHUB_TOKEN }}
18 changes: 7 additions & 11 deletions .github/workflows/release-nightly.yml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ jobs:
skip: ${{ steps.release.outputs.skip }}
pip-version: ${{ steps.release.outputs.pip-version }}
setuptools-version: ${{ steps.release.outputs.setuptools-version }}
matrix: ${{ steps.release.outputs.matrix }}
steps:
- uses: actions/checkout@v4
- name: Bootstrap
Expand All @@ -37,15 +38,17 @@ jobs:
echo "version=$(poetry version -s)" >> $GITHUB_OUTPUT
echo "pip-version=${{ steps.bootstrap.outputs.pip-version }}" >> "$GITHUB_OUTPUT"
echo "setuptools-version=${{ steps.bootstrap.outputs.setuptools-version }}" >> "$GITHUB_OUTPUT"
python dev/build-docker-image-matrix.py --flwr-version nightly --simple > matrix.json
echo "matrix=$(cat matrix.json)" >> $GITHUB_OUTPUT

build-docker-base-images:
name: Build nightly base images
if: github.repository == 'adap/flower' && needs.release-nightly.outputs.skip != 'true'
uses: ./.github/workflows/_docker-build.yml
needs: release-nightly
with:
namespace-repository: flwr/base
file-dir: src/docker/base/ubuntu
namespace-repository: ${{ fromJson(needs.release-nightly.outputs.matrix).base[0].namespace_repository }}
file-dir: ${{ fromJson(needs.release-nightly.outputs.matrix).base[0].file_dir }}
build-args: |
PIP_VERSION=${{ needs.release-nightly.outputs.pip-version }}
SETUPTOOLS_VERSION=${{ needs.release-nightly.outputs.setuptools-version }}
Expand All @@ -65,16 +68,9 @@ jobs:
needs: [release-nightly, build-docker-base-images]
strategy:
fail-fast: false
matrix:
images: [
{ repository: "flwr/superlink", file_dir: "src/docker/superlink" },
{ repository: "flwr/supernode", file_dir: "src/docker/supernode" },
{ repository: "flwr/serverapp", file_dir: "src/docker/serverapp" },
{ repository: "flwr/superexec", file_dir: "src/docker/superexec" },
{ repository: "flwr/clientapp", file_dir: "src/docker/clientapp" }
]
matrix: ${{ fromJson(needs.release-nightly.outputs.matrix).binary }}
with:
namespace-repository: ${{ matrix.images.repository }}
namespace-repository: ${{ matrix.images.namespace_repository }}
file-dir: ${{ matrix.images.file_dir }}
build-args: BASE_IMAGE=${{ needs.release-nightly.outputs.version }}
tags: |
Expand Down
Loading