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

Document and add validation on enable_cri_dockerd option #415

Merged
merged 6 commits into from
Nov 16, 2023

Conversation

felipe-colussi
Copy link
Contributor

@felipe-colussi felipe-colussi commented Sep 4, 2023

Issue: #404

Problem

K8s 1.24+ needs enable_cri_dockerd to be set to true for clusters to successfully provision.

Solution

We cant do that change.

a) change the default value of enable_cri_dockerd to true

This would change the behavior of cluster using k8s 1.23 or less.

b) Set enable_cri_dockerd to true during execution time:

This would generate inconsistency on terraform plans and would result on the resource being applied every single time.

Add to the docs that the user need to change the value of the field.

Also added a validation function on the Expander that is used to create/update the cluster.

This can't be a "ValidationFunc" from terraform as it depends on 2 fields.

Testing

Added a validation for the function that checks the k8s version

Engineering Testing

Manual Testing

Automated Testing

  • Test types added/modified:

    • None
  • If "None" - Reason:
    Change to the docs.

Summary:

QA Testing Considerations

Regressions Considerations

There is no regression, this is a doc only

@felipe-colussi felipe-colussi requested review from a-blender and a team September 13, 2023 18:46
@a-blender a-blender changed the title doccument cri Document enable_cri_dockerd option for users Sep 26, 2023
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.

Note: There was significant discussion around the decision for the RKE enable_cri_dockerd option and this was the simplest solution instead of handling the value in an unexpected way on the backend based on the version of the cluster. Once k8s 1.23 is deprecated, we will update the default to true and users can provision RKE clusters without manually setting the value.

@a-blender a-blender requested a review from a team October 27, 2023 22:52
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.

Can you add validation for the provider to error out if a user sets false on a 1.24+ cluster? Either in a ValidateFunc or if you don't have access to the k8s version, perhaps here

if _, ok := d.Get("enable_cri_dockerd").(bool); ok && in.EnableCRIDockerd != nil {
d.Set("enable_cri_dockerd", *in.EnableCRIDockerd)
}

Thank you @jiaqiluo for the feedback!

@a-blender a-blender requested a review from a team October 27, 2023 23:30
@felipe-colussi felipe-colussi changed the title Document enable_cri_dockerd option for users Document and add validation on enable_cri_dockerd option Oct 30, 2023
rke/structure_rke_cluster.go Outdated Show resolved Hide resolved
rke/structure_rke_cluster.go Outdated Show resolved Hide resolved
rke/structure_rke_cluster_test.go Show resolved Hide resolved
rke/structure_rke_cluster.go Outdated Show resolved Hide resolved
@a-blender a-blender requested a review from a team November 2, 2023 19:21
rke/structure_rke_cluster.go Outdated Show resolved Hide resolved
rke/structure_rke_cluster.go Outdated Show resolved Hide resolved
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.

Lgtm now, please squash on merge

@felipe-colussi felipe-colussi merged commit 5f20366 into rancher:master Nov 16, 2023
1 check passed
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.

3 participants