Add integration test for Firecracker with clippy-tracing instrumentation #5256

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

Jump to bottom

Open

demoncoder-crypto wants to merge 1 commit into firecracker-microvm:main

from demoncoder-crypto:add-firecracker-tracing-integration-test

Open

Add integration test for Firecracker with clippy-tracing instrumentation #5256

demoncoder-crypto wants to merge 1 commit into firecracker-microvm:main from demoncoder-crypto:add-firecracker-tracing-integration-test

Conversation

demoncoder-crypto

Copy link

@demoncoder-crypto demoncoder-crypto commented Jun 9, 2025

Addresses Test instrumented binary #4215

@demoncoder-crypto demoncoder-crypto mentioned this pull request

Jun 9, 2025

Test instrumented binary #4215

Open

kalyazin

kalyazin reviewed

Jun 9, 2025

View reviewed changes

Copy link

Contributor

@kalyazin kalyazin left a comment

Choose a reason for hiding this comment

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

@demoncoder-crypto thanks for your contribution!

I added some inline comments. Apart from that, could you please make changes to the commit message:

add the test: scope tag at the beginning of its title
add a (small) paragraph with a more detailed description of the commit
add the Signed-off-by line (can be generated via git commit -s)

I also triggered a CI build to see if tests pass.

tests/integration_tests/functional/test_firecracker_tracing.py Outdated

def test_firecracker_tracing_with_vm_lifecycle(instrumented_firecracker_binary):

"""Test that instrumented Firecracker works through a complete VM lifecycle with tracing."""

# Skip this test if we don't have the necessary resources

if not global_props.host_linux_version_tup >= (4, 14):

Copy link

Contributor

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

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

this is now 5.10: https://github.com/firecracker-microvm/firecracker/blob/main/docs/kernel-policy.md#host-kernel

Copy link

Author

@demoncoder-crypto demoncoder-crypto Jun 10, 2025

Choose a reason for hiding this comment

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

understood

tests/integration_tests/functional/test_firecracker_tracing.py Outdated

assert found_meaningful, f"Expected to find traces from meaningful functions, but traces were: {trace_matches[:10]}"

def test_firecracker_tracing_with_vm_lifecycle(instrumented_firecracker_binary):

Copy link

Contributor

@kalyazin kalyazin Jun 9, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

this test looks a bit like a subset of the test_firecracker_tracing_basic_functionality. What do you think it adds to that one?

tests/integration_tests/functional/test_firecracker_tracing.py Outdated

# Should complete within reasonable time (30 seconds is very generous)

# This is just to catch major performance regressions or hangs

assert elapsed < 30, f"Instrumented Firecracker took too long to start and handle API calls: {elapsed}s"

Copy link

Contributor

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

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

this looks like a good idea

tests/integration_tests/functional/test_firecracker_tracing.py Outdated

# The new log data should contain trace information

new_log_data = final_log_data[len(initial_log_data):]

assert "TRACE" in new_log_data, "Expected TRACE logs after setting log level to Trace"

Copy link

Contributor

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

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

shall we also verify that no TRACE is present in the initial_log_data?

Copy link

Author

@demoncoder-crypto demoncoder-crypto Jun 10, 2025

Choose a reason for hiding this comment

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

Done

tests/integration_tests/functional/test_firecracker_tracing.py Outdated

@@ -0,0 +1,305 @@

Copy link

Contributor

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

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

it is 2025 now

Copy link

Author

@demoncoder-crypto demoncoder-crypto Jun 10, 2025

Choose a reason for hiding this comment

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

LLM error

tests/integration_tests/functional/test_firecracker_tracing.py Outdated

"""Test that Firecracker works correctly when instrumented with tracing and trace level logs are enabled.

This test addresses GitHub issue about adding a test that checks if simple integration tests work

Copy link

Contributor

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

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

I don't think we should refer to a Github issue here. It should be possible to trace it back via commit -> PR -> GH issue.

tests/integration_tests/functional/test_firecracker_tracing.py Outdated

This test addresses GitHub issue about adding a test that checks if simple integration tests work

when Firecracker is instrumented with src/clippy-tracing and trace level logs are enabled.

The existing test_log_instrument.py only tests the log-instrument crate with example programs.

Copy link

Contributor

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

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

We can probably omit this paragraph as well as comments should describe the present state (when the PR is already merged).

@demoncoder-crypto demoncoder-crypto force-pushed the add-firecracker-tracing-integration-test branch from 0595ad0 to b8bf90e Compare

June 10, 2025 07:34

@demoncoder-crypto

Copy link

Author

demoncoder-crypto commented Jun 10, 2025

@kalyazin I have pushed the changes as requested

@demoncoder-crypto demoncoder-crypto requested a review from kalyazin

June 10, 2025 08:14

@codecov Codecov

Copy link

codecov bot commented Jun 10, 2025 •

edited

Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.90%. Comparing base (e36e774) to head (ac6cfa0).
Report is 1 commits behind head on main.

Additional details and impacted files

@@ Coverage Diff @@
## main #5256 +/- ##
==========================================
+ Coverage 82.84% 82.90% +0.05% 
==========================================
 Files 250 250 
 Lines 26967 26967 
==========================================
+ Hits 22342 22356 +14 
+ Misses 4625 4611 -14

Flag	Coverage Δ
5.10-c5n.metal	`83.33% <ø> (+<0.01%)`	⬆️
5.10-m5n.metal	`83.33% <ø> (+<0.01%)`	⬆️
5.10-m6a.metal	`82.54% <ø> (ø)`
5.10-m6g.metal	`79.16% <ø> (ø)`
5.10-m6i.metal	`83.33% <ø> (+<0.01%)`	⬆️
5.10-m7a.metal-48xl	`82.53% <ø> (?)`
5.10-m7g.metal	`79.16% <ø> (ø)`
5.10-m7i.metal-24xl	`83.29% <ø> (?)`
5.10-m7i.metal-48xl	`83.29% <ø> (?)`
5.10-m8g.metal-24xl	`79.16% <ø> (?)`
5.10-m8g.metal-48xl	`79.16% <ø> (?)`
6.1-c5n.metal	`83.38% <ø> (ø)`
6.1-m5n.metal	`83.38% <ø> (+<0.01%)`	⬆️
6.1-m6a.metal	`82.59% <ø> (ø)`
6.1-m6g.metal	`79.16% <ø> (ø)`
6.1-m6i.metal	`83.37% <ø> (ø)`
6.1-m7a.metal-48xl	`82.58% <ø> (?)`
6.1-m7g.metal	`79.16% <ø> (ø)`
6.1-m7i.metal-24xl	`83.38% <ø> (?)`
6.1-m7i.metal-48xl	`83.39% <ø> (?)`
6.1-m8g.metal-24xl	`79.16% <ø> (?)`
6.1-m8g.metal-48xl	`79.16% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@demoncoder-crypto


 test: Add Firecracker tracing integration test

ac6cfa0

Verifies Firecracker works with clippy-tracing instrumentation
and trace-level logging enabled.
Signed-off-by: demoncoder-crypto <dhillonprabhjitsingh@gmail.com>

@demoncoder-crypto demoncoder-crypto force-pushed the add-firecracker-tracing-integration-test branch from b8bf90e to ac6cfa0 Compare

June 10, 2025 12:41

@kalyazin

Copy link

Contributor

kalyazin commented Jun 10, 2025

I ran the CI checks, it looks like there is a minor style issue and and a problem with calling clippy-tracing:

E error: the argument '--path <PATH>' cannot be used multiple times

https://buildkite.com/firecracker/firecracker-pr/builds/13695#_

You should be able to run the tests locally via the devtool: https://github.com/firecracker-microvm/firecracker/blob/main/tests/README.md .

@kalyazin

Copy link

Contributor

kalyazin commented Aug 6, 2025

Hi @demoncoder-crypto . Do you need any help on how to fix the remaining CI tests failures?

Labels

None yet

2 participants

@demoncoder-crypto @kalyazin

Add integration test for Firecracker with clippy-tracing instrumentation #5256

Are you sure you want to change the base?

Add integration test for Firecracker with clippy-tracing instrumentation #5256

Uh oh!

Conversation

@demoncoder-crypto demoncoder-crypto commented Jun 9, 2025

Uh oh!

@kalyazin kalyazin left a comment

Choose a reason for hiding this comment

Uh oh!

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

@demoncoder-crypto demoncoder-crypto Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

@kalyazin kalyazin Jun 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

@demoncoder-crypto demoncoder-crypto Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

@demoncoder-crypto demoncoder-crypto Jun 10, 2025

Choose a reason for hiding this comment

Uh oh!

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

@kalyazin kalyazin Jun 9, 2025

Choose a reason for hiding this comment

Uh oh!

demoncoder-crypto commented Jun 10, 2025

Uh oh!

codecov bot commented Jun 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kalyazin commented Jun 10, 2025

Uh oh!

kalyazin commented Aug 6, 2025

Uh oh!

Uh oh!

@kalyazin kalyazin Jun 9, 2025 •

edited

Loading

codecov bot commented Jun 10, 2025 •

edited

Loading