Add purger script for removing stale jobs #55

apostergiou · 2018-04-24T12:58:10Z

A best-effort, temporary solution to #25.

The script will be replaced later when we implement a solution built-in to mistryd.

ctrochalakis · 2018-04-24T13:09:04Z

contrib/build_purger.rb

+  # jobs that have a symlink either as a group or as latest won't be deleted
+  groups_and_latest = []
+  group_jobs = Dir["#{groups_path}/*"]
+  groups_and_latest << `readlink #{Dir["latest_path"]}`


You can use File.readlink() and avoid the shell escape.

good catch, fixed

ctrochalakis · 2018-04-24T13:11:18Z

contrib/build_purger.rb

+    groups_and_latest << `readlink #{j}`.strip
+  end
+
+  ready_path = File.join(data_path, "/ready")


nit, you should drop the slash. File.join is used to avoid hardcoding the file seperator (/) for portability (e.g. on windows systems)

ctrochalakis · 2018-04-24T13:14:25Z

We have jobs that don't have a buildinfo file,right? We should have a plan for them.

ctrochalakis · 2018-04-24T13:21:29Z

Generally, the file join logic is not consistent throughout the script, you both use File.join and string concatenation with a /.

agis · 2018-04-24T14:08:26Z

contrib/build_purger.rb

+options = {}
+OptionParser.new do |opts|
+  opts.banner = "Usage: build_purger.rb [options]"
+  opts.on('--days DAYS', 'Stale point') { |v| options[:stale_point] = Time.now - v.to_i*24*60*60 }


Use the more aggressive Integer(v).

Also, can we use something more descriptive than "Stale point"? For example --older-than would've been better imo:

--older-than DAYS - remove builds older than DAYS days

agis · 2018-04-24T14:11:17Z

contrib/build_purger.rb

+  puts "would delete jobs: #{stale_jobs}"
+else
+  stale_jobs.each do |sj|
+    `btrfs subvolume delete #{sj}`


btrfs subvolume delete also accepts multiple arguments, so you can do this in a single command.

Great catch, thank you for pointing this out! The code has been fixed to play with multiple arguments.

agis · 2018-04-24T14:13:44Z

contrib/build_purger.rb

@@ -0,0 +1,49 @@
+#!/usr/bin/env ruby


You can drop the .rb extension since this you added the shebang. Also a suggestion for a more idiomatic name mistry-purge-builds.

dropped the extension and renamed

agis · 2018-04-24T14:14:02Z

contrib/build_purger.rb

+require 'json'
+require 'time'
+require 'optparse'
+require 'yaml'


Is this actually needed?

nope, removed. I added it as part of the experimentation with a static yaml file for the commands but it was overkill.

agis · 2018-04-24T14:16:47Z

contrib/build_purger.rb

+
+options = {}
+OptionParser.new do |opts|
+  opts.banner = "Usage: build_purger.rb [options]"


We could add a small description here so that the user knows what this is about. Something like:

Purge old mistry builds from the file system.

apostergiou · 2018-04-24T14:18:36Z

We have jobs that don't have a buildinfo file,right? We should have a plan for them.

Jobs that don't have the buildinfo file will be considered stale and eventually will be deleted, except those that are the symbolic link target of groups and latest.

ctrochalakis · 2018-04-24T14:26:59Z

Jobs that don't have the buildinfo file will be considered stale and eventually will be deleted, except those that are the symbolic link target of groups and latest.

Not sure about that, I think it's better to delete them (we should make sure that it doesn't affect the server readlink logic). The problem is that this way we will never delete obsolete groups and reclaim their space.

agis · 2018-04-24T14:27:12Z

Please describe in the commit message (and PR) why this change is needed (ie. that it's a best-effort, temporary solution to #25) and that it will be replaced later when we implement a solution built-in to mistryd.

apostergiou · 2018-04-25T06:20:42Z

Not sure about that, I think it's better to delete them (we should make sure that it doesn't affect the server readlink logic). The problem is that this way we will never delete obsolete groups and reclaim their space.

Sounds good. As far as the server readlink logic is concerned, we will be dropping the symbolic links if their target job directory has been removed.

apostergiou · 2018-04-25T06:38:11Z

Comments addressed and updated PR description.

agis · 2018-04-25T06:40:52Z

contrib/mistry-purge-builds

+if options[:dry_run]
+  puts "would delete jobs: #{stale_jobs}"
+else
+  `btrfs subvolume delete #{stale_jobs.join(' ')}`


What will happen if there are no stale jobs?

Unless I'm very much mistaken, the command will print an error. Added a simple condition to mitigate this.

agis · 2018-04-25T07:18:26Z

contrib/mistry-purge-builds

+
+options = {}
+OptionParser.new do |opts|
+  opts.banner = "Purge old mistry builds from the file system.\nUsage: mistry-purge-builds [options]"


You can use $0 for the current file name.

True, thanks 👍

A best-effort, temporary solution to #25. The script will be replaced later when we implement a solution built-in to mistryd.

apostergiou added the enhancement label Apr 24, 2018

apostergiou added this to the 1.0 milestone Apr 24, 2018

apostergiou self-assigned this Apr 24, 2018

apostergiou requested review from agis and dtheodor April 24, 2018 12:58

ctrochalakis reviewed Apr 24, 2018

View reviewed changes

agis suggested changes Apr 24, 2018

View reviewed changes

agis reviewed Apr 24, 2018

View reviewed changes

skroutz deleted a comment from agis Apr 24, 2018

apostergiou force-pushed the build-purger branch from 303f566 to 45eb939 Compare April 25, 2018 06:37

apostergiou force-pushed the build-purger branch from 45eb939 to 8970581 Compare April 25, 2018 07:12

agis suggested changes Apr 25, 2018

View reviewed changes

agis approved these changes Apr 25, 2018

View reviewed changes

Add purger script for removing stale jobs

ca903e5

A best-effort, temporary solution to #25. The script will be replaced later when we implement a solution built-in to mistryd.

apostergiou force-pushed the build-purger branch from d0fb94d to ca903e5 Compare April 25, 2018 09:32

apostergiou merged commit b528fa0 into master Apr 25, 2018

apostergiou deleted the build-purger branch April 25, 2018 09:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add purger script for removing stale jobs #55

Add purger script for removing stale jobs #55

apostergiou commented Apr 24, 2018 •

edited

Loading

ctrochalakis Apr 24, 2018

apostergiou Apr 24, 2018

ctrochalakis Apr 24, 2018

apostergiou Apr 24, 2018

ctrochalakis commented Apr 24, 2018

ctrochalakis commented Apr 24, 2018

agis Apr 24, 2018

apostergiou Apr 25, 2018

agis Apr 24, 2018

apostergiou Apr 25, 2018

agis Apr 24, 2018

apostergiou Apr 25, 2018

agis Apr 24, 2018

apostergiou Apr 25, 2018

agis Apr 24, 2018

apostergiou Apr 25, 2018

apostergiou commented Apr 24, 2018

ctrochalakis commented Apr 24, 2018

agis commented Apr 24, 2018

apostergiou commented Apr 25, 2018

apostergiou commented Apr 25, 2018

agis Apr 25, 2018

apostergiou Apr 25, 2018

agis Apr 25, 2018

apostergiou Apr 25, 2018

Add purger script for removing stale jobs #55

Add purger script for removing stale jobs #55

Conversation

apostergiou commented Apr 24, 2018 • edited Loading

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

ctrochalakis commented Apr 24, 2018

ctrochalakis commented Apr 24, 2018

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

apostergiou commented Apr 24, 2018

ctrochalakis commented Apr 24, 2018

agis commented Apr 24, 2018

apostergiou commented Apr 25, 2018

apostergiou commented Apr 25, 2018

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

apostergiou commented Apr 24, 2018 •

edited

Loading