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

Ignore sysfs file read errors #799

Merged
merged 1 commit into from Jun 12, 2023

Conversation

harshavardhana
Copy link
Member

No description provided.

@Praveenrajmani
Copy link
Collaborator

we can set false to the readFirstLine function here -

s, err := readFirstLine("/sys/class/block/"+name+"/size", true)

@balamurugana
Copy link
Member

we can set false to the readFirstLine function here -

s, err := readFirstLine("/sys/class/block/"+name+"/size", true)

@harshavardhana @Praveenrajmani Do you guys see sysfs devices with no size entry?

@Praveenrajmani
Copy link
Collaborator

yes @balamurugana

portworx volumes are missing this

@harshavardhana
Copy link
Member Author

we can set false to the readFirstLine function here -

s, err := readFirstLine("/sys/class/block/"+name+"/size", true)

@harshavardhana @Praveenrajmani Do you guys see sysfs devices with no size entry?

Yes portworx devices seems to have it missing, an another CSI product.

@harshavardhana
Copy link
Member Author

we can set false to the readFirstLine function here -

s, err := readFirstLine("/sys/class/block/"+name+"/size", true)

We can simply ignore all devices that do not comply with what we want. I am not big fan of failing pods with no easy to understand errors.

Unless we want to log why we are ignoring the drives? That is okay too.

@balamurugana
Copy link
Member

@harshavardhana You could pass false at https://github.com/minio/directpv/blob/master/pkg/device/sysfs_linux.go#L75 which fixes the issue.

@harshavardhana
Copy link
Member Author

@harshavardhana You could pass false at https://github.com/minio/directpv/blob/master/pkg/device/sysfs_linux.go#L75 which fixes the issue.

No @balamurugana that is not a complete fix. We should ignore all devices that do not comply with Disk metadata that we want.

It is not just about /size there are other files that may be missing. There is no document that indicates that these files "must" be present or they are mandatory.

The current PR simply addresses this more comprehensively. For example, portworx has a couple of other files missing as well. If we only fixed /size alone is not sufficient.

IMO we shouldn't crash our pods unnecessarily. We should work when we have other good drives we can manage.

This is a support nightmare and also a bad POC experience. We should be able to deploy DirectPV without @balamurugana or @Praveenrajmani's involvement.

pkg/device/probe_linux.go Outdated Show resolved Hide resolved
pkg/device/probe_linux.go Outdated Show resolved Hide resolved
pkg/device/probe_linux.go Outdated Show resolved Hide resolved
@balamurugana balamurugana changed the title ignore devices with device metadata missing Ignore sysfs file read errors Jun 12, 2023
@Praveenrajmani Praveenrajmani merged commit 0ed3d97 into minio:master Jun 12, 2023
22 checks passed
@harshavardhana harshavardhana deleted the ignore-meta branch June 12, 2023 02:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants