-
Notifications
You must be signed in to change notification settings - Fork 462
[release-4.4] crio: Add explicit paths for configuration settings #1700
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
There is a window during machine set scale up where a 1.14 crio uses this configuration and it doesn't understand these empty settings. Restoring the settings back to full paths till we also update the boot image for machine sets as part of updates. Signed-off-by: Mrunal Patel <[email protected]>
|
@mrunalp: No Bugzilla bug is referenced in the title of this pull request. DetailsIn 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/test-infra repository. |
|
@mrunalp: No Bugzilla bug is referenced in the title of this pull request. DetailsIn 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/test-infra repository. |
|
/bugzilla refresh |
|
@kikisdeliveryservice: No Bugzilla bug is referenced in the title of this pull request. DetailsIn 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/test-infra repository. |
|
@mrunalp: This pull request references Bugzilla bug 1829642, which is invalid:
Comment DetailsIn 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/test-infra repository. |
|
New round of CI now that #1698 has landed. |
|
@mrunalp: This pull request references Bugzilla bug 1829642, which is invalid:
Comment DetailsIn 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/test-infra repository. |
|
Verified that in 4.2: |
|
/retest |
| # for the runtime. If not specified, then the internal default seccomp profile | ||
| # will be used. | ||
| seccomp_profile = "" | ||
| seccomp_profile = "/etc/crio/seccomp.json" |
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.
Oh, wait. This isn't a revert of 606bd2b (#1414). Before #1414, this line was # seccomp_profile... (commented out). In #1414 it became master's current empty string. With this commit it is becoming an uncommented, nonempty value. So was there a default baked into CRI-O at some point, but no longer? And now we need to provide the old default explicitly here to support CRI-O that are recent enough to have dropped the baked-in-default? But bug 1829642 is about supporting older CRI-O? I'm confused about how these pieces fit together.
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.
it was commented out further back: 69025e8#diff-5c79cb2bed55d5971e8178e82d18cdc9
via #1216
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.
Ah, ok. And looks like #1216 went out in 4.3 and was never backported to 4.2.
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.
pretty sure that 4.2/1/.12 fails if these are empty so it cant be backported
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.
Yeah this is needed, so that cri-o 1.14 can use this config and not fail to start up like it was.
|
just adding for safety.. /hold |
|
/retest ci/prow/e2e-aws |
|
It seems like we should include this on master branch until such a time that we're managing the machineset image on updates. There's no assurances that will happen before 4.5 so we may run into 4.2 to 4.3 to 4.4 to 4.5 upgrades with a 4.2 bootimage is still in use during scaleup. |
|
@sdodson: once the present PR merges, I will cherry-pick it on top of master in a new PR and assign it to you. DetailsIn 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/test-infra repository. |
|
@ashcrow: The
Use DetailsIn 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/test-infra repository. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: kikisdeliveryservice, mrunalp, umohnani8 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 |
|
@sdodson this will not cleanly cherry-pick on master since we reworked the ctrcfg controller to use drop-in files instead of modifying crio.conf every time. I can open a PR to master with similar changes. |
|
/hold cancel |
|
/hold adding it back based on slack discussions |
|
/retest |
|
/skip |
|
/retest |
|
@mrunalp: No Bugzilla bug is referenced in the title of this pull request. DetailsIn 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/test-infra repository. |
|
/bugzilla refresh |
|
@kikisdeliveryservice: No Bugzilla bug is referenced in the title of this pull request. DetailsIn 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/test-infra repository. |
|
unlinked from https://bugzilla.redhat.com/show_bug.cgi?id=1829642 as we have #1706 |
|
@mrunalp: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. DetailsInstructions 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/test-infra repository. I understand the commands that are listed here. |
|
Closed per #1707 (comment) |
There is a window during machine set scale up where a 1.14 crio
uses this configuration and it doesn't understand these empty
settings. Restoring the settings back to full paths till
we also update the boot image for machine sets as part of updates.
Signed-off-by: Mrunal Patel [email protected]
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1829642
- What I did
Restore some crio settings to older values to make them forward compatible with older crio versions.
- How to verify it
Test that cri-o 4.2 can still understand the default rendered 4.4 crio.conf.
- Description for the changelog
Add back explicit path for settings to be forwards compatible with older crio versions.