Skip to content

Commit

Permalink
Add storage flags to vm pools.
Browse files Browse the repository at this point in the history
Previously it was possible that we return a VM with wrong storage flags
from a vm pool, because vm pools were not filtered by storage flags.

This PR adds storage flags to vm pools to fix that.

Note that we only include flags that affect the storage device
guarantees. These flags are:
* `skip_sync`: which ignores FLUSH requests to the block device. This
  increases the performance, but makes the device not crash-recoverable.
* `encrypted`

We didn't include `use_bdev_ubi` because it only affects performance,
and not the storage guarantees.
  • Loading branch information
pykello committed Jan 19, 2024
1 parent 1fe6c14 commit 4c29c74
Show file tree
Hide file tree
Showing 4 changed files with 44 additions and 11 deletions.
2 changes: 2 additions & 0 deletions prog/vm/github_runner.rb
Expand Up @@ -50,6 +50,8 @@ def pick_vm
boot_image: label_data["boot_image"],
location: label_data["location"],
storage_size_gib: label_data["storage_size_gib"],
storage_encrypted: vm_storage_params[:encrypted],
storage_skip_sync: vm_storage_params[:skip_sync],
arch: label_data["arch"]
).first

Expand Down
12 changes: 10 additions & 2 deletions prog/vm/vm_pool.rb
Expand Up @@ -7,14 +7,17 @@ class Prog::Vm::VmPool < Prog::Base

semaphore :destroy

def self.assemble(size:, vm_size:, boot_image:, location:, storage_size_gib:, arch:)
def self.assemble(size:, vm_size:, boot_image:, location:, storage_size_gib:,
storage_encrypted:, storage_skip_sync:, arch:)
DB.transaction do
vm_pool = VmPool.create_with_id(
size: size,
vm_size: vm_size,
boot_image: boot_image,
location: location,
storage_size_gib: storage_size_gib,
storage_encrypted: storage_encrypted,
storage_skip_sync: storage_skip_sync,
arch: arch
)
Strand.create(prog: "Vm::VmPool", label: "create_new_vm") { _1.id = vm_pool.id }
Expand All @@ -30,13 +33,18 @@ def before_run
end

