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

Falco to build on aarch64 #1442

Merged
merged 11 commits into from
Nov 10, 2020
Merged

Falco to build on aarch64 #1442

merged 11 commits into from
Nov 10, 2020

Conversation

fntlnz
Copy link
Contributor

@fntlnz fntlnz commented Oct 13, 2020

What type of PR is this?

/kind feature

Any specific area of the project related to this PR?

/area build

What this PR does / why we need it:

What happens here:

  • libscap, libsinsp and drivers updated to the fntlnz-aarch64 branch, this is because we needed to add some preprocessor gates for aarch64. This needs to be changed with the specific commit id and SHA before merging aarch64 syscalls surface draios/sysdig#1701
  • luajit updated to git tag 1d8b747c161db457e032a023ebbff511f5de5ec2 - this is because luajit didn't tag since 2017, but they are actively developing it. Initially we wanted to switch to moonjit to support also ppc64le but the new luajit commits also address that along with the 64 bit changes needed for arm64. Please
  • gRPC updated to 1.32.0 read more
  • cpack fixes to produce aarch64 packees
  • CI build targets - opened an issue for this CI: pipeline, test and release targets for aarch64 and armv7 #1445

Which issue(s) this PR fixes:

Fixes #520

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

build: updated LuaJIT to 1d8b747c161db457e032a023ebbff511f5de5ec2
build: updated gRPC to v1.32.0
build: updated sinsp, scap and the drivers to 5c0b863ddade7a45568c0ac97d037422c9efb750
build: use fields_info from libsinsp
build: updated b64 to ce864b17ea0e24a91e77c7dd3eb2d1ac4175b3f0

@fntlnz
Copy link
Contributor Author

fntlnz commented Oct 13, 2020

This also supersedes the changes made for ppc64le #1225 since the only change needed here was to use moonjit instead of luajit. Many projects and OSes (like Alpine) already use moonjit as a synonim of luajit.

This will likely require some testing but the next release is on December 1st so there's no better time than now.

@fntlnz
Copy link
Contributor Author

fntlnz commented Oct 13, 2020

This PR also updates libsinsp and libscap, it's important to note that those already depend on moonjit for certain architectures and that there was a breaking change that needed to be addressed: draios/sysdig#1693

@fntlnz
Copy link
Contributor Author

fntlnz commented Oct 13, 2020

Note: 2.1.2 seems to be the latest moonjit that works for our lua code.

Tried to update to 2.2.0 and we start having issues. I know we want to start making plans to remove lua as we already did for the outputs in #1412 so I'll just note this here and we can follow up on that if we don't make such plan in the near future.

---
- required_engine_version: 7
# Currently disabled as read/write are ignored syscalls. The nearly
# similar open_write/open_read check for files being opened for
# reading/writing.
# - macro: write
#   condition: (syscall.type=write and fd.type in (file, directory))
# - macro: read
#   condition: (syscall.type=read and evt.dir=> and fd.type in (file, directory))
---. Exiting.

@fntlnz
Copy link
Contributor Author

fntlnz commented Oct 13, 2020

I switched back to Luajit but got the latest master. See details in the commit 0a08a04

@fntlnz fntlnz changed the title wip: build on aarch64 wip: Falco to build on aarch64 Oct 15, 2020
@fntlnz fntlnz changed the title wip: Falco to build on aarch64 Falco to build on aarch64 Oct 15, 2020
leodido
leodido previously approved these changes Oct 15, 2020
@poiana
Copy link
Contributor

poiana commented Oct 15, 2020

LGTM label has been added.

Git tree hash: 38666a08c0d209db4431bccb4523719f66de4e8d

@fntlnz
Copy link
Contributor Author

fntlnz commented Oct 16, 2020

The minimal build fails are related to our sinsp dependency. Working on it draios/sysdig#1703

@fntlnz
Copy link
Contributor Author

fntlnz commented Oct 16, 2020

/hold

fntlnz and others added 11 commits November 7, 2020 19:01
This is needed because Luajit does not support many architectures
such as aarch64 and ppcle64.

Note: some operating systems, such as Alpine, already use moonjit as a dropin
replacement for luajit.

Signed-off-by: Lorenzo Fontana <[email protected]>
moonjit is unmaintaned [0], and lujit recently [1] added support
for the aarch64 architecture.

[0] https://twitter.com/siddhesh_p/status/1308594269502885889?s=20
[1] LuaJIT/LuaJIT@e9af1ab

Signed-off-by: Lorenzo Fontana <[email protected]>
Besides all the other improvements, we are really interested
in getting the Make options for other ISAs than x86_64 when it
comes to compiling abseil [0].

This is what happens on aarch64

```
make[4]: *** [Makefile:2968: /root/falco/build-musl/grpc-prefix/src/grpc/objs/opt/third_party/abseil-cpp/absl/base/internal/thread_identity.o] Error 1
c++: error: unrecognized command line option '-maes'
c++: error: unrecognized command line option '-msse4'
c++: error: unrecognized command line option '-msse4'
c++: error: unrecognized command line option '-maes'
```

[0] grpc/grpc@bf87ec9

Signed-off-by: Lorenzo Fontana <[email protected]>
@fntlnz
Copy link
Contributor Author

fntlnz commented Nov 7, 2020

/hold cancel

Ready for review

Copy link
Member

@leogr leogr left a comment

Choose a reason for hiding this comment

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

Just tried on an aarch64 instance with 18.04.5 LTS (Bionic Beaver).
It works nicely! 👏

@poiana poiana added the lgtm label Nov 9, 2020
@poiana
Copy link
Contributor

poiana commented Nov 9, 2020

LGTM label has been added.

Git tree hash: 9c0e213a6aa767f2bd46356c2d3ec416697bb734

@poiana poiana added the approved label Nov 9, 2020
@@ -57,6 +57,7 @@ add_subdirectory("${SYSDIG_SOURCE_DIR}/driver" "${PROJECT_BINARY_DIR}/driver")
# Add libscap directory
add_definitions(-D_GNU_SOURCE)
add_definitions(-DHAS_CAPTURE)
add_definitions(-DNOCURSESUI)
Copy link
Member

Choose a reason for hiding this comment

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

💯 finally!

Copy link
Member

@leodido leodido left a comment

Choose a reason for hiding this comment

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

Great job! 👏

Left a suggestion only

webserver.cpp
grpc_context.cpp
grpc_server_impl.cpp
grpc_request_context.cpp
grpc_server.cpp
webserver.cpp
Copy link
Member

Choose a reason for hiding this comment

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

Is this duplicate? See line 69

Suggested change
webserver.cpp

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 thanks!


add_dependencies(falco string-view-lite)
list(
Copy link
Member

Choose a reason for hiding this comment

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

Cool approach

@poiana
Copy link
Contributor

poiana commented Nov 10, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: leodido, leogr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ARM64 Build
4 participants