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

add actuated runners #32135

Merged
merged 5 commits into from
Apr 8, 2024
Merged

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented Apr 3, 2024

Description:
Add an ARM build using Actuated, which will run an ARM github action runner on CNCF allocated Equinix colocated ARM servers.

Link to tracking Issue:
Fixes #12920
Fixes #32136

@atoulme atoulme requested review from a team and andrzej-stencel April 3, 2024 00:48
"system.paging.usage",
}

var archSpecificMetrics = map[string][]string{
Copy link
Member

Choose a reason for hiding this comment

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

If I understand correctly, this change is caused by the fact that the Host Metrics receiver's test TestGatherMetrics_EndToEnd is tightly coupled to the underlying operating system specifics. In this case, depending on whether the system has a swap partition or not, the test will pass or fail. The runners from Actuated apparently do not have swap partition, and this is why the metric system.paging.usage is not created on them, causing the tests to fail.

This change tries to alleviate it by expecting that this metric does not exist on ARM64. This is not strictly correct, because whether this metric is created or not does not depend on the architecture, but on the specific machine the test is run on. For example, the original test passes on my Raspberry Pi (which has a swap partition), but it will fail after this change.

This should probably be fixed by making the test pass independently of whether the underlying system has a swap partition or not.

Copy link
Contributor Author

@atoulme atoulme Apr 4, 2024

Choose a reason for hiding this comment

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

I'm not sure what you want here, as specifically this test is trying to exercise the swap partition if present as that's what it reports on. The tests already do a lot of interesting things to pass for Windows for example.

Copy link
Member

Choose a reason for hiding this comment

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

I mostly wanted to see your response to my messy thoughts 😄 I think the two options we have are:

  1. Merge this PR in its current state, with the fix to unit tests that is tailored specifically to our CI and the runners we use
  2. Remove the unit test fixes from this PR and only merge the addition of new runners, letting the Host Metrics receiver unit tests to fail on Actuated runners until this is resolved (properly) on the Host Metrics receiver side.

I suppose 1. is fine. As you point out, it doesn't make the Host Metrics receiver unit tests much worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't know. I think the codeowners of the receiver might need to do some soul searching and create end to end tests at some point in some really controlled environments to make sure everything comes in properly.

@andrzej-stencel andrzej-stencel merged commit 7d97cbf into open-telemetry:main Apr 8, 2024
170 checks passed
@github-actions github-actions bot added this to the next release milestone Apr 8, 2024
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.

[receiver/hostmetrics] ARM github action runner fails to run tests Add ARM64 CI tests
3 participants