-
Notifications
You must be signed in to change notification settings - Fork 39.4k
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 for mismatched usage of filesystem/block volumes #79796
Conversation
470d221
to
7b07cb4
Compare
Removed WIP, the test is already useful. It's only slow because there is no fast way how to detect that a pod can't be started because of mismatched volume mode. It's related to #74545. |
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.
Thank you for adding tests for block volmue.
It almost looks good to me. Let me leave some minor comments that won't be necessary to be changed.
7b07cb4
to
c0f1e97
Compare
c0f1e97
to
9f5d629
Compare
/retest |
/retest |
2a42830
to
764ebe3
Compare
} | ||
err = e2epod.WaitTimeoutForPodEvent(l.cs, pod.Name, l.ns.Name, eventSelector, msg, framework.PodStartTimeout) | ||
// Events are unreliable, don't depend on the event. It's used only to speed up the test. | ||
if err != nil { |
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.
Very good idea to speed up tests without any risks of flake! Let's apply similar way to other places to speed up tests later in another commit.
|
||
// Check the pod is still not running | ||
p, err := l.cs.CoreV1().Pods(l.ns.Name).Get(pod.Name, metav1.GetOptions{}) | ||
framework.ExpectError(err, "could not re-read the pod after event (or timeout)") |
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.
It should be ExpectNoError
not ExpectError
, then all tests should pass.
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.
good catch :-)
|
||
var msg string | ||
if pattern.VolMode == v1.PersistentVolumeBlock { | ||
msg = fmt.Sprintf("has volumeMode Block, but is specified in volumeMounts") |
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.
fmt.Sprintf
is not needed here and the one in else
clause.
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.
fixed
// Check the pod is still not running | ||
p, err := l.cs.CoreV1().Pods(l.ns.Name).Get(pod.Name, metav1.GetOptions{}) | ||
framework.ExpectError(err, "could not re-read the pod after event (or timeout)") | ||
gomega.Expect(p.Status.Phase).To(gomega.Equal(v1.PodPending)) |
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.
It would be better to replace with framework.ExpectEqual(p.Status.Phase, v1.PodPending
...
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.
fixed
dd4e319
to
40b1867
Compare
/test pull-kubernetes-e2e-gce-csi-serial |
It looks good to me now. Could you also take a look just in case? |
/assign |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jsafrane, msau42 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 |
/retest Review the full test history for this PR. Silence the bot with an |
7 similar comments
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/retest Review the full test history for this PR. Silence the bot with an |
/kind feature
/sig storage
What this PR does / why we need it:
This test checks that a block PV cannot be used as
volumeMount
in a pod and a filesystem PV can't be used asdeviceMount
.Which issue(s) this PR fixes:
Part of #74545
Does this PR introduce a user-facing change?: