Accurately report when the lvmsync receiver fails

This work was most cleanly achieved by refactoring out a lot of "behind the
scenes" logic that should be buried inside a "LogicalVolume" class, so I
made one, and used that.  Overall, I think it's a definite net win.

Author: mpalmer

bin/lvmsync

@@ -20,6 +20,7 @@
 require 'optparse'
 require 'lvm'
 require 'git-version-bump'
+require 'open3'
 
 PROTOCOL_VERSION = "lvmsync PROTO[3]"
 
@@ -163,27 +164,19 @@ def run_client(opts)
 	destdev = opts[:destdev]
 	outfd = nil
 
-	vg, lv = parse_snapshot_name(snapshot)
-
-	vgconfig = LVM::VGConfig.new(vg)
-
-	if vgconfig.logical_volumes[lv].nil?
-		$stderr.puts "#{snapshot}: Could not find logical volume"
+	lv = begin
+		LVM::LogicalVolume.new(snapshot)
+	rescue RuntimeError => e
+		$stderr.puts "#{snapshot}: could not find logical volume (#{e.message})"
 		exit 1
 	end
 
-	snap = if vgconfig.logical_volumes[lv].snapshot?
-		if vgconfig.logical_volumes[lv].thin?
-			LVM::ThinSnapshot.new(vg, lv)
-		else
-			LVM::Snapshot.new(vg, lv)
-		end
-	else
+	unless lv.snapshot?
 		$stderr.puts "#{snapshot}: Not a snapshot device"
 		exit 1
 	end
 
-	$stderr.puts "Origin device: #{vg}/#{snap.origin}" if opts[:verbose]
+	$stderr.puts "Origin device: #{lv.origin.path}" if opts[:verbose]
 
 	# Since, in principle, we're not supposed to be reading from snapshot
 	# devices directly, the kernel makes no attempt to make the device's read
@@ -195,27 +188,41 @@ def run_client(opts)
 	snapback = opts[:snapback] ? "--snapback #{opts[:snapback]}" : ''
 
 	if opts[:stdout]
-		outfd = $stdout
+		dump_changes(lv, $stdout, opts)
 	else
 		server_cmd = if desthost
 			"ssh #{desthost} lvmsync --apply - #{snapback} #{destdev}"
 		else
 			"#{$0} --apply - #{snapback} #{destdev}"
 		end
 
-		outfd = IO.popen(server_cmd, 'w')
+		exit_status = nil
+		errors = nil
+
+		Open3.popen3(server_cmd) do |stdin_fd, stdout_fd, stderr_fd, wait_thr|
+			dump_changes(lv, stdin_fd, opts)
+			stdin_fd.close
+			errors = stderr_fd.read
+			exit_status = wait_thr.value if wait_thr
+		end
+
+		if (exit_status or $?).exitstatus != 0
+			$stderr.puts "APPLY FAILED."
+			$stderr.puts errors.split("\n").map { |l| "remote: #{l}" }.join("\n")
+		end
 	end
+end
 
+def dump_changes(lv, outfd, opts)
 	outfd.puts PROTOCOL_VERSION
 
 	start_time = Time.now
 	xfer_count = 0
 	xfer_size  = 0
 	total_size = 0
 
-	originfile = "/dev/mapper/#{vg.gsub('-', '--')}-#{snap.origin.gsub('-', '--')}"
-	File.open(originfile, 'r') do |origindev|
-		snap.differences.each do |r|
+	File.open(lv.origin.path, 'r') do |origindev|
+		lv.changes.each do |r|
 			xfer_count += 1
 			chunk_size = r.last - r.first + 1
 			xfer_size  += chunk_size
@@ -247,8 +254,6 @@ def run_client(opts)
 
 	$stderr.printf "You transferred your changes %.2fx faster than a full dd!\n",
 	               total_size.to_f / xfer_size
-ensure
-	outfd.close unless outfd.nil? or outfd == $stdout
 end
 
 # Take a device name in any number of different formats and return a [VG, LV] pair.

