MGMT-19100: Install to the current boot device when CoreosImage is set #1003

carbonin · 2025-01-09T21:24:31Z

This indicates the container image that should be installed and booted for the installed host. It also indicates that we should install to the currently booted disk, in a new stateroot, rather than expecting a device path.

https://issues.redhat.com/browse/MGMT-19100

cc @tsorya @rccrdpccl @eranco74

openshift-ci-robot · 2025-01-09T21:24:35Z

@carbonin: This pull request references MGMT-19100 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 task to target the "4.19.0" version, but no target version was set.

In response to this:

This indicates the container image that should be installed and booted for the installed host. It also indicates that we should install to the currently booted disk, in a new stateroot, rather than expecting a device path.

https://issues.redhat.com/browse/MGMT-19100

cc @tsorya @rccrdpccl @eranco74

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-01-09T21:25:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carbonin

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

codecov · 2025-01-09T21:57:43Z

Codecov Report

Attention: Patch coverage is 32.72727% with 37 lines in your changes missing coverage. Please review.

Project coverage is 54.81%. Comparing base (34bae2a) to head (8996860).

Files with missing lines	Patch %	Lines
src/ops/ops.go	22.50%	31 Missing ⚠️
src/installer/installer.go	57.14%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1003      +/-   ##
==========================================
- Coverage   55.20%   54.81%   -0.39%     
==========================================
  Files          15       15              
  Lines        3286     3333      +47     
==========================================
+ Hits         1814     1827      +13     
- Misses       1292     1326      +34     
  Partials      180      180

Files with missing lines	Coverage Δ
src/config/config.go	`70.58% <100.00%> (+0.43%)`	⬆️
src/installer/installer.go	`68.00% <57.14%> (-0.19%)`	⬇️
src/ops/ops.go	`42.05% <22.50%> (-1.35%)`	⬇️

paul-maidment · 2025-01-10T15:33:39Z

src/ops/ops.go

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

