server: fix artifact handling reset on restart #444
Conversation
luatest/server.lua
Outdated
|
|
||
| local prefix = fio.pathjoin(Server.vardir, 'artifacts', self.rs_id or '') | ||
| self.artifacts = fio.pathjoin(prefix, self.id) | ||
| if fio.path.exists(self.artifacts) then |
There was a problem hiding this comment.
Why do we have to delete the artifacts on restart? Artifacts should be saved only when the server is dropped, no?
Also, the comment to server:stop() seems outdated - it says that artifacts are saved only on test failures but it looks like they are saved unconditionally.
There was a problem hiding this comment.
Artifacts should be saved only when the server is dropped, no?
Yes, I think you're right. Fixed.
mandesero
force-pushed
the
mandesero/gh-409-server-artifacts
branch
from
ac276a5 to
a299af1
Compare
luatest/server.lua
Outdated
|
|
||
| local prefix = fio.pathjoin(Server.vardir, 'artifacts', self.rs_id or '') | ||
| self.artifacts = fio.pathjoin(prefix, self.id) | ||
| self.artifacts_saved = false |
There was a problem hiding this comment.
I still don't think this is a proper fix. Simply clearing the flag would result in rewriting artifacts to the same directory. AFAIU the problem here is that wait_for_condition is called and fails in Server:stop(), which is expected because the process is killed. The whole artifact handling procedure is a total mess now. Look - save_artifacts is called both on server stop and in the runner. We need to rework it. Please take a look at the commit history - maybe it'll help you come up with a proper fix.
There was a problem hiding this comment.
I added a save_artifacts flag so that artifacts are saved only when a test actually fails, and no longer saved during a normal Server:stop() (there’s a test covering this behavior now). I also moved the logic that decides whether artifacts should be saved into Server:drop().
mandesero
force-pushed
the
mandesero/gh-409-server-artifacts
branch
from
a299af1 to
18ceda0
Compare
luatest/server.lua
Outdated
| end | ||
|
|
||
| if should_save then | ||
| self:save_artifacts() |
There was a problem hiding this comment.
Why do we save artifacts here if we save them in the runner anyway? Why do we save artifacts in wait_condition? I don't understand from the code.
There was a problem hiding this comment.
You're right, artifacts were previously being saved in multiple places (in wait_for_condition, in Server:stop(), and in Runner:update_status()), which created confusion and redundancy. In the latest changes, I centralized the artifact-saving logic in the end_test method in Runner. Now, artifacts are saved only at the end of the test (if the test fails), which should eliminate the duplication.
Also, the changes required updating the existing tests.
Please take a look at the latest updates.
mandesero
force-pushed
the
mandesero/gh-409-server-artifacts
branch
2 times, most recently
from
48f2121 to
342acf7
Compare
Fix a bug when server initialization didn't reset artifact handling, which caused stale artifacts to persist after server restarts. Closes tarantool#409
mandesero
force-pushed
the
mandesero/gh-409-server-artifacts
branch
from
342acf7 to
29e8538
Compare
2 participants
Fix a bug when server initialization didn't reset artifact handling, which caused stale artifacts to persist after server restarts.
Closes #409