inspect: ignore ENOENT during device lookup #20250
Conversation
openshift-ci
bot
added
release-note
approved
When we walk the /dev tree we need to lookup all device paths. Now in order to get the major and minor version we have to actually stat each device. This can again fail of course. There is at least a race between the readdir at stat call so it must ignore ENOENT errors to avoid the race condition as this is not a user problem. Second, we should also not return other errors and just log them instead, returning an error means stopping the walk and returning early which means inspect fails with an error which would be bad. Also there seems to be cases were ENOENT will be returned all the time, e.g. when a device is forcefully removed. In the reported bug this is triggered with iSCSI devices. Because the caller does already lookup the device from the created map it reports a warning there if the device is missing on the host so it is not a problem to ignore a error during lookup here. [NO NEW TESTS NEEDED] Requires special device setup to trigger consistentlyand we cannot do that in CI. Fixes https://issues.redhat.com/browse/RHEL-11158 Signed-off-by: Paul Holzinger <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Luap99, vrothberg 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 |
| @@ -48,7 +48,16 @@ func FindDeviceNodes() (map[string]string, error) { | |||
|
|
|||
| info, err := d.Info() | |||
There was a problem hiding this comment.
A few lines up (line 45) there's d.Type(), doesn't that require a stat() also? Sorry for the drive-by comment, I've tried to read the go doc but it's not as helpful as I need.
There was a problem hiding this comment.
No that is one of the reasons go introduced WalkDir() vs the old Walk(). If you read the directory entries via getdents() system call the structure will include the file type so there is no need for a stat on each file.
Walking is just a simple open(), getdents(), close(). The fs.DirEntry interface exposes basically parts of the linux_dirent structure. Only if we ask for full info it needs to stat.
|
/lgtm |
openshift-ci
bot
added
the
do-not-merge/hold
|
LGTM |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
github-actions
bot
added
the
locked - please file new issue/PR
When we walk the /dev tree we need to lookup all device paths. Now in order to get the major and minor version we have to actually stat each device. This can again fail of course. There is at least a race between the readdir at stat call so it must ignore ENOENT errors to avoid the race condition as this is not a user problem. Second, we should also not return other errors and just log them instead, returning an error means stopping the walk and returning early which means inspect fails with an error which would be bad.
Also there seems to be cases were ENOENT will be returned all the time, e.g. when a device is forcefully removed. In the reported bug this is triggered with iSCSI devices.
Because the caller does already lookup the device from the created map it reports a warning there if the device is missing on the host so it is not a problem to ignore a error during lookup here.
Fixes https://issues.redhat.com/browse/RHEL-11158
Does this PR introduce a user-facing change?