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

[BUILD] otel-webserver add support for arm64 #400

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Fydon
Copy link

@Fydon Fydon commented Mar 18, 2024

Fixes #343.

Attempt to enable otel-webserver support for ARM64. The build succeeds and I appear to able to use the webserver module on ARM64.

  1. Should the Dockerfiles, e.g. instrumentation\otel-webserver-module\docker\ubuntu20.04\Dockerfile be updated to have the platforms in different paths?
  2. Should the ARM64 build use the gccArchFlag of -march=armv8-a or should it be one of the others.

Copy link

linux-foundation-easycla bot commented Mar 18, 2024

CLA Signed

The committers listed above are authorized under a signed CLA.

@Fydon Fydon changed the title otel-webserver support for arm64 [WIP] otel-webserver support for arm64 Mar 18, 2024
@Fydon Fydon changed the title [WIP] otel-webserver support for arm64 otel-webserver support for arm64 Mar 18, 2024
@Fydon Fydon marked this pull request as ready for review March 18, 2024 13:57
@Fydon Fydon requested a review from a team March 18, 2024 13:57
@Fydon
Copy link
Author

Fydon commented Mar 18, 2024

The failure for centos7 is ERROR: write /root/.gradle/wrapper/dists/gradle-4.10.2-all/9fahxiiecdb76a5g3aw9oi8rv/gradle-4.10.2-all.zip: no space left on device

@aryanishan1001
Copy link
Contributor

Hey, can you pull the code again and push your changes. Some builds were failing, so changes have been made to the build pipeline.

@Fydon
Copy link
Author

Fydon commented Apr 4, 2024

Would someone be able to run to pipeline to see if it now works? Also please let me know if I should make any changes, e.g. in relation to the questions in my original post.

@Fydon
Copy link
Author

Fydon commented Apr 4, 2024

Sorry I see that docker compose v1 (docker-compose) was in use. Updated to docker compose v2 (docker compose), which I think resolves the error. Also it probably required QEMU to perform ARM64 builds.

@Fydon
Copy link
Author

Fydon commented Apr 5, 2024

Thank you for running the workflow. The current GitHub workflow is only building the x64 artifacts. Should I duplicate the jobs so that there is a build for each platform and operating system, i.e. webserver-build-test-ubuntu-x64, webserver-build-test-ubuntu-arm64, webserver-build-test-centos7-x64 and webserver-build-test-centos7-arm64, or should the build step change to build both in a single build step, e.g. docker-compose --profile ubuntu20.04 build? With the single build step, there would probably need to still be separate unit test, upload artifacts and run integrationtest steps for each platform.

@marcalff
Copy link
Member

marcalff commented Apr 5, 2024

Thank you for running the workflow. The current GitHub workflow is only building the x64 artifacts. Should I duplicate the jobs so that there is a build for each platform and operating system, i.e. webserver-build-test-ubuntu-x64, webserver-build-test-ubuntu-arm64, webserver-build-test-centos7-x64 and webserver-build-test-centos7-arm64, or should the build step change to build both in a single build step, e.g. docker-compose --profile ubuntu20.04 build? With the single build step, there would probably need to still be separate unit test, upload artifacts and run integrationtest steps for each platform.

No idea, this is a question for the otel-webserver maintainers, who know this area best.

I just press the workflow button ;-)

@Fydon
Copy link
Author

Fydon commented Apr 5, 2024

Thank you @marcalff. I assume that the maintainers would also be reviewing these changes at some point and so be able to advise?

I also noticed that the unit tests are currently platform specific, so I'm working on updating gradle to handle this.

@marcalff marcalff changed the title otel-webserver support for arm64 [BUILD] otel-webserver add support for arm64 Apr 5, 2024
@Fydon
Copy link
Author

Fydon commented Apr 8, 2024

I've gone for having extra jobs for ARM64. The unit tests now work for both platforms, but the integrations tests don't work on my Windows PC for x64 or ARM64. They may work in the GitHub Action.

When I run CentOS 7 with ARM64 locally, I run into a problem with QEMU and yum so instead I attempted to have the Ubuntu job build the artifacts. Given that there are difference between the CentOS and Ubuntu Dockerfiles, is there a problem with using Ubuntu for releases?

@@ -8,6 +8,9 @@ defaultTasks 'build'
ext {
ARCH = ''
Copy link
Author

Choose a reason for hiding this comment

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

Is this used anywhere? I think I've added a case statement to have it work for ARM64

ENV GOSU_ARCH amd64
ENV JDK_ARCH x64
ARG GOSU_ARCH='amd64'
ENV GOSU_ARCH=${GOSU_ARCH}
Copy link
Author

Choose a reason for hiding this comment

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

Are the GOSU_ARCH and JDK_ARCH environment variables used somewhere?


LABEL NAME=apm/build-image-webserver-agent-centos6-x64 VERSION=$BUILD_NUMBER
LABEL NAME=apm/build-image-webserver-agent-centos6-${BUILD_ARCH} VERSION=$BUILD_NUMBER
Copy link
Author

Choose a reason for hiding this comment

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

Should this be centos7?

ENV GOSU_ARCH amd64
ENV JDK_ARCH x64
ARG GOSU_ARCH='amd64'
ENV GOSU_ARCH=${GOSU_ARCH}
Copy link
Author

Choose a reason for hiding this comment

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

Are the GOSU_ARCH and JDK_ARCH environment variables used somewhere?

@Fydon
Copy link
Author

Fydon commented Apr 11, 2024

I tried using podman 5 instead of docker to build the CentOS 7 ARM release and the build succeeded. Unfortunately it got stuck exporting the layers of the image and so couldn't progress beyond that. However as the integration test failed in the GitHub action with the Ubuntu build, I thought it worth seeing if the GitHub action would get beyond the first yum command (4th command in Dockerfile), unlike my experience with Docker Desktop 4.29.0 (145265).

@deepaksrivastavaz
Copy link

Hi @Fydon , I am assuming this is supposed to work on below arch as well, can you please confirm.
5.10.199-190.747.amzn2.aarch64

We thought of instrumenting nginx with otel but it failed there.

@Fydon
Copy link
Author

Fydon commented Apr 12, 2024

I have only used the apache functionality. Yes aarch64 is ARM64. This is a step to having this work, but it may require more work. Particularly the integration tests may need to be changed if they aren't testing ARM64. If using a clone of the latest version of this pull request is not working for you feel free to make the necessary changes and contribute them once this is merged.

@marcalff marcalff added the Webserver This represents the otel-webserver-module in the instrumentation directory label Apr 13, 2024
@Fydon
Copy link
Author

Fydon commented Apr 17, 2024

GitHub has a beta for ARM based runners for GitHub Actions. That should significantly improve the performance of these builds. Perhaps for now the solution is to review the changes, but not run the CentOS ARM64 build?

I'm using the Apache ARM64 build on Ubuntu 22.04 to obtain traces, but have not tested Nginx.

Copy link
Member

@DebajitDas DebajitDas left a comment

Choose a reason for hiding this comment

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

We need to add a separate dockerfile for arm64 based inside docker directory and generate a artifact out of that and same should be used in workflow. The existing dockerfile should not be touched.

@Fydon Fydon force-pushed the feature/webserver-arm64 branch from 6aba801 to 2f6cd80 Compare May 2, 2024 14:27
@Fydon Fydon force-pushed the feature/webserver-arm64 branch 6 times, most recently from 42626ad to 9ecd1c9 Compare May 2, 2024 14:45
@Fydon
Copy link
Author

Fydon commented May 2, 2024

Okay. I think that is done

@Fydon
Copy link
Author

Fydon commented May 3, 2024

Sorry I realised that I left the build arguments in the CentOS 7 ARM build. I've committed but not pushed removing these as it will cause another need for a build. There will probably be more changes as a part of the review so they will be a part of the next update.

@Fydon
Copy link
Author

Fydon commented Jun 4, 2024

ARM64 hosted runners have now been released. Could you please set one up so that the workflow can run quicker? If so, please let me know the name so that I can update the workflow.

@pl4nty
Copy link

pl4nty commented Aug 29, 2024

there are x64 references in otel-webserver-module/src/nginx/script.sh and Makefile that caused an undefined symbol: matchIgnorePathRegex fatal error with my similar patchset, and might need to be addressed here

@Fydon
Copy link
Author

Fydon commented Aug 29, 2024

@pl4nty I don't currently use nginx. If you can provide the necessary changes, I can add them. I've use the ARM64 build with Apache so that does work.

However this pull request probably needs a maintainer to add a ARM64 host runner so that the build can complete.

@pl4nty
Copy link

pl4nty commented Aug 30, 2024

yeah I've been running httpd too, only tested nginx yesterday. not sure if there's a way to do this with variables

--- a/instrumentation/otel-webserver-module/src/nginx/Makefile
+++ a/instrumentation/otel-webserver-module/src/nginx/Makefile
@@ -350,6 +350,7 @@ objs/nginx: objs/src/core/nginx.o \
        objs/src/http/modules/ngx_http_upstream_zone_module.o \
        objs/ngx_modules.o \
        -L/root/webserver-agent/build/linux-x64/opentelemetry-webserver-sdk/sdk_lib/lib -lopentelemetry_webserver_sdk -ldl -lrt -lpthread -lcrypt -lpcre -lz \
+       -L/root/webserver-agent/build/linux-arm64/opentelemetry-webserver-sdk/sdk_lib/lib -lopentelemetry_webserver_sdk -ldl -lrt -lpthread -lcrypt -lpcre -lz \
        -Wl,-E
        
 
@@ -1207,6 +1208,8 @@ objs/ngx_http_opentelemetry_module.so:    objs/addon/nginx/ngx_http_opentelemetry_l
        objs/addon/nginx/ngx_http_opentelemetry_module.o \
        objs/ngx_http_opentelemetry_module_modules.o \
        -L../linux-x64/opentelemetry-webserver-sdk/sdk_lib/lib \
+    -lopentelemetry_webserver_sdk \
+       -L../linux-arm64/opentelemetry-webserver-sdk/sdk_lib/lib \
     -lopentelemetry_webserver_sdk \
        -shared
--- a/instrumentation/otel-webserver-module/src/nginx/script.sh
+++ b/instrumentation/otel-webserver-module/src/nginx/script.sh
@@ -2,4 +2,6 @@
 fileName=$1
 
 sed -i "s/-L\/otel-webserver-module\/build\/linux-x64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ -ldl\ -lpthread\ -lcrypt\ -lpcre\ -lz\ \\\/-L\/otel-webserver-module\/build\/linux-x64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ -ldl\ -lrt\ -lpthread\ -lcrypt\ -lpcre\ -lz\ \\\/g" $fileName
+sed -i "s/-L\/otel-webserver-module\/build\/linux-arm64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ -ldl\ -lpthread\ -lcrypt\ -lpcre\ -lz\ \\\/-L\/otel-webserver-module\/build\/linux-arm64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ -ldl\ -lrt\ -lpthread\ -lcrypt\ -lpcre\ -lz\ \\\/g" $fileName
 sed -i "s/-L\/otel-webserver-module\/build\/linux-x64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ \\\/-L\/otel-webserver-module\/build\/linux-x64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ \\\/g" $fileName
+sed -i "s/-L\/otel-webserver-module\/build\/linux-arm64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ \\\/-L\/otel-webserver-module\/build\/linux-arm64\/opentelemetry-webserver-sdk\/sdk_lib\/lib\ -lopentelemetry_webserver_sdk\ \\\/g" $fileName

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Webserver This represents the otel-webserver-module in the instrumentation directory
Projects
None yet
Development

Successfully merging this pull request may close these issues.

otel-webserver support for arm64
6 participants