kubelet: enable crio #52

rphillips · 2018-09-11T21:17:50Z

Enables crio for the kubelet container runtime.

openshift-ci-robot · 2018-09-11T21:17:55Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: rphillips
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: yifan-gu

If they are not already assigned, you can assign the PR to them by writing /assign @yifan-gu in a comment when ready.

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

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

abhinavdahiya · 2018-09-11T21:27:00Z

templates/_base/master/files/container-registries.yaml

@rphillips

why is this file required?

who uses this ?

how will we configure it if we somebody needs to configure it for their internal registry ??

/cc @crawford

The file is needed since crio gets installed with a default config which only includes the registry.access.redhat.com registry. docker.io needs to be added to the registry list to pull in defaulted docker images.

/cc @sjenning

If this only affects images like openshift/origin-machine-config-operator:latest as this is not a fully qualified name, it should have been docker.io/openshift/origin-machine-config-operator:latest.

I rather have people use fully qualified names, than put this file on each machine... @crawford WDYT?

@rphillips can you change this file to suggestion made in the installer here

abhinavdahiya · 2018-09-11T21:38:11Z

@rphillips also can you run go test ./pkg/controller/template/... -u to update the golden test files.

rphillips · 2018-09-11T21:42:53Z

@abhinavdahiya regenerated

rphillips · 2018-09-11T21:45:49Z

/hold

We may want to merge the cross-repo crio PR's all at once.

crawford · 2018-09-12T17:28:10Z

templates/_base/master/files/container-registries.yaml

As mentioned in openshift/installer#234 (comment), can you remove these empty sections?

wking · 2018-09-12T17:45:25Z

pkg/controller/template/test_data/templates/aws/master/units/kubelet.service

Why did you need to set this? Did you hit an error without it, or is it just increasing robustness? What's the default value? How did you decide on 10m?

default is 2m. probably doesn't need to be changed. i think the cri-o docs recommend this, but i don't think it is needed.

default is 2m. probably doesn't need to be changed

I'm in favor of dropping it then. The fewer local opinions we need to maintain, the better :).

Yeah, I took the runtime-request-timeout from this recommendation: https://github.com/kubernetes-sigs/cri-o/blob/master/kubernetes.md#preparing-kubelet

Let's keep the time out then, as it might be required when we pull hyberkube image on slower network.

wking · 2018-09-12T17:48:03Z

templates/_base/worker/files/container-registries.yaml

It DRY things up a bit to have a templates/_base/all/files/container-registries.yaml or some such which would be added to both masters and workers. But perhaps that is more than we want to bite off in this particular PR.

rphillips · 2018-09-13T13:59:04Z

Updated the PR and removed the --runtime-request-timeout.

The default --runtime-request-timeout enforces a 2 minute timeout on image pulls... We may want to bump it up for slower networks.

rphillips · 2018-09-13T14:00:56Z

/hold cancel

abhinavdahiya · 2018-09-13T14:01:51Z

templates/_base/worker/units/kubelet.yaml

--cni-conf-dir, --cni-bin-dir, --network-pluging are no longer required as cri-o handles everything.

Thanks. Removed those options.

abhinavdahiya · 2018-09-13T14:07:17Z

/hold

I was testing these changes yesterday, networking is not ready to be used with crio in installer yet. openshift/installer#235 (comment)

abhinavdahiya · 2018-09-13T14:15:58Z

The default --runtime-request-timeout enforces a 2 minute timeout on image pulls... We may want to bump it up for slower networks.

As it is recommended by https://github.com/kubernetes-sigs/cri-o/blob/master/kubernetes.md#preparing-kubelet Let's set it to 10m

rphillips · 2018-09-13T14:19:47Z

@abhinavdahiya I added support for a 10m timeout, and an environment variable and file to be able to overwrite the default

sjenning · 2018-09-13T16:51:59Z

I think all the feedback has been addressed. I've tested this an it works:

$ oc get nodes -o wide
NAME            STATUS    ROLES       AGE       VERSION           INTERNAL-IP      EXTERNAL-IP   OS-IMAGE                  KERNEL-VERSION               CONTAINER-RUNTIME
dev-bootstrap   Ready     bootstrap   5m        v1.11.0+d4cacc0   192.168.124.10   <none>        Red Hat CoreOS 4.0.5470   3.10.0-862.14.1.el7.x86_64   cri-o://1.11.2
dev-master-0    Ready     master      4m        v1.11.0+d4cacc0   192.168.124.11   <none>        Red Hat CoreOS 4.0.5470   3.10.0-862.14.1.el7.x86_64   cri-o://1.11.2
dev-worker-0    Ready     worker      4m        v1.11.0+d4cacc0   192.168.124.50   <none>        Red Hat CoreOS 4.0.5470   3.10.0-862.14.1.el7.x86_64   cri-o://1.11.2
dev-worker-1    Ready     worker      4m        v1.11.0+d4cacc0   192.168.124.51   <none>        Red Hat CoreOS 4.0.5470   3.10.0-862.14.1.el7.x86_64   cri-o://1.11.2

can this get approved?

wking · 2018-09-13T23:42:47Z

This looks good to me. @abhinavdahiya, do you want to mark your requested-change resolved?

abhinavdahiya · 2018-09-13T23:50:42Z

@wking #52 (comment)

lgtm this now will break installer.

abhinavdahiya · 2018-09-14T17:48:49Z

@rphillips @sjenning
Can you add these files to the PR:

$ cat templates/_base/master/files/etc-sysconfig-crio-network.yaml
filesystem: "root"
mode: 0644
path: "/etc/sysconfig/crio-network"
contents:
  inline: |
    CRIO_NETWORK_OPTIONS=--cni-config-dir=/etc/kubernetes/cni/net.d --cni-plugin-dir=/etc/kubernetes/cni/bin

$ cat templates/_base/worker/files/etc-sysconfig-crio-network.yaml
filesystem: "root"
mode: 0644
path: "/etc/sysconfig/crio-network"
contents:
  inline: |
    CRIO_NETWORK_OPTIONS=--cni-config-dir=/etc/kubernetes/cni/net.d --cni-plugin-dir=/etc/kubernetes/cni/bin
``

Also rebase to 2 commits, one with changes and one with generated test_data `go test ./pkg/controller/templates/.. -u`

rphillips · 2018-09-14T18:24:00Z

Closing in favor of #63

logging cleanup for bz debug, progressing debug

openshift-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 11, 2018

openshift-ci-robot requested review from crawford and wking September 11, 2018 21:17

rphillips force-pushed the fixes/enable_crio branch from 9d5fb58 to 39d072e Compare September 11, 2018 21:21

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2018

abhinavdahiya suggested changes Sep 11, 2018

View reviewed changes

openshift-ci-robot requested a review from sjenning September 11, 2018 21:40

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 11, 2018

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

rphillips changed the title ~~kubelet: enable crio~~ WIP: kubelet: enable crio Sep 11, 2018

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 11, 2018

crawford mentioned this pull request Sep 12, 2018

inject registries.conf with docker.io to use crio for bootstrap kubelet openshift/installer#234

Closed

crawford reviewed Sep 12, 2018

View reviewed changes

wking reviewed Sep 12, 2018

View reviewed changes

wking mentioned this pull request Sep 12, 2018

kubelet: enable crio runtime openshift/installer#235

Closed

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 13, 2018

rphillips force-pushed the fixes/enable_crio branch from 1927d5f to fcae8cc Compare September 13, 2018 13:57

rphillips changed the title ~~WIP: kubelet: enable crio~~ kubelet: enable crio Sep 13, 2018

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 Sep 13, 2018

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

abhinavdahiya reviewed Sep 13, 2018

View reviewed changes

kubelet: enable crio

114b898

rphillips force-pushed the fixes/enable_crio branch from fcae8cc to 114b898 Compare September 13, 2018 14:03

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

add runtime-request-timeout

99552f9

sjenning mentioned this pull request Sep 14, 2018

switch to crio #62

Merged

rphillips closed this Sep 14, 2018

osherdp pushed a commit to osherdp/machine-config-operator that referenced this pull request Apr 13, 2021

Merge pull request openshift#52 from gabemontero/progressing-fixes

5b69c7b

logging cleanup for bz debug, progressing debug

kubelet: enable crio #52

kubelet: enable crio #52

Uh oh!

Conversation

rphillips commented Sep 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Sep 11, 2018

Uh oh!

rphillips commented Sep 11, 2018

Uh oh!

rphillips commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphillips Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphillips commented Sep 13, 2018

Uh oh!

rphillips commented Sep 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhinavdahiya commented Sep 13, 2018

Uh oh!

abhinavdahiya commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rphillips commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sjenning commented Sep 13, 2018

Uh oh!

wking commented Sep 13, 2018

Uh oh!

abhinavdahiya commented Sep 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abhinavdahiya commented Sep 14, 2018

Uh oh!

rphillips commented Sep 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

rphillips commented Sep 11, 2018 •

edited

Loading

rphillips Sep 13, 2018 •

edited

Loading

abhinavdahiya commented Sep 13, 2018 •

edited

Loading

rphillips commented Sep 13, 2018 •

edited

Loading

abhinavdahiya commented Sep 13, 2018 •

edited

Loading