Conversation
rustybee42
force-pushed
the
rb/fix-v7-import
branch
from
c8a92fd to
1c61240
Compare
iamjoemccormick
left a comment
iamjoemccormick
left a comment
There was a problem hiding this comment.
Presuming one doesn't already exist, should we also add an integration test that exercises the full end-to-end v7 import? A fixture (v7 mgmtd data) that includes all importable data types (nodes, targets, buddy groups, pools, etc.) would complement the existing unit tests without needing to fully re-verify the imported values, just that the import succeeds without constraint errors and maybe sanity check node, target, quota, etc. counts match up.
There was a problem hiding this comment.
todo: The target.rs:set_get_meta test should assert the root_inode contents after all targets are inserted, to ensure the first target is used as the root and not overwritten by subsequent insertions.
There was a problem hiding this comment.
This is a bit unrelated to this PR. I could add it nonetheless, but I don't really see the point as it's currently working and there is no change.
And the plan is to get rid of this db abstraction anyway long term and put tests on the handlers and other stuff instead.
There was a problem hiding this comment.
I agree its technically unrelated to this PR, but since this isn't the first issue we've had related to setting the root inode, I was just generally looking at ways to improve our test coverage in this area.
rustybee42
force-pushed
the
rb/fix-v7-import
branch
2 times, most recently
from
2f0b281 to
03bdcd2
Compare
iamjoemccormick
left a comment
iamjoemccormick
left a comment
There was a problem hiding this comment.
LGTM - though I would still recommend we find a way to increase our test coverage around v7 imports.
This let the import fail. We now call target::insert directly for meta nodes which does not insert into root_inode. Also remove target::insert_meta
rustybee42
force-pushed
the
rb/fix-v7-import
branch
from
03bdcd2 to
c0b851d
Compare
Uses a v7 management data folder created with 7.4
rustybee42
force-pushed
the
rb/fix-v7-import
branch
from
38eaad8 to
ca06f84
Compare
| #[test] | ||
| fn import_v7() { | ||
| // Setup | ||
| let tmp_dir = std::env::temp_dir().join(".beegfs_import_v7_test"); |
There was a problem hiding this comment.
nitpick: unless temp_dir does it automatically (it doesn't look like it does), I'd suggest adding some unique/random identifier to the test directory name. For example in case there are two test binary instances running concurrently on the same machine. This also ensures each test is always isolated and won't be affected by stale/leftover state from a previous test, for example if somehow the test was killed before remove_dir_all() executed.
This is a nitpick because it is unlikely in our current test environment, but more just a general testing best practice.
2 participants
Fixes v7 import, which broke on 8.3 due to a double insert into the
root_inodetable.