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

[CI] Upstream premerge terraform configuration #117397

Closed

Conversation

boomanaiden154
Copy link
Contributor

@boomanaiden154 boomanaiden154 commented Nov 22, 2024

This patch contains the terraform configuration for the premerge infrastructure. Actually applying changes requires someone from within Google with access to the GCP project to apply the changes, but the entire infrastructure is described within the TF in this patch.

Co-authored-by: Nathan Gauër [email protected]

@boomanaiden154 boomanaiden154 force-pushed the upstream-terraform-infra branch from 3cd9936 to eff7820 Compare November 22, 2024 23:13
This patch contains the terraform configuration for the premerge
infrastructure. Actually applying changes requires someone from within
Google with access to the GCP project to apply the changes, but the
entire infrastructure is described within the TF in this patch.
@@ -0,0 +1,31 @@
# Premerge Infrastructure

This folder contains the terraform configuration files that define the GCP
Copy link
Collaborator

Choose a reason for hiding this comment

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

.ci/infrastructure folder is a bit of a "general" name for this: should we nest it in .ci/infrastructure/terraform/ instead to leave room for other things here?

Also so far all of that kind of infrastructure has been in Zorg (like here for example), I'm not sure how we think about Zorg vs this right now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can rename it to something like .ci/terraform. Nesting it into something like .ci/infrastructure/terraform seems a bit premature to me. I don't think there are other major infrastructure pieces that we need.

Good point on bringing up Zorg. I think it makes more sense to keep all of the precommit configurations in the monorepo. The infra is somewhat coupled to the workflows/associated scripting, which have to be in the monorepo. Given that, I think keeping the infra definitions here makes sense too.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this more coupled than the way zorg interacts with the monorepo already? At some point Zorg seems also to be defining some infrastructure which refers to commands/scripts/options that are defined in the monorepo, so from far I can see a similarity here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main difference is that all of the associated scripts/build configurations/setup is defined in the monorepo. The premerge CI scripts are already here, and all the GHA workflows will have to be in the monorepo. So yes, I think the premerge infra is significantly more coupled to the monorepo than the postcommit infra in zorg.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It isn’t obvious to me why a terraform script that somewhere calls a premerge script in the mono repo is more coupled than any infra setup in Zerg that calls a similar script in the mono repo (or even call cmake+ninja, which isn’t different than a script entry point really).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Zorg (from when I've hacked on it) mostly contains Python scripts that define testing workflows for the various builders. The equivalent testing workflows for premerge are defined in the monorepo (currently shell scripts in .ci, eventually GHA workflow definitions in .github), which leads me to the conclusion that premerge is more tightly coupled to the monorepo.

Putting premerge infra in Zorg seems a bit out of place to me. Currently, it solely contains postcommit configurations/infra. We already have a spot where all of the in-tree premerge scripts and what not live, and that's .ci/ in the monorepo.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've found it very useful to be able to make changes to pre-merge configuration and script, and test them right away as a regular PR to the monorepo. It seems that the workflow for zorg changes is less straightforward. Since this PR is concerned with pre-merge stuff, I prefer to keep the changes in the monorepo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've found it very useful to be able to make changes to pre-merge configuration and script, and test them right away as a regular PR to the monorepo.

I probably misunderstand what this PR is doing then, because it seemed to me that nothing here is about something that you would be able to test with a PR, instead it defined the underlying infra (kube, machines, runners) that need to be setup offline before you can rely on it for testing.

[...] which leads me to the conclusion that premerge is more tightly coupled to the monorepo.

Sorry: I don't get it, I read this as mixing up unrelated things here, but I may be misunderstanding what you're doing here. It seems that the two things are 1) the config of the infra (machines and runners) and 2) "what is executed by the runner based on some event".

In the monorepo we have so far:

  1. the cmake config
  2. the premerge scripts that define how to invoke cmake & ninja & ... on premerge events.

In Zorg there is:

  1. buildbot configs for the machines
  2. terraform config for some clusters
  3. Docker images for the runners
  4. the post merge scripts which define what cmake/ninja/... to invoke on post-merge event.

So it is true that 2 and 4 are at odds (but for good reason, with property such as what @Endilll mentioned above).
But I'm not following why this implies anything about the rest of the config, which is really more "infra" than anything. The question of coupling is really about this: how is the actual kube/docker config more coupled to the execution script (the 2./4. bullet point above that just wraps cmake/ninja basically) in one case than the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with the points made so far. Is is true that most of what's proposed here is mostly internals for infra management so this could indeed live in llvm-zorg.

And since a googler has to run a terraform apply to any change anyway, having them in this repo or the other doesn't bring IMO any sync/atomicity benefit.

Copy link
Contributor

@Endilll Endilll left a comment

Choose a reason for hiding this comment

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

I asked my friend to take a look at this, and here's their feedback.
I hope you find it useful.

.ci/infrastructure/main.tf Outdated Show resolved Hide resolved
.ci/infrastructure/main.tf Show resolved Hide resolved
Copy link
Contributor

@Keenuts Keenuts left a comment

Choose a reason for hiding this comment

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

LGTM the for code itself, but agreeing with the discussion about merging this into the infra repo.

@boomanaiden154
Copy link
Contributor Author

Created llvm/llvm-zorg#324 to merge into Zorg.

@boomanaiden154 boomanaiden154 deleted the upstream-terraform-infra branch November 29, 2024 07:54
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.

4 participants