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

NH-27837: add aarch64 support #20

Merged
merged 17 commits into from
Jan 20, 2023
Merged

NH-27837: add aarch64 support #20

merged 17 commits into from
Jan 20, 2023

Conversation

xuan-cao-swi
Copy link
Contributor

@xuan-cao-swi xuan-cao-swi commented Dec 14, 2022

Why?

Have the agent be able to run on aarch64 architecture.

Impact?

Update the extconf.rb file that download correct liboboe binary
Update the test case with support on aarch64 (I interchange aarch64 with arm64) docker images
Update the github action file to have support for aarch64 images

Supplementary

  1. Test can't be performed with grpc and memcached on aarch64 images because grpc and memcached gem can't be installed on aarch64 images. So I simply skipped these test case on every aarch64 os

    • for memcached, the newest version 1.8.0 has new interface, and our test case is out-of-date. I can install memcached on aarch64 if I added --build=aarch64-unknown-linux-gnu in extconf.rb like this issue. Futhermore, the memcached gem is not maintained anymore, and people are shifting to dalli (we also instrumented dalli)
    • for grpc, I really have no clue what is going on for aarch64, but I opened the issue here
  2. Removed the support for centos 7 aarch64 support because of the linking issue. @jerrytfleung may offer help to overcome the issue I mentioned in here. But the centos 7 will be de-supported so I am ready to drop it off.

PS. also had failed testing it on aws ubuntu aarch64 with docker container

Work around for centos: upgrade gcc by building from the source.

# Install the new gcc from source (https://gcc.gnu.org/wiki/InstallingGCC)
# Mirror location: (http://mirrors.concertpass.com/gcc/releases/)

yum install wget -y
wget http://mirrors.concertpass.com/gcc/releases/gcc-5.1.0/gcc-5.1.0.tar.gz 

tar xzf gcc-5.1.0.tar.gz
cd gcc-5.1.0
./contrib/download_prerequisites
cd ..
mkdir gcc5
cd gcc5
$PWD/../gcc-5.1.0/configure --prefix=$HOME/gcc-5.1.0 --enable-languages=c,c++,fortran,go
make
make install

# in ~/.bash_profile, put following commands
LD_LIBRARY_PATH=$LD_LIBRARY_PATH:/code/gcc5/x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs/
export LD_LIBRARY_PATH

P.S. Jerry will update the build dependency for liboboe 11.1.0, this step is not necessary anymore.
P.S. centos 7 aarch64 can't use solarwinds-apm-ruby, but using the above instruction can help overcome the old library issue

@tammy-baylis-swi
Copy link

Thanks @xuan-cao-swi ! Lgtm, though I'd like for others to have a look at this too 😺

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

Thanks @xuan-cao-swi, the extconf.rb change to support ARM itself looks good (I gave the RC version in packagecloud a spin in Linux ARM containers on an M1 mac and worked as advertised).

The rest of this PR you grappled with building test containers and trying to get the extensive testsuite to run and pass on ARM, which seems a bit unresolved since the GHA test runs so far haven't passed.

Since some test dependencies don't yet support ARM, and in GHA we're forced to run test containers under simulation (aka sloooooow), I'd like to suggest a different approach: create a set of "install tests" that cover the platforms supported -- i.e. ruby versions x distros x architecture. These install tests can be fairly simple similar to the post release generate-a-few-traces test already in place, or for a much more intense experience check out https://github.com/solarwindscloud/solarwinds-apm-python/blob/main/.github/workflows/verify_install.yaml. The underlying idea is the same -- our APM library's platform dependence hinges on liboboe, which a large part of it is exercised simply by running an instrumented app and reporting traces (connection to backend, retrieval of settings, batching and transmission of data).

@xuan-cao-swi xuan-cao-swi requested a review from a team January 20, 2023 17:54
Copy link

@tammy-baylis-swi tammy-baylis-swi left a comment

Choose a reason for hiding this comment

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

Lgtm, though I'd recommend @cheempz have another look before merging to check the latest changes against the suggestions from this comment.

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM, looks like the verify install changes from other PR have been merged into this one, thanks @xuan-cao-swi!

@xuan-cao-swi xuan-cao-swi merged commit 6736d08 into main Jan 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants