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

Add more integration tests for deltas #267

Closed
wants to merge 1 commit into from
Closed

Conversation

lmbarros
Copy link
Collaborator

This adds two new integration tests:

  • TestDeltaSize: this is meant to catch regressions on delta sizes. It
    generates deltas and compare their sizes with the delta sizes we get
    as of now. If the size increased, the test fails.
  • TestDeltaCorrectness: checks if applying a delta indeed results in the
    same image as we had originally.

A number of different test cases (different images with distinct
features) are tested for each of these integration tests.

Points of attention for review:

  • Any other test case to add? (N.B.: I'd like to keep these tests limited to reasonably small images (because they run faster and because the test data is included in the repo)
  • Any ideas for the TestDeltaSize test? I have a number of doubts about it:
    • I am not entirely sure these tests with small images will reflect real use cases. They shall be enough to catch regressions, but there may be better ways to do this.
    • In its current form, this test is likely to fail for most changes in deltas. I mean, even if we make changes that overall improve delta sizes, some of the test cases will probably fail (because improving the common case is likely to worsen the uncommon cases a bit). So, we'd need to adjust the "expected sizes" for these test cases as we keep working on delta size improvements. Maybe this is too cumbersome? Any ideas for improvement?

@lmbarros lmbarros mentioned this pull request Aug 25, 2021
@lmbarros lmbarros self-assigned this Aug 25, 2021
@lmbarros lmbarros force-pushed the lmb/add-delta-tests branch 2 times, most recently from 6e43b69 to ee64fc0 Compare September 7, 2021 21:39
@lmbarros lmbarros marked this pull request as ready for review September 9, 2021 13:43
integration/image/delta_test.go Outdated Show resolved Hide resolved
@lmbarros
Copy link
Collaborator Author

Updated the branch. Now using the right path to get the test data.

@lmbarros
Copy link
Collaborator Author

@balena-ci rebase

@ghost ghost force-pushed the lmb/add-delta-tests branch from b2ae552 to b07c3e5 Compare October 18, 2021 21:37
@ghost
Copy link

ghost commented Oct 18, 2021

The preview site has been deleted.

@robertgzr
Copy link
Contributor

The DeltaSize test fails for 5/10 cases:

=== Failed
=== FAIL: amd64.integration.image TestDeltaSize/delta-000-001 (0.36s)
    --- FAIL: TestDeltaSize/delta-000-001 (0.36s)
        delta_test.go:300: Delta too big: got 1556 bytes, expected at most 1044

=== FAIL: amd64.integration.image TestDeltaSize/delta-001-003 (0.76s)
    --- FAIL: TestDeltaSize/delta-001-003 (0.76s)
        delta_test.go:300: Delta too big: got 9248 bytes, expected at most 8736

=== FAIL: amd64.integration.image TestDeltaSize/delta-000-003 (0.49s)
    --- FAIL: TestDeltaSize/delta-000-003 (0.49s)
        delta_test.go:300: Delta too big: got 6757 bytes, expected at most 5733

=== FAIL: amd64.integration.image TestDeltaSize/delta-010-011 (0.67s)
    --- FAIL: TestDeltaSize/delta-010-011 (0.67s)
        delta_test.go:300: Delta too big: got 59940 bytes, expected at most 49188

=== FAIL: amd64.integration.image TestDeltaSize/delta-010-012 (1.12s)
    --- FAIL: TestDeltaSize/delta-010-012 (1.12s)
        delta_test.go:300: Delta too big: got 1049132 bytes, expected at most 1049125
    delta_test.go:310: -------------------------------------------------------------
    delta_test.go:311: Test case           Want size     Got size       
    delta_test.go:312: -------------------------------------------------------------
    delta_test.go:320: delta-000-001       1044          1556           BAD!
    delta_test.go:320: delta-000-002       1045          1045           
    delta_test.go:320: delta-001-003       8736          9248           BAD!
    delta_test.go:320: delta-000-003       5733          6757           BAD!
    delta_test.go:320: delta-004-005       0             0              
    delta_test.go:320: delta-006-007       21888         21888          
    delta_test.go:320: delta-006-008       21622         21622          
    delta_test.go:320: delta-008-006       381           381            
    delta_test.go:320: delta-010-011       49188         59940          BAD!
    delta_test.go:320: delta-010-012       1049125       1049132        BAD!
    delta_test.go:322: -------------------------------------------------------------

=== FAIL: amd64.integration.image TestDeltaSize (9.15s)

@lmbarros
Copy link
Collaborator Author

That's weird, tests pass for me. I expect this test case to be easy to break when changing in the delta implementation, but the results of creating deltas should be deterministic, so we both should see the same results. Will look into this, thanks for pointing out!

@lmbarros lmbarros force-pushed the lmb/add-delta-tests branch 2 times, most recently from 41290ac to f3714a9 Compare October 29, 2021 22:13
@robertgzr
Copy link
Contributor

@lmbarros I gave this another try and they seem to be better now although I still get failures when running the Size tests:

--- PASS: TestDeltaCorrectness (29.34s)
    --- PASS: TestDeltaCorrectness/delta-000-001 (1.02s)
    --- PASS: TestDeltaCorrectness/delta-000-002 (1.37s)
    --- PASS: TestDeltaCorrectness/delta-001-003 (2.69s)
    --- PASS: TestDeltaCorrectness/delta-000-003 (3.15s)
    --- PASS: TestDeltaCorrectness/delta-004-005 (2.55s)
    --- PASS: TestDeltaCorrectness/delta-006-007 (7.45s)
    --- PASS: TestDeltaCorrectness/delta-006-008 (2.14s)
    --- PASS: TestDeltaCorrectness/delta-008-006 (4.47s)
    --- PASS: TestDeltaCorrectness/delta-010-011 (2.05s)
    --- PASS: TestDeltaCorrectness/delta-010-012 (2.32s)
FAIL

=== Failed
=== FAIL: amd64.integration.image. TestDeltaSize/delta-010-011 (0.62s)
    --- FAIL: TestDeltaSize/delta-010-011 (0.62s)
        delta_test.go:301: Delta too big: got 59940 bytes, expected at most 49188

=== FAIL: amd64.integration.image. TestDeltaSize/delta-010-012 (1.11s)
    --- FAIL: TestDeltaSize/delta-010-012 (1.11s)
        delta_test.go:301: Delta too big: got 1049132 bytes, expected at most 1049125
    delta_test.go:311: -------------------------------------------------------------
    delta_test.go:312: Test case           Want size     Got size       
    delta_test.go:313: -------------------------------------------------------------
    delta_test.go:321: delta-000-001       1044          1044           
    delta_test.go:321: delta-000-002       1045          1045           
    delta_test.go:321: delta-001-003       8736          8736           
    delta_test.go:321: delta-000-003       5733          5733           
    delta_test.go:321: delta-004-005       0             0              
    delta_test.go:321: delta-006-007       21888         21888          
    delta_test.go:321: delta-006-008       21622         21622          
    delta_test.go:321: delta-008-006       381           381            
    delta_test.go:321: delta-010-011       49188         59940          BAD!
    delta_test.go:321: delta-010-012       1049125       1049132        BAD!
    delta_test.go:323: -------------------------------------------------------------

=== FAIL: amd64.integration.image. TestDeltaSize (9.16s)

These results are consistent over multiple test runs

I tested on another machine and I get

=== Failed
=== FAIL: amd64.integration.image. TestDeltaSize/delta-000-001 (1.24s)
    --- FAIL: TestDeltaSize/delta-000-001 (1.24s)
        delta_test.go:301: Delta too big: got 1556 bytes, expected at most 1044

=== FAIL: amd64.integration.image. TestDeltaSize/delta-001-003 (3.13s)
    --- FAIL: TestDeltaSize/delta-001-003 (3.13s)
        delta_test.go:301: Delta too big: got 9248 bytes, expected at most 8736

=== FAIL: amd64.integration.image. TestDeltaSize/delta-000-003 (1.87s)
    --- FAIL: TestDeltaSize/delta-000-003 (1.87s)
        delta_test.go:301: Delta too big: got 6757 bytes, expected at most 5733

=== FAIL: amd64.integration.image. TestDeltaSize/delta-010-011 (2.32s)
    --- FAIL: TestDeltaSize/delta-010-011 (2.32s)
        delta_test.go:301: Delta too big: got 59940 bytes, expected at most 49188

=== FAIL: amd64.integration.image. TestDeltaSize/delta-010-012 (2.81s)
    --- FAIL: TestDeltaSize/delta-010-012 (2.81s)
        delta_test.go:301: Delta too big: got 1049132 bytes, expected at most 1049125
    delta_test.go:311: -------------------------------------------------------------
    delta_test.go:312: Test case           Want size     Got size       
    delta_test.go:313: -------------------------------------------------------------
    delta_test.go:321: delta-000-001       1044          1556           BAD!
    delta_test.go:321: delta-000-002       1045          1045           
    delta_test.go:321: delta-001-003       8736          9248           BAD!
    delta_test.go:321: delta-000-003       5733          6757           BAD!
    delta_test.go:321: delta-004-005       0             0              
    delta_test.go:321: delta-006-007       21888         21888          
    delta_test.go:321: delta-006-008       21622         21622          
    delta_test.go:321: delta-008-006       381           381            
    delta_test.go:321: delta-010-011       49188         59940          BAD!
    delta_test.go:321: delta-010-012       1049125       1049132        BAD!
    delta_test.go:323: -------------------------------------------------------------

=== FAIL: amd64.integration.image. TestDeltaSize (35.05s)

@lmbarros
Copy link
Collaborator Author

lmbarros commented Jan 6, 2022

@robertgzr I pushed another update, now using intervals of acceptable delta sizes. Now we have at least a clue that this variation is likely coming from differences in the underlying implementation of whatever union filesystem is used on the machine running the tests. I don't think it is worth to try to be independent of these differences.

I considered adding some "rule" or "fixed safety margin" instead of manual min/max bounds, but the variations depends a lot on what image is being used for which test. Any reasonable rule lax enough to work in all cases would be so lax as to make most tests moot.

This adds two new integration tests:

* TestDeltaSize: this is meant to catch regressions on delta sizes. It
  generates deltas and compare their sizes with the delta sizes we get
  as of now. If the size increases, the test fails.
* TestDeltaCorrectness: checks if applying a delta indeed results in the
  same image as we had originally.

A number of different test cases (different images with distinct
features) are tested for each of these integration tests.

Signed-off-by: Leandro Motta Barros <leandro@balena.io>
Change-type: patch
@robertgzr robertgzr force-pushed the lmb/add-delta-tests branch 2 times, most recently from d89cc4e to 59f6cd6 Compare March 21, 2022 18:44
@robertgzr
Copy link
Contributor

@lmbarros I rebased your work on the master branch

INFO: Testing against a local daemon
=== RUN   TestDeltaSize
=== RUN   TestDeltaSize/delta-000-001
=== RUN   TestDeltaSize/delta-000-002
=== RUN   TestDeltaSize/delta-001-003
=== RUN   TestDeltaSize/delta-000-003
=== RUN   TestDeltaSize/delta-004-005
=== RUN   TestDeltaSize/delta-006-007
=== RUN   TestDeltaSize/delta-006-008
=== RUN   TestDeltaSize/delta-008-006
=== RUN   TestDeltaSize/delta-010-011
=== RUN   TestDeltaSize/delta-010-012
=== CONT  TestDeltaSize
    delta_test.go:329: -------------------------------------------------------------
    delta_test.go:330: Test case           Want size           Got size       
    delta_test.go:331: -------------------------------------------------------------
    delta_test.go:340: delta-000-001       1044..1556          1556           
    delta_test.go:340: delta-000-002       1045..1045          1045           
    delta_test.go:340: delta-001-003       8736..9248          9248           
    delta_test.go:340: delta-000-003       5733..6757          6757           
    delta_test.go:340: delta-004-005       0..0                0              
    delta_test.go:340: delta-006-007       21888..21888        21888          
    delta_test.go:340: delta-006-008       21622..21622        21622          
    delta_test.go:340: delta-008-006       381..381            381            
    delta_test.go:340: delta-010-011       49188..59940        59940          
    delta_test.go:340: delta-010-012       1049125..1049132    1049132        
    delta_test.go:342: -------------------------------------------------------------
--- PASS: TestDeltaSize (74.45s)
    --- PASS: TestDeltaSize/delta-000-001 (2.47s)
    --- PASS: TestDeltaSize/delta-000-002 (3.17s)
    --- PASS: TestDeltaSize/delta-001-003 (8.26s)
    --- PASS: TestDeltaSize/delta-000-003 (4.60s)
    --- PASS: TestDeltaSize/delta-004-005 (10.58s)
    --- PASS: TestDeltaSize/delta-006-007 (27.20s)
    --- PASS: TestDeltaSize/delta-006-008 (3.10s)
    --- PASS: TestDeltaSize/delta-008-006 (7.48s)
    --- PASS: TestDeltaSize/delta-010-011 (3.48s)
    --- PASS: TestDeltaSize/delta-010-012 (4.10s)
PASS

DONE 11 tests in 102.525s

just to have it in writing here again, we suspect the size differences to be due to the graphdriver used? or the backing filesystem?

@lmbarros
Copy link
Collaborator Author

@robertgzr, the mismatch seemed to be caused by differences in the underlying union filesystem implementation (that is, overlayfs), and more specifically how they use whiteout file.

I was seeing a difference in image sizes generated on two different computers, both of which were using the same filesystem (ext4) and graphdriver (overlay2). But inspecting the contents of the layers I found that one of them was creating whiteout files that other wasn't (IIRC, these were redundant whiteout files, e.g., to clean up a directory that was already empty on the lower layer).

@lmbarros
Copy link
Collaborator Author

lmbarros commented Apr 1, 2022

@balena-ci I self-certify!

@lmbarros
Copy link
Collaborator Author

lmbarros commented Apr 1, 2022

Looks like I can't merge this because I am missing a formal approval from Robert about a change request that has been already been discussed and addressed. I'll close this PR and open a new one from the same branch to self-certify.

@lmbarros lmbarros closed this Apr 1, 2022
@lmbarros lmbarros deleted the lmb/add-delta-tests branch April 4, 2022 17:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants