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.

Sign up for GitHub

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

Jump to bottom

Add csi-sanity tests #2254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add csi-sanity tests #2254

wants to merge 1 commit into from

Conversation

ElijahQuinones
Copy link
Member

@ElijahQuinones ElijahQuinones commented Dec 4, 2024 •
edited
Loading

What type of PR is this?

/kind feature

What is this PR about? / Why do we need it?

This PR adds csi-sanity tests to the driver to be enable in CI afterwards.

How was this change tested?

Ran make test-sanity
and received the following results

Ran 76 of 92 Specs in 0.355 seconds
SUCCESS! -- 76 Passed | 0 Failed | 1 Pending | 15 Skipped
--- PASS: TestSanity (0.36s)
PASS
ok  	github.com/kubernetes-sigs/aws-ebs-csi-driver/tests/sanity	1.429s

Does this PR introduce a user-facing change?

Add csi-sanity tests

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Dec 4, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from elijahquinones. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 4, 2024
@ElijahQuinones
Copy link
Member Author

/hold

@k8s-ci-robot 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. labels Dec 4, 2024
@ElijahQuinones ElijahQuinones added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 4, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 4, 2024 •
edited
Loading

Code Coverage Diff

File Old Coverage New Coverage Delta
github.com/kubernetes-sigs/aws-ebs-csi-driver/pkg/driver/controller_modify_volume.go 78.0% 77.4% -0.7

@ElijahQuinones
Copy link
Member Author

/retest

@ElijahQuinones ElijahQuinones force-pushed the sanity branch 2 times, most recently from 1cd6007 to 79b840d Compare December 5, 2024 21:01
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 5, 2024
@ElijahQuinones ElijahQuinones force-pushed the sanity branch 2 times, most recently from e328221 to 508b90e Compare December 6, 2024 22:22
@ElijahQuinones ElijahQuinones removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Dec 6, 2024
AndrewSirenko
AndrewSirenko reviewed Dec 16, 2024
View reviewed changes
tests/sanity/sanity_test.go
IDGen: csisanity.DefaultIDGenerator{},
TestVolumeSize: 10 * util.GiB,
TestVolumeAccessType: "mount",
TestVolumeMutableParameters: map[string]string{"iops": "3014", "throughput": "153"},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we don't also provide a TestVolumeParameters struct, like we provide TestVolumeMutableParameters, TestVolumeAccessType, etc?

Do no sanity tests utilize it?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope I would be happy to add it though if we want to . TestVolumeMutableParameters was explicitly added here as without it we don't succeeded on our modify_volume tests as we quit early when no parameters to modify the volume are specified.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah I would add something, like volumeType: gp3, iops: 3000 to TestVolumeParameters, in case future sanity tests rely on it.

I know there is some discussion of using in kubernetes-csi/csi-test#549

Thanks!

tests/sanity/sanity_test.go
return fakeMounter, fakeCloud, fakeMetadataService, fakeKubernetesClient
}

func mockNodeService(m *mounter.MockMounter, md *metadata.MockMetadataService) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason you prefer relying heavily on mocks within these two mega-mock functions in sanity_test.go (for node/controller service), instead of creating fake implementations similar to mount_unsupported https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/pkg/mounter/mount_unsupported.go?

Perhaps this was was cleaner than the alternative? But the presence of so many m.Expect().<Some mounter function>(gomock.Any()).DoAndReturn(...) piqued my interest.

Copy link
Member Author

Choose a reason for hiding this comment

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

No the only reason I had for doing so was to keep in party with previous CSI Sanity tests Eddie implemented. I would be more than happy to add a fake_sanity_node.go and fake_sanity_controller.go if folks think this is cleaner.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1. Fakes are easier to maintain and lot cleaner.

Copy link
Member Author

Choose a reason for hiding this comment

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

Perfect will do in next rev

Copy link
Contributor

@AndrewSirenko AndrewSirenko Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

No the only reason I had for doing so was to keep in party with previous CSI Sanity tests Eddie implemented

Ah, IIRC poor eddie had to do that because he implemented those tests before his lovely mounter package refactor.

If you do decide to make the fakes, please hide them inside tests/sanity to make sure they are only used here (in line with issue #220)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rdpsin rdpsin rdpsin left review comments

@AndrewSirenko AndrewSirenko AndrewSirenko left review comments

@ConnorJC3 ConnorJC3 Awaiting requested review from ConnorJC3

@torredil torredil Awaiting requested review from torredil

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ElijahQuinones @k8s-ci-robot @AndrewSirenko @rdpsin