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 CSI driver for AWS EBS #1597

Merged
merged 4 commits into from
Nov 2, 2021
Merged

Conversation

embik
Copy link
Member

@embik embik commented Oct 27, 2021

What this PR does / why we need it:
Adds the CSI driver for AWS EBS volumes to kubeone, offering an out-of-tree alternative to the built-in driver.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #1565

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Support for AWS EBS via CSI driver added

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
@kubermatic-bot kubermatic-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. labels Oct 27, 2021
@kubermatic-bot
Copy link
Contributor

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@kubermatic-bot kubermatic-bot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Oct 27, 2021
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
@embik embik changed the title WIP: Add CSI driver for AWS EBS (block storage) Add CSI driver for AWS EBS (block storage) Oct 28, 2021
@embik embik marked this pull request as ready for review October 28, 2021 07:32
@kubermatic-bot kubermatic-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 28, 2021
@embik embik changed the title Add CSI driver for AWS EBS (block storage) Add CSI driver for AWS EBS Oct 28, 2021
@embik embik requested a review from xmudrii October 28, 2021 08:23
@embik
Copy link
Member Author

embik commented Oct 28, 2021

/assign @xmudrii

I don't understand the failing test case tbh. It seems to be related to YAML processing, which this PR doesn't touch as far as I can say.

@kron4eg
Copy link
Member

kron4eg commented Oct 28, 2021

/retest

@kron4eg
Copy link
Member

kron4eg commented Oct 28, 2021

@embik this is a flake fail, it's based on the iteration over the map ordering and of course in Go this is randomized so sometimes we get this error. nothing to worry about.

@xmudrii
Copy link
Member

xmudrii commented Oct 29, 2021

/assign
I’ll take a look and test this on Monday.

@xmudrii
Copy link
Member

xmudrii commented Nov 1, 2021

The documentation for the CSI driver says that the csi-snapshotter needs to be installed as a prerequisite if the snapshotting feature is supposed to be used. It's not included in the YAML files published by the CSI driver. Should we embed it into this addon?

That's not all. The docs state that we should also enable a bunch of feature gates on Kubelet and API server. Please take care of those feature gates as well.

Regarding the CSI snapshotter, we deploy it for some other providers, so I think it makes sense to do it for AWS too. Take a look at Azure or vSphere CSI addons as an example.

The resources have been adapted to a) run on the control plane nodes only and b) use the system's identity. I can change this to using the AWS credentials already passed to machine-controller.

I think this is fine.

As a side note, the problem with hostnames for the worker nodes will be resolved by kubermatic/machine-controller#1087.

@embik
Copy link
Member Author

embik commented Nov 2, 2021

@xmudrii the feature gates listed there should be all enabled by default in the Kubernetes versions we support. I assume the documentation lists them for completeness' sake or hasn't been updated:

https://kubernetes.io/docs/reference/command-line-tools-reference/feature-gates/

  • CSINodeInfo enabled by default since 1.14 as Beta (GA since 1.17)
  • CSIDriverRegistry enabled by default since 1.14 as Beta (GA since 1.18)
  • CSIBlockVolume enabled by default since 1.14 as Beta (GA since 1.18)
  • VolumeSnapshotDataSource enabled by default since 1.17 as Beta (GA since 1.20)

So I think we don't need to change anything about the feature flags.

Signed-off-by: Marvin Beckers <marvin@kubermatic.com>
@kubermatic-bot kubermatic-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Nov 2, 2021
@kubermatic-bot kubermatic-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 2, 2021
@kubermatic-bot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 9e103a1a4bffeae279076bf08d61e0cd50584288

@xmudrii
Copy link
Member

xmudrii commented Nov 2, 2021

/approve

@kubermatic-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: embik, xmudrii

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

@kubermatic-bot kubermatic-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 2, 2021
@kubermatic-bot kubermatic-bot merged commit 648744e into kubermatic:master Nov 2, 2021
@kubermatic-bot kubermatic-bot added this to the KubeOne 1.4 milestone Nov 2, 2021
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. dco-signoff: yes Denotes that all commits in the pull request have the valid DCO signoff message. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a CSI driver addon for AWS
4 participants