Set up dynamic provisioning of persistent volumes on AWS #423

BrainBlasted · 2020-05-12T21:43:54Z

Sets up the plumbing required to support dynamic provisioning on AWS.

BrainBlasted · 2020-05-12T21:45:28Z

To test, follow the example here: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/tree/master/examples/kubernetes/dynamic-provisioning

iaguis

Thanks for the PR, this works after fixing a small issue I mention in the review, great work!

My main concern with this is not using the upstream Helm chart, I think we should use it if possible, see my comment about that.

We should consider exposing a knob to turn this on and off, some users might not be interested in this.

I think this won't be updated when we update the chart in Lokomotive, maybe @invidian has some ideas on how to do this.

Also, I think we should have a test that checks that dynamic provisioning works, the test would deploy a minimal app with a PVC and check it's bound or something similar.

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/workers/workers.tf

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/aws-ebs-csi-driver/Chart.yaml

...ts/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/aws-ebs-csi-driver/templates/node.yaml

invidian · 2020-05-13T13:04:01Z

I agree with @iaguis that this should perhaps be optional behavior. I suggest following changes:

move IAM role creation code to controller module and make it optional, using Terraform variable and count = var.set_worker_iam_role ? 1 : 0. I don't think it is trivial to make it optional per worker pool, as then the pods which require storage must be scheduled on the pools, which has storage enabled.
produce IAM role name as an Terraform module output in controller module and pass it to all worker module instances in Go template. If role is disabled, perhaps pass null or empty string ("").
try to use upstream helm chart to avoid maintaining code on our own. If you have some issues with it @BrainBlasted, please let us know and we are happy to help.
add CSI driver as a component, instead of cluster component. This way:
- we have an easy method to upgrade it (as we currently don't have a way to upgrade platform-specific controlplane components)
- we don't do so much in Terraform, which should be preferred (see Reduce the amount of tasks handled by Terraform #181)
- we treat this CSI driver as yet another storage provider for Kubernetes

iaguis · 2020-05-20T10:19:06Z

I tested the new EBS component and it worked fine 👍. What I would like to see to move this forward is address the rest of @invidian's comments in #423 (comment):

Use an upstream Helm chart
Make setting the IAM role optional by using a Terraform variable and exposing it to the AWS cluster configuration in the lokocfg file.
Move IAM role creation to the controller Terraform module and pass its name to all worker module instances so they can use it.

If you have any questions about this let us know.

invidian · 2020-05-20T10:39:59Z

One thing, which definitely should be added is a storage class, with parametrized default annotation, without that, prometheus operator will be in pending state until one creates the default storage class manually and removes the PVCs and pods to trigger the recreation.

Sample storage class, which I think can be used:

kind: StorageClass
apiVersion: storage.k8s.io/v1
metadata:
  name: ebs-sc
  annotations:
    storageclass.kubernetes.io/is-default-class: "true"
provisioner: ebs.csi.aws.com
volumeBindingMode: WaitForFirstConsumer

Another thing which should think about is handling orphan volumes. I suspect, that if you create the volumes, then you destroy the cluster, the volumes will remain in AWS account.

invidian · 2020-05-20T12:25:12Z

As expected, I can find orphan EBS volumes in AWS console after destroying the cluster 😕

We need to find a way to deal with those somehow.

johananl · 2020-05-20T20:29:45Z

@BrainBlasted I wanted to ensure you've seen #379 (comment). Sorry to add this info now (this came up in today's sprint planning). I hope I'm not adding too much work.

BrainBlasted · 2020-05-21T16:16:43Z

Yes, I saw. Once everything else is finished I'll add a policy similar to the ones in #464 that allows this to work

BrainBlasted · 2020-05-22T22:56:32Z

Use an upstream Helm chart

Make setting the IAM role optional by using a Terraform variable and exposing it to the AWS cluster configuration in the lokocfg file.

Move IAM role creation to the controller Terraform module and pass its name to all worker module instances so they can use it.

I think these are largely done now, save for the third item. However, everything seems to work without the worker nodes using the IAM roles now that the controller is using it (which is needed for the upstream helm chart to work)

BrainBlasted · 2020-05-22T22:58:47Z

I don't quite understand why the CI failed here.

invidian

I don't quite understand why the CI failed here.

About that, I'm not sure either. Better to just run the job again I guess, to see if it fails consistently.

The PR looks very good @BrainBlasted, nice work :)

We need to add documentation of new parameters to docs/configuration-reference/platforms/aws.md for new flag and to docs/configuration-reference/components/ for new component.

If you could also re-organize the commits, perhaps into 2: one for adding IAM-related code and one for new component, it would make it easier for other reviewers to review the PR and it will make our git history a bit simpler after merging.

If the IAM role is only attached to the controller nodes, the severity of blocking access to metadata drops in my opinion. What do you think @iaguis @johananl?

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/controllers.tf

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/variables.tf

pkg/platform/aws/aws.go

pkg/platform/aws/template.go

BrainBlasted · 2020-05-25T09:06:40Z

Reworked into two commits, as requested.

cli/cmd/component.go

docs/configuration-reference/platforms/aws.md

pkg/components/aws-ebs-csi-driver/component.go

BrainBlasted · 2020-06-02T22:59:30Z

I made that change, but CI is still hanging on that test for some reason.

invidian · 2020-06-03T08:16:27Z

Oops, component "aws-ebs-csi-driver" {} line was missing in ci/aws/aws-cluster.lokocfg.envsubst. I added it now, I hope you don't mind @BrainBlasted.

BrainBlasted · 2020-06-03T08:24:38Z

I don't mind - thanksOn Jun 3, 2020 1:16 AM, Mateusz Gozdek <notifications@github.com> wrote: Oops, component "aws-ebs-csi-driver" {} line was missing in ci/aws/aws-cluster.lokocfg.envsubst. I added it now, I hope you don't mind @BrainBlasted. —You are receiving this because you were mentioned.Reply to this email directly, view it on GitHub, or unsubscribe.

invidian

LGTM

johananl

Thanks a lot for the PR @BrainBlasted. I've added some comments.

johananl · 2020-06-03T10:35:39Z

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/controllers.tf

+resource "aws_iam_role_policy" "csi-driver-role-policy" {
+  count = var.set_csi_driver_iam_role ? 1 : 0
+  name  = "${var.cluster_name}-policy"
+  role  = join("", aws_iam_role.csi-driver-role.*.id)


I think it's safer and more intuitive to do role = aws_iam_role.csi-driver-role.[count.index].id.

This won't work, because the resource aws_iam_role.csi-driver-role won't exist. Also, if done like this, you will look up on index 1, so 2nd instance of the resource. Or does your syntax magically handles it somehow?

As mentioned, count.index does not work here if the roles aren't set.

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/controllers.tf

johananl · 2020-06-03T10:42:58Z

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/controllers.tf

-  user_data = data.ct_config.controller-ignitions[count.index].rendered
+  ami                  = local.ami_id
+  user_data            = data.ct_config.controller-ignitions[count.index].rendered
+  iam_instance_profile = join("", aws_iam_instance_profile.csi-driver-instance-profile.*.name)


I would use aws_iam_instance_profile.csi-driver-instance-profile.[0].name. There can't be more than one instance profile so join doesn't make sense to me here.

This suggestion doesn't work when set_csi_driver_iam_role = false

docs/configuration-reference/components/aws-ebs-csi-driver.md

pkg/components/aws-ebs-csi-driver/component_test.go

invidian

I found few more nits, which we could improve, though nothing really blocking.

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/controllers.tf

pkg/platform/aws/template.go

ci/aws/aws-cluster.lokocfg.envsubst

invidian · 2020-06-08T07:56:43Z

pkg/components/aws-ebs-csi-driver/component.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package awsebscsidriver


@johananl we don't do that for any other components, so maybe to avoid introducing inconsistencies, we can leave it as it is for now to move forward with this PR and we re-visit it later for all components?

invidian · 2020-06-08T08:01:27Z

pkg/components/aws-ebs-csi-driver/component.go

+	return gohcl.DecodeBody(*configBody, evalContext, c)
+}
+
+// RenderManifests renders the helm chart templates with values provided.


I suspect @johananl meant https://helm.sh/docs/chart_best_practices/conventions/, even though even https://helm.sh does not respect it, as they often use Helm Charts term.

invidian

LGTM. @johananl can you have a look as well please?

iaguis

Some small comments. Good work!

Can you rework your commit messages so they follow our guidelines, right now lines are too long.

The only question I have is whether we should set enable_csi to true by default. I understand we want to make this the default behavior. However, since we need to install the component anyway to have this working it might be fine to have it false by default. When we have a way to install this automatically on AWS we might revisit this to make it work for every AWS cluster.

Happy to hear some other opinions.

iaguis · 2020-06-09T13:19:29Z

docs/configuration-reference/components/aws-ebs-csi-driver.md

+## Introduction
+
+The [CSI Driver for Amazon EBS](https://github.com/kubernetes-sigs/aws-ebs-csi-driver)
+provides a CSI interface used by Container Orchestrators to manage the lifecycle


I think he means this

Suggested change

provides a CSI interface used by Container Orchestrators to manage the lifecycle

provides a CSI interface used by container orchestrators to manage the lifecycle

iaguis · 2020-06-09T13:20:39Z

pkg/components/aws-ebs-csi-driver/component.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package awsebscsidriver


Let's revisit this later.

docs/configuration-reference/components/aws-ebs-csi-driver.md

iaguis · 2020-06-09T13:23:28Z

pkg/components/aws-ebs-csi-driver/component.go

+	return gohcl.DecodeBody(*configBody, evalContext, c)
+}
+
+// RenderManifests renders the helm chart templates with values provided.


I'd say

Suggested change

// RenderManifests renders the helm chart templates with values provided.

// RenderManifests renders the Helm chart templates with values provided.

docs/configuration-reference/components/aws-ebs-csi-driver.md

invidian · 2020-06-09T16:07:17Z

The only question I have is whether we should set enable_csi to true by default. I understand we want to make this the default behavior. However, since we need to install the component anyway to have this working it might be fine to have it false by default. When we have a way to install this automatically on AWS we might revisit this to make it work for every AWS cluster.

Having it false by default now and changing it to true in the future seems reasonable to me.

Optionally sets up an IAM role that is required for dynamic volume provisioning to work on AWS. Related to #379

Creates a simple component for aws-ebs-csi-driver, which is required for dynamic volume provisioning to work on AWS. Related to #379

BrainBlasted · 2020-06-09T19:17:10Z

Re-worked the commits to follow the guidelines. I think it's fine to leave enable_csi as false by default as well for now.

iaguis

LGTM

BrainBlasted requested review from iaguis, invidian and johananl May 12, 2020 21:44

iaguis suggested changes May 13, 2020

View reviewed changes

BrainBlasted force-pushed the cdavis/dynamic-pvcs-aws branch from 7440384 to 6480e33 Compare May 14, 2020 21:03

BrainBlasted requested review from ipochi and surajssd as code owners May 19, 2020 02:21

BrainBlasted force-pushed the cdavis/dynamic-pvcs-aws branch 2 times, most recently from 2440fda to 4dc2cee Compare May 20, 2020 20:27

BrainBlasted force-pushed the cdavis/dynamic-pvcs-aws branch 2 times, most recently from ad0721b to a559e06 Compare May 22, 2020 22:52

invidian suggested changes May 25, 2020

View reviewed changes

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/controllers.tf Outdated Show resolved Hide resolved

assets/lokomotive-kubernetes/aws/flatcar-linux/kubernetes/variables.tf Outdated Show resolved Hide resolved

pkg/platform/aws/aws.go Outdated Show resolved Hide resolved

invidian reviewed May 25, 2020

View reviewed changes

pkg/platform/aws/template.go Outdated Show resolved Hide resolved

BrainBlasted force-pushed the cdavis/dynamic-pvcs-aws branch 2 times, most recently from 07861c7 to 3ac4fe2 Compare May 25, 2020 09:03

BrainBlasted changed the title ~~WIP: Set up dynamic provisioning of persistent volumes on AWS~~ Set up dynamic provisioning of persistent volumes on AWS May 25, 2020

BrainBlasted requested review from invidian and iaguis May 25, 2020 09:07

invidian suggested changes May 25, 2020

View reviewed changes

BrainBlasted force-pushed the cdavis/dynamic-pvcs-aws branch from 3ac4fe2 to 1bc6e1c Compare May 26, 2020 17:33

BrainBlasted requested a review from pothos as a code owner June 2, 2020 22:14

BrainBlasted requested a review from invidian June 2, 2020 22:59

invidian force-pushed the cdavis/dynamic-pvcs-aws branch from a8011b9 to 6b5eee6 Compare June 3, 2020 08:15

invidian previously approved these changes Jun 3, 2020

View reviewed changes

johananl reviewed Jun 3, 2020

View reviewed changes

BrainBlasted dismissed invidian’s stale review via 7dba4fa June 5, 2020 22:46

