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

[feature] Refactor Concurrency #599

Merged
merged 14 commits into from
Feb 16, 2024
Merged

[feature] Refactor Concurrency #599

merged 14 commits into from
Feb 16, 2024

Conversation

KCarretto
Copy link
Collaborator

@KCarretto KCarretto commented Feb 15, 2024

What type of PR is this?

/kind feature

What this PR does / why we need it:

  • Introduces eldritch::runtime::Message and associated message types for communication between the caller and an eldritch runtime (on separate threads)
  • Fixes a bug where imix would leave open an established connection (forever)
  • Fixes some Tavern issues
  • Adds a small rate limit to imix, to prevent overloading SQLlite (1req/25ms)
  • Implements a file.report API for sending files to tavern

Which issue(s) this PR fixes:

No one asked for this

Copy link
Collaborator

@hulto hulto left a comment

Choose a reason for hiding this comment

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

Couple thoughts

implants/golem/src/main.rs Show resolved Hide resolved
implants/golem/src/main.rs Show resolved Hide resolved
}
}
#[cfg(debug_assertions)]
log::info!("{output}");
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's unclear what's happening here - but I think we want imix install to print in realtime similar to golem.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This does not change any behavior, but currently this will print all output after evaluation. The print_stdout is a feature flag we likely don't want enabled for imix.

implants/imix/src/task.rs Show resolved Hide resolved
implants/imix/src/task.rs Show resolved Hide resolved
implants/lib/eldritch/src/assets/copy_impl.rs Show resolved Hide resolved
implants/lib/pb/build.rs Show resolved Hide resolved
implants/lib/transport/Cargo.toml Show resolved Hide resolved
implants/lib/transport/src/mock.rs Show resolved Hide resolved
.vscode/settings.json Outdated Show resolved Hide resolved
Copy link

codecov bot commented Feb 15, 2024

Codecov Report

Attention: 271 lines in your changes are missing coverage. Please review.

Comparison is base (acaae92) 67.09% compared to head (9de85fe) 70.49%.

Files Patch % Lines
...s/lib/eldritch/src/runtime/messages/report_file.rs 0.00% 55 Missing ⚠️
implants/lib/transport/src/mock.rs 0.00% 35 Missing ⚠️
implants/lib/transport/src/grpc.rs 0.00% 20 Missing ⚠️
implants/imix/src/task.rs 0.00% 19 Missing ⚠️
implants/imix/src/install.rs 0.00% 16 Missing ⚠️
.../lib/eldritch/src/runtime/messages/report_error.rs 0.00% 16 Missing ⚠️
...lib/eldritch/src/runtime/messages/report_finish.rs 0.00% 16 Missing ⚠️
.../lib/eldritch/src/runtime/messages/report_start.rs 0.00% 16 Missing ⚠️
...s/lib/eldritch/src/runtime/messages/report_text.rs 15.78% 16 Missing ⚠️
implants/lib/eldritch/src/runtime/messages/mod.rs 7.14% 13 Missing ⚠️
... and 8 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #599      +/-   ##
==========================================
+ Coverage   67.09%   70.49%   +3.40%     
==========================================
  Files         147      159      +12     
  Lines        9741    10488     +747     
==========================================
+ Hits         6536     7394     +858     
+ Misses       3026     2915     -111     
  Partials      179      179              

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

Cictrone
Cictrone previously approved these changes Feb 16, 2024
Copy link
Collaborator

@Cictrone Cictrone left a comment

Choose a reason for hiding this comment

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

frodo-baggins-its-over

@KCarretto KCarretto merged commit 41dabe4 into main Feb 16, 2024
6 checks passed
@KCarretto KCarretto deleted the messages-refactor branch February 16, 2024 05:28
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.

None yet

3 participants