Update and reduce templates for CRI-O 1.16 #1216
Conversation
openshift-ci-robot
added
the
size/XL
|
/retest |
cgwalters
left a comment
cgwalters
left a comment
There was a problem hiding this comment.
In general big
/approve
from me. I'm curious if it's necessary for us to still specify some of the values.
I think that all of this would be cleaner if we did something more like:
cp /usr/etc/crio/crio.conf /etc/crio.conf
# append MCO changes to /etc/crio.conf
(Though for the first bit on classic RPM systems we'd need to keep a pristine /etc/crio.conf copy ourself since RPM doesn't have it)
There was a problem hiding this comment.
Does this one still need explicit specification?
There was a problem hiding this comment.
for the time being yes. This is a situation in which the CRI-O rpm needs to move in lock step with MCO. I guess we can have the rpm put it in both spots (it may already I have to check). Once that happens, we can drop this, and eventually drop it from /usr/libexec all together
There was a problem hiding this comment.
ah actually I don't think this one is needed. good catch
openshift-ci-robot
added
the
approved
haircommander
force-pushed
the
update-templates-4.3
branch
2 times, most recently
from
452b272 to
2bb2bc9
Compare
|
/retest |
1 similar comment
|
/retest |
haircommander
force-pushed
the
update-templates-4.3
branch
from
2bb2bc9 to
0ab0ba4
Compare
|
/retest |
|
@umohnani8 @mtrmac PTAL |
|
I think we should do this now to make future upgrades easier. Code overall (Though do we need separate crio.conf on masters vs workers? Could we move it into |
There was a problem hiding this comment.
This doesn't look right. Should be /var/lib/containers/storage
There was a problem hiding this comment.
oh nvm, it is commented out. But we should probably still fix it.
There was a problem hiding this comment.
since this is not system.slice, we should it update it here.
There was a problem hiding this comment.
move to system.slice here as well
|
/hold |
openshift-ci-robot
added
the
do-not-merge/hold
We have historically run into issues by the MCO setting more fields than it really needs to. It can take the default value for most fields, and override those that matter. Do that overriding here, and comment out the rest of the crio config. This allows crio to decide how to set values, so it and MCO don't have to move in lock-step Signed-off-by: Peter Hunt <[email protected]>
haircommander
force-pushed
the
update-templates-4.3
branch
from
0ab0ba4 to
69025e8
Compare
|
comments addressed :) |
|
LGTM |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cgwalters, haircommander The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
1 similar comment
|
/retest |
|
/hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
🙃 |
5 participants
We have historically run into issues by the MCO setting more fields than it really needs to. Instead, we only really need to have the select fields MCO cares about, and comment out the rest.
- What I did
Override the default values with the ones MCO set in the past, then comment out the rest
- How to verify it
crio --config "" config > default.conf
crio --config $OLD_TEMPLATE config > mco.conf
sdiff default.conf mco.conf | grep '|'
and the resulting values are the only values uncommented here
- Description for the changelog
Update crio.yaml templates to match CRI-O 1.16 config format as well as removed default values