Implement ostree pause/resume #1007
Conversation
patriotyk
force-pushed
the
ostree_pause
branch
3 times, most recently
from
02814f8
to
c63165a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1007 +/- ##
==========================================
+ Coverage 82.03% 82.19% +0.15%
==========================================
Files 188 188
Lines 13229 13241 +12
==========================================
+ Hits 10853 10884 +31
+ Misses 2376 2357 -19
Continue to review full report at Codecov.
|
patriotyk
force-pushed
the
ostree_pause
branch
3 times, most recently
from
b5ea82a
to
54e52f6
Compare
| @@ -86,7 +86,7 @@ TEST(fetcher, fetch_with_pause) { | |||
| std::this_thread::sleep_for(std::chrono::milliseconds(200)); | |||
| EXPECT_EQ(f.setPause(true), PauseResult::kPaused); | |||
| EXPECT_EQ(f.setPause(true), PauseResult::kAlreadyPaused); | |||
| std::this_thread::sleep_for(std::chrono::seconds(2)); | |||
| std::this_thread::sleep_for(std::chrono::seconds(15)); | |||
There was a problem hiding this comment.
Why does it need to sleep so long?
There was a problem hiding this comment.
I just tried to understand who blocks the thread, I have already found it, but havn't restored previouse values
|
We have several issues here. But I can't figure out what is going here. |
|
Wow, not running test with pull was quite an overlook! Good that we are making progress. For reference, which version of ostree are you using on your laptop? I'll try the last release 2018.9. |
|
2018.8 |
|
Here is your error with a better stacktrace: I used the bionic docker with ostree 2018.9, compiled with I also got these leaks: Are they new? |
|
The ioctl stuff seems to be a valgrind bug (to confirm): https://bugs.kde.org/show_bug.cgi?id=397605 |
|
Possibly lost leaks doesn't cause tests to FAIL. And yes I have also those issues on my laptop, but we don't know is it new or not because we didn't test such functionality before. |
|
Ok fair enough. I would update the dockerfiles to use the same version as meta-updater (2018.7) if it doesn't have the other leak and add the ioctl error to our suppression list. |
|
I knew that our code is the best, and issues are somewhere else ))))))) |
patriotyk
force-pushed
the
ostree_pause
branch
from
54e52f6
to
0da40b6
Compare
| has_been_paused = true; | ||
| } else { | ||
| mt->pause_mutex->unlock(); | ||
| } |
There was a problem hiding this comment.
I'm not sure I entirely understand this logic. So if the mutex is already locked, we want to acquire it again? I assume that is just to wait until it is freed/unpaused. But then why wait until the end of this function before unlocking it again?
Whatever the intention, a comment would be helpful here. This logic is tricky enough that it is worth stating the goals explicitly.
There was a problem hiding this comment.
Updated with comments
| sleep(3); | ||
|
|
There was a problem hiding this comment.
You will need to rebase; we've improved this logic so that now we can wait for the server to be ready automatically instead of mandating 3 seconds.
| target_json["length"] = 0; | ||
| Uptane::Target target("pause", target_json); | ||
| test_pause(target); | ||
| } |
There was a problem hiding this comment.
Can we also have test_pause_binary, which would basically be the same as how this used to be, but now reusing the test_pause function?
There was a problem hiding this comment.
Hm, strange. I made this function exactly to use in both tests.
There was a problem hiding this comment.
I figured that was your intention, and it makes perfect sense to me!
tests/sota_tools/treehub_server.py
Outdated
| @@ -31,6 +32,8 @@ def do_GET(self): | |||
| break | |||
| self.wfile.write(data) | |||
| else: | |||
| if path.endswith("ef2f2629dc9263fdf3c0f032563a2d757623bbc11cf99df25c3c3f258dccbe.commitmeta"): | |||
| time.sleep(1.5) | |||
There was a problem hiding this comment.
Why is this sleep necessary? Might be worth a comment with a reference to the appropriate test case since it is not obvious.
There was a problem hiding this comment.
Because ostree calls progress callback every N miliseconds. To be sure our callback will be called we need to have this sleep.
There was a problem hiding this comment.
Ah yes, that makes sense. A comment in the code is still a good idea, though. Thanks!
patriotyk
force-pushed
the
ostree_pause
branch
4 times, most recently
from
45d8bd5
to
79df4a3
Compare
|
I have updated PR and now there is only ostree memory leaks which has been fixed in new version. Should I supress those leaks too? Or we will use newer version of ostree? |
patriotyk
force-pushed
the
ostree_pause
branch
3 times, most recently
from
2dd7a63
to
a0067ed
Compare
|
The failure on Jenkins: |
|
Looks like it is permissions problem. Do we run under root? ostree librarry can't change owner of fetched files. |
|
To answer your question: no we do not run under root and it would be nice to keep it that way if possible. |
|
But I can't do here anything, we should be able to change owner of files in our temporary directory. It is issue of jenkins configuration. |
|
Did not work... That's annoying. edit: also noticed the opcua build failure, I'll fix it in #1012. |
|
As #1012 has been merged, rebasing should fix the Travis failure. As for the fchown permission, I've just tried using It indicates that it is more likely to be system/docker related and not from jenkins' configuration. |
Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>
patriotyk
force-pushed
the
ostree_pause
branch
from
7ca1ef2
to
4ed24ce
Compare
|
How it resolves this issue? |
|
I found the root of problem and solution. The problem is in ostree code. I created fchown call wrapper and loaded it with LD_PRELOAD. It showed me, that ostree tries to change owner of deleted file. On some systems it works, but on some doesn't. I think it depends on filesystem type. Also I have found that ostree doesn't call fchown on 'bare-user' repo type. So if i change repo from 'bare' to 'bare-user' the problem goes away. Also I removed '--previleged' commit because it is no needed. |
patriotyk
force-pushed
the
ostree_pause
branch
from
2d6c3c9
to
eb4abfc
Compare
|
Great work @patriotyk! For what it's worth I run btrfs on my machine, where the test works and I think ostree has some custom code to take advantage of the CoW mechanism there. So maybe it breaks on non-btrfs? If we have easy reproduction steps, it could be worth it to report it upstream. |
Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>
patriotyk
force-pushed
the
ostree_pause
branch
from
eb4abfc
to
5150d38
Compare
|
I use ext4 and it works too. What the filesystem is on Jenkins machine? |
|
It seems to have failed the same way on both Jenkins machines. One is using ext4, the other btrfs... |
|
|
||
| TemporaryDirectory temp_dir; | ||
| int r = system((std::string("cp -r ") + argv[1] + std::string(" ") + temp_dir.PathString()).c_str()); |
There was a problem hiding this comment.
Could we use Utils::copyDir?
There was a problem hiding this comment.
I tried but it doesn't work. Looks like 'Utils::copyDir' has problems with symlink copying.
There was a problem hiding this comment.
Ok that should be fine for now then. Could you add a comment to document that?
@lbonn Did you restart the CI jobs, or how did this pass? @patriotyk Really good catch, by the way! |
|
@patrickvacek the fix is to use The question was: what makes it fail in But it shouldn't hold the PR, this was for the sake of understanding. |
|
@patriotyk Also, do you mind "checking" the boxes in |
|
@lbonn Ah, okay, thanks, I misunderstood your previous comment. I was concerned there was still something going wrong. |
patriotyk
force-pushed
the
ostree_pause
branch
2 times, most recently
from
ee17a26
to
d68e809
Compare
patriotyk
force-pushed
the
master
branch
2 times, most recently
from
013e501
to
19adf3c
Compare
patriotyk
force-pushed
the
ostree_pause
branch
from
d68e809
to
6d9cc79
Compare
Signed-off-by: Serhiy Stetskovych <patriotyk@gmail.com>
patriotyk
force-pushed
the
ostree_pause
branch
from
6d9cc79
to
e6d74dc
Compare
|
I'm testing this now with qemu and pausing does not seem to have an effect. It continues to download despite claiming to be paused. @patriotyk Can you reproduce? |
No description provided.