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

Secret hiding base CI resource #5096

Open
wants to merge 3 commits into
base: feature/secret-hiding
Choose a base branch
from

Conversation

JackThomson2
Copy link
Contributor

Changes

Adding the base script to be used for building custom kernels with secret hiding patches applied for testing

Reason

...

License Acceptance

By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.

PR Checklist

  • I have read and understand CONTRIBUTING.md.
  • I have run tools/devtool checkstyle to verify that the PR passes the
    automated style checks.
  • I have described what is done in these changes, why they are needed, and
    how they are solving the problem in a clear and encompassing way.
  • I have updated any relevant documentation (both in code and in the docs)
    in the PR.
  • I have mentioned all user-facing changes in CHANGELOG.md.
  • If a specific issue led to this PR, this PR closes the issue.
  • When making API changes, I have followed the
    Runbook for Firecracker API changes.
  • I have tested all new and changed functionalities in unit tests and/or
    integration tests.
  • I have linked an issue to every new TODO.

  • This functionality cannot be added in rust-vmm.

@JackThomson2 JackThomson2 requested a review from roypat March 19, 2025 15:33
Copy link

codecov bot commented Mar 19, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.14%. Comparing base (c9fa6fe) to head (9a7c7e9).

Additional details and impacted files
@@                  Coverage Diff                   @@
##           feature/secret-hiding    #5096   +/-   ##
======================================================
  Coverage                  83.14%   83.14%           
======================================================
  Files                        248      248           
  Lines                      26923    26923           
======================================================
  Hits                       22386    22386           
  Misses                      4537     4537           
Flag Coverage Δ
5.10-c5n.metal 83.53% <ø> (ø)
5.10-m5n.metal 83.52% <ø> (ø)
5.10-m6a.metal 82.71% <ø> (-0.01%) ⬇️
5.10-m6g.metal 79.56% <ø> (ø)
5.10-m6i.metal 83.51% <ø> (-0.01%) ⬇️
5.10-m7a.metal-48xl 82.70% <ø> (ø)
5.10-m7g.metal 79.56% <ø> (ø)
6.1-c5n.metal 83.58% <ø> (ø)
6.1-m5n.metal 83.56% <ø> (-0.01%) ⬇️
6.1-m6a.metal 82.75% <ø> (ø)
6.1-m6g.metal 79.56% <ø> (ø)
6.1-m6i.metal 83.56% <ø> (+<0.01%) ⬆️
6.1-m7a.metal-48xl 82.75% <ø> (ø)
6.1-m7g.metal 79.56% <ø> (ø)

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.

@JackThomson2 JackThomson2 force-pushed the ci/kernel_installation branch from 37b9dcc to 3bca636 Compare March 19, 2025 15:39
@JackThomson2 JackThomson2 force-pushed the ci/kernel_installation branch from dae2d55 to 451a7d2 Compare March 20, 2025 09:55
Copy link
Contributor

@roypat roypat left a comment

Choose a reason for hiding this comment

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

Should we also add the "kernel builds" integration test as part of this? Just so that the code is tested/used straight away


# Finally run olddefconfig again to make sure any
# new options are configured before build
make olddefconfig
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add some sort of assertion here to verify that .config contains everything we specify in kernel_config_overwrites? Just as a sanity check, to avoid ever running into any hard/weird to debug issues

@JackThomson2 JackThomson2 force-pushed the ci/kernel_installation branch 6 times, most recently from 070590a to ffcab3f Compare March 21, 2025 15:32
Copy link
Contributor

@roypat roypat left a comment

Choose a reason for hiding this comment

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

Very nice!!

pipeline.build_group_per_arch(
"🕵️ Build Secret Hiding Kernel",
pipeline.devtool_test(
pytest_opts="-m nonci integration_tests/build/test_hiding_kernel.py",
Copy link
Contributor

Choose a reason for hiding this comment

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

mh, I guess as long as we don't setup an nightly pipelines, reusing nonci is fine, but if we ever have a nightly pipeline on this branch that also passes -m nonci, then it might pick up this test by accident, which we don't want probably. Thought on using a dedicated pytest mark for this test? :o

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a new secret_hiding mark for the test

Creating a script to build and install a modified kernel with patches
applied.

Signed-off-by: Jack Thomson <jackabt@amazon.com>
Update the script to allow it to run the build without installing.

Allow us to add patches in different formats, we now allow .lore
files which are a link to the patch series, these will be automatically
downloaded and applied. We also accept .mbox files which again will be
patched onto our kernel.

Signed-off-by: Jack Thomson <jackabt@amazon.com>
Adding a new integration test to assert that the kernel build script
will succeed.

Signed-off-by: Jack Thomson <jackabt@amazon.com>
@JackThomson2 JackThomson2 force-pushed the ci/kernel_installation branch from cc851c5 to 9a7c7e9 Compare March 21, 2025 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants