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

Vm priority and spot instance settings #894

Merged
merged 13 commits into from
Mar 17, 2022

Conversation

bigjonroberts
Copy link
Contributor

@bigjonroberts bigjonroberts commented Mar 12, 2022

The changes in this PR are as follows:

  • Adds Priority and Spot Instance settings for VMs

I have read the contributing guidelines and have completed the following:

  • Tested my code end-to-end against a live Azure subscription.
  • Updated the documentation in the docs folder for the affected changes.
  • Written unit tests against the modified code that I have made.
  • Updated the release notes with a new entry for this PR.
  • Checked the coding standards outlined in the contributions guide and ensured my code adheres to them.

Below is a minimal example configuration that includes the new features, which can be used to deploy to Azure:

#r "nuget:Farmer"

open Farmer
open Farmer.Builders
open Farmer.Vm

let myVm = vm {
    name "isaacsVM"
    username "isaac"
    spot_instance Deallocate
    vm_size Standard_A2
    operating_system WindowsServer_2012Datacenter
    os_disk 128 StandardSSD_LRS
    add_ssd_disk 128
    add_slow_disk 512
    diagnostics_support
    system_identity
}

let deployment = arm {
    location Location.NorthEurope
    add_resource myVm
}

deployment
|> Deploy.execute "my-resource-group-name" Deploy.NoParameters

@bigjonroberts bigjonroberts changed the title Vm spot instance Vm priority and spot instance settings Mar 12, 2022
@bigjonroberts
Copy link
Contributor Author

@isaacabraham @ninjarobot
Do you have any suggestions for automated tests for this change? Or does what I have here now look sufficient?

else this.Identity.ToArmJson
properties =
{| hardwareProfile = {| vmSize = this.Size.ArmValue |}
let properties =
Copy link
Contributor Author

@bigjonroberts bigjonroberts Mar 16, 2022

Choose a reason for hiding this comment

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

Implemented this way because ARM chokes on any priority setting than Spot if evictionPolicy or billingProfile are included.

@@ -7,7 +7,7 @@ open Farmer.Vm
open System
open System.Text

let virtualMachines = ResourceType ("Microsoft.Compute/virtualMachines", "2018-10-01")
let virtualMachines = ResourceType ("Microsoft.Compute/virtualMachines", "2019-03-01")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version 2019-03-01 is the oldest version to support the spot instance properties.

Copy link
Collaborator

@ninjarobot ninjarobot left a comment

Choose a reason for hiding this comment

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

Thanks for contributing this Jon! I have a couple small suggestions where I commented in the code. As for unit testing, it would be helpful to add a test that the logic around the priority and billing profile holds up even if both priority and spot_instance are set, regardless of order.

src/Farmer/Builders/Builders.Vm.fs Outdated Show resolved Hide resolved
src/Farmer/Common.fs Outdated Show resolved Hide resolved
@ninjarobot ninjarobot modified the milestones: 1.6.31, 1.6.30 Mar 17, 2022
@ninjarobot
Copy link
Collaborator

@bigjonroberts it might simplify to take out priority from the builder for this PR so there is no need to deal with the possibility of priority and spot_instance being added to a vm builder at the same time. That reduces the testing surface and would allow this PR to go out and enable spot_instance provisioning right away. Then a follow up PR could add priority back to the VM builder along with whatever tests and validation are needed. What do you think?

@bigjonroberts
Copy link
Contributor Author

@ninjarobot I think my latest design change addresses this concern in the correct way. It's covered with tests now, too. Take a look at the last commit and see if you agree.

@ninjarobot
Copy link
Collaborator

@ninjarobot I think my latest design change addresses this concern in the correct way. It's covered with tests now, too. Take a look at the last commit and see if you agree.

Yes, cutting it down to a single field simplifies the state to check, and validation messages look great. Thanks for also adding tests to cover that logic.

Copy link
Collaborator

@ninjarobot ninjarobot left a comment

Choose a reason for hiding this comment

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

Thanks again for the contribution!

@ninjarobot ninjarobot merged commit 19a03d0 into CompositionalIT:master Mar 17, 2022
@ninjarobot
Copy link
Collaborator

Thanks for the contribution, this is released in farmer 1.6.30.

@bigjonroberts bigjonroberts deleted the vm-spot-instance branch March 18, 2022 03:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants