Conversation
jshiwamV
force-pushed
the
observability-azure
branch
3 times, most recently
from
a39221a to
796e7d0
Compare
bobbyiliev
left a comment
bobbyiliev
left a comment
There was a problem hiding this comment.
Looks good! I think that we have to rebase? I've also added a few questions.
gcp/examples/simple/variables.tf
Outdated
| # Observability outputs (only when enabled) | ||
| output "prometheus_url" { | ||
| description = "Internal URL for Prometheus server" | ||
| value = var.enable_observability ? module.prometheus[0].prometheus_url : null | ||
| } | ||
|
|
||
| output "grafana_url" { | ||
| description = "Internal URL for Grafana" | ||
| value = var.enable_observability ? module.grafana[0].grafana_url : null | ||
| } | ||
|
|
||
| output "grafana_admin_password" { | ||
| description = "`admin` password for Grafana" | ||
| value = var.enable_observability ? module.grafana[0].admin_password : null | ||
| sensitive = true | ||
| } |
There was a problem hiding this comment.
Why are the outputs defined in variables.tf instead of outputs.tf? Should these be moved to a separate outputs.tf file or added to the existing one?
There was a problem hiding this comment.
that was my bad. fixed it.
| } | ||
|
|
||
|
|
||
| module "prometheus" { |
There was a problem hiding this comment.
In the AWS example, the operator module gets helm_values configured to enable Prometheus scrape annotations when observability is enabled. Azure and GCP seem to be missing this. Won't Prometheus fail to scrape metrics without these annotations?
There was a problem hiding this comment.
huh, i remember adding this to all the operators, not sure if I did a bad rebase. Thanks for pointing out.
| ] | ||
| } | ||
|
|
||
| module "prometheus" { |
There was a problem hiding this comment.
Same as Azure - missing the operator helm_values for enabling scrape annotations. Do you think that we need those?
| ] | ||
|
|
||
| # https://learn.microsoft.com/en-us/azure/aks/concepts-storage#storage-classes | ||
| storage_class = "managed-csi" |
There was a problem hiding this comment.
Does this require enabling the CSI driver addon first or is it available out of the box?
There was a problem hiding this comment.
https://registry.terraform.io/providers/hashicorp/azurerm/latest/docs/resources/kubernetes_cluster#disk_driver_enabled-1 it is enabled by default but let me make it explicit for visibility.
| } | ||
| }] | ||
| }) | ||
| storage_class = "standard-rwo" # default storage class in gcp |
There was a problem hiding this comment.
Is "standard-rwo" the default, I have some vague memory that it was "standard"? Also, does GKE require any specific configuration to use CSI-based storage classes?
There was a problem hiding this comment.
yes it is enabled by default, i will make it explicit though. Here you can read more about standard-rwo.https://docs.cloud.google.com/kubernetes-engine/docs/how-to/persistent-volumes/gce-pd-csi-driver#create_a_storageclass
|
|
||
| depends_on = [ | ||
| module.operator, | ||
| module.aks, |
There was a problem hiding this comment.
Should this also depend on the node groups to ensure nodes are ready? AWS has module.base_node_group in its depends_on.
There was a problem hiding this comment.
in azure base node group is the part of AKS cluster config.
| storage_class = local.storage_class | ||
| depends_on = [ | ||
| module.operator, | ||
| module.gke, |
There was a problem hiding this comment.
Same question as Azure - should this depend on node pools being ready? The AWS example has module.nodepool_generic in depends_on but GCP only depends on the cluster.
There was a problem hiding this comment.
yes, actually since it depends on operator, it indirectly is dependent on nodepool_generic, but its good to keep it consistent across cloud providers, thanks for pointing it out.
azure/examples/simple/main.tf
Outdated
|
|
||
|
|
There was a problem hiding this comment.
nit: there's an extra blank line before the prometheus module declaration here.
jshiwamV
force-pushed
the
observability-azure
branch
from
796e7d0 to
83e23c1
Compare
bobbyiliev
left a comment
bobbyiliev
left a comment
There was a problem hiding this comment.
Overall this looks very good. A few questions and nits below which should not really be blockers.
| # Enable Prometheus scrape annotations when observability is enabled | ||
| helm_values = var.enable_observability ? { | ||
| observability = { | ||
| enabled : true |
There was a problem hiding this comment.
nit: Should this use = instead of : for consistency with the rest of the codebase?
| } | ||
| variable "dns_service_ip" { |
There was a problem hiding this comment.
nit: missing blank line between disk_driver_enabled and dns_service_ip variables?
|
|
||
|
|
||
| variable "enable_observability" { |
There was a problem hiding this comment.
nit: extra blank line here, worth keeping consistent?
| ] | ||
| } | ||
|
|
||
| provider "registry.terraform.io/hashicorp/http" { |
There was a problem hiding this comment.
The hashicorp/http provider was added to both Azure and GCP lock files, but I don't see it explicitly used. Is it a transitive dependency from the prometheus/grafana modules?
2 participants
Uh oh!
There was an error while loading. Please reload this page.