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

buildextend-metal: Make sysroot-ro an opt-in image.yaml parameter #1235

Closed
wants to merge 1 commit into from

Conversation

cgwalters
Copy link
Member

See
coreos/fedora-coreos-tracker#343 (comment)
Basically we need to make raw ostree operations work and not
just ostree admin.

@jlebon
Copy link
Member

jlebon commented Mar 12, 2020

Hmm, the reason I didn't do this was because we already have systems out there that have sysroot.readonly. I think going back to turning it off now would just make things more confusing.

Let's just work towards fixing coreos/fedora-coreos-tracker#343? If it takes too long and we have to e.g. ship an ostree fix/new rpm-ostree feature that needs an ostree bump, we can disable it at compile-time for now in Fedora and drop the lockfile pin on ostree (coreos/fedora-coreos-config#300).

@cgwalters
Copy link
Member Author

I think you're right, but we can split the difference here - switch fedora-coreos-config to keep it on if you prefer.

I commented on this around the ipv6 bits too - this is another thing that's really a property of the package versions. Having future changes like this be opt-in via image.yaml breaks that. I think we do want features.yaml or so. Or, something to detect versions of packages.

jlebon added a commit to jlebon/fedora-coreos-config that referenced this pull request Mar 16, 2020
We want to use the new read-only `/sysroot` feature of libostree. Opt-in
to that to tell cosa we support it and want it.

For more details, see:
ostreedev/ostree#1265
coreos/coreos-assembler#1235
@jlebon
Copy link
Member

jlebon commented Mar 16, 2020

Sure, WFM: coreos/fedora-coreos-config#304

This is tricky though. I think we should consider read-only sysroot as part of the "CoreOS" model. So then, it's not really something that we opt into, but is just hardcoded as part of cosa. OTOH, supporting it in RHCOS for example requires updating at least ostree and rpm-ostree, and having a knob in the config to make ratcheting it in easier makes sense. But I hope eventually we can drop that knob entirely and just always have it enabled. (Unlike the IPv6 work for example, there's not really a "preference" here; having a read-only /sysroot is just clearly better.)

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.

One minor bikeshed, LGTM otherwise!

src/cmd-buildextend-metal Outdated Show resolved Hide resolved
Copy link
Member

@ashcrow ashcrow left a comment

Choose a reason for hiding this comment

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

(needs rebase but logic LGTM)

@cgwalters
Copy link
Member Author

I think we should consider read-only sysroot as part of the "CoreOS" model. So then, it's not really something that we opt into, but is just hardcoded as part of cosa.

I think we're in agreement that it is part of CoreOS, but we have already a split between "policy" and "mechanism" in the config repo and coreos-assembler (ish) and this is a bit more "policy".

jlebon added a commit to jlebon/fedora-coreos-config that referenced this pull request Mar 16, 2020
We want to use the new read-only `/sysroot` feature of libostree. Opt-in
to that to tell cosa we support it and want it.

For more details, see:
ostreedev/ostree#1265
coreos/coreos-assembler#1235
cgwalters pushed a commit to jlebon/fedora-coreos-config that referenced this pull request May 18, 2020
We want to use the new read-only `/sysroot` feature of libostree. Opt-in
to that to tell cosa we support it and want it.

For more details, see:
ostreedev/ostree#1265
coreos/coreos-assembler#1235
See
coreos/fedora-coreos-tracker#343 (comment)
Basically we need to make raw `ostree` operations work and not
just `ostree admin`.

Pairs with coreos/fedora-coreos-config#304
@cgwalters
Copy link
Member Author

OK rebased this and renamed to sysroot-readonly. For some reason I'm seeing more fallout from this trying to update ostree in RHCOS, e.g. coreos/fedora-coreos-config#403
Plus it shook out other things like https://gitlab.cee.redhat.com/coreos/redhat-coreos/merge_requests/951

Now that I look at this I'm feeling we need to rework this in ostree, see ostreedev/ostree#2104

I also force pushed coreos/fedora-coreos-config#304 to use the new name (and that should merge first).

@ashcrow
Copy link
Member

ashcrow commented May 18, 2020

I also force pushed coreos/fedora-coreos-config#304 to use the new name (and that should merge first).

/hold

until coreos/fedora-coreos-config#304 merges

ashcrow pushed a commit to coreos/fedora-coreos-config that referenced this pull request May 18, 2020
We want to use the new read-only `/sysroot` feature of libostree. Opt-in
to that to tell cosa we support it and want it.

For more details, see:
ostreedev/ostree#1265
coreos/coreos-assembler#1235
@ashcrow
Copy link
Member

ashcrow commented May 18, 2020

/hold cancel

coreos/fedora-coreos-config#304 merged

@cgwalters
Copy link
Member Author

Can someone lgtm this please?

@jlebon
Copy link
Member

jlebon commented May 20, 2020

/lgtm

Copy link
Contributor

@darkmuggle darkmuggle left a comment

Choose a reason for hiding this comment

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

LGTM

I also kicked the CI to re-test.

@@ -189,6 +189,11 @@ if [ "${image_type}" == metal4k ]; then
disk_args+=("--no-x86-bios-partition")
fi

sysroot_ro="$(python3 -c 'import sys, yaml; v=yaml.safe_load(sys.stdin).get("sysroot-readonly", False); print("1" if v else "")' < "$configdir/image.yaml")"
Copy link
Contributor

Choose a reason for hiding this comment

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

I really am not a fan of this shell pattern. We need a bash helper function.

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashcrow, cgwalters, darkmuggle, 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 [ashcrow,cgwalters,darkmuggle,jlebon]

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

@cgwalters
Copy link
Member Author

Oops I think this one is obsolete, we patched ostree and the git master ostree is also fixed in ostreedev/ostree#2113

@cgwalters cgwalters closed this May 28, 2020
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.

5 participants