-
Notifications
You must be signed in to change notification settings - Fork 305
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
WIP libostree: Add support for finding remote URIs by ref name #782
Conversation
e58c07b
to
aec1f6c
Compare
configure.ac
Outdated
@@ -75,7 +75,7 @@ AM_PATH_GLIB_2_0(,,AC_MSG_ERROR([GLib not found])) | |||
dnl When bumping the gio-unix-2.0 dependency (or glib-2.0 in general), | |||
dnl remember to bump GLIB_VERSION_MIN_REQUIRED and | |||
dnl GLIB_VERSION_MAX_ALLOWED in Makefile.am | |||
GIO_DEPENDENCY="gio-unix-2.0 >= 2.40.0" | |||
GIO_DEPENDENCY="gio-unix-2.0 >= 2.50.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's factor this out as a separate commit? I think the main drag here is Debian Jessie. Still somewhat old is the Fedora 24 glib2. I honestly don't care about F24 myself but anyways, let's discuss this in a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve factored it out twice: e03fd8f bumps the dependency to 2.44, which is what it should be on master
, since g_autoptr()
requires that. 4e288b0 increases the maximum dependency to 2.50, which is needed for g_drive_is_removable()
. If 2.50 is not available, the g_drive_is_removable()
call is dropped, which hits performance but not functionality (technically it does, but in reality it’s not a problem).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, OK this is a lot of new code 😄. I have only skimmed it so far. Quality looks generally excellent (as expected).
I think what would help me in the short term is a few comments in strategic places - like PointerTable
- I found myself wondering what the heck that is.
Having this be a compile time option is in line with how we do everything else so far, but is going to put me in an interesting position for Fedora at least, since ostree is used for Atomic Host, and while I think Avahi is potentially interesting for bare metal servers, in those types of controlled environments people tend to have more "static" solutions. And Avahi doesn't make much sense at all in a virtualized cloud (AWS/GCE or private). Dunno. Making it a plugin seems like it'd be quite painful though.
Will read more this week. One thing I'd say though is please try to keep a feed of smaller PRs going. Maybe for example we could merge the bloom filter implementation and tests.
AvahiStringList *l; | ||
g_autoptr(GHashTable) out = NULL; | ||
|
||
out = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, (GDestroyNotify) g_bytes_unref); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine, but I'll note we're using C99 decl-after-stmt in more places in ostree now. See this PR for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ll try and bear that in mind, although I personally prefer putting all declarations at the start of a block since it helps me visualise the state in a function more easily (and whether there’s too much state and some refactoring is needed).
I’ve spruced up the comments in all the commits except the bloom filter and Avahi ones (haven’t had time to do that yet). I will continue working on this.
API-wise, the Avahi stuff is all contained in an implementation of a
I’ve split it up a bit more. None of the commits are ready for merging yet apart from the really trivial ones. Any commit which is not ready for merging says so in its commit message. Note that the APIs in 313b35f have not actually been tested yet. I’d like to get a bit more feedback on the APIs (primarily, commit 313b35f) before spending much more time on polishing or testing the other commits. |
/* Resolve all symlinks in @file’s path, returning a normalised, canonical path | ||
* containing no symlinks. This returns the same errors as expand_symlink(). */ | ||
static GFile * | ||
expand_all_symlinks (GFile *file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this just realpath()
? As a general note...for most newer ostree code I tend to have a preference to avoid GFile
, at least when accessing a real FS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it’s effectively a version of realpath()
for GFile
s. Using GFile
here is a bit pointless, since the file system has to be local in order to have an OSTree repository on it, so GFile
brings no benefits. I’ll try and find time to rewrite this to use FDs.
g_autoptr(GFile) ostree_dir = NULL; | ||
g_autoptr(GFile) repos_dir = NULL; | ||
g_autoptr(GFile) repo_dir = NULL; | ||
g_autoptr(GFile) expanded_mount_root = NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This kind of thing is where I think the C99 style comes out nicer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah. I’ll see how it looks after rewriting to use FDs.
src/libostree/ostree-repo-finder.h
Outdated
/* TODO: Should I extend this to also support mirrors and metalink? Maybe add a GVariant *metadata too? | ||
* For example, should a result be able to force GPG verification? */ | ||
/* TODO: would it support torrenting? */ | ||
gchar *uri; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason the result isn't just a remote? Among other things, this seems like it'll lose e.g. TLS CA pinning configuration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In what sense? Do you mean it should be OstreeRemote *remote
rather than gchar *uri
? I think that would make sense; I didn’t realise OstreeRemote
existed.
That would mean ensuring that all the code which handles OstreeRemote
s doesn’t assume they’re backed by a configuration file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also mean making OstreeRemote
public (since OstreeRepoFinderResult
is public). What do you think about that? If so, I’d be inclined to make it a GInterface
to allow for multiple remote implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a lot of API surface today which takes remote names as strings. It seems OK to me if this is just s,gchar *uri,gchar *remote,
- right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And transparently create some kind of a transient remote internally, with some made-up name (I would probably just use the URI as the name)? Sounds reasonable to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I was thinking about when I made the original comment is - what about the case where we have a predefined remote for the host-ostree and it's all that's available? Does the "find process" return the empty set, then the app (eos-updater) needs to fall back to using it? Or is the existing remote integrated into it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synthesizing a remote internally would be good. Right now it's remotes that specify GPG for example (and in some cases which key). So perhaps this API should take a remote as input and use its GPG config?
I think we’re agreed on synthesising a remote and doing s/gchar *uri/gchar *remote_name/
then. 👍
I guess this is where things get interesting - in the case where host-ostree+flatpak are in /ostree/repo (your case AIUI) - do we expect to update both in one "transaction"?
You are correct in your understanding — in Endless OS, both OS and flatpak objects are stored in /ostree/repo
, but they come from several remotes (4, at the current count).
I would expect both to be pulled in one transaction, iff the set of ref names passed to ostree_repo_find_remotes()
includes OS and flatpak refs. Deploying those refs is a separate issue, and one which I would not necessarily expect to be atomic (although that would be interesting).
I don’t think this is a requirement which changes the implementation: if we were to limit to updating refs from the OS separately from flatpak refs, for example, the code should still support pulling from multiple remote URIs if it found multiple peers on the LAN. Pulling from multiple remote URIs for multiple refs is not more complex, as far as I can see.
(Technically we could limit to pulling from a single remote URI per ref, and that would be a simplification; but at the cost of not being able to use a single transaction to pull ref sets A+B if set A and set B are only available on separate LAN peers. I’d rather not have that limitation.)
Currently, the refname approach allows it, but I think it'd be easier to implement if the "find process" was per remote. We don't expect people to have a lot of those in general. (I.e. we're not doing this per app).
I think we might be talking cross-purposes again. ostree_repo_find_remotes()
can’t be per-remote, since at the time it’s called, the possible remotes (including LAN peers and mount points) haven’t been enumerated. Are you using ‘remote’ here to mean ‘a particular repository and set of ref names which are published somewhere by a vendor’ rather than ‘a URI to pull from’?
What I was thinking about when I made the original comment is - what about the case where we have a predefined remote for the host-ostree and it's all that's available? Does the "find process" return the empty set, then the app (eos-updater) needs to fall back to using it? Or is the existing remote integrated into it?
As the branch currently stands, ostree_repo_find_remotes()
will return the empty set, but that’s not the intention. I think clients shouldn’t have to do any fallbacks from ostree_repo_find_remotes()
.
My original intention was that OstreeRepoFinderConfig
would supersede the [remote "blah"]
groups in /ostree/repo/config
with /etc/ostree/refs.d/$ref_name
files instead, but I admit I haven’t actually worked out a transition path for it. I suspect we might need an additional OstreeRepoFinderOldConfig
(strawman name) which returns results from the OstreeRepo
’s config
file. It would return all the configured remotes from there, then the code in ostree_repo_find_remotes()
which downloads the summary files and checks them would filter out the refs which aren’t in each remote (i.e. it would assign the OS refs to the OS remote and the flatpak refs to the flatpak remote). That would mean downloading more summary
files than ostree_repo_pull()
currently does (one per remote, rather than one overall), but this shouldn’t really be a problem given that they’re cached locally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I've been thinking of this is that the RepoFinder is basically a much fancier mirrorlist
type implementation for existing remotes. Basically, flatpak update
would try using repofinder, and if that was empty, would just fall back to its existing remote.
It sounds like you're thinking of this more like "eos-updater does everything"? What would flatpak update
do? Do you see it being ported to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I've been thinking of this is that the RepoFinder is basically a much fancier
mirrorlist
type implementation for existing remotes. Basically,flatpak update
would try using repofinder, and if that was empty, would just fall back to its existing remote.
I think we’re still talking a bit cross-purposes here. What do you mean by ‘existing remote’? The Avahi stuff, by its very nature, cannot rely on ‘existing remotes’ since the set of peers on the local network can vary.
It sounds like you're thinking of this more like "eos-updater does everything"? What would
flatpak update
do? Do you see it being ported to this?
Correct, I am thinking of this as effectively a replacement for the existing ostree_repo_pull()
API. flatpak update
would definitely be ported to this, and would pass it the refs for the set of flatpaks it wants to update (most likely, all the flatpaks the user has installed).
I hope I mentioned this before (I probably didn’t): the overall goal here is that flatpak can do updates from LAN and USB remotes, just like eos-updater
can; and that the code bases for the two converge so that all the underlying LAN and USB functionality is in libostree. We probably eventually want to gut eos-updater
and move it into a GNOME Software plugin, but that’s less relevant here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Synthesizing a remote internally would be good. Right now it's remotes that specify GPG for example (and in some cases which key). So perhaps this API should take a remote as input and use its GPG config?
I think we’re agreed on synthesising a remote and doing
s/gchar *uri/gchar *remote_name/
then. 👍
Gah, this won’t work, since the remotes are cached in the OstreeRepo
, which the OstreeRepoFinder
implementations don’t have access to (and it doesn’t make sense for them to have access to). I’ll see what I can come up with, but it might mean making OstreeRemote
public (even if most API continues to use const gchar *remote
to access remotes by name, for simplicity).
tests/test-repo-finder-config.c
Outdated
|
||
/* Recursively delete @file and its children. @file may be a file or a directory. */ | ||
static gboolean | ||
rm_rf (GFile *file, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is glnx_shutil_rm_rf_at()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, thanks. That’s a good incentive for me to FD-ise this code.
As far as Avahi, compile time is OK for now. Particuarly if we can make it a plugin down the line, and I think we could? |
src/libostree/ostree-repo-pull.c
Outdated
GCancellable *cancellable, | ||
GError **error) | ||
{ | ||
/* TODO: Support disabling certain repo finders. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The thing that has me hesitating a bit about the "try everything" model - it feels like we'll likely end up needing to hardcode things like "prefer USB finder first". But even there, I worry about needing some mechanism for the client app to configure individual finders. We can theoretically do any arbitrary extensions later via new API, but it's worth thinking about a bit now maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve changed this locally to take an OstreeRepoFinder **finders
parameter. If that’s NULL
then a list of default finders will be used.
src/libostree/ostree-repo-pull.c
Outdated
} | ||
} | ||
|
||
/* TODO: ensure this checks the commit signature; or that the checksum matches the downloaded content */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't do either of those.
src/libostree/ostree-repo-pull.c
Outdated
refs_to_pull->pdata); | ||
|
||
/* TODO: Progress will yo-yo here. */ | ||
ostree_repo_pull_with_options (self, result->uri, local_options, progress, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah...would be really messy to try to drive multi-remote logic down farther unfortunately.
We could indeed make it a plugin further down the line, as long as we were careful about the API for determining which |
@cgwalters and I have discussed this over the course of a couple of calls, and I think we’ve now got a direction sorted out for it. The new API is roughly the right shape, and we’re happy with the split from I need to spend a few days updating this branch to reflect what we’ve decided on (which it currently doesn’t). I’ve split some of the minor, unrelated, changes out into PR #810. Notes from the calls2017-04-21How does updating a single app work in future? -> provide a single ref eliminate the preordained knowledge about binding between refs and remote configurations BitTorrent is an (eventual) use case: it could be used within data centres, for example Need an ostree command line tool to visualise where content can come from (i.e. the results of find_remotes()) Philip to think about how GPG keys are handled in ostree_repo_pull() If we ignore the remote config when we pull from multiple refs, we lose TLS pinning Use case for pulling from the internet remote first: if you got the app/OS from the internet, and expect to continue getting updates from there, you would want to know if somehow you randomly started getting updates from somewhere else — the corporate workspace environment When do we pull the superblock? better define the split between metadata and data How do we get progress information out of ostree_repo_pull_from_remotes()? we want stats per ref => does this fit into OstreeAsyncProgress? Can branches= in /ostree/repo/config replace the existing refs.d code in the lan-and-usb branch? Actions:
2017-04-25Colin is open to solving the problem with a new API:
Could retrofit this onto ostree_repo_pull() using caching in the OstreeRepo instance
Discussion about refactoring existing pull on top:
API stability:
refs.d vs config: 0 hits in flatpak code; need to ask Alex or Matthias why it exists and whether we can make it useful for OstreeRepoFinderConfig
Discussion about remotes, GPG keys, and bindings:
Actions:
|
☔ The latest upstream changes (presumably cbe3989) made this pull request unmergeable. Please resolve the merge conflicts. |
974a8d8
to
d27fa6f
Compare
☔ The latest upstream changes (presumably f3cc0eb) made this pull request unmergeable. Please resolve the merge conflicts. |
a1541c7
to
693e44f
Compare
☔ The latest upstream changes (presumably 9690a54) made this pull request unmergeable. Please resolve the merge conflicts. |
☔ The latest upstream changes (presumably 63497c6) made this pull request unmergeable. Please resolve the merge conflicts. |
2793957
to
43c487b
Compare
☔ The latest upstream changes (presumably add88c3) made this pull request unmergeable. Please resolve the merge conflicts. |
a549f3c
to
4f210d3
Compare
Add an initial OstreeRepoFinder interface (but no implementations), which will find remote URIs by ref names, which we consider to be globally unique. This is an API preview. It compiles, but has not been tested beyond that, and the implementation is unfinished in various places, and unpolished everywhere else. It is intended as a strawman for the proposed API. The new API is used in a new ostree_repo_find_updates() function, which resolves a list of ref names to update into a set of remote URIs to pull them from, which can be treated as mirrors. It is an attempt to generalise resolution of the URIs to pull from, and to generalise determination of the order and parallelisation which they should be downloaded from in. Includes fixes by Krzesimir Nowak <[email protected]>. Signed-off-by: Philip Withnall <[email protected]>
This is a basic implementation of OstreeRepoFinder which resolves ref names to remote URIs by looking them up in configuration files in /etc/ostree/refs.d. Unit tests are included. Signed-off-by: Philip Withnall <[email protected]>
This is a basic implementation of OstreeRepoFinder which resolves ref names to remote URIs by looking for them on any currently mounted removable storage volumes. The idea is to support OS and app updates via USB stick. This commit is unpolished and not ready to be merged. It is intended as a strawman for feedback. Unit tests are included. This bumps libostree’s maximum GLib dependency from 2.44 to 2.50 for g_drive_is_removable(). If GLib 2.50 is not available, the call which needs it will be omitted and the OstreeRepoFinderMount implementation will scan all volumes (not just removable ones); this is a performance hit, but not a functionality hit. Signed-off-by: Philip Withnall <[email protected]>
This will be used in an upcoming commit. It adds a basic bloom filter implementation, using the SipHash family of hash functions. The implementation (including its parameter choices and hash functions) will become a protocol detail in future, so must not be changed so that its output is bitwise incompatible between OSTree versions. This commit is unpolished and not ready to be merged. It is intended as a strawman for feedback. Unit tests are included. Signed-off-by: Philip Withnall <[email protected]>
This is a more complex implementation of OstreeRepoFinder which resolves ref names to remote URIs by looking for refs advertised by peers on the local network using DNS-SD records and mDNS (Avahi). The idea is to allow OS and app updates to be propagated over local networks, without the internet. It requires an OSTree server and code to generate the DNS-SD adverts in order to be fully functional — support for this will be added separately. This commit is unpolished and not ready to be merged. It is intended as a strawman for feedback. Unit tests are included. Includes fixes by Krzesimir Nowak <[email protected]>. Signed-off-by: Philip Withnall <[email protected]>
This is a wrapper around the new ostree_repo_find_remotes() method; it tries to find available remotes which can serve updates for the user-provided refs. Signed-off-by: Philip Withnall <[email protected]>
This will pull the remotes after finding them. This potentially needs to go in its own pull-from-remotes built-in command, but it will be fine here for now. Signed-off-by: Philip Withnall <[email protected]>
I can't get the damn thing to link
need to add the remote to the repo just in case. usually remotes we get from avahi are not added, so reading configuration in the pulling code fails. also, do not clear the error prematurely - that caused the find-remotes --pull command to "succeed" while actually not pulling anything at all.
I’ve opened a new version of this as PR #924, ready for review. |
This is a strawman PR created for basic feedback on the proposed API and general implementation of support for resolving remote URIs from ref names (as discussed on the mailing list). It’s not a request for detailed feedback. I’ll repurpose this PR later (or create a new one) once the basic API is agreed.
WIP work to add an OstreeRepoFinder interface and implementations which
find remote URIs by ref names, which we consider to be globally unique.
This is an API preview. It compiles, but has not been tested beyond
that, and the implementation is unfinished in various places, and
unpolished everywhere else. It is intended as a strawman for the
proposed API.
The new API is used in a new ostree_repo_find_updates() function, which
resolves a list of ref names to update into a set of remote URIs to pull
them from, which can be treated as mirrors.
A major feature this adds is the ability to automatically pull from
other machines on the local network (as long as they are running an
OSTree server and appropriate Avahi DNS-SD adverts, support for which
will be added separately). Similarly, it adds the ability to pull from
removable file systems without needing further configuration.
This bumps the GLib dependency to 2.50, and adds an optional dependency
on Avahi.
Signed-off-by: Philip Withnall [email protected]