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

Add hidden coreos-rootfs seal command #1911

Merged
merged 1 commit into from
Oct 4, 2019

Conversation

cgwalters
Copy link
Member

All this does is put the immutable bit on the target directory.
The intention is to replace this bit to start:
https://github.com/coreos/coreos-assembler/blob/8b205bfbb971707382ace76bbb39e46ed3fc560d/src/create_disk.sh#L229

However, the real goal here is to add code in this file
to handle redeploying the rootfs for Fedora CoreOS which
combines OSTree+Ignition:
coreos/fedora-coreos-tracker#94

Basically doing this in proper Rust is going to be a lot
nicer than shell script in dracut modules. Among other
details, coreutils mv doesn't seem to do the right thing
for SELinux labels when policy isn't loaded.

use libc;

/// A reference to a *directory* file descriptor. Unfortunately,
/// the openat crate always uses O_PATH which doesn't support ioctl().
Copy link
Member Author

Choose a reason for hiding this comment

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

And yeah, this turned out to be a lot more code in Rust than it would have been in C particularly if we just copy-pasted the libostree code but I doggedly kept at it 😉

@rh-atomic-bot
Copy link

☔ The latest upstream changes (presumably 553be6e) made this pull request unmergeable. Please resolve the merge 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.

I'm conflicted on this. On one hand, it's definitely convenient. On the other hand, putting this here feels a bit forced. It's not the first time and I'm sure not the last that we'll want/need something better than shell to run in the initrd.

I'm not sure of a better place for it though. Guess cosa could grow a compile-on-the-fly option? (And then having this code live in fedora-coreos-config). Would probably tie in with the move to make local dev easier too.

Anyway, open to trying this out!


#[derive(Debug, StructOpt)]
#[structopt(name = "coreos-rootfs")]
#[structopt(rename_all = "kebab-case")]
Copy link
Member

Choose a reason for hiding this comment

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

kebab-case is best case

tests/vmcheck/test-misc-1.sh Outdated Show resolved Hide resolved
@cgwalters
Copy link
Member Author

I'm conflicted on this. On one hand, it's definitely convenient. On the other hand, putting this here feels a bit forced. It's not the first time and I'm sure not the last that we'll want/need something better than shell to run in the initrd.

Yeah I was too. But...when I narrowed down to:

  • compiled lang, not shell script
  • dependency on ostree
  • preference for Rust
  • We want package layering in the initramfs later anyways

That's how I got here.

@cgwalters
Copy link
Member Author

/approve

@jlebon
Copy link
Member

jlebon commented Sep 30, 2019

@rh-atomic-bot r+ 6d0f8af

@rh-atomic-bot
Copy link

⌛ Testing commit 6d0f8af with merge bad7cbc...

rh-atomic-bot pushed a commit that referenced this pull request Sep 30, 2019
All this does is put the immutable bit on the target directory.
The intention is to replace this bit to start:
https://github.com/coreos/coreos-assembler/blob/8b205bfbb971707382ace76bbb39e46ed3fc560d/src/create_disk.sh#L229

However, the real goal here is to add code in this file
to handle redeploying the rootfs for Fedora CoreOS which
combines OSTree+Ignition:
coreos/fedora-coreos-tracker#94

Basically doing this in proper Rust is going to be a lot
nicer than shell script in dracut modules.  Among other
details, coreutils `mv` doesn't seem to do the right thing
for SELinux labels when policy isn't loaded.

Closes: #1911
Approved by: jlebon
@rh-atomic-bot
Copy link

💔 Test failed - status-papr

@jlebon
Copy link
Member

jlebon commented Sep 30, 2019

PAPR is failing because OpenStack no longer enables nested virt by default. Requested access again for our tenant.

@jlebon
Copy link
Member

jlebon commented Oct 2, 2019

bot, retest this please

1 similar comment
@jlebon
Copy link
Member

jlebon commented Oct 2, 2019

bot, retest this please

@jlebon
Copy link
Member

jlebon commented Oct 3, 2019

/lgtm

@jlebon
Copy link
Member

jlebon commented Oct 3, 2019

Hmm weird. The PR status page shows it's "Good to be merged". Not sure what tide is waiting for here.

@cgwalters
Copy link
Member Author

Ah, I think this was a race condition because we added the new Prow job. The tide status showed waiting for ci/prow/sanity, but I'm guessing it wasn't triggered.

Pushed a rebase on master.

@jlebon
Copy link
Member

jlebon commented Oct 3, 2019

bot, retest this please

@jlebon
Copy link
Member

jlebon commented Oct 3, 2019

/lgtm

What's going to be tricky here is retrying jobs that Prow doesn't control. That was a nice thing with Homu's model of just pushing to a branch and letting different CI services pick up the event naturally.

All this does is put the immutable bit on the target directory.
The intention is to replace this bit to start:
https://github.com/coreos/coreos-assembler/blob/8b205bfbb971707382ace76bbb39e46ed3fc560d/src/create_disk.sh#L229

However, the real goal here is to add code in this file
to handle redeploying the rootfs for Fedora CoreOS which
combines OSTree+Ignition:
coreos/fedora-coreos-tracker#94

Basically doing this in proper Rust is going to be a lot
nicer than shell script in dracut modules.  Among other
details, coreutils `mv` doesn't seem to do the right thing
for SELinux labels when policy isn't loaded.
@cgwalters
Copy link
Member Author

Rebased 🏄‍♂️ also to retrigger CI.

@jlebon
Copy link
Member

jlebon commented Oct 4, 2019

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, 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:

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

/override f29-primary
cached-rpm-diffs failed with

Error: Error downloading packages:
  Status code: 503 for https://mirrors.fedoraproject.org/metalink?repo=fedora-30&arch=x86_64

but that can't break from this.

@openshift-ci-robot
Copy link
Collaborator

@cgwalters: Overrode contexts on behalf of cgwalters: f29-primary

In response to this:

/override f29-primary
cached-rpm-diffs failed with

Error: Error downloading packages:
 Status code: 503 for https://mirrors.fedoraproject.org/metalink?repo=fedora-30&arch=x86_64

but that can't break from this.

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.

@openshift-merge-robot openshift-merge-robot merged commit c8113bd into coreos:master Oct 4, 2019
@cgwalters
Copy link
Member Author

Hmm although misc-1 also failed with an unusual error.

@jlebon
Copy link
Member

jlebon commented Oct 4, 2019

Hmm, I had fixed this in this PR, but it got lost somehow?

There it is:
https://github.com/coreos/rpm-ostree/compare/1a256382f1465865f45d97a138160b992aa5ccd5..6d0f8af7ad18df2bfdeedff26e46955fbabd2a97

@jlebon
Copy link
Member

jlebon commented Oct 4, 2019

#1922

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