Automattic / Automattic/hostmgr

Improve/fix implementation of `hostmgr vm clean` and `vm-usage` / `VMUsageTracker`

Open
#107 0 comments 0 reactions 0 assignees View on GitHub
bug enhancement hostmgr
Dominant language
Swift
Stars
12
Forks
3
Avg merge
4h 50m
Merged PRs (30d)
4

Description

# Context

The command `hostmgr vm clean` looks at the `vm-usage` file—that reports all starts of a new VM (logged as VM name + date)—to find all the VMs in that list that have not been launched in >90 days.

This command is run as part of the `pre-exit` hook of our buildkite-agents running on each macOS host, so that the cleanup of old local VMs is done regularly, thus freeing up space

The current implementation has a couple of issues though

# The small issues around `vm-usage`

- `vm clean` logs the same VMs over and over even if they have already been cleaned
- `vm clean` logic doesn't detect VMs that have _never_ been used yet (though admittedly if that's the case it's unlikely that the vm would have been downloaded in the first place…)
- cutoff date is not customizable
- We might want to log more data in `vm-usage`
- We might want to truncate old entries from `vm-usage`

All of those [could be solved at once using a single PR with little refactoring around `VMUsageTracker`+`VMClean`](#proposed-solution)

## `vm clean` logs already-cleaned VMs every time

Because the current implementation of `vm clean` looks at the `vm-usage` file only, and that file is not cleaned up / truncated, this means that even once old VMs have been deleted by a call to `vm clean`, the subsequent calls will still find those old VMs in the list from the `vm-usage` file, and log again that this VM will be deleted.

In practice, [the call to `try await vmManager.removeVM(name: image.vmName)`](https://github.com/Automattic/hostmgr/blob/trunk/Sources/hostmgr/commands/vm/VMClean.swift#L28) does nothing if the VM we're trying to remove doesn't exist (i.e. was already removed before), but [the log about that VM being removed is still printed](https://github.com/Automattic/hostmgr/blob/trunk/Sources/hostmgr/commands/vm/VMClean.swift#L28) in all cases.
That makes it look like every CI build tries to remove the same VMs over and over, as if it didn't succeed the previous time. While in practice it did, but it's just still finding the same VMs in the `vm-usage` file even if they have already been removed.

## `vm clean` doesn't detect VMs that have never been used

The way `vm clean` is implemented today is that it looks at the `vm-usage` file **first**, and use that as the source to decide what VMs to delete, instead of starting from the list of VMs present locally and removing the ones not listed in `vm-usage` in the last 90 days. This means that a VM present locally but that has never been launched, and thus is not listed in `vm-usage`, will never be cleaned by `vm clean`.

This is an edge case that will likely never happen in production, because if a VM exists on one of our macOS host machines, it's likely only because it has been requested (and thus launched) by at least one build, otherwise it would never have been fetched (i.e. downloaded from S3—or our NAS mirroring our S3 bucket—into the machine) in the first place. But it still makes the logic inconsistent with the description of what the command is supposed to do in the first place.

## The cutoff date is not customizable

Not that big a deal, but it feels odd for the "90 days cutoff date" of `vm clean` to be hardcoded. It'd probably make sense to make it configurable via `hostmgr.json`, even if 90 days remains the default value if not specified.

## `vm-usage` could benefit from recording the CI pipeline+build

Currently `vm-usage` only records the VM name and launch date. This makes sense for its current purposes.
That being said, it could be interesting to also log the value of the `BUILDKITE_SLUG` and `BUILDKITE_BUILD_NUMBER` env vars if they are available, so that it'd be easy to check from that log file which Buildkite projects/repo are still requesting which VM.

For example, if we notice that an old `xcode-15.1` VM is still present on a host and never cleaned, while we thought we already migrated all the repos to newer VMs… it could be cumbersome to look at all our known repos one by one to find which one might still use that old `IMAGE_ID`. But if we added the `BUILDKITE_SLUG` to the `vm-usage` log file, we'd be able to `grep xcode-15.1 /opt/ci/hostmgr/state/vm-usage | cut -f1,3 | sort | uniq` and immediately see which repos were still requesting that old VM.
And adding the `BUILDKITE_BUILD_NUMBER` could allow us to easily find the build that requested that VM in Buildkite (to see which commit/PR it was associated with, or see if it was a scheduled build rather than a build triggered via API or PR, investigate its logs in more details…)

## `vm-usage` file is never cleaned or truncated

This is debatable if that's a problem or not.
- Over time, the more VMs are launched on an agent, the bigger the `vm-usage` file will get. So far this hasn't become a problem though, as that file is still a reasonable size despite `hostmgr` having started a lot of VM on those machines (example: on 001 that file has 8K lines, totaling 288KB)
- On the other end, it could be nice to keep the whole history, especially if we implement the previous point about adding more info in that log file. Though adding those info means the file will grow faster as well.

---

# Proposed solution

- `VMUsageTracker`
- [Make `VMUsageTracker` record](https://github.com/Automattic/hostmgr/blob/trunk/Sources/libhostmgr/Model/VMUsageTracker.swift#L14) the values of `BUILDKITE_SLUG` and `BUILDKITE_BUILD_NUMBER` if they are defined.
- [During parsing](https://github.com/Automattic/hostmgr/blob/trunk/Sources/libhostmgr/Model/VMUsageTracker.swift#L48-L52), ensure that it wouldn't be an error for the line not to have those 3rd and 4th column present though (it might be ok to ignore those 3rd and 4th column while parsing if the only intend for those would be for a manual `grep … | cut -f1,3 | sort | uniq`; though it'd be nice to consider improving `hostmgr vm stats` command in the future so it can print more detailed information like `vm stats --buildkite-slug --vm=xcode-15.1` could print all the buildkite-slug values for a given VM, etc).
- Add a method to remove old entries in the log file that are older than N days, to prevent the log file from growing too much. Allow the threshold (N days) to be [configurable](https://github.com/Automattic/hostmgr/blob/trunk/Sources/libhostmgr/Model/Configuration.swift), though only allow values of N that are greater the cutoff duration for `vm clean` so that we are guaranteed to keep enough history for `vm clean` to work as expected

- `VMClean` command
- Reimplement the method so that it calls `VMManager.list()` to get the list of local VMs first, and then removes the ones from that list that are also in `vmManager.getVMImages(unusedSince: cutoff)`. While at it, it would probably be worth moving that `getVMImages(unusedSince:)` method from `VMManager` to `VMUsageTracker` to make it clearer that it operates on the `vm-usage` tracker file and not the list of local VMs that `VMManager` is all about manipulating.
- Allow the cutoff date to be [configured via the Configuration file](https://github.com/Automattic/hostmgr/blob/trunk/Sources/libhostmgr/Model/Configuration.swift)

Contributor guide

No contributing guide indexed for this repository

Research direction

Start with Sources/libhostmgr/Model/VMUsageTracker.swift, Sources/hostmgr/commands/vm/VMClean.swift, and Configuration.swift, then trace VMManager.list() and the existing unusedSince logic. Done means local unused VMs are cleaned consistently, the cutoff and retention are configurable, and optional Buildkite fields are recorded and tolerated during parsing.

Written by the indexing model from the issue text.

Assessment

Tech stack
swift
Domain
cli, infrastructure
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Mostly clear
Newbie friendliness
28/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.