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

Healing process should not heal root disk #7089

Merged
merged 1 commit into from Jan 23, 2019

Conversation

krishnasrinivas
Copy link
Contributor

Description

If an admin umounts a disk and runs heal, currently we endup healing a directory on the root FS. This PR ensures that when we heal, format healing is done only on non-root disks.

Motivation and Context

If we heal a directory on root disk we will end up filling up the root disk

Regression

No

How Has This Been Tested?

By using a loop mounted FS, I was able to simulate actualy disk mount to test this.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added unit tests to cover my changes.
  • I have added/updated functional tests in mint. (If yes, add mint PR # here: )
  • All new and existing tests passed.

@minio-ops
Copy link

Mint Automation

Test Result
mint-tls.sh ✔️
mint-compression-xl.sh ✔️
mint-xl.sh ✔️
mint-gateway-nas.sh ✔️
mint-large-bucket.sh ✔️
mint-compression-fs.sh ✔️
mint-worm.sh ✔️
mint-fs.sh ✔️
mint-dist-xl.sh ✔️

cmd/posix.go Outdated
if diskStat.Dev == rootStat.Dev {
// Indicate if the disk path is on root disk. This is used to indicate the healing
// process not to format the drive and end up healing it.
IsRootDisk = true
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be simplied to IsRootDisk = diskStat.Dev == rootStat.Dev , also variable name can be just rootDisk which has true/false

I like the idea to determine this during Heal instead of newPosix() avoids state and false positives.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krishnasrinivas can you take a look

@codecov
Copy link

codecov bot commented Jan 17, 2019

Codecov Report

Merging #7089 into master will increase coverage by 0.23%.
The diff coverage is 25.71%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7089      +/-   ##
==========================================
+ Coverage   51.19%   51.42%   +0.23%     
==========================================
  Files         267      279      +12     
  Lines       33667    43976   +10309     
==========================================
+ Hits        17236    22616    +5380     
- Misses      14344    19273    +4929     
  Partials     2087     2087
Impacted Files Coverage Δ
pkg/disk/root-disk-unix.go 0% <0%> (ø)
pkg/disk/root-disk-windows.go 0% <0%> (ø)
cmd/xl-sets.go 56.3% <40%> (-0.8%) ⬇️
cmd/posix.go 65.27% <62.5%> (+0.18%) ⬆️
cmd/http/conn_bug_21133.go 66.66% <0%> (-8.34%) ⬇️
cmd/gateway-main.go 15.53% <0%> (-7.35%) ⬇️
main.go 50% <0%> (-7.15%) ⬇️
pkg/madmin/utils.go 16.66% <0%> (-7.15%) ⬇️
cmd/config-dir.go 40.74% <0%> (-7.09%) ⬇️
cmd/globals.go 55.88% <0%> (-7.09%) ⬇️
... and 250 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 74c2048...fdd7b1b. Read the comment docs.

harshavardhana
harshavardhana previously approved these changes Jan 17, 2019
Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tested

@harshavardhana
Copy link
Member

@krishnasrinivas you need to fix windows build issues https://travis-ci.org/minio/minio/jobs/481015811

Copy link
Member

@harshavardhana harshavardhana left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tested

Copy link
Contributor

@kannappanr kannappanr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Tested.

@kannappanr kannappanr merged commit 82af0be into minio:master Jan 23, 2019
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

4 participants