Date: Mon, 23 Dec 2019 17:51:44 +0200
From: Timo Teras <timo.teras@iki.fi>
To: Drew DeVault <sir@cmpwn.com>
Cc: ~alpine/devel@lists.alpinelinux.org
Subject: Re: [PATCH] Split download and extract into separate steps
Message-ID: <20191223175144.2064159d@vostro>
In-Reply-To: <20191223141220.52218-1-sir@cmpwn.com>
References: <20191223141220.52218-1-sir@cmpwn.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit

Drew,

Thanks for working on this. This is something I was planning to
implement myself in Jan/Feb. Though, I'm first doing file format
changes and hoping to publish that work still this year. I hope you saw
the heads up I sent some days ago about that.

Along with that, I am planing to fix things like this. Though, I was
planning to change the overall system so that each package is extracted
immediately. But the final moving of files to in place with the real
name would done only after *all* packaged were extracted.

I am not decided yet if downloading packages to cache should be the
default when cache is enabled. Perhaps yes, especially since we are
seeing more setups where the same cache is shared across containers.

However, I'd rather not overload apk_db_unpack_pkg() for this purpose.
I was hoping that the commit code would instead schedule pure "fetch"
jobs. (This is by the way a place I'm hoping to rewrite completely to
be more "job" based and use thread pools to do the
download/fetch/extract/verify pipelines in separate threads. Then
synchronize and do the final renameat() as last step.)

Perhaps I should write my "road map" email sooner than later to
indicate how I'd hope things to be done.

Timo

On Mon, 23 Dec 2019 09:12:20 -0500
Drew DeVault <sir@cmpwn.com> wrote:

