-
Notifications
You must be signed in to change notification settings - Fork 810
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
Refactor E2E: Streamline setting of CreateVolume Parameters in test-cases. #1845
Conversation
Code Coverage DiffThis PR does not change the code coverage |
e1c9e91
to
330fcdf
Compare
@@ -82,6 +82,9 @@ const ( | |||
// BlockExpressKey increases the iops limit for io2 volumes to the block express limit | |||
BlockExpressKey = "blockexpress" | |||
|
|||
// FSTypeKey configures the file system type that will be formatted during volume creation. | |||
FSTypeKey = "csi.storage.k8s.io/fstype" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this isn't used in the driver itself, it should be in the e2e package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should use it in the driver here:
aws-ebs-csi-driver/pkg/driver/controller.go
Line 160 in 61c721b
klog.InfoS("\"fstype\" is deprecated, please use \"csi.storage.k8s.io/fstype\" instead") |
If all other parameter keys mentioned in our docs are in constants.go
I think that FSTypeKey belongs there as well. Especially because if one day we do use this key in the driver for something more substantial, we would have a duplicated constant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Refactor E2E: Remove hidden performance parameter setting Fixup E2E: Pre-Provisioned volumes don't use VolumeType Fixup E2E: Pre-Provisioned defaultVolumeType typo Refactor E2E: Standardize adding CreateVolume parameters to testcases Refactor E2E: Generalize format_options CreateVolumeDetails helper fcn.
e4e429a
to
73547e1
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: torredil 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 |
Is this a bug fix or adding new feature?
Refactor
What is this PR about? / Why do we need it?
The aws-ebs-csi-driver's E2E tests currently rely on test-specific helper-function behavior and kitchen-sink wrapper-structs (See the git-blame for the VolumeDetails). While test-specific extra helper-function-logic and wrapper-struct-fields helped increase the driver's test-coverage sooner, they negatively impact the discoverability and readability of these utility functions when writing more e2e tests.
This PR begins to standardize these disparate ways of writing e2e tests by having any test that dynamically provisions volumes rely on explicitly setting CreateVolume (StorageClass) Parameters within the test-cases.
These commits are bundled into one PR to simplify code review + CI (Prow tests + Merging).
The purposes of each commit are:
iops
,iopsPerGb
, orthroughput
based on thevolumeType
passed into aVolumeDetails
struct. Worse, E2E packages should not be responsible for maintaining a source of truth for EBS limits like maximum throughput for each volume type. Instead, unless E2E tests are explicitly testing these performance parameters, their default should be left to the EC2 API (via the driver) or be explicitly set in test-case parameters via constants likeDefaultE2EIopsIoVolumes
.VolumeType
,FSType
, andEncrypted
are CreateVolume parameters and should not be treated differently from parameters likeiops
andblockSize
when creating tests.What testing is done?
All E2E tests passed locally after each commit.