lib/lvm.rb

@@ -1,5 +1,6 @@
 require 'lvm/helpers'
 require 'lvm/thin_snapshot'
 require 'lvm/snapshot'
+require 'lvm/logical_volume'
 require 'lvm/lv_config'
 require 'lvm/vg_config'

lib/lvm/logical_volume.rb

@@ -0,0 +1,83 @@
+module LVM; end
+
+# This class represents an LVM logical volume, in all its glory.  You can
+# perform various operations on it.
+class LVM::LogicalVolume
+	# Create a new instance of LVM::LogicalVolume.
+	#
+	# New instances can be created in one of two ways:
+	#
+	#  * Pass a single argument, containing any path which LVM can resolve to
+	#    a logical volume.  Typically, this will either be `/dev/<vg>/<lv>`
+	#    or `/dev/mapper/<vg>-<lv>`, but we don't try to parse it ourselves,
+	#    relying instead on `lvs` to do the heavy lifting.
+	#
+	#  * Pass two arguments, which are the volume group name and logical
+	#    volume name, respectively.
+	#
+	# This method will raise `RuntimeError` if the path specified can't be
+	# resolved to an LV, or if the specified VG name or LV name don't resolve
+	# to an active logical volume.
+	#
+	def initialize(path_or_vg_name, lv_name=nil)
+		if lv_name.nil?
+			path = path_or_vg_name
+			@vg_name, @lv_name = `lvs --noheadings -o vg_name,lv_name #{path} 2>/dev/null`.strip.split(/\s+/, 2)
+			if $?.exitstatus != 0
+				raise RuntimeError,
+				      "Failed to interrogate LVM about '#{path}'.  Perhaps you misspelt it?"
+			end
+		else
+			@vg_name = path_or_vg_name
+			@lv_name = lv_name
+		end
+
+		@vgcfg = LVM::VGConfig.new(@vg_name)
+		@lvcfg = @vgcfg.logical_volumes[@lv_name]
+
+		if @lvcfg.nil?
+			raise RuntimeError,
+			      "Logical volume #{@lv_name} does not exist in volume group #{@vg_name}"
+		end
+	end
+
+	# Return a string containing a canonical path to the block device
+	# representing this LV.
+	def path
+		"/dev/mapper/#{@vg_name.gsub('-', '--')}-#{@lv_name.gsub('-', '--')}"
+	end
+
+	# Is this LV a snapshot?
+	def snapshot?
+		@lvcfg.snapshot?
+	end
+
+	# Return an LVM::LogicalVolume object which is the origin volume of
+	# this one (if this LV is a snapshot), or `nil` otherwise.
+	def origin
+		return nil unless snapshot?
+
+		if @lvcfg.origin
+			LVM::LogicalVolume.new(@vg_name, @lvcfg.origin)
+		else
+			origin_lv_name = @vgcfg.logical_volumes.values.find { |lv| lv.cow_store == @lv_name }.origin
+			LVM::LogicalVolume.new(@vg_name, origin_lv_name)
+		end
+	end
+
+	# Return an array of ranges, each of which represents an inclusive range
+	# of bytes which are different between this logical volume and its
+	# origin.
+	#
+	# If this LV is not a snapshot, this method returns an empty array.
+	#
+	def changes
+		return [] unless snapshot?
+
+		if @lvcfg.thin?
+			LVM::ThinSnapshot.new(@vg_name, @lv_name)
+		else
+			LVM::Snapshot.new(@vg_name, @lv_name)
+		end.differences
+	end
+end

lvmsync.gemspec

@@ -19,6 +19,7 @@ Gem::Specification.new do |s|
 		bin/lvmsync
 		lib/lvm.rb
 		lib/lvm/lv_config.rb
+		lib/lvm/logical_volume.rb
 		lib/lvm/thin_snapshot.rb
 		lib/lvm/snapshot.rb
 		lib/lvm/vg_config.rb