fixes #21198 - unmounts/removes snapshots if failure #549

cfouant · 2017-10-04T19:00:07Z

No description provided.

theforeman-bot · 2017-10-04T19:00:32Z

Issues: #21198

cfouant · 2017-10-04T19:30:45Z

@evgeni - could you review this please? You're the best versed at snapshots 💯

evgeni

the change looks fine from a logic perspective, but I think we can refactor the code to be cleaner ;)

evgeni · 2017-10-05T06:40:33Z

katello/helper.rb

@@ -13,7 +13,7 @@ def error_message
    end

    def foreman_rpm_installed?
-      system("rpm -q foreman")
+      system("rpm -q foreman >/dev/null")


Is this change relevant here?

It prints the output needlessly, and since we aren't doing anything except distinguishing satellite from capsule, I don't think the output created should be displayed.

fair enough.

this has been fixed via http://projects.theforeman.org/issues/21219 now

evgeni · 2017-10-05T06:41:44Z

katello/backup.rb

@@ -32,6 +32,12 @@ def cleanup(exitstatus=-1)
      puts "Cleaning up backup folder and starting any stopped services... "
      FileUtils.cd("/")
      FileUtils.rm_rf @dir unless @options[:no_subdir]
+      if @options[:snapshot]
+        @databases.each do |database|
+          system("umount #{File.join(@mountdir, database)} >/dev/null")


This is more or less duplicating code from https://github.com/cfouant/katello-packaging/blob/86547ae08f4eaebbddc6a5623ec847c68c42be41/katello/backup.rb#L195-L200, shall we move these lines into an own function and call it here and in L195?

I was considering the same, however, I wanted to suppress the output here, as the output is terrible if there are no mounted filesystems. I will add that into the function.

On second thought, I think these do need to be kept separate. We want the process in backup/destroy to go through the run_cmd method; however, as we've previously found in calling run_cmd in cleanup, we can end up caught in an endless loop.

haha. right. I wonder if we should teach run_cmd something like "ignore_all_errors" next to the list of accepted exit codes. anyways, that is a discussion for another PR/redmine.

let me see how this code actually runs, and ack that PR afterwards.

would you mind adding a comment "not using run_cmd here because …" so that we don't forget that in the future?

Absolutely :)

evgeni · 2017-10-05T15:08:14Z

katello/backup.rb

+      if @options[:snapshot]
+        @databases.each do |database|
+          system("umount #{File.join(@mountdir, database)} >/dev/null")
+          system("lvremove -f #{get_snapshot_location(database)} >/dev/null")


You should first get get_snapshot_location(database) and only execute lvremove if the get actually returned anything. Otherwise (when the snapshot in question does not exist), you end up with ugly errors:

No command with matching syntax recognised. Run 'lvremove --help' for more information. Correct command syntax is:

evgeni · 2017-10-05T15:14:03Z

katello/backup.rb

    def cleanup(exitstatus=-1)
      puts "Cleaning up backup folder and starting any stopped services... "
      FileUtils.cd("/")
      FileUtils.rm_rf @dir unless @options[:no_subdir]
+      if @options[:snapshot]
+        @databases.each do |database|
+          system("umount #{File.join(@mountdir, database)} >/dev/null")


This will still output an error (but continue), if the mount point is not mounted:

umount: /var/snap/mongodb: mountpoint not found

cfouant · 2017-10-06T22:54:57Z

@evgeni - thanks for the suggestions, thanks for testing!
@johnpmitsch - i changed the command in helper.rb because system() automatically returns true/false and is less expensive than backtick calls. (just learned this, and thinking we might want to test using system() throughout, although doubt it would work everywhere). Let me know what you think! I can take it out if you prefer.

johnpmitsch · 2017-10-09T13:25:00Z

katello/helper.rb

-      # using backticks here so there is no output
-      `rpm -q foreman`
-      $?.success?
+      system("rpm -q foreman >/dev/null")


Either way works fine for me as long as there is no output. Do you mind adding a space after then > pipe?

If we are going to update to anything across the scripts, using http://ruby-doc.org/stdlib-1.9.3/libdoc/open3/rdoc/Open3.html#method-c-popen3 would be ideal. It starts a new external process that can be monitored. I'm not saying this should be done in this PR, but would be worth looking into more.

We definitely do not want to update across the scripts at this time. system() returns a true/false/nil value, and run_cmd returns output. Updating with the space now 👍

johnpmitsch · 2017-10-09T18:00:02Z

katello/backup.rb

