The recommended approach for not running unsuitable tests is to skip
them at runtime with an explanation. Filtering out unsuitable test
patters and thus not even defining unsuitable tests was done earlier
because it was faster than skipping tests at runtime.
But now these tests can be skipped efficiently, so this special case
can be removed.
CreateDriver (now called SetupTest) is a potentially expensive
operation, depending on the driver. Creating and tearing down a
framework instance also takes time (measured at 6 seconds on a fast
machine) and produces quite a bit of log output.
Both can be avoided for tests that skip based on static
information (like for instance the current OS, vendor, driver and test
pattern) by making the test suite responsible for creating framework
and driver.
The lifecycle of the TestConfig instance was confusing because it was
stored inside the DriverInfo, a struct which conceptually is static,
while the TestConfig is dynamic. It is cleaner to separate the two,
even if that means that an additional pointer must be passed into some
functions. Now CreateDriver is responsible for initializing the
PerTestConfig that is to be used by the test.
To make this approach simpler to implement (= less functions which
need the pointer) and the tests easier to read, the entire setup and
test definition is now contained in a single function. This is how it
is normally done in Ginkgo. This is easier to read because one can see
at a glance where variables are set, instead of having to trace values
though two additional structs (TestResource and TestInput).
Because we are changing the API already, also other changes are made:
- some function prototypes get simplified
- the naming of functions is changed to match their purpose
(tests aren't executed by the test suite, they only get defined
for later execution)
- unused methods get removed (TestSuite.skipUnsupportedTest is redundant)
TestDynamicProvisioning had multiple ways of choosing additional
checks:
- the PvCheck callback
- the builtin write/read check controlled by a boolean
- the snapshot testing
Complicating matters further, that builtin write/read test had been
more customizable with new fields `NodeSelector` and
`ExpectUnschedulable` which were only set by one particular test (see
https://github.com/kubernetes/kubernetes/pull/70941).
That is confusing and will only get more confusing when adding more
checks in the future. Therefore the write/read check is now a separate
function that must be enabled explicitly by tests that want to run it.
The snapshot checking is also defined only for the snapshot test.
The test that expects unschedulable pods now also checks for that
particular situation itself. Instead of testing it with two pods (the
behavior from the write/read check) that both fail to start, only a
single unschedulable pod is created.
Because node name, node selector and the `ExpectUnschedulable` were
only used for checking, it is possible to simplify `StorageClassTest`
by removing all of these fields.
Expect(err).NotTo(HaveOccurred()) is an anti-pattern in Ginkgo testing
because a test failure doesn't explain what failed (see
https://github.com/kubernetes/kubernetes/issues/34059). We avoid it
now by making the check function itself responsible for checking
errors and including more information in those checks.
PR #70862 made each driver responsible for resetting its config, but
as it turned out, one place was missed in that PR: the in-tree gcepd
sets a node selector. Not resetting that caused other tests to fail
randomly depending on test execution order.
Now the test suite resets the config by taking a copy after setting up
the driver and restoring that copy before each test.
Long term the intention is to separate the entire test config from the
static driver info (https://github.com/kubernetes/kubernetes/issues/72288),
but for now resetting the config is the fastest way to fix the test flake.
Fixes: #72378
Exposing framework.VolumeTestConfig as part of the testsuite package
API was confusing because it was unclear which of the values in it
really have an effect. How it was set also was a bit awkward: a test
driver had a copy that had to be overwritten at test runtime and then
might have been updated and/or overwritten again by the driver.
Now testsuites has its own test config structure. It contains the
values that might have to be set dynamically at runtime. Instead of
overwriting a copy of that struct inside the test driver, the test
driver takes some common defaults (specifically, the framework pointer
and the prefix) when it gets initialized and then manages its own
copy. For example, the hostpath driver has to lock the pods to a
single node.
framework.VolumeTestConfig is still used internally and test drivers
can decide to run tests with a fully populated instance if needed (for
example, after setting up an NFS server).
This makes it possible to use the testsuites package out-of-tree
without pulling in unnecessary dependencies and code (in
test/e2e/storage/vsphere) that defines tests that are not wanted in a
custom test suite.
The setup of the V0 hostpath driver was done with copy-and-paste and
then changing just the driver name and the manifests. The same can be
achieved by making the base struct a bit more configurable, which
simplifies future changes (less code).
Renaming the provisioner container was unnecessary and was reverted to
make it possible to use the same patch configuration.
While at it, also fix the InitHostV0PathCSIDriver typo.
Debugging the CSI driver tests depends a lot on the output of the CSI
sidecar containers and the CSI driver, but that information was not
captured automatically and thus unavailable after a test run. This is
particularly bad when running in a remote CI system, but also manually
watching the cluster during a test was cumbersome.
Now pod events and log messages get copied to the test's output at the
time that they happen (when running without report directory) or get
written to individual log files (when running with report directory in
the CI).
Ensuring that CSI drivers get deployed for testing exactly as intended
was problematic because the original .yaml files had to be converted
into code. e2e/manifest helped a bit, but not enough:
- could not load all entities
- didn't handle loading .yaml files with multiple entities
- actually creating and deleting entities still had to be done in tests
The new framework utility code handles all of that, including the
tricky cleanup operation that tests got wrong (AfterEach does not get
called after test failures!).
In addition, it is ensuring that each test gets its own instance of the
entities.
The PSP role binding for hostpath is now necessary because we switch
from creating a pod directly to creation via the StatefulSet
controller, which runs with less privileges.
Without this, the hostpath test runs into these errors in the
kubernetes-e2e-gce job:
Oct 19 16:30:09.225: INFO: At 2018-10-19 16:25:07 +0000 UTC - event for csi-hostpath-attacher: {statefulset-controller } FailedCreate: create Pod csi-hostpath-attacher-0 in StatefulSet csi-hostpath-attacher failed error: pods "csi-hostpath-attacher-0" is forbidden: unable to validate against any pod security policy: []
Oct 19 16:30:09.225: INFO: At 2018-10-19 16:25:07 +0000 UTC - event for csi-hostpath-provisioner: {statefulset-controller } FailedCreate: create Pod csi-hostpath-provisioner-0 in StatefulSet csi-hostpath-provisioner failed error: pods "csi-hostpath-provisioner-0" is forbidden: unable to validate against any pod security policy: []
Oct 19 16:30:09.225: INFO: At 2018-10-19 16:25:07 +0000 UTC - event for csi-hostpathplugin: {daemonset-controller } FailedCreate: Error creating: pods "csi-hostpathplugin-" is forbidden: unable to validate against any pod security policy: []
The extra role binding is silently ignored on clusters which don't
have this particular role.
Jan Safranek