oo-accept-node: check_quotas: cope with loop mount

Miciah commented 8 years ago

`oo-accept-node`: Add and use `do_warn`

Define and use the do_warn method.

`oo-accept-node`: `check_quotas`: cope with loop mount

Modify oo-accept-node's quota check to check whether the mount is using a loop device mount and, if so, check the quota on the loop device. In addition, print a warning that using a loop mount may impact performance negatively.

This commit fixes bug 1265811.

https://bugzilla.redhat.com/show_bug.cgi?id=1265811

`oo-accept-node`: `check-quotas`: simplify code

Simplify the logic in oo-accept-node's check_quotas function to run the df command and parse its output only once.

@sferich888, @tiwillia

I could not resist making the gratuitous cleanup, but I can drop it if you think I ought.

Miciah commented 8 years ago

Please [test]!

tiwillia commented 8 years ago

@Miciah do we need both commits here, or can we squash them into one?

This LGTM otherwise.

Miciah commented 8 years ago

I'd rather keep them in separate commits, or even drop the simplification, rather than squash, which I believe would make the changes more difficult to read.

sferich888 commented 8 years ago

Is it really wise to support loop devices for the /var/lib/openshift directory. Wouldn't this cause performance issues if allowed?

Miciah commented 8 years ago

There probably generally is a better solution to any problem that someone solves using loop devices.

If an administrator does use a loop device for /var/lib/openshift/, oo-accept-node currently prints a bogus failure. This PR enables oo-accept-node to function properly. An alternative would be to make oo-accept-node clearly and explicitly fail (or print a warning) if /var/lib/openshift/ is on a loop device, with an accurate error message. Would that alternative be better?

sferich888 commented 8 years ago

I propos allowing for this and posting a clear warning On Jan 27, 2016 5:39 PM, "Miciah Dashiel Butler Masters" < notifications@github.com> wrote:

There probably generally is a better solution to any problem that someone solves using loop devices.

If an administrator does use a loop device for /var/lib/openshift/, oo-accept-node currently prints a bogus failure. This PR enables oo-accept-node to function properly. An alternative would be to make oo-accept-node clearly and explicitly fail (or print a warning) if /var/lib/openshift/ is on a loop device, with an accurate error message. Would that alternative be better?

— Reply to this email directly or view it on GitHub https://github.com/openshift/origin-server/pull/6350#issuecomment-175897411 .

openshift-bot commented 8 years ago

Evaluated for online test up to 73caba72e8f78aaa520f50368e494f6e4cc8236f

Miciah commented 8 years ago

I have updated the test to print a warning if /var/lib/openshift/ is a loop mount:

      do_warn "#{oo_mount} is a loop mount (loop device: #{oo_device}).  Using a loop mount may reduce performance."

@tiwillia, does this look good to you?

openshift-bot commented 8 years ago

Online Test Results: SUCCESS (https://ci.dev.openshift.redhat.com/jenkins/job/test_pull_requests/9191/)

tiwillia commented 8 years ago

@Miciah :+1: LGTM

Shall I merge?

Miciah commented 8 years ago

Thanks! I'll do the honours...

openshift-bot, please [merge]!

openshift-bot commented 8 years ago

Online Merge Results: SUCCESS (https://ci.dev.openshift.redhat.com/jenkins/job/merge_pull_requests/6706/) (Image: devenv_5767)

openshift-bot commented 8 years ago

Evaluated for online merge up to 73caba72e8f78aaa520f50368e494f6e4cc8236f

openshift / origin-server