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 gp3 storageclass #2941

Closed
wants to merge 1 commit into from
Closed

Add gp3 storageclass #2941

wants to merge 1 commit into from

Conversation

yuvipanda
Copy link
Member

We add this unconditionally to all clusters for simplification, so we can set storageClass: gp3 for new clusters that come up on AWS without issue. This doesn't change the default, and does not change the storageclass in existing clusters. In addition to using gp3, it also sets reclaimPolicy to Retain, so if the PVC is deleted, it does not delete the PV or the underlying EBS volume.

Ref #2906
Ref #2717

We add this unconditionally to all clusters for simplification,
so we can set storageClass: gp3 for new clusters that come up on
AWS without issue. This doesn't change the default, and does not
change the storageclass in existing clusters. In addition to using
gp3, it also sets reclaimPolicy to Retain, so if the PVC is deleted,
it does not delete the PV or the underlying EBS volume.

Ref 2i2c-org#2906
Ref 2i2c-org#2717
@github-actions
Copy link

github-actions bot commented Aug 7, 2023

Merging this PR will trigger the following deployment actions.

Support and Staging deployments

Cloud Provider Cluster Name Upgrade Support? Reason for Support Redeploy Upgrade Staging? Reason for Staging Redeploy
gcp leap Yes Support helm chart has been modified No
gcp cloudbank Yes Support helm chart has been modified No
aws nasa-cryo Yes Support helm chart has been modified No
gcp meom-ige Yes Support helm chart has been modified No
gcp catalystproject-latam Yes Support helm chart has been modified No
gcp callysto Yes Support helm chart has been modified No
aws gridsst Yes Support helm chart has been modified No
gcp m2lines Yes Support helm chart has been modified No
gcp pangeo-hubs Yes Support helm chart has been modified No
gcp 2i2c Yes Support helm chart has been modified No
aws smithsonian Yes Support helm chart has been modified No
aws victor Yes Support helm chart has been modified No
aws nasa-veda Yes Support helm chart has been modified No
aws catalystproject-africa Yes Support helm chart has been modified No
kubeconfig utoronto Yes Support helm chart has been modified No
gcp awi-ciroh Yes Support helm chart has been modified No
aws openscapes Yes Support helm chart has been modified No
aws 2i2c-aws-us Yes Support helm chart has been modified No
gcp qcl Yes Support helm chart has been modified No
aws jupyter-meets-the-earth Yes Support helm chart has been modified No
gcp linked-earth Yes Support helm chart has been modified No
aws ubc-eoas Yes Support helm chart has been modified No
gcp 2i2c-uk Yes Support helm chart has been modified No
aws carbonplan Yes Support helm chart has been modified No

Production deployments

No production hub upgrades will be triggered

@@ -0,0 +1,18 @@
apiVersion: storage.k8s.io/v1
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
apiVersion: storage.k8s.io/v1
# By default EKS clusters doesn't provide a StorageClass for gp3 based storage, that is
# reported to be more cost efficient than gp2 based storage. Due to that we install a gp3
# StorageClass that can be explicitly referenced in AWS clusters.
apiVersion: storage.k8s.io/v1

kind: StorageClass
metadata:
name: gp3
# Use the AWS EBS CSI provisioner
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Use the AWS EBS CSI provisioner

Comment on lines +1 to +18
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
name: gp3
# Use the AWS EBS CSI provisioner
provisioner: ebs.csi.aws.com
parameters:
# gp3 is cheaper than gp2
type: gp3
fsType: ext4
# Do not delete the underlying EBS volume whenever this PV & PVC
# are deleted! This allows us recovery in case of accidental deletion
reclaimPolicy: Retain
# Create the EBS volume only when the pod consuming it tries to use it.
# This ensures the EBS volume is created in the correct *zone*, so we
# don't end up with issues where the volume is in a different zone (but
# same region) as our pods.
volumeBindingMode: WaitForFirstConsumer
Copy link
Contributor

Choose a reason for hiding this comment

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

Provisioner value?

This is from a gp2 StorageClass resource in a recent AWS cluster (catalystproject-africa), and it looked the same in the older 2i2c-aws-us cluster.

allowVolumeExpansion: true
apiVersion: storage.k8s.io/v1
kind: StorageClass
metadata:
    storageclass.kubernetes.io/is-default-class: "true"
  name: gp2
parameters:
  fsType: ext4
  type: gp2
provisioner: kubernetes.io/aws-ebs
reclaimPolicy: Delete
volumeBindingMode: WaitForFirstConsumer

Note that provisioner is set to something else than proposed in this PR. Should we go for whats observed in our clusters?

Default storage class?

Reading up about StorageClass defaults, it seems that the annotation controls it, and having two with the same annotation is not good. It would be good if we could have this a default storage class, but it seems out of control for the Helm chart to make it so.

I guess we can't make it the default without also having a pre-setup step for new clusters to make the gp2 StorageClass no longer the default storage class. So, let's not think about this now.

Installable in GKE/AKS?

Is it okay to create a storageclass that references a provisioner that isn't available? Yes maybe, but its also a bit confusing that we create a resource of no relevance.

Can we make this install only in AWS/EKS based clusters?

Copy link
Member Author

Choose a reason for hiding this comment

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

provisioner: kubernetes.io/aws-ebs is deprecated since 1.17 - see https://aws.amazon.com/blogs/containers/amazon-ebs-csi-driver-is-now-generally-available-in-amazon-eks-add-ons/, search for the word 'deprecated'. I'm annoyed EKS still ships with that.

I agree that we should not make this the default, don't want to fiddle with that.

Let me think about the optionality question. We have been shipping a GKE SSD provisioner (with a deprecated provisioner even!) for all our clusters apparently.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, making this not be present by default but explicitly be set is the right thing to do! I'll do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, making this not be present by default but explicitly be set is the right thing to do! I'll do that.

I think making the choice to install a StorageClass resource explicit is a great!

@yuvipanda
Copy link
Member Author

Going to close this in favor of opening something along the lines of #3112 later for AWS, once I get the GCP stuff done.

@yuvipanda yuvipanda closed this Sep 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done 🎉
Development

Successfully merging this pull request may close these issues.

2 participants