BrainBlasted force-pushed the cdavis/dynamic-pvcs-aws branch 3 times, most recently from 75297e3 to 6ecb4b4 Compare June 5, 2020 23:17

BrainBlasted requested a review from johananl June 5, 2020 23:17

invidian suggested changes Jun 8, 2020

View reviewed changes

BrainBlasted force-pushed the cdavis/dynamic-pvcs-aws branch 3 times, most recently from 3d6abaa to 8aa2b93 Compare June 9, 2020 01:36

invidian previously approved these changes Jun 9, 2020

View reviewed changes

iaguis suggested changes Jun 9, 2020

View reviewed changes

BrainBlasted added 2 commits June 9, 2020 12:13

aws/controllers: Create IAM role for dynamic volume provisioning support

a80fc42

Optionally sets up an IAM role that is required for dynamic volume provisioning to work on AWS. Related to #379

components: Create component for aws-ebs-csi-driver

7284bfa

Creates a simple component for aws-ebs-csi-driver, which is required for dynamic volume provisioning to work on AWS. Related to #379

BrainBlasted dismissed invidian’s stale review via 7284bfa June 9, 2020 19:14

BrainBlasted force-pushed the cdavis/dynamic-pvcs-aws branch from 8aa2b93 to 7284bfa Compare June 9, 2020 19:14

invidian approved these changes Jun 9, 2020

View reviewed changes

iaguis approved these changes Jun 10, 2020

View reviewed changes

iaguis merged commit a1c91a1 into master Jun 10, 2020

iaguis deleted the cdavis/dynamic-pvcs-aws branch June 10, 2020 12:40

iaguis mentioned this pull request Jun 10, 2020

Support Dynamic Provisioning of PersistentVolumes on AWS #379

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set up dynamic provisioning of persistent volumes on AWS #423

Set up dynamic provisioning of persistent volumes on AWS #423

BrainBlasted commented May 12, 2020

BrainBlasted commented May 12, 2020

iaguis left a comment

invidian commented May 13, 2020

iaguis commented May 20, 2020

invidian commented May 20, 2020

invidian commented May 20, 2020

johananl commented May 20, 2020

BrainBlasted commented May 21, 2020

BrainBlasted commented May 22, 2020

BrainBlasted commented May 22, 2020

invidian left a comment •

edited

Loading

BrainBlasted commented May 25, 2020

BrainBlasted commented Jun 2, 2020

invidian commented Jun 3, 2020

BrainBlasted commented Jun 3, 2020 via email

invidian left a comment

johananl left a comment

johananl Jun 3, 2020

invidian Jun 3, 2020

BrainBlasted Jun 5, 2020

johananl Jun 3, 2020

BrainBlasted Jun 5, 2020

invidian left a comment

invidian Jun 8, 2020

invidian Jun 8, 2020

invidian left a comment

iaguis left a comment •

edited

Loading

iaguis Jun 9, 2020

iaguis Jun 9, 2020

iaguis Jun 9, 2020

invidian commented Jun 9, 2020

BrainBlasted commented Jun 9, 2020

iaguis left a comment

	provides a CSI interface used by Container Orchestrators to manage the lifecycle
	provides a CSI interface used by container orchestrators to manage the lifecycle

	// RenderManifests renders the helm chart templates with values provided.
	// RenderManifests renders the Helm chart templates with values provided.

Set up dynamic provisioning of persistent volumes on AWS #423

Set up dynamic provisioning of persistent volumes on AWS #423

Conversation

BrainBlasted commented May 12, 2020

BrainBlasted commented May 12, 2020

iaguis left a comment

Choose a reason for hiding this comment

invidian commented May 13, 2020

iaguis commented May 20, 2020

invidian commented May 20, 2020

invidian commented May 20, 2020

johananl commented May 20, 2020

BrainBlasted commented May 21, 2020

BrainBlasted commented May 22, 2020

BrainBlasted commented May 22, 2020

invidian left a comment • edited Loading

Choose a reason for hiding this comment

BrainBlasted commented May 25, 2020

BrainBlasted commented Jun 2, 2020

invidian commented Jun 3, 2020

BrainBlasted commented Jun 3, 2020 via email

invidian left a comment

Choose a reason for hiding this comment

johananl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian left a comment

Choose a reason for hiding this comment

iaguis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

invidian commented Jun 9, 2020

BrainBlasted commented Jun 9, 2020

iaguis left a comment

Choose a reason for hiding this comment

invidian left a comment •

edited

Loading

iaguis left a comment •

edited

Loading