MGMT-20153: Fix ostree race between node-image-pull and installing to rootfs #1076

carbonin · 2025-03-24T14:29:21Z

ostree container unencapsulate panics if the image passed to it is already present in the repo. Use ostree container image pull instead. Additionally attempting to pull the same os image as the node-image-pull service at the same time also can lead to errors writing to the repo. So make startBootstrap run synchronously with the rest of InstallNode to ensure we are done applying the node image before attempting to pull the image for install.

Resolves https://issues.redhat.com/browse/MGMT-20153

openshift-ci-robot · 2025-03-24T14:29:28Z

@carbonin: This pull request references MGMT-20153 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

ostree container unencapsulate panics if the image passed to it is already present in the repo. Use ostree container image pull instead. Additionally attempting to pull the same os image as the node-image-pull service at the same time also can lead to errors writing to the repo. So make startBootstrap run synchronously with the rest of InstallNode to ensure we are don applying the node image before attempting to pull the image for install.

Resolves https://issues.redhat.com/browse/MGMT-20153

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-03-24T14:29:30Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

carbonin · 2025-03-24T14:30:28Z

Related to openshift/installer#9570

codecov · 2025-03-24T21:07:26Z

Codecov Report

Attention: Patch coverage is 61.11111% with 28 lines in your changes missing coverage. Please review.

Project coverage is 55.59%. Comparing base (1076581) to head (4505593).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/ops/ops.go	59.42%	16 Missing and 12 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1076      +/-   ##
==========================================
- Coverage   55.67%   55.59%   -0.08%     
==========================================
  Files          15       15              
  Lines        3393     3421      +28     
==========================================
+ Hits         1889     1902      +13     
- Misses       1311     1317       +6     
- Partials      193      202       +9

Files with missing lines	Coverage Δ
src/installer/installer.go	`68.37% <100.00%> (-0.21%)`	⬇️
src/ops/ops.go	`45.62% <59.42%> (+0.37%)`	⬆️

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

carbonin · 2025-03-25T12:00:39Z

/test edge-e2e-ai-operator-ztp

carbonin · 2025-03-25T15:30:02Z

/retest

tsorya · 2025-03-25T15:47:54Z

src/ops/ops.go

-var ostreeOutputRegex = regexp.MustCompile(`Imported: (\w+)`)
+func refFilePath(refName string) string {
+	return path.Join(ostreeRepo, "refs/heads", refName)
+}

 func (o *ops) importOSTreeCommit(liveLogger io.Writer) (string, error) {


maybe at some point worth thinking about ostree object?

🤷 I'm not sure what it gains us right now.
Also the not-so-long-term plan I think is to rip all this out and move to bootc so I don't know how much we really need to change.

I can put this stuff in a separate file if that makes it a bit more organized

tsorya · 2025-03-25T15:50:05Z

src/ops/ops.go

@@ -174,6 +200,20 @@ func (o *ops) WriteImageToExistingRoot(liveLogger io.Writer, ignitionPath string
 		return errors.Wrapf(err, "failed to remount boot: %s", out)
 	}

+	// Remove the node image reference if it still exists, but also ensure it isn't garbage collected just yet
+	if o.FileExists(refFilePath(nodeImageOSTreeRefName)) {


Lets move this part to function

openshift-ci-robot · 2025-03-25T15:51:13Z

@carbonin: This pull request references MGMT-20153 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.19.0" version, but no target version was set.

In response to this:

ostree container unencapsulate panics if the image passed to it is already present in the repo. Use ostree container image pull instead. Additionally attempting to pull the same os image as the node-image-pull service at the same time also can lead to errors writing to the repo. So make startBootstrap run synchronously with the rest of InstallNode to ensure we are done applying the node image before attempting to pull the image for install.

Resolves https://issues.redhat.com/browse/MGMT-20153

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 openshift-eng/jira-lifecycle-plugin repository.

cgwalters · 2025-03-25T16:05:36Z

I only skimmed this but FYI a lot of this I would consider internal implementation details subject to change, especically the ostree ref structures.

I'd like to have a good understanding of exactly what this is trying to do because it will need to be supported directly in bootc - and for bootc we have plans to switch to a new composefs-native store and stop doing the ostree-container flow, so directly invoking ostree will break when that happens.

cgwalters · 2025-03-25T18:33:55Z

src/ops/ops.go

 }

-func ostreeArgs(commit string, installerArgs []string) []string {
+func ostreeArgs(ref string, installerArgs []string) []string {
 	ostreeArgs := []string{"admin", "deploy",


We had a live chat on this and I think you can drop a lot of the low-level ostree commands if you switch to using ostree container image deploy instead

Does this command have something similar to the --karg-append and --karg-delete that I'm using below?

I see --karg, but not the others.

I don't see something in the --help

--karg is the same as --karg-append, now you're right it doesn't have --karg-delete right now; would be easy to add but can you elaborate on why it's needed?

We include almost no default kernel arguments so I am uncertain when one would need to delete any.

Filed it as bootc-dev/bootc#1229

In the short term it'd probably work to do the deploy, and then remove any unwanted kargs as a post-install step instead.

can you elaborate on why it's needed?

Adding and deleting kargs is an API we've exposed through assisted because they were options for coreos-installer. I'm just trying to keep parity here.

I'm (effectively) running this in my last attempt:

ostree container image deploy --stateroot install --sysroot /sysroot --image <rhcos-content-image-from-release>

Then the next boot fails.

I booted a live ISO and mounted the boot and sysroot partitions and I see the old boot entry in the boot partition's entry, but I see the new one in the sysroot /boot/loader/entries directory ... This doesn't seem right. Should I have maybe specified / for --sysroot in the ostree command?

rpm-ostree kargs --os=install --append/--delete ... should work to edit the kargs post-deployment. (Aside: opened coreos/rpm-ostree#5345 so you can also type --stateroot for consistency.)

Yeah, try with --sysroot=/.

Yeah, try with --sysroot=/

This seems to have helped. Doing some more testing then will update the PR.

rpm-ostree kargs --os=install --append/--delete ... should work to edit the kargs post-deployment

Nice, thanks. I'll try this out too.

Noticed this while reviewing openshift/assisted-installer#1076.

The `--stateroot` term is more often used now in libostree, so let's match that here too. Noticed this while reviewing openshift/assisted-installer#1076.

jlebon · 2025-03-26T14:06:29Z

Let's work on getting openshift/installer#9570 tested and merged so we can clean up the ref manipulations here a bit.

carbonin · 2025-03-26T15:17:22Z

Let's work on getting openshift/installer#9570 tested and merged so we can clean up the ref manipulations here a bit.

I'll take a look at this after I get a reasonable general approach working. Once I have something that works and folks are happy with I'll test pulling out my addition to remove the node image ref in favor of the installer change.

I suspect if we want to test with 4.19 ec releases right after this is merged though I'll need to at least keep the touch in there then remove it after we get a build with your changes (or just after GA to ensure we're not using releases without the change)

`ostree container unencapsulate` panics if the image it is attempting to import is already present on the system. This will be the case on the bootstrap node if the node overlay service runs before installing to disk. Instead, use container image deploy for installing to a stateroot.

This process only takes about a minute and when installing to the existing rootfs in multi-node cases it is required that the node image is downloaded before we attempt to use the same image to install. Previously this race condition between layering the node image and installing to the rootfs would cause the ostree commands to fail.

This requires calling `ostree admin finalize-staged` after install and again after editing kargs, but this is currently the only way to handle karg editing outside of unencapsulate

This makes the logic in WriteImageToExistingRoot easier to understand

carbonin · 2025-03-31T19:27:23Z

This requires #1076. Without it the cleanup service fails on the installed node.

carbonin · 2025-04-01T14:03:31Z

/retest

jlebon

Looks sane overall. One thing that might be worth mentioning in the first commit message is that with the switch to the layered node image soon, the unencapsulate flow will just not work anyway so it's good that we're switching away from that.

jlebon · 2025-04-01T15:10:05Z

src/ops/ops.go

+	lastArgIndex := len(installerArgs) - 1
+	for i, arg := range installerArgs {
+		if arg == "--append-karg" && i < lastArgIndex {
+			commandArgs = append(commandArgs, "--append", installerArgs[i+1])
+			continue
+		}
+		if arg == "--delete-karg" && i < lastArgIndex {
+			commandArgs = append(commandArgs, "--delete-if-present", installerArgs[i+1])
+			continue
+		}
+	}


Feels like we should error out if --append-karg or --delete-karg is last rather than do our best to work with it.

I think we have a validation around this in assisted (where the user adds these). And if we don't I'd rather it fail there than here.

jlebon · 2025-04-01T15:13:10Z

src/ops/ops.go

@@ -130,6 +130,19 @@ func (o *ops) SystemctlAction(action string, args ...string) error {
 	return errors.Wrapf(err, "Failed executing systemctl %s %s", action, args)
 }

+func (o *ops) remountFilesystems(liveLogger io.Writer) error {


Hmm, why do we need this BTW? ostree and rpm-ostree should both know to remount within a mount namespace to do their operations.

At the very least we still need boot because we're putting networking files and the ignition there. I never considered that I might be able to remove sysroot

openshift-ci · 2025-04-01T15:14:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin, 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~~ [carbonin]

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

carbonin · 2025-04-01T18:07:36Z

/retest

carbonin · 2025-04-01T18:10:56Z

/hold

until the assisted-service PR is merged.

openshift-ci · 2025-04-01T19:11:44Z

@carbonin: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

carbonin · 2025-04-02T12:32:51Z

/unhold

openshift-bot · 2025-04-02T17:49:37Z

[ART PR BUILD NOTIFIER]

Distgit: ose-agent-installer-orchestrator
This PR has been included in build ose-agent-installer-orchestrator-container-v4.19.0-202504021544.p0.g0e954b1.assembly.stream.el9.
All builds following this will include this PR.

openshift-bot · 2025-04-02T17:51:54Z

[ART PR BUILD NOTIFIER]

Distgit: ose-agent-installer-csr-approver
This PR has been included in build ose-agent-installer-csr-approver-container-v4.20.0-202504021544.p0.g0e954b1.assembly.stream.el9.
All builds following this will include this PR.

carbonin requested review from eranco74, jlebon, tsorya and rccrdpccl March 24, 2025 14:29

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 24, 2025

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2025

openshift-ci bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 24, 2025

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 24, 2025

carbonin force-pushed the ostree-image-pull branch from a3bddf7 to f14fb38 Compare March 24, 2025 20:06

openshift-ci bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 24, 2025

carbonin marked this pull request as ready for review March 24, 2025 20:13

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 24, 2025

openshift-ci bot requested review from adriengentil and danielerez March 24, 2025 20:13

tsorya reviewed Mar 25, 2025

View reviewed changes

carbonin force-pushed the ostree-image-pull branch from f14fb38 to 71f250c Compare March 25, 2025 15:58

cgwalters reviewed Mar 25, 2025

View reviewed changes

cgwalters mentioned this pull request Mar 25, 2025

install: Add --karg-delete bootc-dev/bootc#1229

Open

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Mar 26, 2025

app/kargs: Add --stateroot alias for --os

c5dec3e

Noticed this while reviewing openshift/assisted-installer#1076.

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Mar 26, 2025

app: Hide --os and add --stateroot alias

0968a8d

The `--stateroot` term is more often used now in libostree, so let's match that here too. Noticed this while reviewing openshift/assisted-installer#1076.

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Mar 26, 2025

app: Hide --os and add --stateroot alias

5a7dae7

The `--stateroot` term is more often used now in libostree, so let's match that here too. Noticed this while reviewing openshift/assisted-installer#1076.

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Mar 26, 2025

app: Hide --os and add --stateroot alias

19ff3bd

The `--stateroot` term is more often used now in libostree, so let's match that here too. Noticed this while reviewing openshift/assisted-installer#1076.

jlebon mentioned this pull request Mar 26, 2025

app: Hide --os and add --stateroot alias coreos/rpm-ostree#5345

Merged

carbonin added 4 commits March 31, 2025 15:17

Edit kargs with rpm-ostree

6e607d5

This requires calling `ostree admin finalize-staged` after install and again after editing kargs, but this is currently the only way to handle karg editing outside of unencapsulate

Extract some functions

4505593

This makes the logic in WriteImageToExistingRoot easier to understand

carbonin force-pushed the ostree-image-pull branch from 71f250c to 4505593 Compare March 31, 2025 19:18

carbonin mentioned this pull request Mar 31, 2025

MGMT-20153: Improve ostree cleanup service openshift/assisted-service#7490

Merged

20 tasks

jlebon approved these changes Apr 1, 2025

View reviewed changes

openshift-ci bot assigned jlebon Apr 1, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 1, 2025

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 1, 2025

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Apr 2, 2025

openshift-merge-bot bot merged commit 0e954b1 into openshift:master Apr 2, 2025
20 checks passed

carbonin deleted the ostree-image-pull branch April 2, 2025 12:45

jlebon mentioned this pull request Aug 6, 2025

bootstrap/node-image-pull.sh: handle "separate stateroot" flow better openshift/installer#9570

Draft

MGMT-20153: Fix ostree race between node-image-pull and installing to rootfs #1076

MGMT-20153: Fix ostree race between node-image-pull and installing to rootfs #1076

Uh oh!

Conversation

carbonin commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Mar 24, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Mar 24, 2025

Uh oh!

carbonin commented Mar 24, 2025

Uh oh!

codecov bot commented Mar 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

carbonin commented Mar 25, 2025

Uh oh!

carbonin commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Mar 25, 2025 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cgwalters commented Mar 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jlebon commented Mar 26, 2025

Uh oh!

carbonin commented Mar 26, 2025

Uh oh!

carbonin commented Mar 31, 2025

Uh oh!

carbonin commented Apr 1, 2025

Uh oh!

jlebon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Apr 1, 2025

Uh oh!

carbonin commented Apr 1, 2025

Uh oh!

carbonin commented Apr 1, 2025

Uh oh!

carbonin commented Mar 24, 2025 •

edited

Loading

openshift-ci-robot commented Mar 24, 2025 •

edited by openshift-ci bot

Loading

codecov bot commented Mar 24, 2025 •

edited

Loading

openshift-ci-robot commented Mar 25, 2025 •

edited by openshift-ci bot

Loading