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

infra: add a Windows node on Azure #16705

Merged
merged 12 commits into from
Apr 20, 2023
Merged

infra: add a Windows node on Azure #16705

merged 12 commits into from
Apr 20, 2023

Conversation

garyverhaegen-da
Copy link
Contributor

No description provided.

@garyverhaegen-da
Copy link
Contributor Author

The diff is a bit hard to follow (and larger than I'd like) so here are a few pointers to review it more effectively:

  1. [.dadew, dev-env/windows/manifests/gcloud.json] We add gcloud to the Windows "dev-env". Not having it was a hermiticity oversight; it worked by accident because the GCP machines have it by default.
  2. [infra/azure.tf] All of the non-Ubuntu-specific Azure configuration that was in infra/ubuntu_azure.tf has been moved here verbatim.
  3. [infra/main.tf, nix/default.nix] We add the random provider for generating the Windows password.
  4. [infra/ubuntu_azure.tf] Everything deleted here moved straight to infra/azure.tf (see 2 above).
  5. [infra/windows.tf] Overall Windows control file mirroring infra/ubuntu.tf. Note that we added 6 Azure nodes but did not remove the existing GCP nodes. More on that below.
  6. [infra/windows_azure.tf] This is the "meat" of this PR, the only real new content. Everything else is arguably noise.
  7. [infra/windows_gcp, infra/windows_startup.ps1] Because we want to reuse the startup sequence, it's been moved to its own file which required a couple minor syntactic changes. Apart from those minor tweaks, the file has simply been split in two, with no significant change on either half. Note the new gcp_logging variable inserted into the template (no content change as seen by the GCP machines).

The omission of gcloud from dev-env places us in a bit of a bind: because the dev-env installation is part of the build, not the machine setup, we cannot run existing jobs on the new machines. But once this PR is merged, future builds will use the new dev-env definition and everything will be fine. So I think this PR should be merged as is and the removal of the GCP nodes should be done in a separate step.

There are also a couple other things missing from this PR which will come later on:

  • Integrating the Windows scale sets into the daily-reset script.
  • Finding a way to remove Windows nodes from Azure Pipelines when they get cycled.

@garyverhaegen-da garyverhaegen-da enabled auto-merge (squash) April 19, 2023 16:08
Copy link

@asangeethada asangeethada left a comment

Choose a reason for hiding this comment

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

LGTM!

@garyverhaegen-da garyverhaegen-da merged commit 99821e0 into main Apr 20, 2023
@garyverhaegen-da garyverhaegen-da deleted the azure-windows branch April 20, 2023 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants