-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add scripts to generate new Virtual Machine Scale Sets and images for build machines #633
Conversation
azure-devops/provision-image.ps1
Outdated
@@ -201,8 +179,4 @@ $environmentKey = Get-ItemProperty -Path 'HKLM:\SYSTEM\CurrentControlSet\Control | |||
Set-ItemProperty -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager\Environment' ` | |||
-Name Path ` | |||
-Value "$($environmentKey.Path);C:\Program Files\CMake\bin;C:\Program Files\LLVM\bin" | |||
InstallZip 'Azure DevOps Agent' $VstsAgentUrl 'C:\agent' | |||
Add-MpPreference -ExclusionPath C:\agent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this still happen at some point? or do we just never enable defender on the generated images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't turn Defender off any more, that's something we'll need to talk to Azure Pipelines team about since we no longer know where the work tree is.
And actually this was broken before since it should have excluded D:\
after we moved the build there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I found an issue that will prevent this from actually working (the branch issue). Other comments aren't critical.
$temp = $env:TEMP | ||
|
||
curl.exe -L -o "$temp\psexec.exe" https://live.sysinternals.com/PsExec64.exe | ||
curl.exe -L -o "$temp\provision-image.ps1" https://raw.githubusercontent.com/microsoft/STL/autoscale/azure-devops/provision-image.ps1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line appears to expect microsoft/STL
to have a branch named autoscale
. I believe it should be master
.
$temp = $env:TEMP | ||
|
||
curl.exe -L -o "$temp\psexec.exe" https://live.sysinternals.com/PsExec64.exe | ||
curl.exe -L -o "$temp\provision-image.ps1" https://raw.githubusercontent.com/microsoft/STL/autoscale/azure-devops/provision-image.ps1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does the machine have this file, yet needs to download provision-image.ps1
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The cmdlet Invoke-AzVMRunCommand
on 204 of create-new-agent-image.ps
takes care of getting this script onto the box. I suppose I could eliminate the need for this with a script that invokes itself....
|
||
curl.exe -L -o "$temp\psexec.exe" https://live.sysinternals.com/PsExec64.exe | ||
curl.exe -L -o "$temp\provision-image.ps1" https://raw.githubusercontent.com/microsoft/STL/autoscale/azure-devops/provision-image.ps1 | ||
& "$temp\psexec.exe" -u AdminUser -p $AdminUserPassword -accepteula -h C:\Windows\System32\WindowsPowerShell\v1.0\powershell.exe -ExecutionPolicy Unrestricted -File "$temp\provision-image.ps1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to wrap to 120?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, we'll see
# | ||
# This script assumes you have installed Azure tools into Powershell by following the instructions | ||
# at https://docs.microsoft.com/en-us/powershell/azure/install-az-ps?view=azps-3.6.1 | ||
# or are running from Azure Cloud Shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Period? Sounds like a complete sentence to me.
|
||
$result = '' | ||
for ($idx = 0; $idx -lt $length; $idx++) { | ||
$result += $Chars[(Get-Random -Minimum 0 -Maximum ($Chars.Length - 1))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this can ever generate '9'
. According to https://docs.microsoft.com/en-us/powershell/module/microsoft.powershell.utility/get-random?view=powershell-7 , "Get-Random
returns a value that is less than the maximum (not equal)." Can you verify?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PS D:\STL> for ($idx = 0; $idx -lt 10000; $idx++) { if ((Get-Random -Minimum 0 -Maximum 1) -eq 1) {
>> Write-Output Pass
>> return
>> }}
PS D:\STL>
Talk about terrible parameter naming.
Write-Output "Location: $Location" | ||
Write-Output "Resource group name: $ResourceGroupName" | ||
Write-Output "User name: AdminUser" | ||
Write-Output "Using Generated Password: $AdminPW" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why Is This Title Case?
|
||
$allowHttp = New-AzNetworkSecurityRuleConfig ` | ||
-Name AllowHTTP ` | ||
-Description 'Allow HTTP(s)' ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalize S
?
#################################################################################################### | ||
Write-Progress -Activity $ProgressActivity -Status -Completed | ||
Write-Reminders $AdminPW | ||
Write-Output 'Finished! Terminate.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Terminate sounds a little ominous, do we need that? END OF LINE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I always like little helper tools like this to emit that message but no skin off my nose to remove it. Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
“Finished” is fine 😸
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with what I see here and the output in ADO, but take my approval with a grain of salt, I don't have psychic debugging powers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one comment, no need to resolve if you don't feel like it.
$VmssIpConfigName = $ResourceGroupName + 'VmssIpConfig' | ||
$VmssIpConfig = New-AzVmssIpConfig -SubnetId $Nic.IpConfigurations[0].Subnet.Id -Primary -Name $VmssIpConfigName | ||
$VmssName = $ResourceGroupName + 'Vmss' | ||
$Vmss = New-AzVmssConfig ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not matter that much but I think for our purposes setting -ScaleInPolicy
to OldestVM
could make sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no errors here so glaringly and obviously incorrect that I noticed them.
After this merges I will update the instructions at https://github.com/microsoft/STL/wiki/Checklist-for-Updating-Toolset-and-or-Dependencies to:
|
|
} | ||
|
||
|
||
Write-Output "AdminUser password not supplied; assuming already running as AdminUser" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can/should this verify something like %USERNAME%
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it must not be that because here we're running as NT AUTHORITY\SYSTEM
(that's the whole reason we need to do this dance in the first place)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, this is in the "not elevating" branch; I think saying AdminUser is good still because it helps explain the explosion that will happen if you try to run the script unelevated :)
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
Do you have other suggestions? It is admittedly jargon I took from TFS world but I've used it in Git and svn worlds before and everyone always has understood what that meant. (I believe I applied your other suggestions) |
I wouldn't use any prefix at all; I might consider linking to the files in |
This change reduces the likelihood of differing behavior between builds due to updates of Visual Studio, since all workers will be updated atomically. It also enables automatic scaling of the number of builders to meet pull request demand, since starting a VM takes a few minutes rather than 40+ minutes.