-
Notifications
You must be signed in to change notification settings - Fork 28
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
feat(infra.ci/agent): new cluster in azure sponsored for infra.ci agents #715
feat(infra.ci/agent): new cluster in azure sponsored for infra.ci agents #715
Conversation
abd7dbf
to
3160315
Compare
locals.tf
Outdated
publick8s_compute_zones = [3] | ||
cijenkinsio_agents_1_compute_zones = [1] | ||
infracijenkinsio_agents_1_compute_zones = [1] | ||
|
||
infraci_jenkins_io_agents_1_pod_cidr = "10.100.0.0/14" # 10.100.0.1 - 10.103.255.255 |
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.
publick8s_compute_zones = [3] | |
cijenkinsio_agents_1_compute_zones = [1] | |
infracijenkinsio_agents_1_compute_zones = [1] | |
infraci_jenkins_io_agents_1_pod_cidr = "10.100.0.0/14" # 10.100.0.1 - 10.103.255.255 | |
infraci_jenkins_io_agents_1_compute_zones = [1] | |
infraci_jenkins_io_agents_1_pod_cidr = "10.100.0.0/14" # 10.100.0.1 - 10.103.255.255 | |
publick8s_compute_zones = [3] |
Nit: ordering and regrouping values, with cijenkinsio_agents_1_compute_zones moved above, and keep the same variable name format.
locals.tf
Outdated
} | ||
ci_jenkins_io_fqdn = "ci.jenkins.io" | ||
ci_jenkins_io_agents_1_pod_cidr = "10.100.0.0/14" | ||
ci_jenkins_io_agents_1_pod_cidr = "10.100.0.0/14" # 10.100.0.1 - 10.103.255.255 |
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.
ci_jenkins_io_agents_1_pod_cidr = "10.100.0.0/14" # 10.100.0.1 - 10.103.255.255 | |
ci_jenkins_io_agents_1_compute_zones = [1] | |
ci_jenkins_io_agents_1_pod_cidr = "10.100.0.0/14" # 10.100.0.1 - 10.103.255.255 |
nit: moved from below and keep the same variable name format.
node_count = 3 # 3 nodes for HA as per AKS best practises | ||
vnet_subnet_id = data.azurerm_subnet.infraci_jenkins_io_kubernetes_agent_sponsorship.id | ||
tags = local.default_tags | ||
zones = local.cijenkinsio_agents_1_compute_zones |
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.
zones = local.cijenkinsio_agents_1_compute_zones | |
zones = local.ci_jenkins_io_agents_1_compute_zones |
(nit, keep same variable name format)
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 idea is to have the cluster Terraform's resource ID the same as the local
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.
Yes, I also suggested the same change in locals so all/most resources have a common naming strategy. (Might need more work, too much?)
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 misunderstood each other: i am not in favor of the proposal. I prefer the local values to be the same as the terraform resource id, which is « cijenkinsio_agents_1_… ».
if you want to change the convention no problem but it us more than a nit as it involves migrating resources (using a moved block) and derails the PR here
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.
Yes, I also suggested the same change in locals so all/most resources have a common naming strategy. (Might need more work, too much?)
With the last changes, I think I see the "naming" missing homogeneity. I would want to make this another PR though otherwise the review here will be too hard (side tracking from the main goal): could you open a first draft PR so we can have the discussion properly?
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.
Sure 🙂
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.
Plan: 2 to add, 0 to change, 0 to destroy.
=> a new RG and the new cluster.
LGTM @smerle33 you can proceed!
This PR failed to deploy with the following error:
#719 created by @smerle33 to revert if we cannot fix the error easily |
Error fixed by jenkins-infra/azure-net#250. Let's retry |
New error:
|
Should be fixed by:
=> new subnet name is 53 chars |
… (shorter) name (#720) This PR aims to fix the error mentioned in #715 (comment) It updates the subnet name used for this cluster as a follow up of jenkins-infra/azure-net#252 Signed-off-by: Damien Duportal <[email protected]>
Next step: #720 |
as per jenkins-infra/helpdesk#3923 and following #715 this PR create 3 nodes pools: - application one in arm64 - agents in x86-64 - agents in arm64 --------- Co-authored-by: Damien Duportal <[email protected]>
as per #715 need to change the os
as per jenkins-infra/helpdesk#3923 (comment)
kubernetes cluster within the sponsored subscription of azure
split in 3 PR:
- creation of the cluster (this one)
- creation of the nodes
- creation of kubernetes-admin-sa with the module
depends on jenkins-infra/azure-net#249 for the network definition