Delete stale servers after N hours #27
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #27 +/- ##
==========================================
- Coverage 92.30% 91.20% -1.10%
==========================================
Files 3 3
Lines 78 91 +13
==========================================
+ Hits 72 83 +11
- Misses 6 8 +2
☔ View full report in Codecov by Sentry. |
|
There's no way to automatically delete VMs that are older than X, so we've to implement this in beaker-hcloud. I did some manual poking in the IRB and that worked fine. I didn't test this code yet. |
| # "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 |
There was a problem hiding this comment.
You can also use native Ruby to avoid the ActiveSupport dependency
| (DateTime.current + @delete_vm_after.hours).to_i.to_s | |
| (DateTime.now + (@delete_vm_after /24)).to_i.to_s |
There was a problem hiding this comment.
ah right, that works as well. We pull in activesupport anyways because it's a dependency for hcloud. So should we use it here?
| 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'] |
There was a problem hiding this comment.
How about a more traditional condition:
| 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 |
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
| @@ -45,7 +45,8 @@ | |||
| 'dns_ptr' => 'server1.example.com', | |||
| }, | |||
| }, | |||
| destroy: true) | |||
| destroy: true, | |||
| labels: { vm_delete_after: '1695385549' }) | |||
| @@ -56,7 +57,8 @@ | |||
| 'dns_ptr' => 'server2.example.com', | |||
| }, | |||
| }, | |||
| destroy: true) | |||
| destroy: true, | |||
| labels: { vm_delete_after: '1695385549' }) | |||
| 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'`. |
There was a problem hiding this comment.
Timestamp formats here (human-readable with TZ) and in the unit tests below (unix time w/o TZ) are different. Is it expected?
|
I raised a new PR that just adds the label, without any VM deletion: #29 |
No description provided.