Skip to content
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

make test_shim work when in non-home, or symlinked directories. #140

Open
petersilva opened this issue Jan 2, 2024 · 9 comments
Open

make test_shim work when in non-home, or symlinked directories. #140

petersilva opened this issue Jan 2, 2024 · 9 comments
Assignees
Labels
bug Something isn't working likely-fixed solution exists, in a PR or merged

Comments

@petersilva
Copy link
Contributor

petersilva commented Jan 2, 2024

  • on supercomputer, home is a symlinked path.
  • shim_tests use realpath, and assume home is a real path, so some tests fail.

purpose is to get the tests to pass when run on supercomputer.

branches with bits of work on this:

which was merged from:

the non_utf8 and test_shim failures in the normal cases have since been isolated and merged, and so the work here is to extract the symlink related patches and get them into something coherent to merge, so in future, people can run the test_shim on the supercomputer.

@petersilva
Copy link
Contributor Author

uh... not putting a priority on this or anything... I'm just thinking that since you're testing this stuff, this should be on your radar next time you're in the area...

@petersilva
Copy link
Contributor Author

I remember this being painful... it might not be a feasible/worthwhile thing to do.

@reidsunderland
Copy link
Member

I committed the changes to the tests that I had made to get the tests working better on ppp. They're on the https://github.com/MetPX/sarrac/tree/test_tweaks branch.

  • I'm pretty sure I have not tested them on Ubuntu/non-network home
  • I think we also thought that (some of?) the changes might have been working around a bug
    • something to do with symlinks and paths/accepts, but I didn't look into it deep enough to figure it out

@petersilva petersilva added the likely-fixed solution exists, in a PR or merged label Oct 28, 2024
@petersilva
Copy link
Contributor Author

so when processes do not close files, the exit_cleanup() routine in libsr3shim.c has to resolve the pathnames for open file descriptors.
it uses realpath to do that. I does not know that the actual path was opened using a symlink...

so we have, in shim_post.sh for example... files being closed their paths are always "realpathed" so posting via symbolic paths fails.

choice between post_baseDir=${HOME}:

[pas037@ppp6login-003 metpx-sr3c]$ [pas037@ppp6login-003 metpx-sr3c]$ grep ERROR shim_post.log
2024-11-18 17:49:52,206 [ERROR] shim posting outside of post_baseDir (/home/pas037) invalid path: /fs/homeu2/ssc/di/pas037/Sarracenia/metpx-sr3c/hoho
2
```
and post_baseDir=`realpath ${HOME}`

```
[pas037@ppp6login-003 metpx-sr3c]$ grep ERROR shim_post.log
2024-11-18 18:33:14,541 [ERROR] shim posting outside of post_baseDir (/fs/homeu2/ssc/di/pas037) invalid path: /home/pas037/Sarracenia/metpx-sr3c/sub_dir1
2024-11-18 18:33:14,587 [ERROR] shim posting outside of post_baseDir (/fs/homeu2/ssc/di/pas037) invalid path: /home/pas037/Sarracenia/metpx-sr3c/sub_dir2
2024-11-18 18:33:14,655 [ERROR] shim posting outside of post_baseDir (/fs/homeu2/ssc/di/pas037) invalid path: /home/pas037/Sarracenia/metpx-sr3c/haha
2024-11-18 18:33:14,722 [ERROR] shim posting outside of post_baseDir (/fs/homeu2/ssc/di/pas037) invalid path: /home/pas037/test/hoho2.log
2024-11-18 18:33:14,744 [ERROR] shim posting outside of post_baseDir (/fs/homeu2/ssc/di/pas037) invalid path: /home/pas037/Sarracenia/metpx-sr3c/hihi
2024-11-18 18:33:14,765 [ERROR] shim posting outside of post_baseDir (/fs/homeu2/ssc/di/pas037) invalid path: /home/pas037/test/hoho2.log
2024-11-18 18:33:14,786 [ERROR] shim posting outside of post_baseDir (/fs/homeu2/ssc/di/pas037) invalid path: /home/pas037/Sarracenia/metpx-sr3c/dirone
2024-11-18 18:33:14,809 [ERROR] shim posting outside of post_baseDir (/fs/homeu2/ssc/di/pas037) invalid path: /home/pas037/Sarracenia/metpx-sr3c/dirone/dirtwo
202
```

so in either case, you get a clash in some cases between the logical path used to reference the file, and the realpath.

@petersilva
Copy link
Contributor Author

so the problem is here:

sarrac/sr_post.c

Lines 725 to 738 in c7b91a2

if (sr_c->cfg->post_baseDir && (strlen(sr_c->cfg->post_baseDir) > 1 ) ) {
// the +1 is to because baseDir is always absolute, and relPath is always relative
drfound = strstr(fn, (sr_c->cfg->post_baseDir)+1);
// replace only if at the beginning of the string.
if (drfound==fn+1) {
drfound += strlen(sr_c->cfg->post_baseDir);
strcpy(m->relPath, drfound);
} else if (absolute_path) {
sr_log_msg(sr_c->cfg->logctx,LOG_ERROR, "%s posting outside of post_baseDir (%s) invalid path: %s\n",
sr_c->cfg->progname, sr_c->cfg->post_baseDir, fn );
return(0);
}
}

Need to allow for ther post_baseDir to be present in symlinked and realpath form not just compare strings.

the removal of the post_baseDir from the full path... needs to work even when given realpath, when post_baseDir is a symlinked one.

... we have a reverse problem... where if post_baseDir is the realpath... then when given a symlinked path... it also requires conversion... but because realpath conversion is of the input path is done by realpath_adjust and such a setting is only valid with realpathPost=True, the reverse case should never occur.

So by fixing the first case, we should be solving the entire problem.

@petersilva
Copy link
Contributor Author

so likely want to:

  • create a realpath_adjust result... or just a straight call to realpath in sr_config_finalize() ... lets call it cfg->realpath_post_baseDir...
  • compare both post_baseDir and realpath_post_baseDir in that snippet above, and only complain if you cannot subtract either of them (if one matches, we're good.)

@petersilva petersilva added bug Something isn't working and removed likely-fixed solution exists, in a PR or merged labels Nov 18, 2024
@petersilva
Copy link
Contributor Author

alternative solutions discussed:

  • add an entry point for "open" and "fopen" ... keep a table of file names opened with their realpath mappings. when a file is "realpathed" from the fd, look it up in the table.
    (This seems a lot more complicated to implement than what just resolving post_baseDir and checking both given and resolved paths.) We will try the above first.

@petersilva
Copy link
Contributor Author

with the merge we have definitely made excellent progress... but not done... make test_shim_post is good... but later ones still fail...

@reidsunderland
Copy link
Member

I'm optimistic that the fix for redirection will fix at least some of those, I'll test that branch again

@reidsunderland reidsunderland added the likely-fixed solution exists, in a PR or merged label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working likely-fixed solution exists, in a PR or merged
Projects
None yet
Development

No branches or pull requests

2 participants