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

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

Merged
merged 3 commits into from Sep 27, 2021

Conversation

@davidkarlsen
Copy link
Member

@davidkarlsen davidkarlsen commented Sep 10, 2021

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
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Sep 10, 2021

@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.

Loading

@k8s-ci-robot k8s-ci-robot requested review from jingxu97 and saad-ali Sep 10, 2021
@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 Sep 10, 2021
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
Signed-off-by: David J. M. Karlsen <david@davidkarlsen.com>
@davidkarlsen
Copy link
Member Author

@davidkarlsen davidkarlsen commented Sep 12, 2021

/assign @jingxu97

Loading

@davidkarlsen
Copy link
Member Author

@davidkarlsen davidkarlsen commented Sep 19, 2021

@saad-ali ptal?

Loading

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

@pohly pohly left a comment

/lgtm

Loading

@pohly
Copy link
Contributor

@pohly pohly commented Sep 21, 2021

/assign @jingxu97

For approval.

Loading

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

@saad-ali saad-ali commented Sep 27, 2021

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.

Loading

@sastorsl
Copy link

@sastorsl sastorsl commented Sep 27, 2021

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.

Loading

Copy link
Member

@saad-ali saad-ali left a comment

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

Loading

@k8s-ci-robot
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot commented Sep 27, 2021

[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

Loading

@k8s-ci-robot k8s-ci-robot merged commit 486ca67 into kubernetes:master Sep 27, 2021
14 checks passed
Loading
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Sep 27, 2021
@davidkarlsen davidkarlsen deleted the xfsFormatIssue branch Sep 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment