Enable EBS CSI driver for snow device #1314

jigisha620 · 2022-07-18T18:27:19Z

linux-foundation-easycla · 2022-07-18T18:27:22Z

The committers listed above are authorized under a signed CLA.

✅ login: jigisha620 (cbe75bb, 05a9a75)

k8s-ci-robot · 2022-07-18T18:27:27Z

Welcome @jigisha620!

It looks like this is your first PR to kubernetes-sigs/aws-ebs-csi-driver 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/aws-ebs-csi-driver has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2022-07-18T18:27:27Z

Hi @jigisha620. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

gtxu · 2022-07-18T18:47:30Z

/ok-to-test

gtxu · 2022-07-25T15:26:25Z

/hold

jigisha620 · 2022-07-28T22:24:14Z

Updating PR with changes that were discussed during the meeting.

wmesard · 2022-08-02T14:25:33Z

pkg/cloud/metadata_test.go

 					t.Errorf("NewMetadataService() failed: got wrong region %v, expected %v", m.GetRegion(), stdRegion)
 				}
-				if m.GetAvailabilityZone() != stdAvailabilityZone {
+				if m.GetAvailabilityZone() != stdAvailabilityZone && m.GetAvailabilityZone() != snowRegion {


Let's not conflate the notion of region and zone. Use separate constants for each. (Even in tests.)

Ack, made changes.

wmesard · 2022-08-02T14:29:22Z

pkg/driver/node.go

@@ -695,6 +698,10 @@ func (d *nodeService) getVolumesLimit() int64 {
 		return d.driverOptions.volumeAttachLimit
 	}

+	if d.metadata.GetRegion() == "snow" {


Let's avoid hardcoded inline constants.
Let's also avoid the assumption that every SBE's region name will be the same. (Five years from now, they may introduce an SBE device whose metadata says it is "snow-with-laser-pistols" or something. Let's define a function called isSBE(region string) bool (or something like that) to futureproofs against this.

Ack, made changes.

wmesard · 2022-08-02T14:39:05Z

pkg/driver/node.go

@@ -695,6 +698,10 @@ func (d *nodeService) getVolumesLimit() int64 {
 		return d.driverOptions.volumeAttachLimit
 	}

+	if d.metadata.GetRegion() == "snow" {
+		return 10


Why is this stuff being special-cased up here? Wouldn't it be cleaner and more maintainable to teach /pkg/cloud about SBE devices?

Did not change, as per our discussion.

wmesard · 2022-08-02T15:00:42Z

pkg/driver/node_test.go

@@ -1924,6 +1925,7 @@ func TestNodeGetInfo(t *testing.T) {
 			instanceID:        "i-123456789abcdef01",
 			instanceType:      "t2.medium",
 			availabilityZone:  "us-west-2b",
+			region:            "us-west-2b",


"us-west-2" is a region. "us-west-2b" is not.

Ack, made changes.

pkg/cloud/metadata_ec2.go

pkg/cloud/metadata_k8s.go

torredil · 2022-08-05T15:15:40Z

Looking real good now, this PR has come a long way. One last quick request: please squash the commits. Thanks for your contribution! :)

rdpsin · 2022-08-05T18:14:45Z

pkg/cloud/metadata_ec2.go

 	}

 	if len(doc.AvailabilityZone) == 0 {
-		return nil, fmt.Errorf("could not get valid EC2 availability zone")
+		if len(regionFromSession) != 0 && util.IsSBE(regionFromSession) {
+			doc.AvailabilityZone = regionFromSession


Why are we putting region name into AZ? Is there no way to determine the AZ?

The availability zone for SBE is same as the region and there is no other way to determine it since snow metadata service doesn't provide any information about it.

rdpsin · 2022-08-05T18:18:08Z

pkg/driver/node.go

@@ -714,6 +718,10 @@ func (d *nodeService) getVolumesLimit() int64 {
 		return d.driverOptions.volumeAttachLimit
 	}

+	if util.IsSBE(d.metadata.GetRegion()) {
+		return 10


Can you store this 10 as a constant up top? Something like

sbeDeviceVolumeAttachmentLimit = 10

Ack, made changes.

rdpsin · 2022-08-05T18:20:01Z

pkg/cloud/cloud.go

@@ -51,6 +51,10 @@ const (
 	VolumeTypeSC1 = "sc1"
 	// VolumeTypeST1 represents a throughput-optimized HDD type of volume.
 	VolumeTypeST1 = "st1"
+	// VolumeTypeSBG1 represents a capacity-optimized HDD type of volume.


np: Can you add something like (only for SBE devices) to the comments, to make it easier for readers to know why we're adding these types.

Ack, made changes.

rdpsin · 2022-08-05T18:21:16Z

pkg/cloud/cloud.go

+		Encrypted:        aws.Bool(diskOptions.Encrypted),
+	}
+
+	if !util.IsSBE(zone) {


What's the CreateVolume behavior if tags are provided for SBE volumes?

It fails with error that input is malformed.

rdpsin · 2022-08-05T18:22:25Z

pkg/cloud/cloud.go

+		}
+		_, err := c.ec2.CreateTagsWithContext(ctx, requestTagsInput)
+		if err != nil {
+			return nil, fmt.Errorf("could not attach tags to volume: %v. %v", volumeID, err)


The volume has already been created. You will need to delete the volume here, otherwise you will leak resources.

Ack, made changes.

gtxu · 2022-08-05T19:26:09Z

Restart the failed test.

Changes to extract snow device path from Block device mappings (#2) * Updating metadata service to recognize snow region * fixing snow device name search for static and dynamic provisioning * fixing inconsistent vendoring * inconsistent vendoring fix * Update Chart.yaml * Update Chart.yaml * Adding logs to find issue with static provisioning * Updated node_linux.go * Updated node.go * testing lsblk * Adding implementation for lsblk * Changing metadata_k8s for temporary testing * Updated node_linux.go * Updated node_linux.go * changing lsblk path * Updated node_linux.go * Updated node_linux.go * Updated node_linux.go * Updated node_linux.go * Updated node_linux.go * Updated Dockerfile to point to Amazon Linux Image * Testing lsblk implementation * Updated node_linux.go * node_linux.go * Updated node_linux.go * Updated node_linux.go * Update node_linux.go * reverting temp changes made to metadata_k8s.go * Update node_linux.go * Updated base image * Updated metadata_k8s.go * Updated Chaart.yaml * Updated helm and testing change from upstream branch * Testing changes fetched from upstream branch * reverting repository changes * Update Chart.yaml * Adding logs to test failure for harbor * Updated mount.go * Adding changes for device name for snow * Updated node_linux.go * Removing dell csi dependency * Fixing unit tests * Update node_linux_test.go * Updated node_linux_test.go * Fixing test cases * Fixed failing tests for snow * Removing temporary changes from chart * Updated node.yaml Co-authored-by: Patil <patixjig@bcd0741b7bce.ant.amazon.com> Reverting kubernetes api version update Changes to resolve review comments Changes as per review comments Incorporating review comments

torredil · 2022-08-05T20:47:33Z

/lgtm

jigisha620 · 2022-08-10T21:25:19Z

/assign @gtxu

gtxu · 2022-08-12T16:36:51Z

/lgtm

k8s-ci-robot · 2022-08-12T16:38:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gtxu, jigisha620

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~~ [gtxu]

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

torredil · 2022-08-12T16:49:45Z

/unhold

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 18, 2022

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 18, 2022

k8s-ci-robot requested review from rdpsin and torredil July 18, 2022 18:27

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 18, 2022

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 Jul 18, 2022

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 25, 2022

wmesard suggested changes Aug 2, 2022

View reviewed changes

torredil reviewed Aug 5, 2022

View reviewed changes

pkg/cloud/metadata_ec2.go Outdated Show resolved Hide resolved

pkg/cloud/metadata_ec2.go Show resolved Hide resolved

pkg/cloud/metadata_k8s.go Outdated Show resolved Hide resolved

jigisha620 force-pushed the snow branch from 37f127b to 48e6ba2 Compare August 5, 2022 17:49

jigisha620 closed this Aug 5, 2022

jigisha620 reopened this Aug 5, 2022

rdpsin reviewed Aug 5, 2022

View reviewed changes

jigisha620 force-pushed the snow branch from 48e6ba2 to ed527f4 Compare August 5, 2022 19:31

k8s-ci-robot assigned torredil Aug 5, 2022

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2022

k8s-ci-robot assigned gtxu Aug 10, 2022

gtxu approved these changes Aug 12, 2022

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2022

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 12, 2022

k8s-ci-robot merged commit 6022149 into kubernetes-sigs:master Aug 12, 2022

TerryHowe mentioned this pull request Aug 26, 2022

EBS CSI driver for snow - Support for single snow device aws/eks-anywhere-packages#228

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable EBS CSI driver for snow device #1314

Enable EBS CSI driver for snow device #1314

jigisha620 commented Jul 18, 2022

linux-foundation-easycla bot commented Jul 18, 2022 •

edited

Loading

k8s-ci-robot commented Jul 18, 2022

k8s-ci-robot commented Jul 18, 2022

gtxu commented Jul 18, 2022

gtxu commented Jul 25, 2022

jigisha620 commented Jul 28, 2022

wmesard Aug 2, 2022

jigisha620 Aug 3, 2022

wmesard Aug 2, 2022

jigisha620 Aug 3, 2022

wmesard Aug 2, 2022

jigisha620 Aug 3, 2022

wmesard Aug 2, 2022

jigisha620 Aug 3, 2022

torredil commented Aug 5, 2022

rdpsin Aug 5, 2022

jigisha620 Aug 5, 2022

rdpsin Aug 5, 2022

jigisha620 Aug 5, 2022

rdpsin Aug 5, 2022

jigisha620 Aug 5, 2022

rdpsin Aug 5, 2022

jigisha620 Aug 5, 2022

rdpsin Aug 5, 2022

jigisha620 Aug 5, 2022

gtxu commented Aug 5, 2022

torredil commented Aug 5, 2022

jigisha620 commented Aug 10, 2022

gtxu commented Aug 12, 2022

k8s-ci-robot commented Aug 12, 2022

torredil commented Aug 12, 2022

Enable EBS CSI driver for snow device #1314

Enable EBS CSI driver for snow device #1314

Conversation

jigisha620 commented Jul 18, 2022

linux-foundation-easycla bot commented Jul 18, 2022 • edited Loading

k8s-ci-robot commented Jul 18, 2022

k8s-ci-robot commented Jul 18, 2022

gtxu commented Jul 18, 2022

gtxu commented Jul 25, 2022

jigisha620 commented Jul 28, 2022

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

torredil commented Aug 5, 2022

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

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

gtxu commented Aug 5, 2022

torredil commented Aug 5, 2022

jigisha620 commented Aug 10, 2022

gtxu commented Aug 12, 2022

k8s-ci-robot commented Aug 12, 2022

torredil commented Aug 12, 2022

linux-foundation-easycla bot commented Jul 18, 2022 •

edited

Loading