[CI] Fix image names in hw-offload CI #2077

abdallahyas · 2021-04-12T12:33:34Z

Following #1617, the default image name in the antrea.yml file changed
to use the harbor image. Although the patch includes a change in the
make file to change the image name correctly in case the image was
built, the hw-offload CI uses docker build to build the image, and
this made it so that the CI would not use the image it was building,
but the upstream latest image.

This patch fixes that by building the antrea manifests with the image
name being the same as the one that was built.

abdallahyas · 2021-04-12T13:23:52Z

/test-hw-offload

abdallahyas · 2021-04-12T13:40:31Z

/test-hw-offload

antoninbas

I don't really have a lot of context for this change, and the commit description doesn't help much. Why is this suddenly an issue?

antoninbas · 2021-04-12T18:33:42Z

ci/jenkins/mellanox/scripts/start_ci.sh

-        sed -i '/start_ovs/a\        - --hw-offload' $WORKSPACE/antrea-cni/build/yamls/antrea.yml
-    fi
+
+    change_image_name antrea/antrea-ubuntu antrea/antrea-ubuntu:ci


why do we need to rename the image to antrea/antrea-ubuntu:ci?

i though of changing it to a name that does not have an upstream image so that the CI will fail in case it could not find the image, but now that i think about it, we dont need that if we are going to change the antrea.yml anyway.

antoninbas · 2021-04-12T18:35:28Z

ci/jenkins/mellanox/scripts/start_ci.sh

+    change_k8s_resource "DaemonSet" "antrea-agent" spec.template.spec.containers[*].image "antrea/antrea-ubuntu:ci" "$WORKSPACE/antrea-cni/build/yamls/antrea.yml"
+    change_k8s_resource "DaemonSet" "antrea-agent" spec.template.spec.initContainers[*].image "antrea/antrea-ubuntu:ci" "$WORKSPACE/antrea-cni/build/yamls/antrea.yml"
+
+    change_k8s_resource "Deployment" "antrea-controller" spec.template.spec.containers[*].image "antrea/antrea-ubuntu:ci" "$WORKSPACE/antrea-cni/build/yamls/antrea.yml"
+
+    local antrea_ovs_container_index=$(get_daemonset_container_index_from_container_name "antrea-agent" "antrea-ovs" "$WORKSPACE/antrea-cni/build/yamls/antrea.yml")
+    change_k8s_resource "DaemonSet" "antrea-agent" spec.template.spec.containers["$antrea_ovs_container_index"].command[+] "--hw-offload" "$WORKSPACE/antrea-cni/build/yamls/antrea.yml"


why this code instead if a single sed command to rename the image?
this looks a bit clunky to me, it may be a good idea to use a dedicated tool to make yaml changes, like kustomize. Or even update https://github.com/vmware-tanzu/antrea/blob/main/hack/generate-manifest.sh to support generating the appropriate YAML for the CI

sed can be unreliable sometimes, for it to work in this case it needs to look for all image occurrences regardless of the value (because the name may change in the future), but in that case if a container is added with a different image, that container may fail.

I agree with you that yaml files should be updated with a dedicated tool, and i did not know that generate-manifest.sh file existed, so i used the yq tool, which may have made things more missy, but it should be reliable.

I updated the patch to use the generate-manifest.sh to change the image, can you have a look please?

abdallahyas · 2021-04-13T10:54:37Z

/test-hw-offload

abdallahyas · 2021-04-13T11:28:08Z

/test-hw-offload

abdallahyas · 2021-04-13T12:01:37Z

/test-hw-offload

antoninbas · 2021-04-15T01:52:02Z

ci/jenkins/mellanox/scripts/start_ci.sh

    if [[ -z "$(grep hw-offload $WORKSPACE/antrea-cni/build/yamls/antrea.yml)" ]];then
-        sed -i '/start_ovs/a\        - --hw-offload' $WORKSPACE/antrea-cni/build/yamls/antrea.yml
+        sed -i '/start_ovs/a\        - --hw-offload' $ARTIFACTS/antrea.yml
    fi


this should be done through kustomize / generate-manifest.sh, by adding an --hw-offload option (similar to how we support Kind / the OVS netdev datapath)

Done, added a commit to support hw-offload, can you have a look?

antoninbas · 2021-04-15T01:55:49Z

ci/jenkins/mellanox/scripts/start_ci.sh

@@ -94,9 +94,16 @@ EOF
    if [[ -z "${ANTREA_CNI_PR}${ANTREA_CNI_BRANCH}" ]];then
        change_image_name $ANTREA_CNI_HARBOR_IMAGE antrea/antrea-ubuntu


since we use docker build -t antrea/antrea-ubuntu:latest above, isn't this already the name of the image? am I missing something?

