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

mount-utils: force-format xfs-filesystems too #104923

Merged
merged 3 commits into from Sep 27, 2021
Merged

mount-utils: force-format xfs-filesystems too #104923

merged 3 commits into from Sep 27, 2021
+7 −2

Conversation

davidkarlsen
Copy link
Member

@davidkarlsen davidkarlsen commented Sep 10, 2021 •
edited

Signed-off-by: David J. M. Karlsen david@davidkarlsen.com

What type of PR is this?

/kind bug
/sig storage

What this PR does / why we need it:

XFS filesystems are not force-formatted, this might lead to failures as initially experienced in openebs/lvm-localpv#135

Which issue(s) this PR fixes:

Fixes: openebs/lvm-localpv#135
Fixes: kubernetes/mount-utils#4

Should I maybe clone the issue into this repo?

Special notes for your reviewer:

See similar behaviour for ext3/4 filesystems.

Does this PR introduce a user-facing change?

NONE

XFS-filesystems are now force-formatted (option `-f`) in order to avoid problems being formatted due to detection of magic super-blocks. This aligns with the behaviour of formatting of ext3/4 filesystems.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 10, 2021
@k8s-ci-robot
Copy link
Contributor

@davidkarlsen: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 10, 2021
@davidkarlsen davidkarlsen mentioned this pull request Sep 10, 2021
Closed
@davidkarlsen davidkarlsen changed the title force-format xfs-filesystems too mount-utils: force-format xfs-filesystems too Sep 10, 2021
@davidkarlsen davidkarlsen force-pushed the xfsFormatIssue branch 2 times, most recently from addda76 to 2d30874 Compare September 10, 2021 22:28
@davidkarlsen
force-format xfs-filesystems too
ca1d25d 
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
This was referenced Sep 10, 2021
Open
Closed
@davidkarlsen
fix tests
e16f755 
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 10, 2021
@davidkarlsen
go fmt
39e13b4 
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2021
@davidkarlsen
Copy link
Member Author

/assign @jingxu97

@jkremser jkremser mentioned this pull request Sep 13, 2021
Merged
@davidkarlsen
Copy link
Member Author

@saad-ali ptal?

pohly
pohly approved these changes Sep 21, 2021
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 21, 2021
@pohly
Copy link
Contributor

pohly commented Sep 21, 2021

/assign @jingxu97

For approval.

@pohly pohly removed their assignment Sep 21, 2021
@saad-ali
Copy link
Member

I'd like to make sure I understand this: Format shouldn't happen if there is no filesystem. What would cause this code path to be hit (i.e. no filesystem detected, but format fails because a filesystem exists)? I couldn't quite follow openebs/lvm-localpv#135
Is this a "hammer" approach masking other bugs?

I checked the PR that introduced force for ext3/ext4 formatting (kubernetes/utils#127) and I didn't see a good explanation there either.

@sastorsl
Copy link

sastorsl commented Sep 27, 2021 •
edited

Is this a "hammer" approach masking other bugs?

The code relies on underlying OS tooling which simply checks the volume for traces of file systems, and gives you a warning that you are about to overwrite a file system you possibly hold dear - "are you really sure...".

The default behavior of mkfs is to stop at this point.

This is fine in a manual environment, but with regards to automatically provisioning file systems this will only halt provisioning needlessly. You create and remove file systems all the time, so there will quickly be traces of old file systems.

-f Force overwrite when an existing filesystem is detected on the device. By default, mkfs.xfs will not write to the device if it suspects that there is a filesystem or partition table on the device already.

When creating a logical volume, lvm will take what is marked as available. This is your security.

Then mkfs comes in and creates a file system.
Yes, there are traces of an old file system there, it is to be expected, and must be bypassed, hence the -f.

saad-ali
saad-ali approved these changes Sep 27, 2021
View reviewed changes
Copy link
Member

@saad-ali saad-ali left a comment •
edited

Choose a reason for hiding this comment

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

You create and remove file systems all the time, so there will quickly be traces of old file systems.
Yes, there are traces of an old file system there, it is to be expected, and must be bypassed, hence the -f.

SGTM, thanks!

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidkarlsen, pohly, saad-ali

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2021
@k8s-ci-robot k8s-ci-robot merged commit 486ca67 into kubernetes:master Sep 27, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 27, 2021
@davidkarlsen davidkarlsen deleted the xfsFormatIssue branch September 27, 2021 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly approved these changes

@saad-ali saad-ali saad-ali approved these changes

@jingxu97 jingxu97 Awaiting requested review from jingxu97

Assignees

@saad-ali saad-ali

@jingxu97 jingxu97

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

fails to format xfs: missing args
6 participants
@davidkarlsen @k8s-ci-robot @pohly @saad-ali @sastorsl @jingxu97