-
Notifications
You must be signed in to change notification settings - Fork 180
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
New cd images (ADX support and ARM64) #5254
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5254 +/- ##
==========================================
- Coverage 55.61% 55.58% -0.03%
==========================================
Files 1002 1002
Lines 96600 96600
==========================================
- Hits 53720 53695 -25
- Misses 38820 38844 +24
- Partials 4060 4061 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
.github/workflows/cd.yml
Outdated
@@ -29,11 +29,17 @@ jobs: | |||
- name: Authenticate docker with gcloud | |||
run: | | |||
gcloud auth configure-docker | |||
- name: Install Tools | |||
run: make install-cross-build-tools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needed for the arm
cross-build
apt-get -y install apt-utils gcc-aarch64-linux-gnu | ||
|
||
.PHONY: docker-native-build-collection | ||
docker-native-build-collection: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
native builds detect whether the current machine supports ADX using crypto_adx_flag.mk
docker-build-collection-without-netgo: | ||
docker build -f cmd/Dockerfile --build-arg TARGET=./cmd/collection --build-arg COMMIT=$(COMMIT) --build-arg VERSION=$(IMAGE_TAG_NO_NETGO) --build-arg GOARCH=$(GOARCH) --build-arg CGO_FLAG=$(CRYPTO_FLAG) --target production \ | ||
.PHONY: docker-build-collection-with-adx | ||
docker-build-collection-with-adx: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optimal case (with ADX and with netgo) - this is called in CD.
Note that the image has the same "tag" as the native builds. Native builds (locally or CI) are not run in CD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the difference between this an docker-native-build-collection
is that native allows overriding ADX while this doesn't?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This target hardcodes the arguments for a specific target (ideally I would want to hardcode GOARCH
too, but left it in case we want to call it on ARM
machines too).
docker-native-build-...
set the arguments based on the host (detect ADX support, set ARCH
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For better clarity and consistency, I hardcoded amd64
for production targets that are not natively built and not meant for arm
d3b99bd (though I can revert it if needed)
docker-build-collection-without-netgo: | ||
docker build -f cmd/Dockerfile --build-arg TARGET=./cmd/collection --build-arg COMMIT=$(COMMIT) --build-arg VERSION=$(IMAGE_TAG_NO_NETGO) --build-arg GOARCH=$(GOARCH) --build-arg CGO_FLAG=$(CRYPTO_FLAG) --target production \ | ||
.PHONY: docker-build-collection-with-adx | ||
docker-build-collection-with-adx: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so the difference between this an docker-native-build-collection
is that native allows overriding ADX while this doesn't?
@tarakby, using this branch, could you please run all of the workflows being modified so that we can verify the execution? Also, as a general comment, I think we will need to update the following lines in the build workflow to use the new |
If the targets are changing, the following workflow will also need to be updated to align with the target changes. This will need to be updated separately, but this will break BN2 until the workflow is updated. |
Co-authored-by: Peter Argue <89119817+peterargue@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding support for ARM builds and unravelling our complex mess of a makefile.
I'm a little confused by all make target names, but I think the changes are correct, so I don't want to hold this up this PR over naming.
We should circle back and refactor the build system soon. It's become pretty complex.
4d163bc
to
03f8c61
Compare
242d37e
to
8d1ad22
Compare
3aed998
to
ee83f78
Compare
@sjonpaulbrown, the PR is now ready. |
Agree, the build system could be made simpler. |
@@ -19,7 +19,9 @@ ifeq (${IMAGE_TAG},) | |||
IMAGE_TAG := ${SHORT_COMMIT} | |||
endif | |||
|
|||
IMAGE_TAG_NO_NETGO := $(IMAGE_TAG)-without-netgo |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Altering this tag name could impact partner automation. We will want to work with @vishalchangrani to ensure that the partners who rely on this know the image tag is changing.
Update the node software process after the latest crypto module changes:
In this PR:
netgo
tag was removed. This fixes the build withoutnetgo
.CGO
flag, passed as an argument toDockerfile
. It offers slower performance.arm64
cross build. The build requires specifying the cross-compiler inCC
.CC
is passed to theDockerfile
as an argument.cd.yml
and add a step to install the cross-compilation tools (works only on Debian)builds.yml
to use the new commands.Makefile
useIMAGE_TAG
only (which is derived from the short commits in most cases)These are the images built in
cd.yml
(assumed to run onamd64
):netgo
.netgo
.( without ADX code and with
netgo
was skipped for now but can be added if needed. With the current PR, if we disable ADX,netgo
has to be disabled too. This assumes that there are no machines that cannot run images withnetgo
disabled)builds.yml
can generate the same images but the arm64 ones, because of the limitation of number of input boxes in githhub workflows.