clearlinux / swupd-server

Software update server (deprecated)
Other
13 stars 17 forks source link

download required files via HTTP on demand #47

Closed pohly closed 6 years ago

pohly commented 7 years ago

At the moment, the assumption is that swupd-server has normal file access to the previous build artifacts. That's not true for the Ostro CI or Ostro developers doing local builds on their workstations.

This problem gets solved in two places:

This relies only on the fullfiles that already gets published. bsdtar is always used for unpacking because it can detect the compression method without an intermediate file.

Because calling external programs is problematic (see commit 557fb49), the initial proof-of-concept with external curl+bsdtar later gets replaced with an implementation using libcurl+libarchive directly. That relies on PR #48, which therefore gets included here.

pohly commented 7 years ago

Rebased onto current master.

I know I've called the current download via external curl "proof-of-concept". However, it is usable enough already that it makes sense to merge as it is and then enhance it later - IMHO. Keeping it out of master just increases the risk of avoidable merge conflicts.

pohly commented 7 years ago

Pushed a revised version of "create_pack: abort delta handling early when impossible due to IMA" which uses the lgetxattr implementation from libc (and thus sys/xattrs.h) instead of relying on libattr (= attr/xattrs.h). This is now consistent with src/xattrs.c and should solve the build issue in the TravisCI environment.

aaronovz1 commented 7 years ago

Can this be rebased and merged?

matthewrsj commented 7 years ago

@pohly a lot of time has passed since this PR was active, so I have a couple clarifying questions.

Depending on the size of the distro, this could trigger a download of a very large number of files, which could potentially take a very long time to complete. What is the specific downside of keeping the previous build artifacts around?

You mention developers doing builds on their local systems as a use-case, but local builds can be done using mixer without requiring swupd-server to reach out to an update server. Has this approach been considered?

pohly commented 7 years ago

@matthewrsj: previous content files are only needed when creating deltas. So it depends a lot on a) against which release deltas are generated and b) what has changed since then. In my experience for generating deltas against fairly new releases, downloading files wasn't such a big deal. If I remember correctly (as you said, it has been a while ;-) this runs multithreaded, so downloading can overlap with delta computation, which mitigates the effect.

The downside of keeping the previous build artifacts were:

At least for Ostro, the first two points were a problem. In the Ostro setup, each build starts with a clean slate with only HTTP(S) access to external resources, produces local artifacts that then get published. There is no persistent read/write filesystem that all builds have access to. This also prevents accidentally modifying that shared state when things go wrong.

@mythi, @okartau: can you elaborate on whether it would be possible to add a persistent read/write filesystem?

@matthewrsj: as builds don't run as root, we have to store the rootfs of old builds as tar archives. Does swupd-server already support unpacking files from archives on demand? Unpacking entire archives in preparation for a build would be overkill. So something conceptually similar to this PR would be needed (retrieve file on demand, whether via HTTP or archive on file system).

Regarding the "local developer build": this is conceptually the same as doing a new CI build and may include modifying the configuration or source of core components, so using mixer is not an option.

Remember that the goal here is to make swupd usable in a Yocto-based workflow. There are update tools which don't force developers to change their workflow. It would be nice if the same was possible also with swupd.

okartau commented 7 years ago

in Ostro CI (and more modern Refkit CI which is based on Ostro CI) there is a persistent shared R/W storage, mounted to builders via NFS. This storage is used for 2 purposes: downloaded sources cache (yocto DL_DIR), and shared state (yocto SSTATE). This store is not used for swupd-related functions.

pohly commented 7 years ago

@okartau is that R/W storage getting backed up? The rootfs files from previous builds must not get lost, as they can't be reconstructed (improvements in reproducible builds notwithstanding).

pohly commented 7 years ago

I confirmed that supporting a CI system with no shared filesystem is important and still relevant, even if Ostro/refkit don't need it anymore.

okartau commented 7 years ago

In ostro and refkit CI cases, this storage is not backed up as it contains cached, non-critical data that is possible to either fetch again (DL_DIR) or recreate in builds (SSTATE). Adding back-up, if needed, is of course possible.

pohly commented 6 years ago

On Wed, 2017-11-01 at 08:53 -0700, Matthew Johnson wrote:

@pohly would you please update this PR to use libcurl/libarchive like you mention in your comment?

I don't know whether I will have time for that going forward. If I can spend time on this, then I'm fine with updating the PR. Let's discuss the resourcing question internally and then track the work in an issue.

pohly commented 6 years ago

@matthewrsj I've started updating this PR to make use of libcurl and libarchive directly.

As I look at the code again, one question comes up: why does make_pack_deltas and everything that it calls ignore errors? It can of course proceed without the deltas, but the result isn't what it should be.

For example, suppose there is a download error because of some temporary server failure or some permanent error because the URL was misconfigured. Such errors should not be ignored. I'd expect swupd_make_pack to fail instead. What's the best way to achieve that? Add error reporting to all the various calls, or abort the process?

pohly commented 6 years ago

stage_entry() contains a fallback where it hardlinks the fullfile .tar archive into the staging area. Can anyone think of a situation where this is actually necessary? I can't, so to me this looks like dead code that doesn't need to be handled when implementing the on-demand-download.

I had a simple solution for this in commit 19621c056a36df870954062b989735f7fe0cda90, but now I'll probably just remove that entirely.

Looking at this once more I realized that it became dead code as part of adding directory copying in 44fd419462. I'll make it even simpler...

matthewrsj commented 6 years ago

why does make_pack_deltas and everything that it calls ignore errors? It can of course proceed without the deltas, but the result isn't what it should be.

Well, that code is over 5 years old. I expect the errors were ignored initially because packs are not necessary, but I agree that the errors should be handled correctly.

100 opened to track.

pohly commented 6 years ago

@matthewrsj this now ready for review.

pohly commented 6 years ago

Rebased and force-pushed.

Now that https://github.com/clearlinux/swupd-client/pull/327 is merged, swupd-client should be ready to handle archives produced with libarchive.

@matthewrsj Can we now review, test and merge this PR?

matthewrsj commented 6 years ago

Note: after email and IRC discussions, we are tabling this PR in this implementation of swupd-server for now as we rewrite swupd-server functionality into https://github.com/clearlinux/mixer-tools (via https://github.com/matthewrsj/mixer-tools). This functionality has been added to the list of work to do for the rewrite.

matthewrsj commented 6 years ago

Closing this PR as this project is no longer being actively developed. If this functionality is still required please open a PR or issue against https://github.com/clearlinux/mixer-tools (the new project)