WIP: try with csi-test upgraded to 2.2.0 and change of #393 added #414
Conversation
okartau
changed the title
okartau
force-pushed
the
try-csi-test-220
branch
from
ed09422 to
27978d9
Compare
okartau
changed the title
|
This is a subset of #401. Can we merge that instead? |
|
The essential change in 2.2.0 is use of ID Generator instead of static strings. Use of generated IDs is good thing as otherwise the static string IDs could remain same and cause hash collisions in VolumeIDs that PMEM-CSI generates. Despite the promise in comment "optional, defaults to DefaultIDGenerator" it does not work without explicit init.value in Config (which I added in separate comment). Without that initialization, sc.Config.IDGen remains nil and causes tester code panic when sc.ConfigIDGen. method is called. |
okartau
force-pushed
the
try-csi-test-220
branch
from
ed6f38b to
636b29a
Compare
|
Olev Kartau <[email protected]> writes:
indeed, it is. somehow, #401 gets tests running without adding IDGen
to Config ?
Did you find that this is necessary?
|
|
yes as I explained in comment above. If I just made change to 2.2.0, test code paniced as sc.Config.IDGen was nil. Code started to run when I added IDGen init in Config. |
Hmm, then why did the PR test of #401 pass? |
|
no idea. My first thought after I saw the panic was that there is something else involved which my isolated version step-up did not bring in. I tried to find it, but IDGen change seems quite well isolated. So I went on and created workaround using Config init.line. |
|
#401 passed PR testing because... no E2E tests ran (https://cloudnative-k8sci.southcentralus.cloudapp.azure.com/blue/organizations/jenkins/pmem-csi/detail/PR-401/2/pipeline/58). That is the Achilles heel of automatically choosing which tests to run in the code: if choosing the tests is buggy, testing may silently not run as intended. I'll investigate that in #401. |
test/e2e/storage/sanity.go
Outdated
| @@ -65,6 +65,7 @@ var _ = Describe("sanity", func() { | |||
| // and deletes all extra entries that it does not know about. | |||
| TargetPath: "/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pmem-sanity-target.XXXXXX", | |||
| StagingPath: "/var/lib/kubelet/plugins/kubernetes.io/csi/pv/pmem-sanity-staging.XXXXXX", | |||
| IDGen: &sanity.DefaultIDGenerator{}, | |||
There was a problem hiding this comment.
This shouldn't be necessary. I think it's a bug in csi-test, which we should fix there: https://github.com/kubernetes-csi/csi-test/blob/82b05190c167c52bb6d5aaf2e1d7c833fa539783/pkg/sanity/sanity.go#L161-L163 was added to Test, but not GinkgoTest.
We should have a common function that is used by both code paths to sanitize the config.
@okartau as you found this, can you do the upstream fix?
In this repo we can keep IDGen: &sanity.DefaultIDGenerator{} as a temporary workaround for 2.2.0, but there should be a TODO comment above it about removing it.
okartau
force-pushed
the
try-csi-test-220
branch
from
636b29a to
7f85c16
Compare
okartau
force-pushed
the
try-csi-test-220
branch
3 times, most recently
from
0642817 to
ae6b5bf
Compare
okartau
changed the title
okartau
force-pushed
the
try-csi-test-220
branch
from
1491b4d to
267e903
Compare
This requires to change claim size from 1MB to 110 MB, otherwise files created by test will not fit (needs 102 MB).
okartau
force-pushed
the
try-csi-test-220
branch
from
267e903 to
8607160
Compare
|
closing as devel stepped up to csi-test 2.2.0 |
No description provided.