-
Notifications
You must be signed in to change notification settings - Fork 511
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
test: update permissions of DAX devices after clearing bad blocks #5611
test: update permissions of DAX devices after clearing bad blocks #5611
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ldorau)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ldorau)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @ldorau)
Codecov Report
@@ Coverage Diff @@
## stable-1.13 #5611 +/- ##
===============================================
- Coverage 74.26% 74.25% -0.01%
===============================================
Files 145 145
Lines 22131 22131
Branches 3704 3705 +1
===============================================
- Hits 16435 16433 -2
- Misses 5696 5698 +2 |
383f440
to
d89bab5
Compare
Added "Ref: #5606" to the commit message. |
d89bab5
to
04566b9
Compare
Rebased to pmem:stable-1.13. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @ldorau)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ldorau)
src/test/unittest/badblock.py
line 113 at r2 (raw file):
# because it removes the old files and creates new ones # with default permissions, so their permissions # have to be updated.
updated -> restored?
Please see the comment below.
src/test/unittest/badblock.py
line 119 at r2 (raw file):
futils.run_command("sudo chmod a+r {}".format(resources), "failed to change permissions " "of DAX devices' resources")
I thought about reading the permissions before calling the culprit command and using the output to restore the permissions as if they were before calling the command.
Would it be that much more complex?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)
src/test/unittest/badblock.py
line 113 at r2 (raw file):
Previously, janekmi (Jan Michalski) wrote…
updated -> restored?
Please see the comment below.
updated -> fixed ?
src/test/unittest/badblock.py
line 119 at r2 (raw file):
Previously, janekmi (Jan Michalski) wrote…
I thought about reading the permissions before calling the culprit command and using the output to restore the permissions as if they were before calling the command.
Would it be that much more complex?
I'm sure it would be much more complex. There can be a lot of those files. How do you want to do that? Is it worth this effort? I doubt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ldorau)
src/test/unittest/badblock.py
line 113 at r2 (raw file):
What would you say to write it down as follows:
The "sudo ndctl clear-errors" command resets permissions of the DAX block device and its resource files because it removes the old files and creates new ones with default permissions which interrupts further test execution. The following commands set permissions which should allow further test execution to work around this issue.
src/test/unittest/badblock.py
line 119 at r2 (raw file):
Previously, ldorau (Lukasz Dorau) wrote…
I'm sure it would be much more complex. There can be a lot of those files. How do you want to do that? Is it worth this effort? I doubt.
Alright. You convinced me. 😆 I think I have to find a way to cut the corners in general to address everything we currently got on our plate. 👍
04566b9
to
453f983
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @grom72, @janekmi, and @osalyk)
src/test/unittest/badblock.py
line 113 at r2 (raw file):
Previously, janekmi (Jan Michalski) wrote…
What would you say to write it down as follows:
The "sudo ndctl clear-errors" command resets permissions of the DAX block device and its resource files because it removes the old files and creates new ones with default permissions which interrupts further test execution. The following commands set permissions which should allow further test execution to work around this issue.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72 and @janekmi)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72 and @janekmi)
The "sudo ndctl clear-errors" command resets permissions of the DAX block device and its resource files because it removes the old files and creates new ones with default permissions which interrupts further test execution. The following commands set permissions which should allow further test execution to work around this issue. Ref: pmem#5606 Signed-off-by: Lukasz Dorau <lukasz.dorau@intel.com>
453f983
to
b207bcd
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @grom72)
The "sudo ndctl clear-errors" command resets permissions of DAX block device and its resource files,
because it removes the old files and creates new ones with default permissions, so their permissions have to be updated.
Fixes: #5606
This change is