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.

Sign up for GitHub

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 basic e2e tests for UBI image #5764

Merged
merged 1 commit into from
Mar 8, 2024
Merged

Add basic e2e tests for UBI image #5764

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,15 +71,26 @@ jobs:
if: ${{ needs.check-changes.outputs.has_changes == 'yes' || github.event_name == 'push' }}
runs-on: [ubuntu-latest]
steps:
- name: Free disk space
# https://github.com/actions/virtual-environments/issues/709
run: |
sudo apt-get clean
df -h
- uses: actions/checkout@v4
with:
show-progress: false
- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3
- uses: actions/setup-go@v4
with:
go-version-file: 'go.mod'
- name: Build Antrea UBI8 Docker image without pushing to registry
if: ${{ github.repository != 'antrea-io/antrea' || github.event_name != 'push' || github.ref != 'refs/heads/main' }}
run: |
./hack/build-antrea-linux-all.sh --pull --distro ubi
- name: Clean up docker build cache
# Clean up build cache to avoid running out of disk space in the following go tests.
run: docker builder prune -f
- name: Build and push Antrea UBI8 Docker image to registry
if: ${{ github.repository == 'antrea-io/antrea' && github.event_name == 'push' && github.ref == 'refs/heads/main' }}
env:
Expand All @@ -91,6 +102,29 @@ jobs:
docker push antrea/antrea-ubi:latest
docker push antrea/antrea-agent-ubi:latest
docker push antrea/antrea-controller-ubi:latest
- name: Install Kind
Copy link
Contributor

Choose a reason for hiding this comment

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

I am OK with running the basic kind test as part of this workflow even though it is not ideal. However, we should probably add a comment explaining why we are doing this (avoid an extra UBI build in a separate workflow)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also don't think this is elegant. Should we move the test part out as a separate job?

Copy link
Contributor

@antoninbas antoninbas Jan 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can move it to kind_ubi.yml, even if that means that the UBI image is built twice (once in build.yml, once in kind_ubi.yml), which is the same as for the Ubuntu image
In the future, we may want to avoid building these images in build.yml for PRs, since there is no point in building them twice (we will still build it for Github push events, since we need to push the new images to the registry in that case). But this should probably be done in a separate PR, and not this one.

Copy link
Member

Choose a reason for hiding this comment

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

How about we make it daily/bidaily job to avoid adding extra time to very basic jobs? I'm more concerned more tests would be added like this if there is a precedent. We could potentially move more rarely failed but long running tests to daily/bidaily job.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the original issue (#5729) I suggested adding a very basic job just to make sure that the UBI-based image is not broken in a pretty obvious way (like it was around 1.14). I was hoping impact on PR test time would be minimal.
One issue that we have today is that even if someone modifies the UBI Dockerfile, we do not check anything besides making sure that the image still builds.

I am not a big fan of the daily jobs, as they can be broken for a long time before someone actually starts looking into the failure.

I think if we go with my suggestion above: skip building the UBI image in build.yml for PRs, and build the image + sanity check it in kind_ubi.yml, the impact on test time will be minimal. The sanity check itself should only take a couple of minutes. Additionally, after I fix #5944, we should see build time reduction for most jobs.

I also think that we should add a trigger to conformance.yml so that we run the K8s conformance tests (at least for the UBI image) for every release PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a bit strange to test the image after pushing it to the registry, but as long as we address #6031 promptly that works for me

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I can follow up with #6031 after this PR.

run: |
KIND_VERSION=$(head -n1 ./ci/kind/version)
curl -Lo ./kind https://github.com/kubernetes-sigs/kind/releases/download/${KIND_VERSION}/kind-$(uname)-amd64
chmod +x ./kind
sudo mv kind /usr/local/bin
- name: Run basic e2e tests
Copy link
Contributor

Choose a reason for hiding this comment

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

I would agree Antonin to put this kind of e2e test to a file kind_ubi.yml which should be similar like kind.yml.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created #6031 to track the follow-up issue.