> This splits apk_db_install_pkg into two steps: APK_INSTALL_DOWNLOAD
> and APK_INSTALL_COMMIT, and accepts a bitfield of steps to perform.
> apk_solver_commit_changeset performs these steps as one if the cache
> is disabled. Otherwise, it downloads everything, then commits
> everything, in two steps.
> 
> By downloading everything upfront, we reduce the risk of leaving the
> system in a half-installed state should network issues come up partway
> through the upgrade. Additionally, this makes it safe to interrupt apk
> during the download step without a similar partially-updated-system
> problem.
> ---
> To onlookers: this could use some testing, please install it locally
> and let me know about any issues you run into.
> 
>  src/apk_database.h |  8 +++++
>  src/commit.c       | 73
> ++++++++++++++++++++++++++++++++-------------- src/database.c     |
> 50 +++++++++++++++++++++---------- 3 files changed, 93 insertions(+),
> 38 deletions(-)
> 
> diff --git a/src/apk_database.h b/src/apk_database.h
> index 2c8bdda..ee9f64b 100644
> --- a/src/apk_database.h
> +++ b/src/apk_database.h
> @@ -12,6 +12,8 @@
>  #ifndef APK_PKGDB_H
>  #define APK_PKGDB_H
>  
> +#include <stdint.h>
> +
>  #include "apk_version.h"
>  #include "apk_hash.h"
>  #include "apk_archive.h"
> @@ -192,6 +194,11 @@ typedef union apk_database_or_void {
>  	void *ptr;
>  } apk_database_t __attribute__ ((__transparent_union__));
>  
> +enum apk_install_operation {
> +	APK_INSTALL_DOWNLOAD 	= 1 << 0,
> +	APK_INSTALL_COMMIT 	= 1 << 1,
> +};
> +
>  struct apk_name *apk_db_get_name(struct apk_database *db, apk_blob_t
> name); struct apk_name *apk_db_query_name(struct apk_database *db,
> apk_blob_t name); int apk_db_get_tag_id(struct apk_database *db,
> apk_blob_t tag); @@ -261,6 +268,7 @@ int
> apk_db_cache_foreach_item(struct apk_database *db, apk_cache_item_cb
> cb); int apk_db_install_pkg(struct apk_database *db, struct
> apk_package *oldpkg, struct apk_package *newpkg,
> +		       uint32_t install_ops,
>  		       apk_progress_cb cb, void *cb_ctx);
>  
>  void apk_name_foreach_matching(struct apk_database *db, struct
> apk_string_array *filter, unsigned int match, diff --git
> a/src/commit.c b/src/commit.c index 4a94ff5..410c5c4 100644
> --- a/src/commit.c
> +++ b/src/commit.c
> @@ -27,7 +27,7 @@ static inline int pkg_available(struct apk_database
> *db, struct apk_package *pkg }
>  
>  static int print_change(struct apk_database *db, struct apk_change
> *change,
> -			int cur, int total)
> +		uint32_t step, int cur, int total)
>  {
>  	struct apk_name *name;
>  	struct apk_package *oldpkg = change->old_pkg;
> @@ -41,9 +41,22 @@ static int print_change(struct apk_database *db,
> struct apk_change *change, 
>  	name = newpkg ? newpkg->name : oldpkg->name;
>  	if (oldpkg == NULL) {
> -		msg = "Installing";
> +		switch (step) {
> +		case APK_INSTALL_DOWNLOAD | APK_INSTALL_COMMIT:
> +			msg = "Installing";
> +			break;
> +		case APK_INSTALL_DOWNLOAD:
> +			msg = "Fetching";
> +			break;
> +		case APK_INSTALL_COMMIT:
> +			msg = "Extracting";
> +			break;
> +		}
>  		oneversion = newpkg->version;
>  	} else if (newpkg == NULL) {
> +		if (!(step & APK_INSTALL_COMMIT)) {
> +			return TRUE;
> +		}
>  		msg = "Purging";
>  		oneversion = oldpkg->version;
>  	} else if (newpkg == oldpkg) {
> @@ -55,6 +68,9 @@ static int print_change(struct apk_database *db,
> struct apk_change *change, } else if (change->old_repository_tag !=
> change->new_repository_tag) { msg = "Updating pinning";
>  		}
> +		if (msg && !(step & APK_INSTALL_COMMIT)) {
> +			msg = "Fetching";
> +		}
>  		oneversion = newpkg->version;
>  	} else {
>  		r = apk_pkg_version_compare(newpkg, oldpkg);
> @@ -69,6 +85,9 @@ static int print_change(struct apk_database *db,
> struct apk_change *change, msg = "Upgrading";
>  			break;
>  		}
> +		if (msg && !(step & APK_INSTALL_COMMIT)) {
> +			msg = "Fetching";
> +		}
>  	}
>  	if (msg == NULL)
>  		return FALSE;
> @@ -259,7 +278,7 @@ int apk_solver_commit_changeset(struct
> apk_database *db, struct apk_changeset *changeset,
>  				struct apk_dependency_array *world)
>  {
> -	struct progress prog;
> +	struct progress prog, _prog;
>  	struct apk_change *change;
>  	char buf[32], size_unit;
>  	ssize_t size_diff = 0;
> @@ -321,26 +340,36 @@ int apk_solver_commit_changeset(struct
> apk_database *db, if (run_commit_hooks(db, PRE_COMMIT_HOOK) == -2)
>  		return -1;
>  
> +	uint32_t steps[3] = { APK_INSTALL_DOWNLOAD,
> APK_INSTALL_COMMIT, 0 };
> +	if (!apk_db_cache_active(db)) {
> +		steps[0] = APK_INSTALL_DOWNLOAD | APK_INSTALL_COMMIT;
> +		steps[1] = 0;
> +	}
> +
> +	memcpy(&_prog, &prog, sizeof(prog));
>  	/* Go through changes */
> -	foreach_array_item(change, changeset->changes) {
> -		r = change->old_pkg &&
> -			(change->old_pkg->ipkg->broken_files ||
> -			 change->old_pkg->ipkg->broken_script);
> -		if (print_change(db, change, prog.done.changes,
> prog.total.changes)) {
> -			prog.pkg = change->new_pkg;
> -			progress_cb(&prog, 0);
> -
> -			if (!(apk_flags & APK_SIMULATE) &&
> -			    ((change->old_pkg != change->new_pkg) ||
> -			     (change->reinstall && pkg_available(db,
> change->new_pkg)))) {
> -				r = apk_db_install_pkg(db,
> change->old_pkg, change->new_pkg,
> -						       progress_cb,
> &prog) != 0;
> +	for (int i = 0; steps[i]; ++i) {
> +		memcpy(&prog, &_prog, sizeof(prog));
> +		foreach_array_item(change, changeset->changes) {
> +			r = change->old_pkg && change->old_pkg->ipkg
> &&
> +				(change->old_pkg->ipkg->broken_files
> ||
> +
> change->old_pkg->ipkg->broken_script);
> +			if (print_change(db, change, steps[i],
> prog.done.changes, prog.total.changes)) {
> +				prog.pkg = change->new_pkg;
> +				progress_cb(&prog, 0);
> +
> +				if (!(apk_flags & APK_SIMULATE) &&
> +				    ((change->old_pkg !=
> change->new_pkg) ||
> +				     (change->reinstall &&
> pkg_available(db, change->new_pkg)))) {
> +					r = apk_db_install_pkg(db,
> change->old_pkg, change->new_pkg,
> +
> steps[i], progress_cb, &prog) != 0;
> +				}
> +				if (r == 0 && change->new_pkg &&
> change->new_pkg->ipkg)
> +
> change->new_pkg->ipkg->repository_tag = change->new_repository_tag; }
> -			if (r == 0 && change->new_pkg &&
> change->new_pkg->ipkg)
> -
> change->new_pkg->ipkg->repository_tag = change->new_repository_tag;
> +			errors += r;
> +			count_change(change, &prog.done);
>  		}
> -		errors += r;
> -		count_change(change, &prog.done);
>  	}
>  	apk_print_progress(prog.total.bytes + prog.total.packages,
>  			   prog.total.bytes + prog.total.packages);
> @@ -625,7 +654,7 @@ void apk_solver_print_errors(struct apk_database
> *db,
>  	 *   b-1:
>  	 *     satisfies: world[b]
>  	 *     conflicts: a-1[foo]
> -	 * 
> +	 *
>  	 *   c-1:
>  	 *     satisfies: world[a]
>  	 *     conflicts: c-1[foo]  (self-conflict by providing foo
> twice) @@ -641,7 +670,7 @@ void apk_solver_print_errors(struct
> apk_database *db,
>  	 * satisfies lists all dependencies that is not satisfiable
> by
>  	 * any other selected version. or all of them with -v.
>  	 */
> - 
> +
>  	apk_error("unsatisfiable constraints:");
>  
>  	/* Construct information about names */
> diff --git a/src/database.c b/src/database.c
> index c699444..6741b67 100644
> --- a/src/database.c
> +++ b/src/database.c
> @@ -2767,7 +2767,8 @@ static void apk_db_migrate_files(struct
> apk_database *db, 
>  static int apk_db_unpack_pkg(struct apk_database *db,
>  			     struct apk_installed_package *ipkg,
> -			     int upgrade, apk_progress_cb cb, void
> *cb_ctx,
> +			     int upgrade, uint32_t install_opts,
> +			     apk_progress_cb cb, void *cb_ctx,
>  			     char **script_args)
>  {
>  	struct install_ctx ctx;
> @@ -2777,7 +2778,7 @@ static int apk_db_unpack_pkg(struct
> apk_database *db, struct apk_package *pkg = ipkg->pkg;
>  	char file[PATH_MAX];
>  	char tmpcacheitem[128], *cacheitem =
> &tmpcacheitem[tmpprefix.len];
> -	int r, filefd = AT_FDCWD, need_copy = FALSE;
> +	int r = 0, filefd = AT_FDCWD, need_copy = FALSE;
>  
>  	if (pkg->filename == NULL) {
>  		repo = apk_db_select_repo(db, pkg);
> @@ -2797,20 +2798,31 @@ static int apk_db_unpack_pkg(struct
> apk_database *db, }
>  		need_copy = TRUE;
>  	}
> -	if (!apk_db_cache_active(db))
> +	if (!apk_db_cache_active(db)) {
>  		need_copy = FALSE;
> -
> -	bs = apk_bstream_from_fd_url(filefd, file);
> -	if (IS_ERR_OR_NULL(bs)) {
> -		r = PTR_ERR(bs);
> -		if (r == -ENOENT && pkg->filename == NULL)
> -			r = -EAPKSTALEINDEX;
> -		goto err_msg;
> -	}
> -	if (need_copy) {
> +	} else {
> +		/* Attempt to install from cache, if present */
>  		apk_blob_t b = APK_BLOB_BUF(tmpcacheitem);
>  		apk_blob_push_blob(&b, tmpprefix);
>  		apk_pkg_format_cache_pkg(b, pkg);
> +		cache_bs = apk_bstream_from_file(db->cache_fd,
> cacheitem);
> +		if (!IS_ERR_OR_NULL(cache_bs)) {
> +			bs = cache_bs;
> +			need_copy = FALSE;
> +		}
> +	}
> +
> +	if (!bs) {
> +		bs = apk_bstream_from_fd_url(filefd, file);
> +		if (IS_ERR_OR_NULL(bs)) {
> +			r = PTR_ERR(bs);
> +			if (r == -ENOENT && pkg->filename == NULL)
> +				r = -EAPKSTALEINDEX;
> +			goto err_msg;
> +		}
> +	}
> +
> +	if (need_copy && (install_opts & APK_INSTALL_DOWNLOAD)) {
>  		cache_bs = apk_bstream_tee(bs, db->cache_fd,
> tmpcacheitem, 1, NULL, NULL); if (!IS_ERR_OR_NULL(cache_bs))
>  			bs = cache_bs;
> @@ -2831,11 +2843,14 @@ static int apk_db_unpack_pkg(struct
> apk_database *db, };
>  	apk_sign_ctx_init(&ctx.sctx, APK_SIGN_VERIFY_IDENTITY,
> &pkg->csum, db->keys_fd); tar = apk_bstream_gunzip_mpart(bs,
> apk_sign_ctx_mpart_cb, &ctx.sctx);
> -	r = apk_tar_parse(tar, apk_db_install_archive_entry, &ctx,
> TRUE, &db->id_cache);
> +	if (install_opts & APK_INSTALL_COMMIT)
> +		r = apk_tar_parse(tar, apk_db_install_archive_entry,
> &ctx, TRUE, &db->id_cache);
> +	else
> +		r = apk_tar_parse(tar, apk_sign_ctx_verify_tar,
> &ctx.sctx, FALSE, &db->id_cache); apk_sign_ctx_free(&ctx.sctx);
>  	apk_istream_close(tar);
>  
> -	if (need_copy) {
> +	if (need_copy && (install_opts & APK_INSTALL_DOWNLOAD)) {
>  		if (r == 0) {
>  			renameat(db->cache_fd, tmpcacheitem,
> db->cache_fd, cacheitem); pkg->repos |= BIT(APK_REPOSITORY_CACHED);
> @@ -2854,7 +2869,8 @@ err_msg:
>  }
>  
>  int apk_db_install_pkg(struct apk_database *db, struct apk_package
> *oldpkg,
> -		       struct apk_package *newpkg, apk_progress_cb
> cb, void *cb_ctx)
> +		struct apk_package *newpkg, uint32_t install_opts,
> +		apk_progress_cb cb, void *cb_ctx)
>  {
>  	char *script_args[] = { NULL, NULL, NULL, NULL };
>  	struct apk_installed_package *ipkg;
> @@ -2870,6 +2886,8 @@ int apk_db_install_pkg(struct apk_database *db,
> struct apk_package *oldpkg, 
>  	/* Just purging? */
>  	if (oldpkg != NULL && newpkg == NULL) {
> +		if (!(install_opts & APK_INSTALL_COMMIT))
> +			goto ret_r;
>  		ipkg = oldpkg->ipkg;
>  		if (ipkg == NULL)
>  			goto ret_r;
> @@ -2893,7 +2911,7 @@ int apk_db_install_pkg(struct apk_database *db,
> struct apk_package *oldpkg, }
>  
>  	if (newpkg->installed_size != 0) {
> -		r = apk_db_unpack_pkg(db, ipkg, (oldpkg != NULL),
> +		r = apk_db_unpack_pkg(db, ipkg, (oldpkg != NULL),
> install_opts, cb, cb_ctx, script_args);
>  		if (r != 0) {
>  			if (oldpkg != newpkg)