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.

Sign up for GitHub

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

Jump to bottom

internal/exec: don't relabel a mountpoint that already exists #1608

Merged
merged 1 commit into from
Jun 28, 2023
Merged

internal/exec: don't relabel a mountpoint that already exists #1608

merged 1 commit into from
Jun 28, 2023

Conversation

Adam0Brien
Copy link
Member

Fix: #1452

@Adam0Brien Adam0Brien self-assigned this Apr 11, 2023
@Adam0Brien Adam0Brien marked this pull request as draft April 11, 2023 14:02
@prestist prestist self-requested a review April 11, 2023 14:07
@Adam0Brien Adam0Brien marked this pull request as ready for review April 11, 2023 14:14
@Adam0Brien
Copy link
Member Author

Marking as ready for review as all tests are passing.

@Adam0Brien Adam0Brien marked this pull request as draft April 11, 2023 15:15
@Adam0Brien Adam0Brien force-pushed the mountpoints-1452 branch 8 times, most recently from 97b3add to a37604d Compare May 5, 2023 15:03
@Adam0Brien Adam0Brien marked this pull request as ready for review May 5, 2023 15:05
@bgilbert bgilbert requested review from prestist and removed request for prestist May 9, 2023 05:07
prestist
prestist reviewed May 10, 2023
View reviewed changes
Copy link
Collaborator

@prestist prestist left a comment

Choose a reason for hiding this comment

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

Great first attempt!

A few changes, and I noticed that your commit message was a bit long, and might need a few more details

docs/release-notes.md Outdated Show resolved Hide resolved
internal/exec/stages/mount/mount.go Show resolved Hide resolved
internal/exec/stages/mount/mount.go Outdated Show resolved Hide resolved
tests/positive/filesystems/skip_readonly.go Outdated Show resolved Hide resolved
@prestist
Copy link
Collaborator

You will also need to rebase off of main to get your CI working due to #1621

@prestist
Copy link
Collaborator

prestist commented May 18, 2023 •
edited
Loading

Could we squash all these commits into one please? Sorry to keep bugging you about that, it makes review easier.

I really do prefer following a process similar to this workflow. I know it takes some getting used to but it does help with the process and I have found that I enjoy it my self.

@Adam0Brien Adam0Brien force-pushed the mountpoints-1452 branch 5 times, most recently from bd7937d to de3f003 Compare May 22, 2023 11:05
bgilbert
bgilbert reviewed Jun 8, 2023
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The release note is essentially correct (one small nit), but unfortunately the rest of the PR does something different than what the release note claims to do. And also, the PR doesn't appear to have been tested against the bug it's trying to fix. Details in comments below.

Blackbox tests have some runtime cost, and are mainly used to validate core functionality. This change is a small fix for a corner case; blackbox tests seem too expensive (and not trivial to set up in this case) and unit tests can't check this. I'd be inclined to recommend not adding tests for this one, and just manually testing that it fixes the bug.

internal/exec/util/selinux.go Outdated Show resolved Hide resolved
internal/exec/stages/mount/mount.go Outdated Show resolved Hide resolved
docs/release-notes.md Outdated Show resolved Hide resolved
tests/positive/filesystems/skip_readonly.go Outdated Show resolved Hide resolved
tests/types/types.go Outdated Show resolved Hide resolved
tests/positive/filesystems/skip_readonly.go Outdated Show resolved Hide resolved
@bgilbert
Copy link
Contributor

bgilbert commented Jun 8, 2023

You'll need to rebase to pick up CI changes.

@Adam0Brien Adam0Brien force-pushed the mountpoints-1452 branch 4 times, most recently from 410f409 to f0633a9 Compare June 16, 2023 12:44
bgilbert
bgilbert reviewed Jun 22, 2023
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Your commits don't appear to be independent changes, so please squash them.

internal/exec/stages/mount/mount.go Outdated Show resolved Hide resolved
internal/exec/stages/mount/mount.go Outdated Show resolved Hide resolved
internal/exec/stages/mount/mount.go Outdated Show resolved Hide resolved
internal/exec/stages/mount/mount.go Outdated Show resolved Hide resolved
@Adam0Brien Adam0Brien force-pushed the mountpoints-1452 branch 11 times, most recently from 0e23b48 to a2552d2 Compare June 28, 2023 08:47
@bgilbert
Copy link
Contributor

Code LGTM. Please update the commit message to correctly describe the change (read-only filesystems aren't relevant here) and link to the issue.

@Adam0Brien Adam0Brien force-pushed the mountpoints-1452 branch 3 times, most recently from 6e46d9a to a4cc298 Compare June 28, 2023 10:51
@Adam0Brien
internal/exec: don't relabel a mountpoint that already exists
9b73b92 
When mounting filesystems, Ignition was relabeling the mountpoint
directory even if it previously existed.
This isn't necessary, and fails if the mountpoint is on a read-only
filesystem, such as /usr/share/oem on Flatcar.
Relabel the mountpoint only if we create it.
Fixes: coreos#1452
@bgilbert bgilbert changed the title internal/exec: Skip read-only Filesystems internal/exec: don't relabel a mountpoint that already exists Jun 28, 2023
bgilbert
bgilbert approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@bgilbert bgilbert left a comment

Choose a reason for hiding this comment

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

🎉

@bgilbert bgilbert enabled auto-merge June 28, 2023 12:01
@bgilbert bgilbert merged commit 1159173 into coreos:main Jun 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prestist prestist prestist left review comments

@bgilbert bgilbert bgilbert approved these changes

Assignees

@Adam0Brien Adam0Brien

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Don't try to relabel mountpoints which already exist
3 participants
@Adam0Brien @prestist @bgilbert