AWS CSI driver #10467

olemarkus · 2020-12-18T18:07:28Z

No description provided.

olemarkus · 2020-12-19T10:52:24Z

/retest

rifelpet · 2020-12-19T14:25:38Z

pkg/apis/kops/validation/aws.go

+		if c.KubeControllerManager == nil || c.KubeControllerManager.ExternalCloudVolumePlugin != "aws" {
+			if c.CloudConfig == nil || c.CloudConfig.AWSEBSCSIDriver == nil || !fi.BoolValue(c.CloudConfig.AWSEBSCSIDriver.Enabled) {
+				allErrs = append(allErrs, field.Forbidden(field.NewPath("spec", "externalCloudControllerManager"),
+					"AWS external CCM cannot be used without enabling spec.cloudConfig.AWSEBSCSIDriver or setting spec.kubeControllerManaager.externalCloudVolumePlugin set to `aws`"))


I'm wondering if this is needs to be a requirement. Users may have clusters that don't run any persistent volumes, for example.

The challenge right now is that everyone else need to actively set one of these values to get a working cluster. I think that when we by default enable either using external CCM for AWS and/or external CSI driver we can relax the relax the requirement.

I suspect that the CSI driver will be more popular than the external CCM for the time being anyway. CSI driver does bring in additional features.

olemarkus · 2020-12-19T15:34:50Z

Not sure how much we really want to be configurable here.
In particular, there is a large amount of images involved and it could get messy allowing each of them to be configured.

rifelpet

Looking at the helm chart's values file, I don't see any parameters that would be particularly important to expose as api fields for an initial implementation.

I think there are additional IAM permissions needed. The docs mention ec2:CreateSnapshot which neither the control plane nor node roles have, and the ebs-snapshot-controller StatefulSet isn't currently restricted to just the control plane nodes. Perhaps this is a good opportunity to use the UseServiceAccountIAM feature flag functionality.

rifelpet · 2020-12-19T15:43:46Z

upup/models/cloudup/resources/addons/aws-ebs-csi-driver.addons.k8s.io/k8s-1.17.yaml.template

+            - --endpoint=$(CSI_ENDPOINT)
+            - --logtostderr
+            - --k8s-tag-cluster-id={{ ClusterName }}
+            - --extra-volume-tags=KubernetesCluster={{ ClusterName }}


This flag is deprecated in favor of --extra-tags:
https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/f54138034204850e54c26f67dde3f8217339c09c/cmd/options/controller_options.go#L39-L40

Also we could consider populating this with the standard set of tags we include on resources, including ClusterSpec.CloudLabels

Good catch.

olemarkus · 2020-12-19T18:28:39Z

I think there are additional IAM permissions needed. The docs mention ec2:CreateSnapshot which neither the control plane nor node roles have, and the ebs-snapshot-controller StatefulSet isn't currently restricted to just the control plane nodes. Perhaps this is a good opportunity to use the UseServiceAccountIAM feature flag functionality.

I like the IRSA concept, and I think strategically we should move in that direction, but is it mature enough to start depending on it now?

olemarkus · 2020-12-19T19:13:37Z

Reading https://github.com/kubernetes-csi/external-snapshotter in more detail, it seems worthy having a dedicated addon for this. Including the snapshotter, CRDs + the webhook admission controller.

olemarkus · 2020-12-20T08:18:17Z

Looking through https://github.com/kubernetes/community/blob/master/contributors/design-proposals/storage/container-storage-interface.md and the docs of the various plugins, I wonder if for kOps, it makes most sense to:

deploy the vendor CSI driver itself as a standalone addon. The controller running as a daemonset on masters
The snapshot controller as a standalone addon. It should also be a daemonset rather than a statefulset (the use of statefulset was somewhat inconsistent. The reason is to avoid running two of them at the same time. But the controller supports leader election, so for us a daemonset makes more sense. ). There should only be one installation of this controller per cluster. This addon should also contain the webhook and CRDs.

olemarkus · 2020-12-22T10:14:20Z

This is more or less done now.

Since we need a cert for the admission controller, this now blocked by #10321

mikesplain · 2020-12-30T18:01:26Z

This is awesome, unfortunately I only found this PR after having set this up myself as well haha.

Only comment, would we want to also modify the storage class for this? For this to be usable you need to modify the provisioner: ebs.csi.aws.com in

kops/upup/models/cloudup/resources/addons/storage-aws.addons.k8s.io/v1.15.0.yaml

Line 7 in 22a9a13

provisioner: kubernetes.io/aws-ebs

I could see that being a follow up PR or a separate flag but just thought I'd mention it. Also an easy time to enable volume expansion by default.

olemarkus · 2021-01-04T09:28:08Z

Updating the provisioner field is forbidden, so we need a new storageclass here. Or force-change the old one.

Do you know if changing this really is needed when CSIMigrationAWS feature gate is set?

olemarkus · 2021-01-04T09:31:11Z

CSIMigrationAWS: Enables shims and translation logic to route volume operations from the AWS-EBS in-tree plugin to EBS CSI plugin. Supports falling back to in-tree EBS plugin if a node does not have EBS CSI plugin installed and configured. Requires CSIMigration feature flag enabled.
CSIMigrationAWSComplete: Stops registering the EBS in-tree plugin in kubelet and volume controllers and enables shims and translation logic to route volume operations from the AWS-EBS in-tree plugin to EBS CSI plugin. Requires CSIMigration and CSIMigrationAWS feature flags enabled and EBS CSI plugin installed and configured on all nodes in the cluster.

Looks like we should not create a new storage class. And we probably should set the CSIMigrationAWSComplete flag to ensure our setup works as expected.

k8s-ci-robot · 2021-01-08T15:01:29Z

[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:

~~OWNERS~~ [olemarkus]

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

olemarkus · 2021-01-08T15:02:59Z

I removed the snapshotter out of the PR now, so we shouldn't have any webhooks issues.
Also set the feature flag that will block the in-cluster EBS plugin so we 100% know the out-of-tree plugin is used.

rifelpet

A few minor comments but looks good overall. We'll want docs for this as well as a release note. Being sure to mention the migration steps involving the feature gates, or just linking to wherever that process is documented.

k8s/crds/kops.k8s.io_clusters.yaml

rifelpet · 2021-01-11T23:56:24Z

pkg/model/components/kubelet.go

+	if _, found := clusterSpec.Kubelet.FeatureGates["ExperimentalCriticalPodAnnotation"]; !found {
+		if b.IsKubernetesLT("1.16") {
+			clusterSpec.Kubelet.FeatureGates["ExperimentalCriticalPodAnnotation"] = "true"
+		}
+	}


I think this is duplicated from Line 210

Thanks for catching!

rifelpet · 2021-01-12T20:56:55Z

/lgtm

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 18, 2020

k8s-ci-robot requested review from hakman and zetaab December 18, 2020 18:07

k8s-ci-robot added the area/addons label Dec 18, 2020

olemarkus force-pushed the ebs-csi branch 2 times, most recently from 450370d to 143a8e7 Compare December 18, 2020 19:21

k8s-ci-robot added the area/api label Dec 18, 2020

olemarkus force-pushed the ebs-csi branch 5 times, most recently from 0adde02 to 9ae6852 Compare December 19, 2020 10:42

rifelpet reviewed Dec 19, 2020

View reviewed changes

olemarkus force-pushed the ebs-csi branch from 9ae6852 to 92d3239 Compare December 19, 2020 15:30

rifelpet reviewed Dec 19, 2020

View reviewed changes

olemarkus force-pushed the ebs-csi branch from 92d3239 to fe7b5cd Compare December 21, 2020 10:27

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 21, 2020

olemarkus force-pushed the ebs-csi branch from fe7b5cd to d1f3c5b Compare December 22, 2020 10:12

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2020

olemarkus force-pushed the ebs-csi branch from d1f3c5b to 609494e Compare January 8, 2021 15:01

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 8, 2021

olemarkus force-pushed the ebs-csi branch from 609494e to 9d4c394 Compare January 8, 2021 17:55

olemarkus changed the title ~~WIP: AWS CSI driver~~ AWS CSI driver Jan 8, 2021

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 Jan 8, 2021

olemarkus requested a review from rifelpet January 8, 2021 19:20

olemarkus force-pushed the ebs-csi branch from 9d4c394 to 76b4ec1 Compare January 8, 2021 19:25

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2021

olemarkus force-pushed the ebs-csi branch from 76b4ec1 to 27ff651 Compare January 9, 2021 19:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 9, 2021

rifelpet reviewed Jan 11, 2021

View reviewed changes

AWS CSI driver

f5ae5f8

olemarkus force-pushed the ebs-csi branch from 27ff651 to f5ae5f8 Compare January 12, 2021 06:09

olemarkus requested a review from rifelpet January 12, 2021 12:07

k8s-ci-robot assigned rifelpet Jan 12, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 12, 2021

k8s-ci-robot merged commit a140168 into kubernetes:master Jan 12, 2021

k8s-ci-robot added this to the v1.20 milestone Jan 12, 2021

rifelpet mentioned this pull request Jan 20, 2021

Kops - use AWS EBS CSI driver in external CCM test job kubernetes/test-infra#20529

Merged

johngmyers mentioned this pull request Sep 4, 2023

Remove dead code for in-tree CCM #15872

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

AWS CSI driver #10467

AWS CSI driver #10467

olemarkus commented Dec 18, 2020

olemarkus commented Dec 19, 2020

rifelpet Dec 19, 2020

olemarkus Dec 19, 2020

olemarkus commented Dec 19, 2020

rifelpet left a comment

rifelpet Dec 19, 2020

olemarkus Dec 19, 2020

olemarkus commented Dec 19, 2020

olemarkus commented Dec 19, 2020

olemarkus commented Dec 20, 2020 •

edited

Loading

olemarkus commented Dec 22, 2020

mikesplain commented Dec 30, 2020

olemarkus commented Jan 4, 2021

olemarkus commented Jan 4, 2021

k8s-ci-robot commented Jan 8, 2021

olemarkus commented Jan 8, 2021

rifelpet left a comment

rifelpet Jan 11, 2021

olemarkus Jan 12, 2021

rifelpet commented Jan 12, 2021

AWS CSI driver #10467

AWS CSI driver #10467

Conversation

olemarkus commented Dec 18, 2020

olemarkus commented Dec 19, 2020

rifelpet Dec 19, 2020

Choose a reason for hiding this comment

olemarkus Dec 19, 2020

Choose a reason for hiding this comment

olemarkus commented Dec 19, 2020

rifelpet left a comment

Choose a reason for hiding this comment

rifelpet Dec 19, 2020

Choose a reason for hiding this comment

olemarkus Dec 19, 2020

Choose a reason for hiding this comment

olemarkus commented Dec 19, 2020

olemarkus commented Dec 19, 2020

olemarkus commented Dec 20, 2020 • edited Loading

olemarkus commented Dec 22, 2020

mikesplain commented Dec 30, 2020

olemarkus commented Jan 4, 2021

olemarkus commented Jan 4, 2021

k8s-ci-robot commented Jan 8, 2021

olemarkus commented Jan 8, 2021

rifelpet left a comment

Choose a reason for hiding this comment

rifelpet Jan 11, 2021

Choose a reason for hiding this comment

olemarkus Jan 12, 2021

Choose a reason for hiding this comment

rifelpet commented Jan 12, 2021

olemarkus commented Dec 20, 2020 •

edited

Loading