-
Notifications
You must be signed in to change notification settings - Fork 210
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
Check for sufficient disk space before upgrade #1381
Conversation
f0d86d4
to
95f9c43
Compare
A number of our supported platforms don't have the
If we wanted to parse |
All of our platforms support the
|
95f9c43
to
94aa3eb
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.
I've confused myself a bit with those numbers 😄
Looks good to me
# TODO(ssd) 2017-08-18: Do we need to worry about sparse files | ||
# here? If so, can we expect the --apparent-size flag to exist on | ||
# all of our platforms. | ||
command = Mixlib::ShellOut.new("du -sk #{path}") |
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.
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.
Nevermind, you've already checked that. (Just now read the message)
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.
-P
is a flag to df
rather than du
. Unfortunately, as far as I can see, the -P
flag doesn't solve the "mount point and file system with space with spaces"
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.
yeah sorry for the noise, ignore me
raise "du failed" | ||
end | ||
rescue Errno::ENOENT | ||
raise "The du utility is not available. Unable to check disk usage" |
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.
Does this block upgrading? Can I override the disk usage check failure if I believe I know what I'm doing?
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.
Currently it does. We could use an environment variable to skip the check perhaps?
# @param path [String] Path to a directory on disk | ||
# @return [Integer] KB used by directory on disk | ||
# | ||
def self.du(path) |
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.
Would it be hard to statfs for this, too? I've got the impression that this would let us stay clear of some potential compatibility issues...
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.
Nevermind 😄
return dir if ::File.exists?(dir) | ||
return dir if ::File.expand_path(dir) == "/" | ||
|
||
dir_or_existing_parent(::File.expand_path("#{dir}/..")) |
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.
Hehe that's clever
else | ||
Chef::Log.error("Insufficient free space on disk to complete upgrade.") | ||
Chef::Log.error("The current postgresql data directory contains #{old_data_dir_size} KB of data but only #{free_disk_space} KB is available on disk.") | ||
Chef::Log.error("The upgrade process requires at least #{free_disk_space/0.90} KB.") |
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.
I'm confused about the division here.... so, we want free disk space to be... uhm...
old_data_dir_size < (free_disk_space * 0.9) # * 1/0.9
old_data_dir_size * 1/0.9 < free_disk_space
so we want free_disk_space to be 1.1111 * old_data_dir_size, don't we?
old_data_dir_size * 1/0.9 < free_disk_space # * 1/0.9
old_data_dir_size * 1/0.9 * 1/0.9 < free_disk_space / 0.90
so, free_disk_space/0.90 = 1.2345679 * old_disk_space?
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.
I think I've just goofed and this should have been old_data_dir_size/0.9
. That is, I used the wrong variable.
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.
I double checked the FFI interface. Looks great to me.
end | ||
|
||
def dir_or_existing_parent(dir) | ||
return dir if ::File.exists?(dir) |
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.
nit: exist?
class Statfs | ||
# | ||
# Statfs provides a simple interface to the statvfs system call. | ||
# Since the statvfs struct varies a bit across platforms, this likly |
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.
sp: likely
The `pg_upgrade` tool copies all of the database files as part of the upgrade. While this could be avoided with the `--link` flag, the `--link` flag would also prevent falling back to the old pg cluster in case of a problem. To avoid upgrade failures, we check that the disk for the new data directory has enough free space for another copy of the database plus some headroom to ensure we have some room to operate after the upgrade. Available disk space is currently queried via an FFI call into libc's statvfs function. The downside of the FFI approach is making sure our representation of the statvfs struct is correct on all of our supported platforms. A more straightforward approach might be to parse `df` output; however, we've gone with the FFI call for now to avoid some edge cases in parsing `df` output on versions of `df` without an --output flag. Signed-off-by: Steven Danna <[email protected]>
c92c1ac
to
9f03aff
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.
🌮
The
pg_upgrade
tool copies all of the database files as part of theupgrade. While this could be avoided with the
--link
flag, the--link
flag would also prevent falling back to the old pg cluster incase of a problem.
To avoid upgrade failures, we check that the disk for the new data
directory has enough free space for another copy of the database plus
some headroom to ensure we have some room to operate after the
upgrade.
Available disk space is currently queried via an FFI call into libc's
statvfs function. The downside of the FFI approach is making sure our
representation of the statvfs struct is correct on all of our
supported platforms. A more straightforward approach might be to parse
df
output; however, the first and the last columns of thedf
output are paths which could theoretically contain spaces making it a
bit of a pain to parse. However, if all of our platforms support the
--output
flag, this problem can be easily avoided.Signed-off-by: Steven Danna [email protected]