From 4c29c74cc5c367a3dd0fab81de71c6e69d6a92f7 Mon Sep 17 00:00:00 2001 From: Hadi Moshayedi Date: Wed, 10 Jan 2024 22:23:32 -0800 Subject: [PATCH] Add storage flags to vm pools. 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. --- prog/vm/github_runner.rb | 2 ++ prog/vm/vm_pool.rb | 12 ++++++++++-- spec/prog/vm/github_runner_spec.rb | 20 ++++++++++++++------ spec/prog/vm/vm_pool_spec.rb | 21 ++++++++++++++++++--- 4 files changed, 44 insertions(+), 11 deletions(-) diff --git a/prog/vm/github_runner.rb b/prog/vm/github_runner.rb index 9810bc1b0..a065eb084 100644 --- a/prog/vm/github_runner.rb +++ b/prog/vm/github_runner.rb @@ -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 diff --git a/prog/vm/vm_pool.rb b/prog/vm/vm_pool.rb index b61ebd41f..cad6f7420 100644 --- a/prog/vm/vm_pool.rb +++ b/prog/vm/vm_pool.rb @@ -7,7 +7,8 @@ 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, @@ -15,6 +16,8 @@ def self.assemble(size:, vm_size:, boot_image:, location:, storage_size_gib:, ar 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 } @@ -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 diff --git a/spec/prog/vm/github_runner_spec.rb b/spec/prog/vm/github_runner_spec.rb index 6d22f38e4..77e7ec0aa 100644 --- a/spec/prog/vm/github_runner_spec.rb +++ b/spec/prog/vm/github_runner_spec.rb @@ -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") @@ -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 @@ -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") diff --git a/spec/prog/vm/vm_pool_spec.rb b/spec/prog/vm/vm_pool_spec.rb index e46afbea9..3f952d03d 100644 --- a/spec/prog/vm/vm_pool_spec.rb +++ b/spec/prog/vm/vm_pool_spec.rb @@ -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 @@ -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) @@ -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