overlay: Diffsize: naive diff when not parent #1691
Conversation
|
/hold |
|
This reintroduces the perf issue. |
False alarm. I used the wrong branch for testing -.- |
|
LGTM |
|
Hypothesis: $ ack 'kill.*\$' contrib
contrib/cirrus/build_and_test.sh
74: kill $(lsns -J -t mnt --output-all | jq '.namespaces[]|select(.command=="sleep 1000s").pid')Root cause is therefore: https://github.com/containers/storage/pull/1535/files#diff-27779cc9489581b15cc0f02985665abb1f2bab23026e256dc72d6823b3d8761b |
|
Suggested fix: index 8b1f3ad10..24bec3c76 100755
--- a/contrib/cirrus/build_and_test.sh
+++ b/contrib/cirrus/build_and_test.sh
@@ -71,7 +71,10 @@ case $TEST_DRIVER in
zfs create $zpool/tmp
TMPDIR="/$zpool/tmp" showrun make STORAGE_DRIVER=$TEST_DRIVER local-test-integration local-test-unit
# Ensure no datasets are held open prior to `zfs destroy` trap.
- kill $(lsns -J -t mnt --output-all | jq '.namespaces[]|select(.command=="sleep 1000s").pid')
+ foo=$(lsns -J -t mnt --output-all | jq '.namespaces[]|select(.command=="sleep 1000s").pid')
+ if [[ -n "$foo" ]]; then
+ kill $foo
+ fi
;;
*)
die "Unknown/Unsupported \$TEST_DRIVER=$TEST_DRIVER (see .cirrus.yml and $(basename $0))"...with a better name than |
mtrmac
force-pushed
the
fix-podman-19467
branch
from
ff917b2 to
51ceb17
Compare
If the specified parent is not ID's parent, use the naive diff. Signed-off-by: Valentin Rothberg <[email protected]>
Commit 224f0b9 added the kill but it seems to not always be the case there's process holding the datasets which ultimately leads to the kill to fail and in turn CI to turn red. Signed-off-by: Valentin Rothberg <[email protected]>
vrothberg
force-pushed
the
fix-podman-19467
branch
from
51ceb17 to
9dc0dc5
Compare
|
Thanks, @edsantiago ! |
| @@ -71,7 +71,10 @@ case $TEST_DRIVER in | |||
| zfs create $zpool/tmp | |||
| TMPDIR="/$zpool/tmp" showrun make STORAGE_DRIVER=$TEST_DRIVER local-test-integration local-test-unit | |||
| # Ensure no datasets are held open prior to `zfs destroy` trap. | |||
| kill $(lsns -J -t mnt --output-all | jq '.namespaces[]|select(.command=="sleep 1000s").pid') | |||
| datasets=$(lsns -J -t mnt --output-all | jq '.namespaces[]|select(.command=="sleep 1000s").pid') | |||
There was a problem hiding this comment.
“dataset” seems to be a ZFS term, something that is held by a process but is not a process. pids?
(I know ~nothing about ZFS and whether this is the right thing to do, or why it suddenly started breaking after ~4 months.)
There was a problem hiding this comment.
@cevich could you weigh in here please? The solution is obvious and already applied, but it would be really nice to give this code a meaningful comment.
There was a problem hiding this comment.
ZFS testing has been a PITA (mostly for me) in this repo. for a long while. However, apparently zfs-testing is actually needed due to a large user-base (I think it's Giuseppe who strongly desires it). I didn't write these script bits, but I would guess they came in when Ubuntu was used in CI. I'm not surprised it's breaking under the much more modern Debian SID. For Ubuntu it was easy/built-in to the distro, but in Debian it's a kind of add-on similar to EPEL (near as I can tell). That part alone smells like trouble to me 😞
There was a problem hiding this comment.
@giuseppe do you have any clues to share on what this datasets is used/needed for?
There was a problem hiding this comment.
I've no idea about ZFS, nor desire to keep it :-)
There was a problem hiding this comment.
in any case, ZFS failures for sure do not depend on this change as it affects only the overlay driver
There was a problem hiding this comment.
There was a problem hiding this comment.
I've no idea about ZFS, nor desire to keep it :-)
Well darn my bad memory...I'm sure somebody said (strongly) that we need it in CI.
That was in response to my wanting to trash all ZFS testing. This is the only repo. that does it. That's really the only high-level context I can add here 😞
@edsantiago I really don't know/remember anything about these build_and_test.sh lines, I don't think I wrote them(?). I welcome your efforts toward making it cleaner, and if I can help somehow I will. I was just questioning if we really do need this at all, since it's so much of a corner-case in the overall/multi-project CI matrix. I think the answer is still "yes"...just wish I remembered who the strong advocate is 😞
There was a problem hiding this comment.
If anyone in the community cares enough to open PRs to maintain these other drivers, then it is fine but I don't see the point for us to worry about anything else than overlay. I am not even talking about RHEL support, even upstream the other backends miss too many features that are present in overlay.
So +1 on "get rid of that "get rid of all ZFS special cases" sounds right if that can simplify our life.
If the specified parent is not ID's parent, use the naive diff.
@mtrmac @giuseppe PTAL