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

Jump to bottom

Modify workflows, ci and manifest scripts for split images #5903

Merged
merged 1 commit into from
Jan 26, 2024
Merged

Modify workflows, ci and manifest scripts for split images #5903

merged 1 commit into from
Jan 26, 2024

Conversation

jainpulkit22
Copy link
Contributor

@jainpulkit22 jainpulkit22 commented Jan 23, 2024 •
edited

Part of #5691.

@jainpulkit22 jainpulkit22 mentioned this pull request Jan 23, 2024
Closed
@jainpulkit22 jainpulkit22 changed the title Modified workflows, ci and manifest scripts for split images Modify workflows, ci and manifest scripts for split images Jan 23, 2024
antoninbas
antoninbas reviewed Jan 24, 2024
View reviewed changes
build/charts/antrea/templates/agent/daemonset.yaml Outdated Show resolved Hide resolved
build/charts/antrea/templates/controller/deployment.yaml Outdated Show resolved Hide resolved
hack/generate-manifest.sh Outdated Show resolved Hide resolved
@jainpulkit22 jainpulkit22 added this to the Antrea v1.15 release milestone Jan 24, 2024
@jainpulkit22 jainpulkit22 added the action/release-note Indicates a PR that should be included in release notes. label Jan 24, 2024
luolanzone
luolanzone reviewed Jan 24, 2024
View reviewed changes
ci/jenkins/test-vmc.sh Outdated
Comment on lines 456 to 457
copy_image antrea-ubuntu.tar docker.io/antrea/antrea-agent-ubuntu ${IPs[$i]} ${DOCKER_IMG_VERSION} true
copy_image "" docker.io/antrea/antrea-controller-ubuntu ${IPs[$i]} ${DOCKER_IMG_VERSION} false
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel it's better to to save the image into different tar files in lines 436-442, and use them correspondingly here.
It's confused to copy an empty image in copy_image

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the line 424 still using antrea-ubuntu:latest?

sed -i "0,/antrea-ubuntu:latest/{s/antrea-ubuntu:latest/antrea-ubuntu:$DOCKER_IMG_VERSION/}" ${GIT_CHECKOUT_DIR}/build/yamls/$antrea_yml

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that we also use old Antrea image in this script from line 462. I feel this might be broken after image split. @XinShuYang could you help to estimate? Thanks.
Looks like OLD_ANTREA_VERSION is only used in {name}-{test_name}-matrix-compatibility-test job. Maybe we should allow this being broken for this change? @antoninbas @tnqn

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 feels like we may have to address it in the follow up PR.
I think it will only be broken after we release 1.15, and we start testing upgrade from Antrea v1.15 to a later version. At this point we may have to account for the fact that the old version can either be using a unified image or split images.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that we also use old Antrea image in this script from line 462. I feel this might be broken after image split. @XinShuYang could you help to estimate? Thanks.

Yes, we only pull old image in {name}-{test_name}-matrix-compatibility-test, I am okay to fix it in a separate PR after 1.15 release.

ci/test-conformance-aks.sh Outdated Show resolved Hide resolved
ci/test-conformance-eks.sh Outdated Show resolved Hide resolved
ci/test-conformance-gke.sh Outdated Show resolved Hide resolved
hack/generate-helm-release.sh Show resolved Hide resolved
hack/generate-standard-manifests.sh Outdated Show resolved Hide resolved
XinShuYang
XinShuYang reviewed Jan 25, 2024
View reviewed changes
ci/jenkins/test-vmc.sh Outdated Show resolved Hide resolved
XinShuYang
XinShuYang reviewed Jan 25, 2024
View reviewed changes
ci/jenkins/test-vmc.sh Outdated Show resolved Hide resolved
XinShuYang
XinShuYang reviewed Jan 25, 2024
View reviewed changes
hack/generate-manifest.sh Outdated
@@ -323,12 +324,14 @@ EOF
fi

if [ "$MODE" == "dev" ]; then
if [[ -z "$IMG_NAME" ]]; then
if [[ -z "$AGENT_IMG_NAME" ]] || [[ -z "$CONTROLLER_IMG_NAME" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

If AGENT_IMG_NAME is set and CONTROLLER_IMG_NAME is unset, both controllerImage and agentImage repository will be hardcoded. Moreover, in this case, if COVERAGE is false, no value will be added to HELM_VALUES. Not sure if this meets you expectation?

Copy link
Contributor

Choose a reason for hiding this comment

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

If AGENT_IMG_NAME is set and CONTROLLER_IMG_NAME is unset, both controllerImage and agentImage repository will be hardcoded.

Please address this comment.

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, in dev mode we will be hardcoding both.

Copy link
Contributor

Choose a reason for hiding this comment

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

to me it feels that both images should be handled separately:

if [[ -z "$AGENT_IMG_NAME" ]]; then
    # ...
fi
if [[ -z "$CONTROLLER_IMG_NAME" ]]; then
    # ...
fi

@jainpulkit22 jainpulkit22 force-pushed the post-build-updates branch 2 times, most recently from 190e5f6 to a4155bb Compare January 25, 2024 07:20
@XinShuYang
Copy link
Contributor

Go / Check tidy, code generation and manifest failed, please correct related code.

XinShuYang
XinShuYang previously approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@XinShuYang XinShuYang left a comment •
edited

Choose a reason for hiding this comment

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

LGTM

@XinShuYang
Copy link
Contributor

/test-e2e

luolanzone
luolanzone previously approved these changes Jan 25, 2024
View reviewed changes
Copy link
Contributor

@luolanzone luolanzone left a comment

Choose a reason for hiding this comment

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

LGTM

@luolanzone
Copy link
Contributor

/test-multicluster-e2e
/test-rancher-e2e

antoninbas
antoninbas reviewed Jan 25, 2024
View reviewed changes
hack/generate-helm-release.sh Outdated Show resolved Hide resolved
hack/generate-manifest.sh Outdated
@@ -323,12 +324,14 @@ EOF
fi

if [ "$MODE" == "dev" ]; then
if [[ -z "$IMG_NAME" ]]; then
if [[ -z "$AGENT_IMG_NAME" ]] || [[ -z "$CONTROLLER_IMG_NAME" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

to me it feels that both images should be handled separately:

if [[ -z "$AGENT_IMG_NAME" ]]; then
    # ...
fi
if [[ -z "$CONTROLLER_IMG_NAME" ]]; then
    # ...
fi

antoninbas
antoninbas previously approved these changes Jan 26, 2024
View reviewed changes
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Jan 26, 2024

@jainpulkit22 please resolve the conflicts, thanks

@jainpulkit22
Copy link
Contributor Author

@jainpulkit22 please resolve the conflicts, thanks

resolved, thanks

tnqn
tnqn reviewed Jan 26, 2024
View reviewed changes
ci/test-conformance-eks.sh Outdated Show resolved Hide resolved
hack/generate-manifest.sh Outdated Show resolved Hide resolved
ci/test-conformance-aks.sh Outdated Show resolved Hide resolved
tnqn
tnqn reviewed Jan 26, 2024
View reviewed changes
ci/test-conformance-eks.sh Outdated Show resolved Hide resolved
ci/test-conformance-gke.sh Outdated Show resolved Hide resolved
Modify workflows, ci and manifest scripts for split images
98f5c94 
Signed-off-by: Pulkit Jain <jainpu@vmware.com>
tnqn
tnqn approved these changes Jan 26, 2024
View reviewed changes
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@tnqn
Copy link
Member

tnqn commented Jan 26, 2024

/test-all

@tnqn tnqn merged commit 7880d78 into antrea-io:main Jan 26, 2024
47 of 53 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tnqn tnqn tnqn approved these changes

@XinShuYang XinShuYang XinShuYang left review comments

@luolanzone luolanzone luolanzone left review comments

@antoninbas antoninbas Awaiting requested review from antoninbas

Assignees
No one assigned
Labels
action/release-note Indicates a PR that should be included in release notes.
Projects
None yet
Milestone
Antrea v1.15 release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jainpulkit22 @XinShuYang @luolanzone @tnqn @antoninbas