sysinfo: Ignore "hidden" sysfs device entries #3260
Conversation
|
Hi @sagigrimberg. Thanks for your PR. I'm waiting for a google member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
|
Thanks for the PR. What is the issue this is solving currently? Are these devices somehow reported currently? |
|
Kubelet fails to access these devices, preventing csi metrics collection. kubelet[4192]: E0102 10:01:46.083009 4192 info.go:99] Failed to get disk map: open /sys/block/nvme0c1n1/dev: no such file or directory |
|
Got it. Is the |
|
Yes, it is a common way. You can also refer to util-linux/lsblk.c that also does the same thing... Unfortunately, we didn't add a proper documentation entry for it in sysfs/block ABI documentation |
sagigrimberg
force-pushed
the
ignore-hidden-devices
branch
from
d1e25fc to
38d4b11
Compare
|
ping? |
|
How can I trigger tests on this PR? |
|
/ok-to-test |
|
Can I get a review on this? |
|
ping? |
sagigrimberg
force-pushed
the
ignore-hidden-devices
branch
from
38d4b11 to
1dfb815
Compare
|
Any reason why this PR was not merged already? @bobbypage @iwankgb ? |
sagigrimberg
force-pushed
the
ignore-hidden-devices
branch
from
1dfb815 to
4149224
Compare
|
@bobbypage this is sitting here forever now, any reason why this is not taken? |
|
/retest |
|
retest |
|
@bobbypage can we get this in please? The failed test looks completely unrelated to the change? This has been sitting here forever and I don't understand why this should be the case TBH. This PR has been primarily ignored while fixing a real issue in cadvisor. |
sagigrimberg
force-pushed
the
ignore-hidden-devices
branch
2 times, most recently
from
ea1dac6 to
2393946
Compare
|
/retest |
|
@sagigrimberg, while I understand your sentiment regarding maintainers not reviewing code in timely manner, I have to say that majority of us are volunteers and we work on cAdvisor in our free time. |
|
@iwankgb I was not assigning blame. I was just stating a fact. This PR was primarily ignored. This PR stands since March 2'nd, which since then cadvisor accumulated a few dosands of of PRs merged, while this one has gone, presumably ready to merge since about then (outside of breaking lately and me fixing it after a rebase). If you don't have free time to work on the project, or you don't think that it is a suitable fix for the project, or if you think that there is risk associated it is fine, but it would be appreciated to get at least some feedback... Glad this at least got some attention though... |
utils/sysfs/sysfs.go
Outdated
| // https://git.kernel.org/pub/scm/utils/util-linux/util-linux.git | ||
| // - c8487d854ba5 ("lsblk: Ignore hidden devices") | ||
| hidden, err := os.ReadFile(path.Join(blockDir, name, "/hidden")) | ||
| if err != nil { |
There was a problem hiding this comment.
Non-nil error that is not os.ErrNotExists must not be silently ignored. Returned wrapped error and handle it accordingly wherever this function is called, please.
sagigrimberg
force-pushed
the
ignore-hidden-devices
branch
from
2393946 to
b7b05f8
Compare
utils/sysfs/sysfs.go
Outdated
| return false, nil | ||
| } | ||
| if err != nil { | ||
| return false, fmt.Errorf("failed to read %s: %s", devHiddenPath, err) |
There was a problem hiding this comment.
I would rather wrap the error:
| return false, fmt.Errorf("failed to read %s: %s", devHiddenPath, err) | |
| return false, fmt.Errorf("failed to read %s: %w", devHiddenPath, err) |
Some devices are "hidden" from userspace due to various reasons. One reason is native nvme multipathing, where the path devices do not present a device node to the user but only exposes the stacking device as the only nvme device. Still there are sysfs attributes for these hidden devices, but we should ignore them altogether because these are not "real" devices at all. Signed-off-by: Sagi Grimberg <[email protected]>
sagigrimberg
force-pushed
the
ignore-hidden-devices
branch
from
b7b05f8 to
88bfa0d
Compare
sysfs: trim spaces in device hidden check (cherry picked from commit 96c346e) Backports google#3260 from cadvisor v0.51.0. This is needed to fix container_fs_ metrics on systems with multipath devices. Without it, errors like: Failed to get disk map: open /sys/block/nvme0c0n1/dev will be logged, and cadvisor will emit nonsensical metrics with the label device="" for filesystem I/O. See google#3693 for full explanation. Signed-off-by: Craig Ringer <[email protected]>
5 participants
Some devices are "hidden" from userspace due to various reasons. One reason is native nvme multipathing, where the path devices do not present a device node to the user but only exposes the stacking device as the only nvme device. Still there are sysfs attributes for these hidden devices, but we should ignore them altogether because these are not "real" devices at all.