@@ -28,10 +28,20 @@ def initialize(foreman_proxy_content, foreman_proxy, program, accepted_scenarios
      setup_opt_parser
    end

+    # do not use run_cmd() inside this method, to avoid a recursive loop


Do you mind being a bit more verbose and say you aren't using run_cmd inside cleanup method because run_cmd calls cleanup and will cause an infinite loop? (I only ask because I was confused to where the loop would come from at first)

cfouant · 2017-10-11T17:48:51Z

ping @evgeni @johnpmitsch - could i haz re-review when you get the opportunity? Thanks!

johnpmitsch · 2017-10-11T17:55:44Z

LGTM 👍 (I'll let evgeni approve to merge)

evgeni · 2017-10-12T12:22:10Z

katello/backup.rb

+      if @options[:snapshot]
+        @databases.each do |database|
+          mount_location = File.join(@mountdir, database)
+          system("umount #{mount_location}") if system("mount | grep #{mount_location}")


instead of doing mount | grep, how about that function:

def is_mounted(path) mounts = File.readlines('/proc/mounts') mounts.each do |mount| mount = mount.split return true if mount[1] == path end return false end

This seems much more costly and clunkier? I think it's also in the style guidelines to do avoid 'return true/false'.

How about
if File.readlines('/proc/mounts').grep(/#{mount_location}).any?

evgeni

ACK 🐑

jlsherrill · 2017-10-12T17:38:09Z

merged! thanks @cfouant !

theforeman-bot added Needs testing Not yet reviewed labels Oct 4, 2017

evgeni suggested changes Oct 5, 2017

View reviewed changes

cfouant force-pushed the snap-cleanup branch from 86547ae to bc115da Compare October 5, 2017 13:57

evgeni suggested changes Oct 5, 2017

View reviewed changes

evgeni reviewed Oct 5, 2017

View reviewed changes

cfouant force-pushed the snap-cleanup branch from bc115da to c92619f Compare October 6, 2017 22:51

johnpmitsch reviewed Oct 9, 2017

View reviewed changes

cfouant force-pushed the snap-cleanup branch from c92619f to 1457def Compare October 9, 2017 15:05

johnpmitsch reviewed Oct 9, 2017

View reviewed changes

cfouant force-pushed the snap-cleanup branch from 1457def to 2592aef Compare October 9, 2017 18:25

evgeni suggested changes Oct 12, 2017

View reviewed changes

fixes #21198 - unmounts/removes snapshots if failure

a0ed9c9

cfouant force-pushed the snap-cleanup branch from 2592aef to a0ed9c9 Compare October 12, 2017 15:17

evgeni approved these changes Oct 12, 2017

View reviewed changes

jlsherrill merged commit 123c432 into Katello:master Oct 12, 2017

cfouant deleted the snap-cleanup branch October 16, 2017 18:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fixes #21198 - unmounts/removes snapshots if failure #549

fixes #21198 - unmounts/removes snapshots if failure #549

cfouant commented Oct 4, 2017

theforeman-bot commented Oct 4, 2017

cfouant commented Oct 4, 2017

evgeni left a comment

evgeni Oct 5, 2017

cfouant Oct 5, 2017

evgeni Oct 5, 2017

evgeni Oct 6, 2017

evgeni Oct 5, 2017

cfouant Oct 5, 2017

cfouant Oct 5, 2017

evgeni Oct 5, 2017

cfouant Oct 5, 2017

evgeni Oct 5, 2017

evgeni Oct 5, 2017

cfouant commented Oct 6, 2017

johnpmitsch Oct 9, 2017

johnpmitsch Oct 9, 2017

cfouant Oct 9, 2017

johnpmitsch Oct 9, 2017

cfouant Oct 9, 2017

cfouant commented Oct 11, 2017

johnpmitsch commented Oct 11, 2017 •

edited

Loading

evgeni Oct 12, 2017 •

edited

Loading

cfouant Oct 12, 2017

cfouant Oct 12, 2017

evgeni left a comment

jlsherrill commented Oct 12, 2017

fixes #21198 - unmounts/removes snapshots if failure #549

fixes #21198 - unmounts/removes snapshots if failure #549

Conversation

cfouant commented Oct 4, 2017

theforeman-bot commented Oct 4, 2017

cfouant commented Oct 4, 2017

evgeni left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfouant commented Oct 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfouant commented Oct 11, 2017

johnpmitsch commented Oct 11, 2017 • edited Loading

evgeni Oct 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evgeni left a comment

Choose a reason for hiding this comment

jlsherrill commented Oct 12, 2017

johnpmitsch commented Oct 11, 2017 •

edited

Loading

evgeni Oct 12, 2017 •

edited

Loading