That is true in cases of building the image (either from a branch or from a PR), but in case of pulling the harbor image (if the scripts were run without specifying either a branch or a PR), the image name is different.

Because we wanted to avoid renaming the image name in the antrea.yml file, we opted with changing the harbor image name.

now that we use the generate-manifests.sh to generate the antrea.yml with a custom image, we can have a unified name.

I did that in the last update and made the harbor image name the unified name.

antoninbas · 2021-04-15T01:57:29Z

hack/generate-manifest.sh

@@ -157,6 +159,11 @@ case $key in
    CUSTOM_ADM_CONTROLLER=true
    shift
    ;;
+    --dev-image)


I would recommend not adding this new parameter since we already use the IMG_NAME env variable. Maybe you can use that one and check if it is set in the code below (instead of DEV_IMAGE)?

Done, thanks for the pointer.

This patch adds the support for specifying the image used for deploying the antrea pods in case of dev mode. This is needed for debugability in case a custom image is required to be tested with the latest manifests or in case the image was built directly using `docker build` and the tag is changed.

Added the ability to enable hw-offload for the antrea-ovs container using the generate-manefists.sh script. This was done by adding a patch file to the `build/yaml/patches` that would override the antrea-ovs command with `["start_ovs", "--hw-offload"]`

Following antrea-io#1617, the default image name in the antrea.yml file changed to use the harbor image. Although the patch includes a change in the make file to change the image name correctly in case the image was built, the hw-offload CI uses `docker build` to build the image, and this made it so that the CI would not use the image it was building, but the upstream latest image. Also changed the way to enable the hw-offload to use the generate-manifests.sh instead of using `sed`. This patch fixes that by building the antrea manifests with the image name being the same as the one that was built.

abdallahyas · 2021-04-15T07:56:33Z

/test-hw-offload

antoninbas

looks great, thanks for making these changes

antoninbas · 2021-04-15T18:36:11Z

/skip-all

abdallahyas force-pushed the fix-image-name-in-hw-offload branch from 09eeafa to 9864712 Compare April 12, 2021 13:25

antoninbas reviewed Apr 12, 2021

View reviewed changes

abdallahyas force-pushed the fix-image-name-in-hw-offload branch 2 times, most recently from e370afc to 9e9fd0e Compare April 13, 2021 10:44

abdallahyas force-pushed the fix-image-name-in-hw-offload branch from 9e9fd0e to 3906dd6 Compare April 13, 2021 11:59

antoninbas reviewed Apr 15, 2021

View reviewed changes

abdallahyas added 3 commits April 15, 2021 06:35

abdallahyas force-pushed the fix-image-name-in-hw-offload branch from 3906dd6 to 7939d36 Compare April 15, 2021 07:47

abdallahyas requested a review from antoninbas April 15, 2021 08:11

abdallahyas mentioned this pull request Apr 15, 2021

Change daemonset images name to match the built one Mellanox/kubernetes-ci#108

Closed

antoninbas approved these changes Apr 15, 2021

View reviewed changes

antoninbas merged commit 607144d into antrea-io:main Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CI] Fix image names in hw-offload CI #2077

[CI] Fix image names in hw-offload CI #2077

abdallahyas commented Apr 12, 2021 •

edited

Loading

abdallahyas commented Apr 12, 2021

abdallahyas commented Apr 12, 2021

antoninbas left a comment

antoninbas Apr 12, 2021

abdallahyas Apr 13, 2021

antoninbas Apr 12, 2021

abdallahyas Apr 13, 2021

abdallahyas commented Apr 13, 2021

abdallahyas commented Apr 13, 2021

abdallahyas commented Apr 13, 2021

antoninbas Apr 15, 2021

abdallahyas Apr 15, 2021

antoninbas Apr 15, 2021

abdallahyas Apr 15, 2021

antoninbas Apr 15, 2021

abdallahyas Apr 15, 2021

abdallahyas commented Apr 15, 2021

antoninbas left a comment

antoninbas commented Apr 15, 2021

		@@ -94,9 +94,16 @@ EOF
		if [[ -z "${ANTREA_CNI_PR}${ANTREA_CNI_BRANCH}" ]];then
		change_image_name $ANTREA_CNI_HARBOR_IMAGE antrea/antrea-ubuntu

[CI] Fix image names in hw-offload CI #2077

[CI] Fix image names in hw-offload CI #2077

Conversation

abdallahyas commented Apr 12, 2021 • edited Loading

abdallahyas commented Apr 12, 2021

abdallahyas commented Apr 12, 2021

antoninbas 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

abdallahyas commented Apr 13, 2021

abdallahyas commented Apr 13, 2021

abdallahyas commented Apr 13, 2021

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

abdallahyas commented Apr 15, 2021

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas commented Apr 15, 2021

abdallahyas commented Apr 12, 2021 •

edited

Loading