+func (o *ops) WriteImageToLocalDevice(liveLogger io.Writer, ignitionPath string, rhelCoreOSImage string) error {


Does any test provide coverage of this function?

I can see a code path in here that would conditionally execute according to the outcome of some the the calls to ExecPrivilegeCommand

I also think this is quite mockable/testable and there are pre-existing examples of how to approach this.

I didn't see much value since there's no actual logic here and every call would be mocked.

I could pull out the ostree output parsing bit since that's the only bit worth testing, otherwise it doesn't feel worth it to me.

I think the parsing part should be pulled out so that we can validate the parsing

paul-maidment · 2025-01-10T16:30:17Z

src/ops/ops.go

+		return errors.Wrapf(err, "failed to unencapsulate rhcos payload image: %s", out)
+	}
+	outputParts := strings.Split(out, " ")
+	if len(outputParts) != 2 {


I am not 100% sure what the output of the unencapsulate command is meant to look like when supplied the parameters above. But is it possible that these output parts could have exactly two parts while containing content we do not expect?

That's fair. I'll build a regex of some sort. The expected output will be in the tests when I write them

FWIW I don't know what all the output could possibly be.

My guess is that if the command is successful we'd only ever see the two parts we're expecting, but better to be sure.

Also just for reference, this is the line I'm parsing https://github.com/ostreedev/ostree-rs-ext/blob/b885f9468829e6af41fa95504d65975c48378d4d/lib/src/cli.rs#L710

carbonin · 2025-01-10T18:09:00Z

src/ops/ops.go

@@ -49,6 +49,7 @@ const (
 type Ops interface {
 	Mkdir(dirName string) error
 	WriteImageToDisk(liveLogger io.Writer, ignitionPath string, device string, extraArgs []string) error
+	WriteImageToLocalDevice(liveLogger io.Writer, ignitionPath string, rhelCoreOSImage string) error


Any ideas for a better name for this?

Technically all the devices are "local". Maybe "BootDevice"? Is that better?

BootDevice seems reasonable.

We are describing the disk we will boot into once the node reboots out of discovery?

Yes, but that's true in either case.

The distinction is if we're installing to a device that is currently being used for the rootfs or one that is not.

So in both cases they are boot disks, how about

WriteImageToBootDiskWithRootFs ?

or even

WriteImageToRootFSDisk as it may be redundant to say "boot disk" if the context is already there?

Maybe WriteImageToExistingRoot to mirror the similar bootc command (which might be something I can use) called bootc install to-existing-root

carbonin · 2025-01-13T15:56:56Z

src/ops/ops.go

+	out, err = o.ExecPrivilegeCommand(liveLogger, "ostree", "admin", "deploy",
+		"--stateroot", "install",
+		"--karg", "$ignition_firstboot",
+		"--karg", defaultIgnitionPlatformId,
+		commit)
+	if err != nil {
+		return errors.Wrapf(err, "failed to deploy commit to stateroot: %s", out)
+	}


Related to #1003 (comment) ...

@eranco74 (or someone you think knows) can/should we use bootc here?

Bootc is already part of rhcos?

I'm not sure, that's part of what I'm asking, but I can check.

Yeah, seems to be there in 4.18 at least

It's also present in 4.17 and 4.16. 4.15 is the latest version that doesn't have it.

I think MCE won't support anything earlier than 4.16 by the time this change gets in so we might be safe there, but if we also want to use this on the SaaS it might not be worth limiting the versions we can install.

Even if bootc works for this and is the better option maybe we can leave it as a follow up for now?

This indicates the container image that should be installed and booted for the installed host. It also indicates that we should install to the existing root filesystem, in a new stateroot, rather than expecting a device path. https://issues.redhat.com/browse/MGMT-19100

openshift-ci · 2025-01-13T23:31:24Z

@carbonin: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-agent-compact-ipv4	`8996860`	link	true	`/test e2e-agent-compact-ipv4`
ci/prow/edge-e2e-ai-operator-ztp	`8996860`	link	true	`/test edge-e2e-ai-operator-ztp`
ci/prow/edge-e2e-metal-assisted-odf-4-17	`8996860`	link	false	`/test edge-e2e-metal-assisted-odf-4-17`
ci/prow/edge-e2e-metal-assisted-ipv6-4-18	`8996860`	link	false	`/test edge-e2e-metal-assisted-ipv6-4-18`
ci/prow/edge-e2e-oci-assisted-4-18	`8996860`	link	false	`/test edge-e2e-oci-assisted-4-18`
ci/prow/okd-scos-e2e-aws-ovn	`8996860`	link	false	`/test okd-scos-e2e-aws-ovn`

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.

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

openshift-ci bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 9, 2025

openshift-ci bot requested review from danmanor and eliorerz January 9, 2025 21:25

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

carbonin force-pushed the image-install branch from 64dd407 to 5199af9 Compare January 10, 2025 13:22

paul-maidment suggested changes Jan 10, 2025

View reviewed changes

openshift-ci bot assigned paul-maidment Jan 10, 2025

paul-maidment reviewed Jan 10, 2025

View reviewed changes

carbonin commented Jan 10, 2025

View reviewed changes

carbonin force-pushed the image-install branch 2 times, most recently from eb0683a to 928a493 Compare January 10, 2025 18:29

carbonin commented Jan 13, 2025

View reviewed changes

carbonin force-pushed the image-install branch from 928a493 to 8996860 Compare January 13, 2025 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MGMT-19100: Install to the current boot device when CoreosImage is set #1003

MGMT-19100: Install to the current boot device when CoreosImage is set #1003

carbonin commented Jan 9, 2025

openshift-ci-robot commented Jan 9, 2025 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Jan 9, 2025

codecov bot commented Jan 9, 2025 •

edited

Loading

paul-maidment Jan 10, 2025

carbonin Jan 10, 2025

paul-maidment Jan 10, 2025

paul-maidment Jan 10, 2025 •

edited

Loading

carbonin Jan 10, 2025

carbonin Jan 10, 2025

carbonin Jan 10, 2025

carbonin Jan 10, 2025

paul-maidment Jan 10, 2025

carbonin Jan 10, 2025

paul-maidment Jan 10, 2025 •

edited

Loading

carbonin Jan 13, 2025

carbonin Jan 13, 2025

tsorya Jan 14, 2025

carbonin Jan 14, 2025

carbonin Jan 14, 2025

carbonin Jan 14, 2025

carbonin Jan 14, 2025

openshift-ci bot commented Jan 13, 2025

MGMT-19100: Install to the current boot device when CoreosImage is set #1003

Are you sure you want to change the base?

MGMT-19100: Install to the current boot device when CoreosImage is set #1003

Conversation

carbonin commented Jan 9, 2025

openshift-ci-robot commented Jan 9, 2025 • edited by openshift-ci bot Loading

openshift-ci bot commented Jan 9, 2025

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-maidment Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paul-maidment Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 13, 2025

openshift-ci-robot commented Jan 9, 2025 •

edited by openshift-ci bot

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

paul-maidment Jan 10, 2025 •

edited

Loading

paul-maidment Jan 10, 2025 •

edited

Loading