label def create_new_vm
storage_params = {
size_gib: vm_pool.storage_size_gib,
encrypted: vm_pool.storage_encrypted,
skip_sync: vm_pool.storage_skip_sync
}
st = Prog::Vm::Nexus.assemble_with_sshable(
"runner",
Config.vm_pool_project_id,
size: vm_pool.vm_size,
location: vm_pool.location,
boot_image: vm_pool.boot_image,
storage_volumes: [{size_gib: vm_pool.storage_size_gib, encrypted: false}],
storage_volumes: [storage_params],
enable_ip4: true,
pool_id: vm_pool.id,
arch: vm_pool.arch
Expand Down
20 changes: 14 additions & 6 deletions spec/prog/vm/github_runner_spec.rb
Expand Up @@ -109,12 +109,16 @@

it "provisions a new vm if pool is valid but there is no vm" do
git_runner_pool = VmPool.create_with_id(size: 2, vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners", storage_size_gib: 150, arch: "x64")
expect(VmPool).to receive(:where).with(vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners", storage_size_gib: 150, arch: "x64").and_return([git_runner_pool])
expect(VmPool).to receive(:where).with(
vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners",
storage_size_gib: 150, storage_encrypted: false,
storage_skip_sync: false, arch: "x64"
).and_return([git_runner_pool])
expect(git_runner_pool).to receive(:pick_vm).and_return(nil)
expect(Prog::Vm::Nexus).to receive(:assemble).and_call_original
expect(Clog).to receive(:emit).with("Pool is empty").and_call_original
expect(FirewallRule).to receive(:create_with_id).and_call_original.at_least(:once)
expect(nx).to receive(:storage_params).and_return({encrypted: false, use_bdev_ubi: false})
expect(nx).to receive(:storage_params).and_return({encrypted: false, use_bdev_ubi: false, skip_sync: false, size_gib: 150})
vm = nx.pick_vm
expect(vm).not_to be_nil
expect(vm.sshable.unix_user).to eq("runner")
Expand All @@ -124,9 +128,13 @@

it "uses the existing vm if pool can pick one" do
git_runner_pool = VmPool.create_with_id(size: 2, vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners", storage_size_gib: 150, arch: "arm64")
expect(VmPool).to receive(:where).with(vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners", storage_size_gib: 150, arch: "arm64").and_return([git_runner_pool])
expect(VmPool).to receive(:where).with(
vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners",
storage_size_gib: 150, storage_encrypted: false,
storage_skip_sync: false, arch: "arm64"
).and_return([git_runner_pool])
expect(git_runner_pool).to receive(:pick_vm).and_return(vm)
expect(nx).to receive(:storage_params).and_return({encrypted: false, use_bdev_ubi: false})
expect(nx).to receive(:storage_params).and_return({encrypted: false, use_bdev_ubi: false, skip_sync: false})
expect(Clog).to receive(:emit).with("Pool is used").and_call_original
expect(github_runner).to receive(:label).and_return("ubicloud-standard-4-arm").at_least(:once)
vm = nx.pick_vm
Expand All @@ -136,12 +144,12 @@

it "doesn't use the pool if use_bdev_ubi is true" do
git_runner_pool = VmPool.create_with_id(size: 2, vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners", storage_size_gib: 150, arch: "x64")
expect(VmPool).to receive(:where).with(vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners", storage_size_gib: 150, arch: "x64").and_return([git_runner_pool])
expect(VmPool).to receive(:where).with(vm_size: "standard-4", boot_image: "github-ubuntu-2204", location: "github-runners", storage_size_gib: 150, storage_encrypted: false, storage_skip_sync: false, arch: "x64").and_return([git_runner_pool])
expect(git_runner_pool).not_to receive(:pick_vm)
expect(Prog::Vm::Nexus).to receive(:assemble).and_call_original
expect(Clog).to receive(:emit).with("Pool is empty").and_call_original
expect(FirewallRule).to receive(:create_with_id).and_call_original.at_least(:once)
expect(nx).to receive(:storage_params).and_return({encrypted: false, use_bdev_ubi: true})
expect(nx).to receive(:storage_params).and_return({encrypted: false, use_bdev_ubi: true, skip_sync: false})
vm = nx.pick_vm
expect(vm).not_to be_nil
expect(vm.sshable.unix_user).to eq("runner")
Expand Down
21 changes: 18 additions & 3 deletions spec/prog/vm/vm_pool_spec.rb
Expand Up @@ -9,13 +9,20 @@

describe ".assemble" do
it "creates the entity and strand properly" do
st = described_class.assemble(size: 3, vm_size: "standard-2", boot_image: "img", location: "hetzner-fsn1", storage_size_gib: 86, arch: "x64")
st = described_class.assemble(
size: 3, vm_size: "standard-2", boot_image: "img", location: "hetzner-fsn1",
storage_size_gib: 86, storage_encrypted: true,
storage_skip_sync: false, arch: "x64"
)
pool = VmPool[st.id]
expect(pool).not_to be_nil
expect(pool.size).to eq(3)
expect(pool.vm_size).to eq("standard-2")
expect(pool.boot_image).to eq("img")
expect(pool.location).to eq("hetzner-fsn1")
expect(pool.storage_size_gib).to eq(86)
expect(pool.storage_encrypted).to be(true)
expect(pool.storage_skip_sync).to be(false)
expect(st.label).to eq("create_new_vm")
end
end
Expand All @@ -27,7 +34,11 @@

it "creates a new vm and hops to wait" do
expect(Config).to receive(:vm_pool_project_id).and_return(prj.id)
st = described_class.assemble(size: 3, vm_size: "standard-2", boot_image: "img", location: "hetzner-fsn1", storage_size_gib: 86, arch: "arm64")
st = described_class.assemble(
size: 3, vm_size: "standard-2", boot_image: "img", location: "hetzner-fsn1",
storage_size_gib: 86, storage_encrypted: true,
storage_skip_sync: false, arch: "arm64"
)
st.update(label: "create_new_vm")
expect(SshKey).to receive(:generate).and_call_original
expect(nx).to receive(:vm_pool).and_return(VmPool[st.id]).at_least(:once)
Expand All @@ -40,7 +51,11 @@

describe "#wait" do
let(:pool) {
VmPool.create_with_id(size: 0, vm_size: "standard-2", boot_image: "img", location: "hetzner-fsn1", storage_size_gib: 86, arch: "x64")
VmPool.create_with_id(
size: 0, vm_size: "standard-2", boot_image: "img", location: "hetzner-fsn1",
storage_size_gib: 86, storage_encrypted: true,
storage_skip_sync: false, arch: "x64"
)
}

it "waits if nothing to do" do
Expand Down

0 comments on commit 4c29c74

Please sign in to comment.