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 test_provisioner_generic_ephemeral skeleton #1820

Conversation

ejweber
Copy link
Contributor

@ejweber ejweber commented Mar 18, 2024

Which issue(s) this PR fixes:

longhorn/longhorn#8198

What this PR does / why we need it:

longhorn/longhorn#8198 uncovered the fact that a change had unknowingly broken Longhorn generic ephemeral volumes. We need an automated test to ensure this can't happen in the future.

Special notes for your reviewer:

I wasn't really sure if this skeleton fit best in test_provisioner.py (since we want to test the ability to provision generic ephemeral volumes) or test_kubernetes.py, since the actual change I made in longhorn/longhorn-manager#2701 ensures volume.status.kubernetesStatus.workloadStatus was correctly populated by the Longhorn Kuberntes PV controller. I went with my gut, but I'm fine with moving it if QA thinks it makes better sense elsewhere.

chriscchien
chriscchien previously approved these changes Mar 19, 2024
Copy link
Contributor

@chriscchien chriscchien left a comment

Choose a reason for hiding this comment

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

LGTM

@ejweber ejweber force-pushed the 8198-generic-ephemeral-volume-skeleton branch from b83d897 to 0e50eab Compare March 19, 2024 15:21
@ejweber ejweber force-pushed the 8198-generic-ephemeral-volume-skeleton branch from 0e50eab to f4b70af Compare March 19, 2024 15:22
@ejweber
Copy link
Contributor Author

ejweber commented Mar 19, 2024

Fixed the linting issues.

james-munson
james-munson previously approved these changes Mar 19, 2024
Copy link
Contributor

@james-munson james-munson left a comment

Choose a reason for hiding this comment

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

Looks fine as a placeholder. Is there an associated issue to fill it in?

@ejweber ejweber force-pushed the 8198-generic-ephemeral-volume-skeleton branch from f4b70af to a00efab Compare March 19, 2024 16:09
@ejweber
Copy link
Contributor Author

ejweber commented Mar 19, 2024

Looks fine as a placeholder. Is there an associated issue to fill it in?

It is linked in the issue: longhorn/longhorn#8203.

- The volume parameters match the StorageClass parameters.
- The volume.status.kubernetesStatus.workloadStatus reflects the
running Pod.
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we add a test step attempting to write data in ephemeral volume?

Copy link
Contributor Author

@ejweber ejweber Mar 20, 2024

Choose a reason for hiding this comment

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

It definitely can't hurt, so I added it in an amend. For context, I wrote this skeleton to mirror test_provisioner_mount (directly above in the file), which does not. Importantly, we cannot follow the pattern of some other tests and read the data back from a different pod, because the life cycle of a generic ephemeral volume mirrors the life cycle of the pod it is created with.

Longhorn 8198

Signed-off-by: Eric Weber <[email protected]>
@ejweber ejweber force-pushed the 8198-generic-ephemeral-volume-skeleton branch from a00efab to 566c8ea Compare March 20, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants