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

Keep last version of deployment #1960

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Conversation

r4f4
Copy link
Contributor

@r4f4 r4f4 commented Oct 28, 2019

Putting this up for initial review while I try to work on adding tests.
This PR is implementing the solution proposed in coreos/rpm-ostree#1905.

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: r4f4
To complete the pull request process, please assign cgwalters
You can assign the PR to them by writing /assign @cgwalters in a comment when ready.

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

Needs approval from an approver in each of these files:

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

@rh-atomic-bot
Copy link

Can one of the admins verify this patch?
I understand the following commands:

  • bot, add author to whitelist
  • bot, test pull request
  • bot, test pull request once

@jlebon
Copy link
Member

jlebon commented Oct 28, 2019

bot, add author to whitelist

src/ostree/ot-admin-builtin-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot-upgrader.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/ostree/ot-admin-builtin-deploy.c Outdated Show resolved Hide resolved
@@ -173,8 +175,11 @@ ot_admin_builtin_deploy (int argc, char **argv, OstreeCommandInvocation *invocat
return glnx_throw (error, "--stage cannot currently be combined with --retain arguments");
if (opt_not_as_default)
return glnx_throw (error, "--stage cannot currently be combined with --not-as-default");
OstreeSysrootSimpleWriteDeploymentFlags flags = 0;
Copy link
Member

Choose a reason for hiding this comment

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

We can also handle opt_not_as_default and the other opt_retain_* options too now, right? Shouldn't be too hard, though you don't have to do this in this PR!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll try to do it in a separate PR.

src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member

Hmm. Not opposed but this feels a bit like it's crossing into logic that may be better purely in rpm-ostree? On the other hand maybe any future apt/ebuild-ostree tools would want this too.

@r4f4
Copy link
Contributor Author

r4f4 commented Nov 11, 2019

Rebased and squashed commits.

@r4f4
Copy link
Contributor Author

r4f4 commented Nov 11, 2019

Hmm. Not opposed but this feels a bit like it's crossing into logic that may be better purely in rpm-ostree? On the other hand maybe any future apt/ebuild-ostree tools would want this too.

@jlebon any comments?

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Sorry for not getting back to this!

Hmm. Not opposed but this feels a bit like it's crossing into logic that may be better purely in rpm-ostree? On the other hand maybe any future apt/ebuild-ostree tools would want this too.

Yeah, I guess this walks a fine line. I can definitely see it belonging in rpm-ostree first before getting upstreamed to ostree once it becomes useful. Though for that, rpm-ostree would have to duplicate a lot of the logic from ostree_sysroot_simple_write_deployment. Also, the version metadata key concept does live here, so it doesn't seem too outlandish either to have this feature here?

src/libostree/ostree-deployment.h Outdated Show resolved Hide resolved
src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
tests/installed/destructive/staged-deploy.yml Outdated Show resolved Hide resolved
@rh-atomic-bot
Copy link

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

src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented Dec 11, 2019

I'm using this rpm-ostree patch to test this:

diff --git a/src/daemon/rpmostree-sysroot-upgrader.c b/src/daemon/rpmostree-sysroot-upgrader.c
index e3f5acef..d16ea710 100644
--- a/src/daemon/rpmostree-sysroot-upgrader.c
+++ b/src/daemon/rpmostree-sysroot-upgrader.c
@@ -1380,11 +1380,12 @@ rpmostree_sysroot_upgrader_deploy (RpmOstreeSysrootUpgrader *self,

       g_auto(RpmOstreeProgress) task = { 0, };
       rpmostree_output_task_begin (&task, "Staging deployment");
-      if (!ostree_sysroot_stage_tree (self->sysroot, self->osname,
+      if (!ostree_sysroot_stage_tree_with_flags (self->sysroot, self->osname,
                                       target_revision, origin,
                                       self->cfg_merge_deployment,
                                       self->kargs_strv,
                                       &new_deployment,
+                                      OSTREE_SYSROOT_SIMPLE_WRITE_DEPLOYMENT_FLAGS_RETAIN_PREVIOUS_VERSION,
                                       cancellable, error))
         return FALSE;
     }

So here's how you would test this using cosa (assuming your cosa workdir is at /srv/fcos):

ostree-dir$ make install DESTDIR=/src/fcos/overrides/rootfs
rpm-ostree-dir$ make install DESTDIR=/src/fcos/overrides/rootfs
/srv/fcos$ cosa build
/srv/fcos$ cosa run

Inside the VM, you can do something like this:

$ ostree commit -b tmp --tree=ref=<checksum-of-booted-deployment> --add-metadata-string version=my-version-1
$ rpm-ostree rebase :tmp
<reboot>
$ ostree commit -b tmp --tree=ref=tmp --add-metadata-string version=my-version-2
$ rpm-ostree upgrade
<reboot>
$ rpm-ostree install ltrace

Once you're up and running, you can use make vmsync in rpm-ostree to hack on top of this without having to rebuild FCOS each time (there's a cosa run --ssh-config which spits out an SSH config file you can give to make vmsync).

src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
@jlebon
Copy link
Member

jlebon commented Feb 4, 2020

@r4f4 Could you rebase and squash all the fixups? Will give it a final review and test!

@r4f4
Copy link
Contributor Author

r4f4 commented Feb 4, 2020

Rebased and squashed!

@jlebon
Copy link
Member

jlebon commented Apr 19, 2021

Yes, I'm still interested in this! I need to set aside some time to test it out.

@r4f4
Copy link
Contributor Author

r4f4 commented Apr 19, 2021

Ok. I'll try to rebase and fix the conflicts.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

OK cool, this works well for me in both the staging and non-staging paths (after adding the missing bit in ot-admin-builtin-deploy.c).

Here's how you can pretty easily test this locally in a VM:

[root@qemu0 ~]# rpm-ostree status
State: idle
Deployments:
* ostree://fedora:fedora/x86_64/coreos/next
                   Version: 34.20210503.1.1 (2021-05-12T22:57:51Z)
                    Commit: c229af6df100607f73216ecda14dfc3a71e4c7094f4a7cdd260dc97a5c650a3e
              GPGSignature: Valid signature by 8C5BA6990BDB26E19F2A1A801161AE6945719A39
                  Unlocked: development

[root@qemu0 ~]# ostree commit -b previous-version --add-metadata-string=version=34.20210503.1.0 --tree=ref=c229af6df100607f73216ecda14dfc
3a71e4c7094f4a7cdd260dc97a5c650a3e
b6437a1dae3345aa73908bee2fc4606f56e81a338cb4a9082c51042572294534

[root@qemu0 ~]# ostree admin deploy previous-version --not-as-default
Copying /etc changes: 8 modified, 0 removed, 30 added
Bootloader updated; bootconfig swap: yes; bootversion: boot.0.1, deployment count change: 1

[root@qemu0 ~]# ostree commit -b same-version --add-metadata-string=version=34.20210503.1.1 --tree=ref=c229af6df100607f73216ecda14dfc3a71
e4c7094f4a7cdd260dc97a5c650a3e
3478fbc53e070b3d57c86893417329385cd7bc746670d8d2933b93e51b595b02

[root@qemu0 ~]# ostree admin deploy same-version --retain-previous-version
Copying /etc changes: 8 modified, 0 removed, 30 added
Bootloader updated; bootconfig swap: yes; bootversion: boot.1.1, deployment count change: 1
Freed objects: 190 bytes

[root@qemu0 ~]# rpm-ostree status
State: idle
Deployments:
  ostree://same-version
                   Version: 34.20210503.1.1 (2021-05-14T20:11:40Z)
                    Commit: 3478fbc53e070b3d57c86893417329385cd7bc746670d8d2933b93e51b595b02

* ostree://fedora:fedora/x86_64/coreos/next
                   Version: 34.20210503.1.1 (2021-05-12T22:57:51Z)
                    Commit: c229af6df100607f73216ecda14dfc3a71e4c7094f4a7cdd260dc97a5c650a3e
              GPGSignature: Valid signature by 8C5BA6990BDB26E19F2A1A801161AE6945719A39
                  Unlocked: development

  ostree://previous-version
                   Version: 34.20210503.1.0 (2021-05-14T20:11:00Z)
                    Commit: b6437a1dae3345aa73908bee2fc4606f56e81a338cb4a9082c51042572294534

Then you can also test the staging path by nuking the pending deployment (rpm-ostree cleanup -p) and re-deploying, but this time with --stage followed by ostree admin finalize-staged.

In practice at the rpm-ostree level I think we should just omit the flag in the rpm-ostree rebase case, which is like what we're doing here since we're changing branches, but this works to test the change here. (Or maybe even only enable this in the NO_PULL_BASE case to start.)

src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-deployment-private.h Outdated Show resolved Hide resolved
src/ostree/ot-admin-builtin-deploy.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot-deploy.c Show resolved Hide resolved
src/libostree/ostree-sysroot.h Outdated Show resolved Hide resolved
tests/kolainst/destructive/staged-deploy.sh Outdated Show resolved Hide resolved
src/ostree/ot-admin-builtin-deploy.c Outdated Show resolved Hide resolved
@r4f4 r4f4 force-pushed the keep-last-version branch 4 times, most recently from 02bf659 to cb32ef0 Compare May 20, 2021 16:39
@r4f4
Copy link
Contributor Author

r4f4 commented May 20, 2021

@jlebon I think I've addressed all the points and the test is now passing [2021-05-20T17:04:53.378Z] --- PASS: ext.ostree.destructive.staged-deploy.sh (151.61s)

src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.h Outdated Show resolved Hide resolved
src/libostree/ostree-deployment.c Outdated Show resolved Hide resolved
src/libostree/ostree-sysroot.c Outdated Show resolved Hide resolved
@openshift-ci
Copy link

openshift-ci bot commented Apr 22, 2022

@r4f4: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@r4f4
Copy link
Contributor Author

r4f4 commented Sep 19, 2024

Update: rebased to fix merge conflicts.

@r4f4
Copy link
Contributor Author

r4f4 commented Sep 21, 2024

/retest

Copy link

openshift-ci bot commented Sep 21, 2024

@r4f4: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@r4f4
Copy link
Contributor Author

r4f4 commented Oct 3, 2024

/assign @jlebon

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.

7 participants