templates: remove /usr/share/containers/oci/hooks.d from crio config by vrutkovs · Pull Request #1314 · openshift/machine-config-operator

vrutkovs · 2019-12-09T14:19:46Z

- What I did
Removed /usr/share/containers/oci/hooks.d from CRI-O config. This folder is not available on RHEL7 hosts when CRI-O is being installed.

This dir is provided by oci-systemd-hook (if I understood correctly), however it is planned to be dropped during 4.3 lifecycle. This PR would ensure CRI-O is configured correctly in preparation to that

- How to verify it

Run RHEL7 scaleup - it should pass

- Description for the changelog
/usr/share/containers/oci/hooks.d is no longer being used as a source of CRI-O hooks.

/cc @mtnbikenc

cgwalters · 2019-12-09T15:06:12Z

It sounds like people were using this hook directory on traditional RHEL systems (since it's writable there), and given /etc/oci/hooks.d is new I'm worried we'll put people in a situation where they need to detect the OS and use the correct dir.

Maybe the simplest thing is to merge this PR, and then also do ln -sr /{etc,usr/share}/containers/oci/hooks.d in the crio RPM?

vrutkovs · 2019-12-09T15:11:57Z

Maybe the simplest thing is to merge this PR, and then also do ln -sr /{etc,usr/share}/containers/oci/hooks.d in the crio RPM?

That sounds sane to me, not sure if anyone has been installing custom cri-o hooks (especially in /usr/share). @lsm5 wdyt?

vrutkovs · 2019-12-09T15:43:55Z

Networking issue

/retest

kikisdeliveryservice · 2019-12-09T18:17:39Z

this PR is making a change to something that was recently added in https://github.com/openshift/machine-config-operator/pull/1299/files

@haircommander PTAL

/hold

haircommander · 2019-12-09T18:26:37Z

I am fine with it if it works with @zvonkok

haircommander · 2019-12-09T18:27:51Z

Since we already have /etc/containers/oci/hooks.d as a default in cri-o (as of cri-o/cri-o@16beb92), I'd just comment out all of hooks_dir so we can allow cri-o to set the default as it sees fit

I don't think its a good idea. I'd rather have MCD be the single source of CRI-O configs rather then relying on CRI-O defaults. Other components might use this config to set their own hooks. Also CRI-O might be tested with oci-systemd-hook installed (which is not the case for FCOS and currently happening on RHCOS), so the test results from CRI-O suite might be inconvincing

per conversations with @cgwalters we decided to keep the templated configs as slim as possible. If other components use this value, they can import the value from cri-o/cri-o/config, which we keep public for that very use case. Removing /usr/share/containers/oci/hooks.d will remove the oci-systemd-hook anyway (as it's only shipped there, and also being removed on rhcos), so after this PR the tests will be the same, regardless of whether this field is commented out or not.

my comment isn't necessarily blocking, but we've set the idiom in the other fields to use cri-o defaults when possible, and in this situation it is possible, so I'm still pushing to follow it

Okay, in that case it should be done in a different PR (and moved to shared masters/workers templates)

vrutkovs · 2019-12-09T19:04:41Z

Flaky tests:

[Feature:OpenShiftControllerManager] TestDockercfgTokenDeletedController [Suite:openshift/conformance/parallel]

Failing tests:

[sig-network] Services should be rejected when no endpoints exist [Suite:openshift/conformance/parallel] [Suite:k8s] [Skipped:Network/OVNKubernetes]

/retest

kikisdeliveryservice · 2019-12-10T18:22:16Z

@haircommander @vrutkovs relevant bug below:
https://bugzilla.redhat.com/show_bug.cgi?id=1781019

openshift-ci-robot · 2019-12-10T19:32:08Z

@vrutkovs: This pull request references Bugzilla bug 1781019, which is invalid:

expected the bug to target the "4.4.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1781019: templates: remove /usr/share/containers/oci/hooks.d from crio config

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.

vrutkovs · 2019-12-10T19:32:56Z

/retest

openshift-bot · 2019-12-11T16:58:37Z

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

openshift-ci-robot · 2019-12-11T16:58:40Z

@openshift-bot: This pull request references Bugzilla bug 1781019, which is invalid:

expected the bug to target the "4.4.0" release, but it targets "4.3.0" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

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.

haircommander · 2019-12-11T17:04:18Z

/retest

haircommander · 2019-12-11T17:05:37Z

@vrutkovs would you mind changing the name to not reference the bug? I'll backport the fix to release-4.3 and link the bug there

@mrunalp PTAL

openshift-ci-robot · 2019-12-11T18:45:18Z

@vrutkovs: No Bugzilla bug is referenced in the title of this pull request.
To reference a bug, add 'Bug XXX:' to the title of this pull request and request another bug refresh with /bugzilla refresh.

Details

In response to this:

templates: remove /usr/share/containers/oci/hooks.d from crio config

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.

vrutkovs · 2019-12-11T18:45:22Z

Done

mrunalp · 2019-12-11T19:17:04Z

Can we drop this comment?

mrunalp · 2019-12-11T19:17:14Z

Can we drop this comment?

Done. Updated commit message too

Its no longer available since RHCOS doesn't install `oci-systemd-hooks`

mrunalp · 2019-12-11T19:31:50Z

/lgtm

cgwalters · 2019-12-11T20:25:23Z

Also adding a
/hold
here since

This dir is provided by oci-systemd-hook (if I understood correctly), however it is planned to be dropped during 4.3 lifecycle.

Reference? It's listed explicitly in the RHCOS manifest I believe because we want to support people doing a "lift and shift" of VMs into containers (where systemd runs in the container) and the like.

Its existence also makes things messier here...I think we probably need both values in the configuration?

haircommander · 2019-12-11T21:41:28Z

/retest

https://gitlab.cee.redhat.com/coreos/redhat-coreos/merge_requests/764 actually dropped oci-systemd-hook from rhcos (as it's no longer needed in our container engines)

thus:
/hold cancel

runcom · 2019-12-12T10:06:34Z

/approve

openshift-ci-robot · 2019-12-12T10:06:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LorbusChris, mrunalp, runcom, vrutkovs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [runcom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-bot · 2019-12-12T10:36:59Z

/retest

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2019-12-12T10:46:23Z

/retest

Please review the full test history for this PR and help us cut down flakes.

haircommander · 2019-12-12T13:30:26Z

/cherry-pick release-4.3

openshift-cherrypick-robot · 2019-12-12T13:30:36Z

@haircommander: new pull request created: #1329

Details

In response to this:

/cherry-pick release-4.3

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.

carbonin · 2020-03-27T17:58:32Z

I saw mention in here that oci-systemd-hooks is not needed anymore on cri-o nodes. (specifically from @haircommander in #1314 (comment)) What does that mean exactly? How does running a container with init work without privileged?

I opened https://bugzilla.redhat.com/show_bug.cgi?id=1807245 a while back and am trying to get some clarity on what's going on.

openshift-ci-robot requested a review from mtnbikenc December 9, 2019 14:19

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Dec 9, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 9, 2019

kikisdeliveryservice requested a review from haircommander December 9, 2019 18:23

haircommander reviewed Dec 9, 2019

View reviewed changes

vrutkovs changed the title ~~WIP templates: remove /usr/share/containers/oci/hooks.d from crio config~~ templates: remove /usr/share/containers/oci/hooks.d from crio config Dec 9, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 9, 2019

vrutkovs changed the title ~~templates: remove /usr/share/containers/oci/hooks.d from crio config~~ Bug 1781019: templates: remove /usr/share/containers/oci/hooks.d from crio config Dec 10, 2019

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 10, 2019

haircommander mentioned this pull request Dec 11, 2019

cri-o: remove hooks dir /usr/share/containers/oci/hooks.d #1326

Closed

vrutkovs changed the title ~~Bug 1781019: templates: remove /usr/share/containers/oci/hooks.d from crio config~~ templates: remove /usr/share/containers/oci/hooks.d from crio config Dec 11, 2019

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Dec 11, 2019

mrunalp reviewed Dec 11, 2019

View reviewed changes

Comment thread templates/worker/01-worker-container-runtime/_base/files/crio.yaml Outdated

Copy link
Copy Markdown

Member

mrunalp Dec 11, 2019

Choose a reason for hiding this comment

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

Can we drop this comment?

mrunalp reviewed Dec 11, 2019

View reviewed changes

templates: remove /usr/share/containers/oci/hooks.d from crio config

58e334a

Its no longer available since RHCOS doesn't install `oci-systemd-hooks`

vrutkovs force-pushed the crio-update-hooks-d branch from 15f10cc to 58e334a Compare December 11, 2019 19:24

openshift-ci-robot assigned mrunalp Dec 11, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Dec 11, 2019

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 11, 2019

vrutkovs mentioned this pull request Dec 12, 2019

bootstrap: replace CRIO's hooks dir on bootstrap openshift/installer#2800

Closed

LorbusChris approved these changes Dec 12, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 12, 2019

vrutkovs mentioned this pull request Dec 12, 2019

[master] [FCOS] templates: remove /usr/share/containers/oci/hooks.d from crio config #1328

Closed

openshift-merge-robot merged commit 8fb7912 into openshift:master Dec 12, 2019

openshift-cherrypick-robot mentioned this pull request Dec 12, 2019

Bug 1781019: [release-4.3] templates: remove /usr/share/containers/oci/hooks.d from crio config #1329

Merged

carbonin mentioned this pull request Mar 27, 2020

[1.16] add default hooks dir to crio.conf cri-o/cri-o#3011

Closed

carbonin mentioned this pull request Mar 27, 2020

httpd-init doesn't deploy on openshift when using cri-o runtime ManageIQ/manageiq-pods#361

Closed

vrutkovs deleted the crio-update-hooks-d branch September 16, 2020 22:29

Conversation

vrutkovs commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Dec 9, 2019

Uh oh!

vrutkovs commented Dec 9, 2019

Uh oh!

vrutkovs commented Dec 9, 2019

Uh oh!

kikisdeliveryservice commented Dec 9, 2019

Uh oh!

haircommander commented Dec 9, 2019

Uh oh!

haircommander Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

vrutkovs Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

haircommander Dec 9, 2019

Choose a reason for hiding this comment

Uh oh!

vrutkovs Dec 12, 2019

Choose a reason for hiding this comment

Uh oh!

vrutkovs commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kikisdeliveryservice commented Dec 10, 2019

Uh oh!

openshift-ci-robot commented Dec 10, 2019

Uh oh!

vrutkovs commented Dec 10, 2019

Uh oh!

openshift-bot commented Dec 11, 2019

Uh oh!

openshift-ci-robot commented Dec 11, 2019

Uh oh!

haircommander commented Dec 11, 2019

Uh oh!

haircommander commented Dec 11, 2019

Uh oh!

openshift-ci-robot commented Dec 11, 2019

Uh oh!

vrutkovs commented Dec 11, 2019

Uh oh!

mrunalp Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

mrunalp Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

vrutkovs Dec 11, 2019

Choose a reason for hiding this comment

Uh oh!

mrunalp commented Dec 11, 2019

Uh oh!

cgwalters commented Dec 11, 2019

Uh oh!

haircommander commented Dec 11, 2019

Uh oh!

runcom commented Dec 12, 2019

Uh oh!

openshift-ci-robot commented Dec 12, 2019

Uh oh!

openshift-bot commented Dec 12, 2019

Uh oh!

openshift-bot commented Dec 12, 2019

Uh oh!

haircommander commented Dec 12, 2019

Uh oh!

openshift-cherrypick-robot commented Dec 12, 2019

Uh oh!

carbonin commented Mar 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

vrutkovs commented Dec 9, 2019 •

edited

Loading

vrutkovs commented Dec 9, 2019 •

edited

Loading