lib/pull: Add `timestamp-check-from-rev` #2099

jlebon · 2020-05-14T17:51:34Z

The way timestamp-check works might be too restrictive in some
situations. Essentially, we need to support the case where users want to
pull an older commit than the current tip, but while still guaranteeing
that it is newer than some even older commit.

This will be used in Fedora CoreOS. For more information see:
coreos/rpm-ostree#2094
coreos/fedora-coreos-tracker#481

The way `timestamp-check` works might be too restrictive in some situations. Essentially, we need to support the case where users want to pull an older commit than the current tip, but while still guaranteeing that it is newer than some even older commit. This will be used in Fedora CoreOS. For more information see: coreos/rpm-ostree#2094 coreos/fedora-coreos-tracker#481

cgwalters · 2020-05-14T18:00:43Z

This seems fine but...I don't see any downsides to changing libostree to do what rpm-ostree is doing by default instead.

jlebon · 2020-05-14T18:07:02Z

This seems fine but...I don't see any downsides to changing libostree to do what rpm-ostree is doing by default instead.

Hmm, can you expand? The pull code has no concept of deployments, right? How would it know what the base revision is?

cgwalters · 2020-05-14T18:11:21Z

Hmm, can you expand? The pull code has no concept of deployments, right? How would it know what the base revision is?

I think OstreeSysrootUpgrader would pass this in internally down to the pull code. But...I guess we forked that for rpm-ostree.

dustymabe · 2020-05-14T18:12:40Z

@cgwalters
This seems fine but...I don't see any downsides to changing libostree to do what rpm-ostree is doing by default instead.

I think I agree (if technically possible). The description of the behavior in coreos/rpm-ostree#2094 (comment) is what I thought downgrade protection meant universally:

For rpm-ostree, it means not deploying any commit which is older than what the current deployment is on.

@jlebon
Hmm, can you expand? The pull code has no concept of deployments, right? How would it know what the base revision is?

If that's true then do we need downgrade protection here (in addition to rpm-ostree)?

cgwalters · 2020-05-14T18:22:04Z

If that's true then do we need downgrade protection here (in addition to rpm-ostree)?

I want libostree to work on its own too - with this we aren't helping ostree admin upgrade. To be clear, fixing OstreeSysrootUpgrader isn't a blocker from my PoV, but let's keep in mind that it should probably be fixed for this too.

cgwalters · 2020-05-14T18:34:37Z

aside: I am pretty happy with how the "layers" in libostree ended up, but man there are some fun "layer-crossing" problems (this always happens).

dustymabe · 2020-05-14T18:41:59Z

If that's true then do we need downgrade protection here (in addition to rpm-ostree)?

I want libostree to work on its own too - with this we aren't helping ostree admin upgrade. To be clear, fixing OstreeSysrootUpgrader isn't a blocker from my PoV, but let's keep in mind that it should probably be fixed for this too.

Yep I was forgetting about libostree without rpm-ostree.

jlebon · 2020-05-14T19:05:37Z

I want libostree to work on its own too - with this we aren't helping ostree admin upgrade. To be clear, fixing OstreeSysrootUpgrader isn't a blocker from my PoV, but let's keep in mind that it should probably be fixed for this too.

Making the OSTree sysroot upgrader use this flag too WFM. I misunderstood your original comment as suggesting an alternative way to solve this.

For the same reason as coreos/rpm-ostree#2094. What we care most about is that the new commit we pull is newer than the one we're currently sitting on, not necessarily that it's newer than the branch itself, which it might not be if e.g. we're trying to deploy a commit older than the tip but still newer than the deployment (via `--override-commit`).

jlebon · 2020-05-14T19:28:02Z

Updated sysroot upgrader! ⬆️

This is a short-term hack until we can depend on the new `timestamp-check-from-rev` from ostree: ostreedev/ostree#2099 That way, we still get downgrade protection, but wrt the checked out deployment, not the local ref. For more information, see coreos#2094 coreos/fedora-coreos-tracker#481

jlebon · 2020-05-15T15:04:09Z

All green!

cgwalters · 2020-05-15T15:14:03Z

/lgtm
Though I'd also like to also change OstreeSysrootUpgrader to use the deployment at some point.

