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

cmdlib.sh: Switch to using OSTree layers for overlay/ #555

Merged
merged 4 commits into from
Jun 14, 2019

Conversation

jlebon
Copy link
Member

@jlebon jlebon commented Jun 12, 2019

So now that we have OSTree layer capabilities in rpm-ostree
(coreos/rpm-ostree#1830), I think it
makes more sense overall to make the overlay an OSTree layer.

The fact that it is an RPM, but differs from all the other RPMs in
various ways will (and already has) come up multiple times when we talk
about signing, changelogs, lockfiles, overrides, etc... (See [1] and [2]
for some already filed issues). Switching to an OSTree layer sidesteps
many of those issues.

Of course, we do lose some things from moving away from RPM:

  1. it's harder to determine from where the overlay files come from on a
    host system (see [3] related to this)
  2. invisible to rpm-ostree db diff (but then again, the diff we
    currently get isn't helpful)
  3. not possible to issue an advisory for it to be consumed like the rest
    of the advisories (though with the current approach, making use of
    this without going through Bodhi would've required some non-trivial
    amount of work too)

Overall though, I think those are things we could (and should strive to)
fix in rpm-ostree rather than avoiding layers entirely.

[1] #544
[2] #553 (comment)
[3] coreos/rpm-ostree#1789

It's an archive repo, so we don't need privs to commit to it. Also use
the `${tmprepo}` var.
Use `-n` so that the OSTree checksum that `ostree commit` prints out is
on the same line.
@jlebon
Copy link
Member Author

jlebon commented Jun 12, 2019

BTW, this totally sidesteps how/whether the lockfile should interact with OSTree layers. And I think there is definitely a use case there for it to be supported in rpm-ostree in some way. But for our purposes I think using a lockfile for the overlay isn't necessary because it's already tied to the specific git commit we're building on.

src/cmdlib.sh Show resolved Hide resolved
src/cmdlib.sh Outdated Show resolved Hide resolved
src/cmdlib.sh Show resolved Hide resolved
@dustymabe
Copy link
Member

will try this out!

So now that we have OSTree layer capabilities in rpm-ostree
(coreos/rpm-ostree#1830), I think it
makes more sense overall to make the overlay an OSTree layer.

The fact that it *is* an RPM, but differs from all the other RPMs in
various ways will (and already has) come up multiple times when we talk
about signing, changelogs, lockfiles, overrides, etc... (See [1] and [2]
for some already filed issues). Switching to an OSTree layer sidesteps
many of those issues.

Of course, we do lose some things from moving away from RPM:
1. it's harder to determine from where the overlay files come from on a
   host system (see [3] related to this)
2. invisible to `rpm-ostree db diff` (but then again, the diff we
   currently get isn't helpful)
3. not possible to issue an advisory for it to be consumed like the rest
   of the advisories (though with the current approach, making use of
   this without going through Bodhi would've required some non-trivial
   amount of work too)

Overall though, I think those are things we could (and should strive to)
fix in rpm-ostree rather than avoiding layers entirely.

[1] coreos#544
[2] coreos#553 (comment)
[3] coreos/rpm-ostree#1789
That way the commit checksum itself is completely stable when
recomposing on the same git commit (modulo a dirty directory, but then
the checksum will change anyway).

Doesn't really matter for now (except for the good feelsies of always
getting the same commit hash between invocations), though it will if we
want to start recording more information about constituent layers in the
compose.
@dustymabe
Copy link
Member

ok I think this mostly looks good. one question about the underlying layers feature and one about the way this is implemented:

  1. In a cacheless model will we get a new layer every time (i.e. will change detection work?)
  2. Can we get the layer reflected in the verbose rpm-ostree output:
[core@coreos ~]$ rpm-ostree status --verbose
State: idle
AutomaticUpdates: disabled
Deployments:
● ostree://fedora-coreos:fedora/x86_64/coreos/testing
        OstreeRemoteStatus: Remote "fedora-coreos" not found
                   Version: 30.3 (2019-06-14T17:40:58Z)
                    Commit: 5a941e92aac88d299adb00f1044eafbc11058c16e11a7a6ab16fa5939d1fdb79
                            ├─ fedora-coreos-pool ((invalid timestamp))
                            ├─ coreos-assembler-local-overrides ((invalid timestamp))
                            ├─ fedora ((invalid timestamp))
                            └─ fedora-updates ((invalid timestamp))
                    Staged: no
                 StateRoot: fedora-coreos

of course invalid timestamp is coreos/rpm-ostree#1734

@jlebon
Copy link
Member Author

jlebon commented Jun 14, 2019

In a cacheless model will we get a new layer every time (i.e. will change detection work?)

Yup, will still work. RPM-OSTree knows to look at the content checksum, not the commit checksum itself (though note in this patch, you'll also get exactly the same commit checksum each time for a given overlay/ & git commit).

Can we get the layer reflected in the verbose rpm-ostree output:

Yeah, we should probably open an issue about the UX around layers.

@jlebon
Copy link
Member Author

jlebon commented Jun 14, 2019

Yeah, we should probably open an issue about the UX around layers.

coreos/rpm-ostree#1857

Copy link
Member

@dustymabe dustymabe left a comment

Choose a reason for hiding this comment

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

LGTM

@jlebon jlebon merged commit 60cfa88 into coreos:master Jun 14, 2019
@jlebon jlebon deleted the pr/overlay-overlay branch July 6, 2020 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants