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

store: enable deltas for core devices too #4639

Merged
merged 2 commits into from Mar 12, 2018

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Feb 8, 2018

This enables delta refreshes everywhere.

Copy link
Collaborator

@zyga zyga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we shipping the required binary in core yet? I don't see it in my system (I'm tracking edge).

@ogra1
Copy link
Contributor

ogra1 commented Feb 8, 2018

yes we do ...

ogra@stream:~$ sudo find / -name '*xdelta*'|grep ^/snap/core|grep bin
/snap/core/3965/usr/bin/xdelta3
/snap/core/3750/usr/bin/xdelta3
/snap/core/3884/usr/bin/xdelta3
ogra@stream:~$ uname -m
armv7l

@zyga
Copy link
Collaborator

zyga commented Feb 8, 2018

Ah. I searched using find /snap/core/current -name xdelta3 and that finds nothing (I need an extra trailing slash).

All good!

@zyga
Copy link
Collaborator

zyga commented Feb 8, 2018

@mvo5 unit tests are failing:

FAIL: store_test.go:3921: remoteRepoTestSuite.TestUbuntuStoreRepositoryDefaultsDeltasOnClassicOnly

store_test.go:3934:
    c.Check(r.Header.Get("X-Ubuntu-Delta-Formats"), Equals, t.deltaFormatStr)
... obtained string = "xdelta3"
... expected string = ""

@mvo5
Copy link
Contributor Author

mvo5 commented Feb 8, 2018

@zyga yeah, sorry for this, just noticed and forced push the fix. If I had seen your comment early enough I would have pushed it as a separate commit.

@mvo5
Copy link
Contributor Author

mvo5 commented Feb 8, 2018

I set this to blocked because of: https://forum.snapcraft.io/t/refresh-causes-download-of-full-snap-file-instead-of-delta/3470/15 - I'm not sure what the best way forward is here, maybe we can contact them via our CE team?

@ogra1
Copy link
Contributor

ogra1 commented Feb 8, 2018

we should really ask them to add the variable to /etc/environment for a test (as i mentioned in the thread i'll do this over the weekend on one customer device here (200MHz single core cpu, 256M RAM) to see the impact).

IIRC when i last measured it for @chipaca on a beaglebone the difference was like:

x86 multi core CPU -> below 1sec to apply a core delta
armhf single core (beagle) CPU -> something like 8-10 sec or so for the same task

and IIRC the worry was that this extrapolates as your snaps get bigger ... though i guess it is not very likely that you run 1GB sized snaps on such HW after all ...

also, as long as we make sure we do not leak this out of edge we should be fine ...

@codecov-io
Copy link

codecov-io commented Feb 8, 2018

Codecov Report

Merging #4639 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4639      +/-   ##
==========================================
- Coverage   78.82%   78.81%   -0.01%     
==========================================
  Files         470      470              
  Lines       34072    34071       -1     
==========================================
- Hits        26857    26853       -4     
- Misses       5048     5050       +2     
- Partials     2167     2168       +1
Impacted Files Coverage Δ
store/store.go 80.71% <100%> (-0.02%) ⬇️
cmd/snap-seccomp/main.go 52.6% <0%> (-0.53%) ⬇️
overlord/snapstate/snapstate.go 81.12% <0%> (-0.21%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06b381b...4a426d4. Read the comment docs.

@mvo5
Copy link
Contributor Author

mvo5 commented Mar 2, 2018

Alex was kind enough to do some tests: https://forum.snapcraft.io/t/refresh-causes-download-of-full-snap-file-instead-of-delta/3470/19 and it looks like even on low end arm hardware the time to apply the delta is very reasonable. I will remove the blocked label and I think we should do this for 2.33 (which will give us plenty of time to test this in edge)

@mvo5 mvo5 removed the ⛔ Blocked label Mar 2, 2018
@mvo5 mvo5 merged commit f7ac698 into snapcore:master Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants