Skip to content
This repository was archived by the owner on Aug 7, 2025. It is now read-only.

stack: dont provide CRI socket#95

Merged
ganeshmaharaj merged 1 commit intoclearlinux:masterfrom
jcvenegas:custom-cri-socket
May 24, 2019
Merged

stack: dont provide CRI socket#95
ganeshmaharaj merged 1 commit intoclearlinux:masterfrom
jcvenegas:custom-cri-socket

Conversation

@jcvenegas
Copy link
Contributor

@jcvenegas jcvenegas commented May 24, 2019

kubeadm autodetects the socket path based on defaults from well known
CRI servers.

Signed-off-by: Jose Carlos Venegas Munoz jose.carlos.venegas.munoz@intel.com

@ganeshmaharaj
Copy link
Contributor

@jcvenegas might i ask you to take a quick peek at the containerd patch and see if we this can be pulled in with that? a.k.a can we fix that one and make it merge-able?

@jcvenegas jcvenegas force-pushed the custom-cri-socket branch from 16fa7ea to 98d3a1f Compare May 24, 2019 03:55
@jcvenegas jcvenegas changed the title stack: Allow define custom cri socket. [DNM]stack: Allow define custom cri socket. May 24, 2019
@jcvenegas
Copy link
Contributor Author

@ganeshmaharaj right, I did not wanted to modify a lot because there was that PR that is looking to fully enable containerd.

On this PR we only add the logic to point to a cri socket, in the containerd PR there the approach used is to duplicate the kubeadm.yml file, I prefer this way.

The containerd PR additionally do the next:

  • Script to install containerd from static binaries. Seems that that should be removed, and use containerd provided by Clear Linux. I share the feeling with @egernst to have scripts to install static binaries, more for development/testing. Probably we need to move those scripts in other place/repo

  • runc-runtimeClass.yaml: I need to confirm if still needed today ( I dont think so, so probably need to remove from the PR as well)

  • containerd.conf I think that could be more dynamic in a script to set correct paths to
    RuntimeRoot = "/usr/local/bin/kata-runtime"

@mythi
Copy link
Contributor

mythi commented May 24, 2019

On this PR we only add the logic to point to a cri socket,

kubeadm is able to auto-detect the socket these days. How about we add kustomize to manage kubeadm.yaml and see if the autodetection works for kubeadm reset?

@jcvenegas jcvenegas force-pushed the custom-cri-socket branch 2 times, most recently from 423f94f to f7871d7 Compare May 24, 2019 14:50
@jcvenegas
Copy link
Contributor Author

jcvenegas commented May 24, 2019

@mythi seems that works fine with containerd, I'll need to check with crio
@ganeshmaharaj @krsna1729 any preference on the method?

Be explicit and allow the user change the default paths or let kubeadm pick the path based en well known paths.

@krsna1729
Copy link
Contributor

Yes lets keep it simple. I have been using autodetection for a few days. Seems to be working. If the user wants custom location they can add. Out of the box crio and containerd locations as packaged by clear must work.

@jcvenegas jcvenegas force-pushed the custom-cri-socket branch from f7871d7 to 0abd925 Compare May 24, 2019 16:03
@jcvenegas jcvenegas changed the title [DNM]stack: Allow define custom cri socket. stack: dont provide CRI socket May 24, 2019
@jcvenegas
Copy link
Contributor Author

@krsna1729 thanks for confirm. I squashed the commits.
@mythi kubeadm.yaml does not need any custom modification at the moment after this change, so kustomize not needed for this.

kubeadm autodetects the socket path based on defaults from well known
CRI servers.

Signed-off-by: Jose Carlos Venegas Munoz <jose.carlos.venegas.munoz@intel.com>
@mythi
Copy link
Contributor

mythi commented May 24, 2019

@krsna1729 @jcvenegas sounds good!

Note typo in the commit message

@jcvenegas jcvenegas force-pushed the custom-cri-socket branch from 0abd925 to 35d3d12 Compare May 24, 2019 17:20
@jcvenegas
Copy link
Contributor Author

typo fixed

@ganeshmaharaj
Copy link
Contributor

Seems we are all happy with this change. time to hit merge.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants