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

[csi/aws] Bump templates + add support for warm pools #11304

Merged
merged 2 commits into from
Apr 26, 2021

Conversation

dntosas
Copy link
Contributor

@dntosas dntosas commented Apr 24, 2021

AWS EBS CSI Driver:
- Update manifests
- Bump components version
- Add API capability of setting Version + VolumeLimit
- Add support for warm pools
- Remove snapshot-controller resources as it should be independent from
any CSI driver, check here --> https://kubernetes.io/blog/2020/12/10/kubernetes-1.20-volume-snapshot-moves-to-ga/#how-to-deploy-volume-snapshots

Signed-off-by: dntosas ntosas@gmail.com

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 24, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @dntosas. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. area/addons labels Apr 24, 2021
@olemarkus
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 24, 2021
@dntosas dntosas force-pushed the update-aws-csi-ebs branch 4 times, most recently from a34cfb0 to e8c8b92 Compare April 24, 2021 08:08
@dntosas dntosas changed the title WIP [csi/aws] Bump templates to latest stable version [csi/aws] Bump templates to latest stable version Apr 24, 2021
@k8s-ci-robot k8s-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 Apr 24, 2021
@dntosas
Copy link
Contributor Author

dntosas commented Apr 24, 2021

qq: where IAM resources that described in here are handled?

@olemarkus
Copy link
Member

they are handled in pkg/model/iam/iam_builder.go.

It would be nice to collect all the privs needed for CSI to one function though, to make this more maintainable. That would also allow us to use IRSA for this addon further down the road.

@dntosas
Copy link
Contributor Author

dntosas commented Apr 24, 2021

@olemarkus are not IAM permissions for attaching etc volumes should be present on regular nodes role also? without them how will the pods of node daemonSet can be allowed to do such operations?

@olemarkus
Copy link
Member

As I understand it, it is the ebs-csi-controller that does the AWS calls. The purpose of the DaemonSet is to extend kubelet with the driver specific functionality.

@dntosas
Copy link
Contributor Author

dntosas commented Apr 24, 2021

As I understand it, it is the ebs-csi-controller that does the AWS calls. The purpose of the DaemonSet is to extend kubelet with the driver specific functionality.

y makes sense ^^

@johngmyers
Copy link
Member

I don't see the code for calculating the default value of Version

@dntosas
Copy link
Contributor Author

dntosas commented Apr 24, 2021

@johngmyers i thought something like this should be enough ^^ if there is a more proper way or place to implement this logic, can you give a hint on where to look?

@johngmyers
Copy link
Member

@dntosas Okay, that does work; I had missed that. Unfortunately it is error prone to have the version in so many places. If one gets missed on an update that is unlikely to be caught in code review.

The other way is to define a template function, like

// AWSCCMTag returns the correct tag for the cloud controller manager based on
// the Kubernetes Version
func (tf *TemplateFunctions) AWSCCMTag() (string, error) {
var tag string
parsed, err := util.ParseKubernetesVersion(tf.Cluster.Spec.KubernetesVersion)
if err != nil {
return "", fmt.Errorf("failed to parse Kubernetes version from cluster spec: %q", err)
}
// Update when we have stable releases
switch parsed.Minor {
case 18:
tag = "v1.18.0-alpha.1"
case 19:
tag = "v1.19.0-alpha.1"
default:
tag = "latest"
}
return tag, nil
}

@dntosas dntosas changed the title [csi/aws] Bump templates to latest stable version WIP [csi/aws] Bump templates to latest stable version Apr 25, 2021
@k8s-ci-robot k8s-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 Apr 25, 2021
@dntosas dntosas force-pushed the update-aws-csi-ebs branch 2 times, most recently from f9adcd7 to 7bf0805 Compare April 25, 2021 07:47
@dntosas dntosas changed the title WIP [csi/aws] Bump templates to latest stable version [csi/aws] Bump templates to latest stable version Apr 25, 2021
@k8s-ci-robot k8s-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 Apr 25, 2021
@dntosas
Copy link
Contributor Author

dntosas commented Apr 25, 2021

@olemarkus @johngmyers thanks for your help :)

pushed changes, i think we are better now ^^

@@ -838,6 +838,16 @@ type CloudConfiguration struct {
type AWSEBSCSIDriver struct {
//Enabled enables the AWS EBS CSI driver
Enabled *bool `json:"enabled,omitempty"`

// Version of the CSI controller container
Copy link
Member

Choose a reason for hiding this comment

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

Nit: go comments should have proper punctuation.

return nil
}

if c.Enabled == nil {
Copy link
Member

Choose a reason for hiding this comment

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

nit: this check is typically noops as we tend to use if fi.BoolValue() which defaults to false.
And you have to use {{ if WithDefaultBool <defaultVaule> }} in templates as templates will only check if the pointer is nil or not.

@olemarkus
Copy link
Member

Just a few nits, otherwise looks good to go to me.

@dntosas dntosas force-pushed the update-aws-csi-ebs branch 2 times, most recently from bd7e329 to 12cd402 Compare April 25, 2021 09:33
- Update manifests
- Bump components version
- Add API capability of setting Version + VolumeLimit
- Remove snapshot-controller resources as it should be independent from
any CSI driver

Signed-off-by: dntosas <ntosas@gmail.com>
@dntosas dntosas changed the title [csi/aws] Bump templates to latest stable version [csi/aws] Bump templates + add support for warm pools Apr 25, 2021
Add pulling needed images as initial task for warming up instances for
csi driver resources.

Signed-off-by: dntosas <ntosas@gmail.com>
@dntosas
Copy link
Contributor Author

dntosas commented Apr 26, 2021

/retest

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Apr 26, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: olemarkus

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 26, 2021
@k8s-ci-robot k8s-ci-robot merged commit d90ace0 into kubernetes:master Apr 26, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.21 milestone Apr 26, 2021
@dntosas dntosas deleted the update-aws-csi-ebs branch April 26, 2021 11:11
k8s-ci-robot added a commit that referenced this pull request May 8, 2021
…304-#11326-#11418-origin-release-1.20

Automated cherry pick of #11304: [csi/aws] Bump templates to latest#11326: Fix arguments to csi-provisioner after bump to v2.2.0#11418: Bump to GA release
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/addons area/api area/nodeup cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants