Skip to content
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

Delete stale servers after N hours #27

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,18 @@ are being created:
# Cleanup

In cases where the beaker process is killed before finishing, it may leave resources in Hetzner cloud. These will need to be manually deleted.

Look for servers in your project named exactly as the ones in your beaker host configuration and SSH keys with names beginning with `Beaker-`.

The gem will try to automatically delete old virtual machines. Every created
cloud instance gets a label `delete_vm_after: 'Fri, 22 Sep 2023 15:39:17 +0200'`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Timestamp formats here (human-readable with TZ) and in the unit tests below (unix time w/o TZ) are different. Is it expected?

By default this is the DateTime during VM creation + an hour. During each beaker
run, beaker-hcloud will check for existing VMs with a `delete_vm_after` label in
the past and delete them.

To work properly, beaker needs to run on a regular basis. You can modify the
default of an hour by setting the `BEAKER_HCLOUD_DELETE_VM_AFTER` environment
variable to any positive integer. It will be interpreted as hours.

# Contributing

Please refer to voxpupuli/beaker's [contributing](https://github.com/voxpupuli/beaker/blob/master/CONTRIBUTING.md) guide.
1 change: 1 addition & 0 deletions beaker-hcloud.gemspec
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ Gem::Specification.new do |s|
s.add_development_dependency 'rspec', '~> 3.12'
s.add_development_dependency 'voxpupuli-rubocop', '~> 2.0.0'

s.add_runtime_dependency 'activesupport', '~> 7.0', '>= 7.0.8'
s.add_runtime_dependency 'bcrypt_pbkdf', '~> 1.0'
s.add_runtime_dependency 'beaker', '~> 5.4'
s.add_runtime_dependency 'ed25519', '~> 1.2'
Expand Down
28 changes: 28 additions & 0 deletions lib/beaker/hypervisor/hcloud.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,11 @@
require 'ed25519'
require 'bcrypt_pbkdf'

# required to calculate the VM deletion point
require 'active_support'
require 'active_support/time'
require 'active_support/core_ext/date'

require_relative '../../beaker-hcloud/ssh_data_patches'

module Beaker
Expand All @@ -15,10 +20,13 @@ def initialize(hosts, options) # rubocop:disable Lint/MissingSuper
@options = options
@logger = options[:logger] || Beaker::Logger.new
@hosts = hosts
@delete_vm_after = ENV.fetch('BEAKER_HCLOUD_DELETE_VM_AFTER', 1).to_i

raise 'BEAKER_HCLOUD_DELETE_VM_AFTER needs to be a positive integer' unless @delete_vm_after.positive?
raise 'You need to pass a token as BEAKER_HCLOUD_TOKEN environment variable' unless ENV['BEAKER_HCLOUD_TOKEN']

@client = ::Hcloud::Client.new(token: ENV.fetch('BEAKER_HCLOUD_TOKEN'))
delete_servers_if_required
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing this in the constructor is dangerous. It means you can't really write unit tests.

Why don't you add this to the cleanup? There is a risk that if things crash you never get to the cleanup, but you can also consider writing a specific clean up only task that runs X times a day in some common repository.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I considered that. maybe we just run a github action as cron somewhere that does the cleanup. That also fixes the problems of cuncurrent cleanup jobs

end

def provision
Expand Down Expand Up @@ -69,6 +77,25 @@ def create_ssh_key
hcloud_ssh_key
end

# we need to save the date as unix timestamp. Hetzner Cloud labels only support:
# "alphanumeric character ([a-z0-9A-Z]) with dashes (-), underscores (_), dots (.), and alphanumerics between."
# https://docs.hetzner.cloud/#labels
def vm_deletion_date
(DateTime.current + @delete_vm_after.hours).to_i.to_s
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can also use native Ruby to avoid the ActiveSupport dependency

Suggested change
(DateTime.current + @delete_vm_after.hours).to_i.to_s
(DateTime.now + (@delete_vm_after /24)).to_i.to_s

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right, that works as well. We pull in activesupport anyways because it's a dependency for hcloud. So should we use it here?

end

# check if a server is too old (delete_vm_after is in the past) and delete it
# delete it also if it has no delete_vm_after label. All servers are supposed to have that
def delete_server_if_required(server)
server.destroy if server.labels['delete_vm_after'] && Time.now > Time.at(server.labels['delete_vm_after'].to_i)
server.destroy unless server.labels['delete_vm_after']
Comment on lines +90 to +91
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about a more traditional condition:

Suggested change
server.destroy if server.labels['delete_vm_after'] && Time.now > Time.at(server.labels['delete_vm_after'].to_i)
server.destroy unless server.labels['delete_vm_after']
if !server.labels['delete_vm_after'] || Time.now > Time.at(server.labels['delete_vm_after'].to_i)
server.destroy
end

end

# iterate through all servers and delete them if required
def delete_servers_if_required
@client.servers.each { |server| delete_server_if_required(server) }
end

def create_server(host)
@logger.notify "provisioning #{host.name}"
location = host[:location] || 'nbg1'
Expand All @@ -79,6 +106,7 @@ def create_server(host)
server_type: server_type,
image: host[:image],
ssh_keys: [ssh_key_name],
labels: { delete_vm_after: vm_deletion_date },
)
while action.status == 'running'
sleep 5
Expand Down
6 changes: 4 additions & 2 deletions spec/beaker/hypervisor/hcloud_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,8 @@
'dns_ptr' => 'server1.example.com',
},
},
destroy: true)
destroy: true,
labels: { vm_delete_after: '1695385549' })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be delete_vm_after?

end
let(:server2) do
double(:server2,
Expand All @@ -56,7 +57,8 @@
'dns_ptr' => 'server2.example.com',
},
},
destroy: true)
destroy: true,
labels: { vm_delete_after: '1695385549' })
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should it be delete_vm_after?

end
let(:action_double) do
double(:action, status: 'success')
Expand Down
Loading