Skip to content

chore(ci): mirror unit and integration tests to arm64 #4655

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

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

Conversation

NDStrahilevitz
Copy link
Collaborator

1. Explain what the PR does

"Replace me with make check-pr output"

2. Explain how to test it

3. Other comments

Resolve #4621
Related #4162

@NDStrahilevitz NDStrahilevitz self-assigned this Mar 13, 2025
@NDStrahilevitz NDStrahilevitz force-pushed the mirror-arm64-tests branch 4 times, most recently from 1a9d5d0 to 4139753 Compare March 13, 2025 15:34
@NDStrahilevitz NDStrahilevitz force-pushed the mirror-arm64-tests branch 2 times, most recently from 72a491b to a2147b4 Compare March 13, 2025 15:53
@NDStrahilevitz NDStrahilevitz marked this pull request as draft March 13, 2025 17:31
@NDStrahilevitz
Copy link
Collaborator Author

Both red from bugs, which is good in the sense we have increased coverage. I will be fixing these and then opening to review.

@NDStrahilevitz
Copy link
Collaborator Author

NDStrahilevitz commented Mar 24, 2025

  1. Unit tests
    • data_test.go: Matching 'syscall' data value of sys_enter as string & Matching 'syscall' data value of hooked_syscall as int
      • Filter defined with string syscall -> converted to int via event definitions
      • Match fails because arm64 syscall event ids differ
      • Possible resolution: formatted strings with programmatic access to the event id
    • "Test O_RDWR | O_CREAT | O_DSYNC | O_LARGEFILE”
      • One of the flags is expanded to include “O_NOFOLLOW” in the string in ARM64. The values creating the flag are arch-neutral (supposedly, from events/parsers/data_parsers.go), but are then converted according to arch-specific parsing values (in events/parsers/data_parsers_arch.go).
  2. Integration Tests
    • Test_EventFilters/comm:_event:_data:_trace_event_set_in_a_specific_policy_with_data_from_fakeprog1_and_fakeprog2_commands
      • Tests for Open and Openat syscalls from the syscaller program. The event ids are set programatically which means this shouldn’t be the issue, unlike unit tests.
      • Therefore the issue is likely a mismatch in arguments or that ARM64 machines will emit a different syscall than intended in this scenario

This raises the need to normalize syscall ids at the definition layer. I think it also implies that we shouldn’t allow setting syscall filters with numerical ids, we should enforce a string usage and provide system appropriate parsing. I am not sure what to make of the flag situation currently, nor the integration test failure.

@geyslan FYI

@geyslan
Copy link
Member

geyslan commented Mar 27, 2025

This raises the need to normalize syscall ids at the definition layer.

For sure, and as we've discussed priorly, one idea is to use the same ids given to grpc api, then any consumer should use only that. Maybe we need to convert all syscall event IDs to a common set - architecture independent. They should align to Tracce's detected syscalls and could be translated in (comming from ebpf) and out (on the event emission) depending on the architecture.

I think it also implies that we shouldn’t allow setting syscall filters with numerical ids, we should enforce a string usage and provide system appropriate parsing.

If we use a common set for Tracee itself, the numeric id wouldn't be a problem - but I assume that it could confuse the user trying to set a syscall ID based on its own arch. So maybe enforcing a syscall as string would be a better solution as you thought for filters. But I'm for the common set (performance wise).

I am not sure what to make of the flag situation currently, nor the integration test failure.

I'm pinging @rscampos to give us a hand as he loves arm64.

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.

Mirror integration tests on ARM64
2 participants