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

stub: pass context to plugins, pass updated resources to UpdateContainers. #40

Merged
merged 6 commits into from
Jun 1, 2023

Conversation

klihub
Copy link
Member

@klihub klihub commented May 17, 2023

Don't hide the ttRPC request context from plugins. Pass the context on to the plugin's request handler. Once we have opentelemetry ttRPC instrumentation support, this should allow us to properly propagate trace spans and have detailed instrumentation on the plugins side properly nested into the related original request on the runtime side.

While making API-breaking changes, also fix UpdateContainer adding an argument with the updated resources.

Update sample plugins and tests with the updated interfaces and add basic tests for UpdateContainer which we lacked so far.

@codecov-commenter
Copy link

codecov-commenter commented May 17, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.50 🎉

Comparison is base (4ba54cc) 64.00% compared to head (dbcf60c) 64.50%.

❗ Current head dbcf60c differs from pull request most recent head 01d5f14. Consider uploading reports for the commit 01d5f14 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #40      +/-   ##
==========================================
+ Coverage   64.00%   64.50%   +0.50%     
==========================================
  Files           9        9              
  Lines        1800     1800              
==========================================
+ Hits         1152     1161       +9     
+ Misses        497      493       -4     
+ Partials      151      146       -5     

see 2 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@klihub klihub force-pushed the fixes/plugin-stub-interface branch from 0067be5 to e4a4790 Compare May 17, 2023 21:34
Copy link
Member

@mikebrow mikebrow 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

plugins/differ/nri-differ.go Show resolved Hide resolved
pkg/adaptation/suite_test.go Show resolved Hide resolved
pkg/stub/stub.go Outdated Show resolved Hide resolved
@klihub klihub force-pushed the fixes/plugin-stub-interface branch from e4a4790 to 81f7daf Compare May 18, 2023 20:19
@klihub klihub changed the title stub: pass context to plugins, fix UpdateContainer signature. stub: pass context to plugins May 18, 2023
@klihub klihub requested a review from mikebrow May 18, 2023 20:38
@klihub klihub changed the title stub: pass context to plugins stub: pass context to plugins, fix UpdateContainers. May 20, 2023
@klihub klihub changed the title stub: pass context to plugins, fix UpdateContainers. stub: pass context to plugins, pass updated resources to UpdateContainers. May 20, 2023
@klihub klihub force-pushed the fixes/plugin-stub-interface branch 2 times, most recently from 7d2910a to 0ba9e87 Compare May 22, 2023 19:25
Don't hide context from plugins, pass the corresponding
context to each plugin handler.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Update mock plugin to implement the updated stub interfaces.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Update plugin event handlers with the updated stub
interfaces.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub klihub force-pushed the fixes/plugin-stub-interface branch from 0ba9e87 to d65f23e Compare May 22, 2023 19:37
Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

nit.. commit header for fix plugin UpdateContainerInterface also states
"Refuse instantiating a Stub for any plugin
that implements the old interface."

Fix UpdateContainer adding an argument for the updated resources.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Add basic tests for UpdateContainer using container creation
tests as a starting point.

Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
Signed-off-by: Krisztian Litkey <krisztian.litkey@intel.com>
@klihub klihub force-pushed the fixes/plugin-stub-interface branch from d65f23e to 01d5f14 Compare May 23, 2023 19:29
@klihub
Copy link
Member Author

klihub commented May 23, 2023

nit.. commit header for fix plugin UpdateContainerInterface also states "Refuse instantiating a Stub for any plugin that implements the old interface."

Argh... Thanks for spotting that ! Fixed.

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

/LGTM

@mikebrow mikebrow added this to the 0.4.0 milestone May 23, 2023
@mikebrow
Copy link
Member

***: requires version tag, and container runtime release notes (containerd/crio) for breaking api change when they vendor the next tag

@klihub klihub requested a review from fuweid May 24, 2023 09:48
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

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.

4 participants