Skip to content
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

Add enable_cri_dockerd parameter #337

Merged
merged 3 commits into from
Jun 6, 2022

Conversation

dje4om
Copy link
Contributor

@dje4om dje4om commented Mar 31, 2022

Hi,

Refered to issue #325

Here is some outputs from my dev env

env var is well defined to the kubelet container, on dev node:

docker exec kubelet env                                                                                                                                                                                                                                                          

PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
HOSTNAME=rke01
RKE_KUBELET_CRIDOCKERD=true
HOME=/root

kubelet args are also well defined

--container-runtime-endpoint=/var/run/dockershim.sock --container-runtime=remote

test is already ensured by rke if we tried an older version than kubernetes 1.21 :

rke_cluster.lab: Refreshing state... [id=f8cc1f51-5939-47b9-acc3-7baa527640c2]
local_file.kubeconfig_yaml: Refreshing state... [id=0b42f3ae3e24322d5cc8d646272f57196e001621]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # rke_cluster.lab will be updated in-place
  ~ resource "rke_cluster" "lab" {
      ~ enable_cri_dockerd        = false -> true
        id                        = "f8cc1f51-5939-47b9-acc3-7baa527640c2"
        # (25 unchanged attributes hidden)

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

rke_cluster.lab: Modifying... [id=f8cc1f51-5939-47b9-acc3-7baa527640c2]
╷
│ Error:
│ ============= RKE outputs ==============
│ time="2022-03-30T23:58:53+02:00" level=info msg="Updating RKE cluster..."
│ time="2022-03-30T23:58:53+02:00" level=info msg="Initiating Kubernetes cluster"
│
│ Failed initializing cluster err:Failed to validate cluster: Enabling cri-dockerd for cluster version [v1.20.13-rancher1-1] is not supported
│ ========================================
│
│
│   with rke_cluster.lab,
│   on lab.tf line 1, in resource "rke_cluster" "lab":
│    1: resource "rke_cluster" "lab" {
│
╵

Copy link
Contributor

@HarrisonWAffel HarrisonWAffel left a comment

Choose a reason for hiding this comment

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

This LGTM, but ill give final say to my colleague @annablender, as she has more experience with this repository

@tsde
Copy link

tsde commented May 17, 2022

I think it is an important PR as Kubernetes 1.24 is now out and support for docker is dropped starting from this version. Having this merged along with #340 would be neat.

Copy link
Contributor

@a-blender a-blender left a comment

Choose a reason for hiding this comment

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

I want to note the rancher docs where this field is listed as supported. The enable_cri_docker field also needs to be added to the RKE config tests in terraform. Otherwise, LGTM

DNM - pending 2.6.5 Terraform release.

@a-blender a-blender added this to the v2.6.6 - Terraform milestone May 18, 2022
@a-blender a-blender changed the title add enable_cri_dockerd parameter [DNM] add enable_cri_dockerd parameter May 18, 2022
@a-blender a-blender self-requested a review May 25, 2022 22:34
@a-blender
Copy link
Contributor

Terraform RKE v1.3.1 patch is released and sufficient smoke testing has been done to verify an RKE all role cluster with cri_dockerd_enable works as expected. Ok to merge.

rke/structure_rke_cluster.go Outdated Show resolved Hide resolved
@dje4om
Copy link
Contributor Author

dje4om commented May 30, 2022

Hi, change has been done, thx for the review

@a-blender a-blender requested review from a-blender and jakefhyde May 31, 2022 12:52
@a-blender a-blender changed the title [DNM] add enable_cri_dockerd parameter Add enable_cri_dockerd parameter May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants