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

Ensure boot directory is open before accessing it for early pruning #3213

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

rborn-tx
Copy link
Contributor

I found an issue while testing Aktualizr (an update client that supports OSTree) with the latest libostree version: basically, the program failed when deploying an update, something that didn't happen with older libostree versions. Through code bisection I found the program stopped working after the commit c561e61 on the ostree project repository:

$ git log -1 c561e6179e965d11bba7e1d80ca35b7ad6fc7bc5
commit c561e6179e965d11bba7e1d80ca35b7ad6fc7bc5
Author: Jonathan Lebon <jonathan@jlebon.com>
Date:   Thu Apr 13 17:22:43 2023 -0400

    lib/sysroot-deploy: Add experimental support for automatic early prune

    During the early design of FCOS and RHCOS, we chose a value of 384M
    for the boot partition. This turned out to be too small: some arches
    ...

With this version (and newer), what happens is that the call made by Aktualizr to ostree_sysroot_simple_write_deployment() fails, particularly in this call-chain:

ostree_sysroot_simple_write_deployment()
  -> ostree_sysroot_write_deployments_with_options()
     -> auto_early_prune_old_deployments()

and the failure occurs inside auto_early_prune_old_deployments() which is a function added in the commit I referenced. The failure happens because the function tries to fstatat on an invalid handle (-1) and that handle comes from sysroot->boot_fd which was not initialized with a proper directory handle yet.

Apparently the function assumes the handle to be always set which appears not to be the case. I found a couple places in the code running before the failure point in the code that could have initialized the boot_fd field but they were protected by conditions that were not true when running from Aktualizr. From there I concluded a call to ensure the field was initialized was simply missing and added it; this solved the issue with Aktualizr.

This fixes a bug in the (early) deployment pruning function which before
tried to access the boot directory without opening it first.

Signed-off-by: Rogerio Guerra Borin <rogerio.borin@toradex.com>
Copy link

openshift-ci bot commented Mar 13, 2024

Hi @rborn-tx. Thanks for your PR.

I'm waiting for a ostreedev member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

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.

Thank you! Yes, that makes sense to me.

@jlebon
Copy link
Member

jlebon commented Mar 13, 2024

/ok-to-test

@jlebon jlebon enabled auto-merge March 13, 2024 16:47
@jlebon jlebon merged commit c314eaa into ostreedev:main Mar 13, 2024
23 of 24 checks passed
@rborn-tx rborn-tx deleted the fix-early-prune branch March 13, 2024 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants