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
tests: do not use memstore.test_temp_dir in two tests #12281
Conversation
@dzafman do you remember why that was necessary ? I think it causes http://tracker.ceph.com/issues/17743 because memstore.test_temp_dir is also used by src/test/objectstore/test_memstore_clone.cc. If both unittest_memstore_clone and src/test/test_objectstore_memstore.sh run in parallel, bad things will happen. |
@dzafman nvm, I figured it out. Sorry for the noise. To be precise, 8c824fd did the right thing and also revealed a name clash that created frequent false negative http://tracker.ceph.com/issues/17743 because the same directory is used by two tests that can run in //. |
@@ -178,7 +178,7 @@ int main(int argc, char** argv) | |||
// default to memstore | |||
vector<const char*> defaults{ | |||
"--osd_objectstore", "memstore", | |||
"--osd_data", "memstore.test_temp_dir", |
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.
The new standard is directory names are like "testname.test_temp_dir". So this could be "memstore_clone.test_temp_dir" or something shorter.
@dzafman updated, thanks for your review :-) |
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.
thanks for fixing this!
@dachary Are you going to address my change request? I don't see the correct format for the test directory name. |
@ldachary I think I did. Did I miss something ? |
@dzafman I did wrong, this should be fixed now : s/test_memstore/memstore_clone/ |
@dachary I thought of the new format being *.test_temp_dir where * is usually the test name but maybe you would abbreviate that if you didn't like memstore_clone.test_temp_dir So I didn't expect you'd abbreviate test_temp_dir to td. I know this is trivial. Am I crazy? |
jenkins test this please (test_objectstore_memstore.sh) |
@dzafman the reason I abreviate to the maximum is jenkins imposing long paths and the problems it creates http://tracker.ceph.com/issues/15249 |
@dachary I would have preferred "msc.test_temp_dir" so you can grep for test directories in the code or find them in a filesystem. |
unittest_memstore_clone and test_objectstore_memstore.sh both use memstore.test_temp_dir and when running in parallel they interfere with each other. src/test/objectstore/store_test_fixture.h will create memstore.test_temp_dir when running via src/test/test_objectstore_memstore.sh src/test/objectstore/test_memstore_clone.cc will also create memstore.test_temp_dir when running via unittest_memstore_clone. Fixes: http://tracker.ceph.com/issues/17743 Signed-off-by: Loic Dachary <loic@dachary.org>
@dzafman I updated the commit with your suggestion. That being said, it does not solve the memstore test failures: they were not caused by this problem. It's a step forward though :-) |
http://tracker.ceph.com/issues/17743