- 
                Notifications
    You must be signed in to change notification settings 
- Fork 1.1k
RuntimeHandler inheritance #8754
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
Conversation
| Hi @MarSik. Thanks for your PR. I'm waiting for a cri-o member to verify that this patch is reasonable to test. If it is, they should reply with  Once the patch is verified, the new status will be reflected by the  I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. | 
| Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 Additional details and impacted files@@            Coverage Diff             @@
##             main    #8754      +/-   ##
==========================================
+ Coverage   46.44%   46.68%   +0.23%     
==========================================
  Files         150      150              
  Lines       21946    22003      +57     
==========================================
+ Hits        10193    10271      +78     
+ Misses      10688    10666      -22     
- Partials     1065     1066       +1      | 
| /ok-to-test | 
c153d15    to
    f5571ce      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to update docs/crio.conf.5.md and pkg/config/template.go. Please also fill the release-notes block in the body of this PR. Is it still WIP? :)
f5571ce    to
    3fe34f4      
    Compare
  
    | @saschagrunert Like this? | 
249bcac    to
    3bb8942      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a nit about the naming, otherwise LGTM
        
          
                pkg/config/config.go
              
                Outdated
          
        
      |  | ||
| // Inheritance request | ||
| // Fill in the Runtime information (paths and type) from the default runtime | ||
| UseDefaultRuntime bool `toml:"use_default_runtime,omitempty"` | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just a bit concerned about the naming, how about inherit_default_options or similar? Maybe other @cri-o/cri-o-maintainers have other thoughts on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is inherit_default_runtime OK? Because it only replaces the runtime_* variables.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, yes!
This allows defining a RuntimeHandler with custom annotations or other fields while inheriting the runtime type from the default runtime. This behavior is useful for layered projects that are not in control of the runtime selection but need to tweak the specific behavior while inheriting the node global runtime. Signed-off-by: Martin Sivak <[email protected]>
3bb8942    to
    76049fe      
    Compare
  
    | [APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MarSik, saschagrunert The full list of commands accepted by this bot can be found here. The pull request process is described here 
Needs approval from an approver in each of these files:
 
 Approvers can indicate their approval by writing  | 
| /retest | 
| /cherry-pick release-1.31 | 
| @saschagrunert: new pull request created: #8762 In response to this: 
 Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. | 
What type of PR is this?
/kind feature
What this PR does / why we need it:
This allows custom RuntimeHandlers to always inherit the default configured runtime paths. This helps with adding extra annotations or other "minor" changes while following what rest of the system does.
Which issue(s) this PR fixes:
Fixes #8753
Special notes for your reviewer:
This is a minimal proof of concept so I could test whether the configuration flow would make sense. I actually do not know if it covers all code paths.
Does this PR introduce a user-facing change?