CalcJob: make sure local_copy_list files do not end up in node repo
#4415
Conversation
Codecov Report
@@ Coverage Diff @@
## release/1.4.2 #4415 +/- ##
=================================================
+ Coverage 79.23% 79.24% +0.02%
=================================================
Files 475 475
Lines 34831 34827 -4
=================================================
+ Hits 27594 27596 +2
+ Misses 7237 7231 -6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
The concept of the `local_copy_list` is to provide a possibility to `CalcJob` plugins to write files to the remote working directory but that are not also copied to the calculation job's repository folder. However, due to commit 9dfad2e this guarantee is broken. The relevant commit refactored the handling of the `local_copy_list` in the `upload_calculation` method to allow the target filepaths in the list to contain nested paths with subdirectories that might not yet necessarily exist. The approach was to first write all files to the sandbox folder, where it is easier to deal with non-existing directories. To make sure that these files weren't then also copied to the node's repository folder, the copied files were also added to the `provenance_exclude_list`. However, the logic in that part of the code did not normalize filepaths, which caused files to be copied that shouldm't have. The reason is that the `provenance_exclude_list` could contain `./path/file_a.txt`, which would be compared to the relative path `path/file_a.txt` which references the same file, but the strings are not equal. The solution is to ensure that all paths are fully normalized before they are compared. This will turn the relative path `./path/file_a.txt` into `path/file_a.txt`.
sphuber
force-pushed
the
fix/4414/local-copy-list-stored-repo
branch
from
aff24c5
to
739eb14
Compare
|
I guess we need to remove the required flag from the 3.5 tests? |
Not really. This is going to be merged in |
|
Thanks for issuing a fix for this asap. Greatly appreciated. What releases have this issue? We need to block them in the VASP plugin. |
|
@espenfl v1.4 and v1.4.1 are affected by this bug. I will release a patch release a.s.a.p. Just waiting for one other potential bug that needs to be fixed. |
|
@sphuber Thanks. I will block those. Also, I will test to see if 1.4 triggers our tests for this. |
Fixes #4414
The concept of the
local_copy_listis to provide a possibility toCalcJobplugins to write files to the remote working directory butthat are not also copied to the calculation job's repository folder.
However, due to commit 9dfad2e this
guarantee is broken.
The relevant commit refactored the handling of the
local_copy_listinthe
upload_calculationmethod to allow the target filepaths in thelist to contain nested paths with subdirectories that might not yet
necessarily exist. The approach was to first write all files to the
sandbox folder, where it is easier to deal with non-existing
directories. To make sure that these files weren't then also copied to
the node's repository folder, the copied files were also added to the
provenance_exclude_list. However, the logic in that part of the codedid not normalize filepaths, which caused files to be copied that
shouldm't have. The reason is that the
provenance_exclude_listcouldcontain
./path/file_a.txt, which would be compared to the relativepath
path/file_a.txtwhich references the same file, but the stringsare not equal.
The solution is to ensure that all paths are fully normalized before
they are compared. This will turn the relative path
./path/file_a.txtinto
path/file_a.txt.