Re: [kubernetes/kubernetes] Proposal: Use `blkid` to detect fs type of device instead of `lsblk`. (#59050)
Jan Šafránek
/assign
I generally do not have anything against blkid, however the test failures look suspicious.
cc: @kubernetes/sig-storage-pr-reviews for broader discussion. Is anyone against using blkid instead of lsblk?
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Huamin Chen
both are in util-linux package, no an issue to me.
Huamin Chen
@rootfs commented on this pull request.
In pkg/util/mount/mount_linux.go:
> @@ -549,23 +549,38 @@ func (mounter *SafeFormatAndMount) GetDiskFormat(disk string) (string, error) {
return "", err
}
- // Split lsblk output into lines. Unformatted devices should contain only
- // "\n". Beware of "\n\n", that's a device with one empty partition.
- output = strings.TrimSuffix(output, "\n") // Avoid last empty line
+ var (
nit var fstype, pttype string
Huamin Chen
> }
- if len(lines) == 1 {
- // The device is unformatted and has no dependent devices
- return "", nil
+ if len(pttype) > 0 {
+ glog.V(4).Infof("Disk %s detected parition table type: %s", pttype)
partition
Huamin Chen
howeever, blkid manpage says otherwise and I do have a problem to run blkid with non privileged id.
It is recommended to use lsblk(8) command to get information about
block devices rather than blkid. lsblk(8) provides more information,
better control on output formatting and it does not require root per‐
missions to get actual information.
Hemant Kumar
I do not have anything against using blkid. I briefly straced invocation of both blkid and lsblk and output of blkid looked good (There was no udev reads). Having said that - yeah, the test failures look suspicious and hopefully nothing breaks.
afaict - kubelet always runs with privileged permissions, so blkid requiring privilged id should not be a blocker.
Michelle Au
I think we should prerun all the storage e2es for this change. The pull e2es may not exercise this path.
I can run against COS.
Michelle Au
/assign
k8s-ci-robot
@cofyc: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|---|---|---|
| pull-kubernetes-e2e-kubeadm-gce-canary | 97e54c1 | link | /test pull-kubernetes-e2e-kubeadm-gce-canary |
| pull-kubernetes-e2e-kops-aws | c5ae9a4 | link | /test pull-kubernetes-e2e-kops-aws |
| pull-kubernetes-e2e-gce | c5ae9a4 | link | /test pull-kubernetes-e2e-gce |
| pull-kubernetes-cross | 5136938 | link | /test pull-kubernetes-cross |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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. I understand the commands that are listed here.
k8s-ci-robot
k8s-ci-robot
[APPROVALNOTIFIER] This PR is NOT APPROVED
This pull-request has been approved by: cofyc
We suggest the following additional approver: jsafrane
Assign the PR to them by writing /assign @jsafrane in a comment when ready.
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
Yecheng Fu
@cofyc commented on this pull request.
In pkg/util/mount/mount_linux.go:
> }
- if len(lines) == 1 {
- // The device is unformatted and has no dependent devices
- return "", nil
+ if len(pttype) > 0 {
+ glog.V(4).Infof("Disk %s detected parition table type: %s", pttype)
Fixed.
Yecheng Fu
> @@ -549,23 +549,38 @@ func (mounter *SafeFormatAndMount) GetDiskFormat(disk string) (string, error) {
return "", err
}
- // Split lsblk output into lines. Unformatted devices should contain only
- // "\n". Beware of "\n\n", that's a device with one empty partition.
- output = strings.TrimSuffix(output, "\n") // Avoid last empty line
+ var (
k8s-ci-robot
@cofyc: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-cross | 5136938 | link | /test pull-kubernetes-cross |
| pull-kubernetes-e2e-kops-aws | 5136938 | link | /test pull-kubernetes-e2e-kops-aws |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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. I understand the commands that are listed here.
—
Yecheng Fu
/test pull-kubernetes-cross
Yecheng Fu
/pull-kubernetes-e2e-gce
/pull-kubernetes-e2e-kops-aws
Yecheng Fu
@cofyc pushed 1 commit.
- 322c094 Use `blkid` to get fs type of device.
—
You are receiving this because you are subscribed to this thread.
View it on GitHub or mute the thread.
k8s-ci-robot
@cofyc: The following tests failed, say /retest to rerun them all:
| Test name | Commit | Details | Rerun command |
|---|
| pull-kubernetes-unit | 322c094 | link | /test pull-kubernetes-unit |
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.
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. I understand the commands that are listed here.
—
You are receiving this because you are on a team that was mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
Yecheng Fu
/pull-kubernetes-unit
Yecheng Fu
/test pull-kubernetes-unit
Michelle Au
@verult ran the e2es and manually tested against GCE PD to make sure the partition detection works
k8s-ci-robot
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: cofyc, jsafrane
The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:pkg/util/mount/OWNERS[jsafrane]
You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment
—
Kubernetes Submit Queue
Automatic merge from submit-queue (batch tested with PRs 51323, 59306, 58991, 59050). If you want to cherry-pick this change to another branch, please follow the instructions here.
Kubernetes Submit Queue
Merged #59050.
Yaniv Kaul
Why not use '--usages filesystem' to filter for file systems in the call to blkid?
Yecheng Fu
I didn't know this parameter when I wrote this patch. It's my mistake. I think we can have it if it does not introduce incompatibilities. Done a quick test, it uses less read calls.
# strace blkid -p -s TYPE -s PTTYPE -o export /dev/sda1 |& grep -i '^read(' | wc -l
29
$ strace blkid -p -u filesystems -s TYPE -s PTTYPE -o export /dev/sda1 |& grep -i '^read(' | wc -l
25