openshift-ci-robot · 2020-05-15T15:14:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [cgwalters,jlebon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jlebon · 2020-05-15T15:17:48Z

Though I'd also like to also change OstreeSysrootUpgrader to use the deployment at some point.

Isn't that what the last commit here is doing?

cgwalters · 2020-05-15T15:31:04Z

Yep, you're right, thanks!

This is a short-term hack until we can depend on the new `timestamp-check-from-rev` from ostree: ostreedev/ostree#2099 That way, we still get downgrade protection, but wrt the checked out deployment, not the local ref. For more information, see #2094 coreos/fedora-coreos-tracker#481

cgwalters · 2020-05-17T14:15:26Z

Hum interesting, this PR changing unrelated code failed here.

I haven't debugged why yet; at first I thought it might be we were making commits in under a second, but that still wouldn't appear as a downgrade. I wonder if it could be something like clock skew on the builder machine.

cgwalters · 2020-05-17T14:18:49Z

PASS: tests/test-admin-deploy-none.sh 21 no duplicate version strings in title
+++ env LD_PRELOAD=/home/jenkins/agent/workspace/ci_ostreedev_ostree_PR-2101-WEW6OONKDMYJBVECT3A3VL423YJK6UHLCQP2P777HY3QTQYSGOKA/tests/libreaddir-rand.so ostree rev-parse --repo=sysroot/ostree/repo testos/buildmaster/x86_64-runtime
++ head_rev=8f8e8eae4021f28891ef763dd8f1a3a9ec2662b55f78010ddb3891f4b9352ada
+++ env LD_PRELOAD=/home/jenkins/agent/workspace/ci_ostreedev_ostree_PR-2101-WEW6OONKDMYJBVECT3A3VL423YJK6UHLCQP2P777HY3QTQYSGOKA/tests/libreaddir-rand.so ostree rev-parse --repo=sysroot/ostree/repo 'testos/buildmaster/x86_64-runtime^^^^'
++ prev_rev=84ec817e489bf8a1e3db3c7f7232086319a892fca603a076b8efbe04ffe6333e
++ assert_not_streq 8f8e8eae4021f28891ef763dd8f1a3a9ec2662b55f78010ddb3891f4b9352ada 84ec817e489bf8a1e3db3c7f7232086319a892fca603a076b8efbe04ffe6333e
++ test 8f8e8eae4021f28891ef763dd8f1a3a9ec2662b55f78010ddb3891f4b9352ada = 84ec817e489bf8a1e3db3c7f7232086319a892fca603a076b8efbe04ffe6333e
++ env LD_PRELOAD=/home/jenkins/agent/workspace/ci_ostreedev_ostree_PR-2101-WEW6OONKDMYJBVECT3A3VL423YJK6UHLCQP2P777HY3QTQYSGOKA/tests/libreaddir-rand.so ostree admin upgrade --os=testos --override-commit=84ec817e489bf8a1e3db3c7f7232086319a892fca603a076b8efbe04ffe6333e
4 metadata, 0 content objects imported
Copying /etc changes: 0 modified, 0 removed, 0 added
Bootloader updated; bootconfig swap: yes; deployment count change: 0
++ fatal 'downgraded without --allow-downgrade?'
++ echo downgraded without '--allow-downgrade?'
downgraded without --allow-downgrade?
++ exit 1
+ run_exit_cmds
+ for expr in "${libtest_exit_cmds[@]}"
+ eval save_core
++ save_core
++ '[' -e core ']'
+ for expr in "${libtest_exit_cmds[@]}"
+ eval libtest_cleanup_gpg
++ libtest_cleanup_gpg
++ local gpg_homedir=/var/tmp/tap-test.vahbPW/gpghome
++ gpg-connect-agent --homedir /var/tmp/tap-test.vahbPW/gpghome killagent /bye
gpg-connect-agent: no running gpg-agent - starting '/usr/bin/gpg-agent'
gpg-connect-agent: waiting for the agent to come up ... (5s)
gpg-connect-agent: connection to agent established
Freed objects: 824 bytes
OK closing connection
ERROR: tests/test-admin-deploy-none.sh - too few tests run (expected 29, got 21)
ERROR: tests/test-admin-deploy-none.sh - exited with status 1

I saw `tests/test-admin-deploy.none.sh` fail in one CI run, and I want to check if it was because of clock skew, so fail fast if we detect that. xref ostreedev#2099 (comment)

jlebon · 2020-05-20T14:40:36Z

Hum interesting, this PR changing unrelated code failed here.

Ahhh I think I know what this is. In the first diff hunk in that file (https://github.com/ostreedev/ostree/pull/2099/files#diff-5092e9dd89d398d2a7b46be8e0dbe787R307), I added a sanity-check that ostree admin upgrade with an override to an older commit failed without --allow-downgrade, but the test isn't strictly ensuring that the two commits are at least one second apart. Will fix!

Otherwise the new check we added there to verify that upgrading without `--allow-downgrade` fails itself fails. See: ostreedev#2099 (comment)

jlebon · 2020-05-20T14:49:29Z

#2106

openshift-ci-robot requested review from d4s and rfairley May 14, 2020 17:51

openshift-ci-robot added the approved label May 14, 2020

This was referenced May 14, 2020

upgrader: Pull using new timestamp-check-from-rev coreos/rpm-ostree#2094

Closed

rpm-ostree/deploy: downgrade logic mistakenly blocks some upgrades coreos/fedora-coreos-tracker#481

Closed

jlebon force-pushed the pr/timestamp-check-from-rev branch from 2ae969e to c8efce0 Compare May 14, 2020 18:00

jlebon mentioned this pull request May 15, 2020

upgrader: Reset ref before fetching commit by override coreos/rpm-ostree#2096

Merged

openshift-ci-robot assigned cgwalters May 15, 2020

openshift-ci-robot added the lgtm label May 15, 2020

openshift-merge-robot merged commit 11b09ec into ostreedev:master May 15, 2020

cgwalters mentioned this pull request May 17, 2020

ci: Test for clock skew #2102

Merged

jlebon added a commit to jlebon/ostree that referenced this pull request May 20, 2020

tests/admin-test: Ensure that commits are 1s apart

70ebb91

Otherwise the new check we added there to verify that upgrading without `--allow-downgrade` fails itself fails. See: ostreedev#2099 (comment)

jlebon mentioned this pull request May 20, 2020

tests/admin-test: Ensure that commits are 1s apart #2106

Merged

cgwalters mentioned this pull request Dec 11, 2020

upgrade coredump scan_commit_object: assertion failed coreos/rpm-ostree#2377

Closed

jlebon deleted the pr/timestamp-check-from-rev branch April 24, 2023 02:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib/pull: Add `timestamp-check-from-rev` #2099

lib/pull: Add `timestamp-check-from-rev` #2099

jlebon commented May 14, 2020

cgwalters commented May 14, 2020

jlebon commented May 14, 2020

cgwalters commented May 14, 2020

dustymabe commented May 14, 2020

cgwalters commented May 14, 2020

cgwalters commented May 14, 2020

dustymabe commented May 14, 2020

jlebon commented May 14, 2020

jlebon commented May 14, 2020

jlebon commented May 15, 2020

cgwalters commented May 15, 2020

openshift-ci-robot commented May 15, 2020

jlebon commented May 15, 2020

cgwalters commented May 15, 2020

cgwalters commented May 17, 2020

cgwalters commented May 17, 2020

jlebon commented May 20, 2020

jlebon commented May 20, 2020

lib/pull: Add timestamp-check-from-rev #2099

lib/pull: Add timestamp-check-from-rev #2099

Conversation

jlebon commented May 14, 2020

cgwalters commented May 14, 2020

jlebon commented May 14, 2020

cgwalters commented May 14, 2020

dustymabe commented May 14, 2020

cgwalters commented May 14, 2020

cgwalters commented May 14, 2020

dustymabe commented May 14, 2020

jlebon commented May 14, 2020

jlebon commented May 14, 2020

jlebon commented May 15, 2020

cgwalters commented May 15, 2020

openshift-ci-robot commented May 15, 2020

jlebon commented May 15, 2020

cgwalters commented May 15, 2020

cgwalters commented May 17, 2020

cgwalters commented May 17, 2020

jlebon commented May 20, 2020

jlebon commented May 20, 2020

lib/pull: Add `timestamp-check-from-rev` #2099

lib/pull: Add `timestamp-check-from-rev` #2099