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

Modify hook-injector plugin to monitor directories to match cri-o #84

Merged
merged 1 commit into from
May 23, 2024

Conversation

AbdelrahmanElawady
Copy link
Contributor

As mentioned in hook-injector plugin README, this plugin achieves CRI-O compatible hooks injection. However, there is a difference between this implementation and CRI-O's implementation as CRI-O watches hook directories for any changes which allows for new hooks to be added to new containers. This is not the case here where this plugin only uses hooks created while the plugin is first running. So, they are not really compatible. Reference CRI-O code.

Also, there are some use-cases where services set up their hooks after the cluster and the plugin are up. CRI-O would register these new hooks but again the plugin currently won't. This PR aims to fix that.

We can also make this optional if you don't want to change the current behavior with a flag like --watch or something like that.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.54%. Comparing base (53d3371) to head (e919232).
Report is 2 commits behind head on main.

Current head e919232 differs from pull request most recent head b122b2d

Please upload reports for the commit b122b2d to get more accurate results.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #84      +/-   ##
==========================================
- Coverage   64.55%   58.54%   -6.01%     
==========================================
  Files          10        4       -6     
  Lines        1845     1305     -540     
==========================================
- Hits         1191      764     -427     
+ Misses        503      420      -83     
+ Partials      151      121      -30     

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

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.

nice..

Suggest adding --disableWatch as an option ... might want to consider outputing the paths being monitored in help for the client or as debug out (unless that is already being done in the hooks package) ..

Could you dump your console output on the PR? Would be nice if we had hook tests running in critest or some other integration testing.

Maybe open an issue for a test bucket?

@AbdelrahmanElawady
Copy link
Contributor Author

AbdelrahmanElawady commented May 22, 2024

new flag added and the output looks like this:

$ sudo ./hook-injector -idx 10
INFO   [0000] Created plugin 10-hook-injector (hook-injector, handles CreateContainer) 
INFO   [0000] watching directories "/usr/share/containers/oci/hooks.d /etc/containers/oci/hooks.d" for new changes 
INFO   [0000] Registering plugin 10-hook-injector...       
INFO   [0000] Configuring plugin 10-hook-injector for runtime containerd/1.7.2... 
INFO   [0000] Started plugin 10-hook-injector...
INFO   [0069] redis/redis: OCI hooks injected

@AbdelrahmanElawady
Copy link
Contributor Author

Maybe open an issue for a test bucket?

Also, created an issue to track hook tests: #86

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
Copy link
Member

@haircommander FYI in case you want to review this one or tag someone

@haircommander
Copy link

LGTM

@mikebrow mikebrow merged commit 46ae8cd into containerd:main May 23, 2024
8 checks passed
@AbdelrahmanElawady AbdelrahmanElawady deleted the match-crio-hooks branch May 23, 2024 20:55
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.

5 participants