PWX-40172 (master): makes chattr ignore 'inappropriate ioctl' errors #2502
Conversation
Signed-off-by: Adam Krpan <[email protected]>
|
|
||
| stderrStr := stderr.String() | ||
| if isInappropriateIoctl(stderrStr) { // Returned in case of filesystem that does not support chattr | ||
| return nil |
There was a problem hiding this comment.
Uh.. this override will unconditionally turn off the inappropriate ioctl for device (also invalid argument while setting flags) errors on chattr for ALL PLATFORMS.
I wonder if we could avoid it?
Some suggestions that might work:
- have
AddImmutable()return theinappropriate ioctl for deviceerror -- and let the caller handle the "error ignoring", or - extend the
AddImmutable()by addingAddImmutable(..., errorsToIgnore []error)and have the caller populate the "errors to ignore" list, or - have the
chattrcommand "overrideable" -- so that the caller may change/override the OpenStorage'schattrcommand with own implementation, before callingAddImmutable()[or, mounting volume]
There was a problem hiding this comment.
Hmm.. I see that your comment above said "// Returned in case of filesystem that does not support chattr"
- so, I suppose you are making an argument that "chattr aways returns the ENOTTY when filesystem does not support the chattr'ing"
- my concern was that we'll also be masking the "legal errors" -- (maybe use a "wrong device" like perhaps
chattr /dev/stderr) -- which is why I'd look for means to reduce the "blast radius" of this change, and limit it only to theSLE-MicroLinux platform
Let's see of others from the team would have any comments / feedback
There was a problem hiding this comment.
That was my assumption (it always returns one of those two errors I linked), but I do not know that for sure and I agree that it would mask "legal" errors.
The "override" option seems the most feasible, especially because when we run that platform detection code we can have it also set perhaps IgnoreChattrIOCTLErrors = true or something. Then the caller has the responsibility of knowing what cases that it matters in.
|
This PR is stale because it has been in review for 3 days with no activity. |
What this PR does / why we need it:
In some OSes like SLE Micro, the
/vardirectory is mounted using an overlay filesystem. Something in this setup does not supportchattr. In this case, we should just ignoreinappropriate ioctl for deviceerrors as there is nothing we can do.Which issue(s) this PR fixes (optional)
PWX-40172
Testing Notes