run: |
mkdir log
ANTREA_LOG_DIR=$PWD/log ./ci/kind/test-e2e-kind.sh --encap-mode encap \
--antrea-controller-image antrea/antrea-controller-ubi \
--antrea-agent-image antrea/antrea-agent-ubi \
--run '^TestBasic$'
- name: Tar log files
if: ${{ failure() }}
run: tar -czf log.tar.gz log
- name: Upload test log
uses: actions/upload-artifact@v3
if: ${{ failure() }}
with:
name: e2e-kind-ubi-basic.tar.gz
path: log.tar.gz
retention-days: 30

build-scale:
needs: check-changes
Expand Down
38 changes: 29 additions & 9 deletions .github/workflows/conformance.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,13 @@ on:
antrea-values:
description: The Antrea Chart values. Multiple values can be separated with commas (e.g. key1=val1,key2=val2). Default configuration will be tested if empty.
required: false
antrea-image-distro:
description: The Antrea image distribution to test. It could be ubuntu or ubi.
type: choice
options:
- ubuntu
- ubi
Copy link
Member

Choose a reason for hiding this comment

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

I didn't know this before, so ubi image can also run on regular clusters?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, it should not be a problem

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. The only difference I noticed is that UBI images only have nf tables available. So it should be OK to test UBI images if the cluster is bootstrapped in nft mode

Copy link
Contributor

@antoninbas antoninbas Feb 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point. Actually the iptables-wrapper repo has a note about this:

RHEL / CentOS / UBI

RHEL 7 ships iptables 1.4, which does not support nft mode. RHEL 8 ships a hacked version of iptables 1.8 that only supports nft mode. Therefore, neither can be used as a basis for a portable iptables-using container image.

I wonder if UBI 9 is different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

9.0_release_notes indicates that iptables-nft has been marked as deprecated. But I think it is safe to use them in UBI 9.

default: ubuntu
k8s-version:
description: The K8s version (e.g. v1.27.1) to test. Kind's default K8s version will be used if empty.
required: false
Expand Down Expand Up @@ -50,36 +57,49 @@ jobs:
run: |
if git show-ref --tags --verify --quiet refs/tags/${{ inputs.antrea-version }}; then
echo "released=true" >> $GITHUB_OUTPUT
echo "image-tag=${{ inputs.antrea-version }}" >> $GITHUB_OUTPUT
else
echo "released=false" >> $GITHUB_OUTPUT
echo "image-tag=latest" >> $GITHUB_OUTPUT
fi
- name: Build Antrea image if required
if: ${{ steps.check-release.outputs.released == 'false' }}
run: |
./hack/build-antrea-linux-all.sh --pull
./hack/build-antrea-linux-all.sh --pull --distro ${{ inputs.antrea-image-distro }}
- name: Install Kind
run: |
KIND_VERSION=$(head -n1 ./ci/kind/version)
KIND_VERSION=$(head -n1 ./ci/kind/version || echo v0.20.0)
Copy link
Member

Choose a reason for hiding this comment

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

thanks for fixing it, I had to switch to a tag when testing previous releases.

curl -Lo ./kind https://github.com/kubernetes-sigs/kind/releases/download/${KIND_VERSION}/kind-$(uname)-amd64
chmod +x ./kind
sudo mv kind /usr/local/bin
- name: Create K8s cluster
run: |
# The command also loads local antrea/antrea-agent-ubuntu:latest and antrea/antrea-controller-ubuntu:latest
# into Nodes if they exist.
images=()
images+=(antrea/antrea-controller-${{ inputs.antrea-image-distro }}:${{ steps.check-release.outputs.image-tag }})
images+=(antrea/antrea-agent-${{ inputs.antrea-image-distro }}:${{ steps.check-release.outputs.image-tag }})
images+=(antrea/antrea-${{ inputs.antrea-image-distro }}:${{ steps.check-release.outputs.image-tag }})
./ci/kind/kind-setup.sh create kind \
--k8s-version "${{ inputs.k8s-version }}"
--k8s-version "${{ inputs.k8s-version }}" \
--images "${images[*]}"
- name: Install Antrea
run: |
helm_args=()
helm_repo="./build/charts/antrea"
if [ ${{ steps.check-release.outputs.released }} == 'true' ]; then
helm_repo="antrea/antrea"
helm_args+=(--version "${{ inputs.antrea-version }}")
helm repo add antrea https://charts.antrea.io
helm repo update
helm install --namespace kube-system antrea antrea/antrea --version "${{ inputs.antrea-version }}" \
--set "${{ inputs.antrea-values }}"
fi
if helm show values ${helm_repo} | grep -q '^controllerImage:'; then
helm_args+=(--set controllerImage.repository="antrea/antrea-controller-${{ inputs.antrea-image-distro }}")
helm_args+=(--set agentImage.repository="antrea/antrea-agent-${{ inputs.antrea-image-distro }}")
else
helm install --namespace kube-system antrea ./build/charts/antrea \
--set "${{ inputs.antrea-values }}"
helm_args+=(--set image.repository="antrea/antrea-${{ inputs.antrea-image-distro }}")
fi
helm install --namespace kube-system antrea ${helm_repo} \
--set "${{ inputs.antrea-values }}" \
"${helm_args[@]}"
kubectl rollout status -n kube-system ds/antrea-agent --timeout=5m
- name: Run e2e tests
run: |
Expand Down
34 changes: 30 additions & 4 deletions ci/kind/test-e2e-kind.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ _usage="Usage: $0 [--encap-mode <mode>] [--ip-family <v4|v6|dual>] [--coverage]
--setup-only Only perform setting up the cluster and run test.
--cleanup-only Only perform cleaning up the cluster.
--test-only Only run test on current cluster. Not set up/clean up the cluster.
--antrea-controller-image The Antrea controller image to use for the test. Default is antrea/antrea-controller-ubuntu.
--antrea-agent-image The Antrea agent image to use for the test. Default is antrea/antrea-agent-ubuntu.
--antrea-image-tag The Antrea image tag to use for the test. Default is latest.
--help, -h Print this message and exit.
"

Expand Down Expand Up @@ -82,6 +85,10 @@ setup_only=false
cleanup_only=false
test_only=false
run=""
antrea_controller_image="antrea/antrea-controller-ubuntu"
antrea_agent_image="antrea/antrea-agent-ubuntu"
use_non_default_images=false
antrea_image_tag="latest"
while [[ $# -gt 0 ]]
do
key="$1"
Expand Down Expand Up @@ -155,6 +162,20 @@ case $key in
test_only=true
shift
;;
--antrea-controller-image)
antrea_controller_image="$2"
use_non_default_images=true
shift 2
;;
--antrea-agent-image)
antrea_agent_image="$2"
use_non_default_images=true
shift 2
;;
--antrea-image-tag)
antrea_image_tag="$2"
shift 2
;;
-h|--help)
print_usage
exit 0
Expand Down Expand Up @@ -186,6 +207,11 @@ if [[ $cleanup_only == "true" ]];then
exit 0
fi

if $use_non_default_images && $coverage; then
echoerr "Cannot use non-default images when coverage is enabled"
exit 1
fi

trap "quit" INT EXIT

manifest_args="$manifest_args --verbose-log"
Expand Down Expand Up @@ -235,11 +261,11 @@ done
# The Antrea images should not be pulled, as we want to use the local build.
if $coverage; then
manifest_args="$manifest_args --coverage"
COMMON_IMAGES_LIST+=("antrea/antrea-agent-ubuntu-coverage:latest" \
"antrea/antrea-controller-ubuntu-coverage:latest")
COMMON_IMAGES_LIST+=("${antrea_controller_image}-coverage:${antrea_image_tag}" \
"${antrea_agent_image}-coverage:${antrea_image_tag}")
Comment on lines -238 to +265
Copy link
Contributor

Choose a reason for hiding this comment

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

it's a bit weird to append the -coverage suffix automatically when a user provides an image names.
I don't have a great solution, but I think we should fail the script if the --coverage flag is used with --antrea-controller-image or --antrea-agent-image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added simple checks to make sure image names cannot be specified with coverage enabled.

else
COMMON_IMAGES_LIST+=("antrea/antrea-agent-ubuntu:latest" \
"antrea/antrea-controller-ubuntu:latest")
COMMON_IMAGES_LIST+=("${antrea_controller_image}:${antrea_image_tag}" \
"${antrea_agent_image}:${antrea_image_tag}")
fi
if $flow_visibility; then
if $coverage; then
Expand Down
Loading