WIP: composetree: Allow url in manifest package list

[mike-nguyen]

This allows URLs to be used in the manifest file when doing a
treecompose only in unified-core mode. This is a convenient way of
adding one off packages for testing purposes but should not be the
preferred method of composing a tree.

Closes: #1572

[mike-nguyen]

I'm looking for some feedback since there are a lot of parts involved in the tree compose and I don't want to inadvertently break other functionality. Once feedback is addressed I'll add tests and update documentation and remove the WIP.

[dustymabe]

thanks @mike-nguyen

[cgwalters]

This is quite tricky as it crosses several of the internal layers.

One of the problematic things here is the implementation needs to be different between --unified-core mode vs not. In particular in --unified-core we need to import the package as an ostree ref.

We also need to carefully think about change detection. IMO this is a really valuable aspect of rpm-ostree.

It may be simplest to download the packages into a subdir of the tmpdir, run createrepo_c on that, and then add it as an rpm-md repo to the config?

[dustymabe]

This is quite tricky as it crosses several of the internal layers.

One of the problematic things here is the implementation needs to be different between --unified-core mode vs not. In particular in --unified-core we need to import the package as an ostree ref.

Should we consider removing non --unified-core mode in the future? I think we may need to modify the pungi ostree code, but that shouldn't be too hard to do.

We also need to carefully think about change detection. IMO this is a really valuable aspect of rpm-ostree.

+1. Even though I think of this feature as mostly being useful for development work, We definitely still want change detection to work.

[cgwalters]

Should we consider removing non --unified-core mode in the future?

That's #1739

[jlebon]

Haven't gotten around to reviewing this more in-depth yet, but just quickly:

• One thing I mentioned in RFE: support URLs to rpms in manifest.json files #1572 is using If-Modified-Since to make sure we only redownload if the file changed. The tricky bit is getting this to mesh with the "import into OSTree" path of unified core. We could store the timestamp as part of the commit metadata though. Or really, just a simple cache file to remember them.
• I think it'd be totally fine to make this exclusive to --unified-core to keep things simpler?
• Let's try to re-use the Rust libcurl helper to download these files.

[mike-nguyen]

This is quite tricky as it crosses several of the internal layers.

One of the problematic things here is the implementation needs to be different between --unified-core mode vs not. In particular in --unified-core we need to import the package as an ostree ref.

@cgwalters Is this okay if this feature is only enabled unified-core mode if the eventual plan is to remove non-unified-core mode? With this implementation, non-unified core mode will error out if the manifest file contains any URL packages. If we want this feature in both modes, then the createrepo_c suggestion might be the best path forward.

We also need to carefully think about change detection. IMO this is a really valuable aspect of rpm-ostree.

URL packages are downloaded are added to the sack using dnf_sack_add_cmdline_package (its repo gets set to @system) and added to the goal installation (we get some depsolving). These steps are performed before the input checksum so a compose will only occur if there are changes.

The URL packages are downloaded every time compose is run because package validation can't happen until it is on the filesystem. We could keep a list of URLs we have already download and added somewhere and cross reference that with the manifest if we didn't want to download the packages. The targeted use case was for testing a build of a package. Ideally users would not compose using all URLs and we would document this.

I've tested the following scenarios with result underneath:

• url packages in non-unified core mode
  compose errors out
• run compose with url package in unified core mode
  package is downloaded and imported into the pkgcache repo and gets picked up by the compose
• run compose twice with url package in unified core mode
  first compose successful, second compose detects no changes
• run compose with url in unified core mode that doesn't exist
  compose errors out with http error
• run compose with url package in unified core moe with missing dependency
  compose errors out with missing dependency error
• ran some package layering operations on the client since some code overlaps
  no immediate issues noticed with package layering

Let me know if this addresses any of the concerns or raises any other concerns. I am open to going the createrepo_c route also!

[jlebon]

OK, looking at this patch now in more details now. (First, thanks for working on this! 👍)

I think what'd be nice here is to reuse the core's ability to automatically fetch packages from the OSTree repo. See the path used on the client side for e.g. rpm-ostree install local-path-to-an.rpm: we first import the RPM into the OSTree repo (see import_local_rpm()), and then tell the core about it when creating the treespec (see generate_treespec()).

The idea here is that we keep the core simple by using existing paths and keeping the logic that deals with invalidating these RPMs separate. Ideally, prepare is only about, well, preparing for the more I/O intensive task of downloading all the yumrepo packages (rpmostree_context_download) and importing them (rpmostree_context_import).

We could keep a list of URLs we have already download and added somewhere and cross reference that with the manifest if we didn't want to download the packages.

I think that'd work to begin with, though it's something we'd want to fix eventually. A URL might not change yet point to a different RPM version. Likely not very common, but a gotcha I can imagine being frustrating to debug.

That said, I'd be totally fine with an initial approach that just redownloads everytime for now. And then we can iterate on making that smarter afterwards.

[jlebon]

So roughly, it'd look something like:

1. download RPM to cache
2. import it into the OSTree
3. add it to cached-packages in rpmostree_composeutil_get_treespec()

[mike-nguyen]

OK, looking at this patch now in more details now. (First, thanks for working on this! )

I think what'd be nice here is to reuse the core's ability to automatically fetch packages from the OSTree repo. See the path used on the client side for e.g. rpm-ostree install local-path-to-an.rpm: we first import the RPM into the OSTree repo (see import_local_rpm()), and then tell the core about it when creating the treespec (see generate_treespec()).

The idea here is that we keep the core simple by using existing paths and keeping the logic that deals with invalidating these RPMs separate. Ideally, prepare is only about, well, preparing for the more I/O intensive task of downloading all the yumrepo packages (rpmostree_context_download) and importing them (rpmostree_context_import).

We could keep a list of URLs we have already download and added somewhere and cross reference that with the manifest if we didn't want to download the packages.

I think that'd work to begin with, though it's something we'd want to fix eventually. A URL might not change yet point to a different RPM version. Likely not very common, but a gotcha I can imagine being frustrating to debug.

That said, I'd be totally fine with an initial approach that just redownloads everytime for now. And then we can iterate on making that smarter afterwards.

@jlebon Thanks for the feedback! I feel like we've had this conversation before 😏 I'll take another look at using import_local_rpm.

[cgwalters]

• run compose with url package in unified core mode
  package is downloaded and imported into the pkgcache repo and gets picked up by the compose

Hum really? Oh wait I see...right, find_pkg_in_ostree handles that. And right, that's how e.g. local overrides work too. OK that does indeed simplify things.

[rh-atomic-bot]

☔ The latest upstream changes (presumably 79dfcea) made this pull request unmergeable. Please resolve the merge conflicts.

[jlebon]

Is this superseded by #1847 then?

[mike-nguyen]

Is this superseded by #1847 then?

Yes abandoning this.