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

feat: add advanced datapath observability config option #1776

Conversation

TheKangaroo
Copy link
Contributor

fixes #1755

I'm not sure about the variable names though. Any ideas for shorter names?

@TheKangaroo TheKangaroo requested review from ericyz and a team as code owners October 20, 2023 14:12
@TheKangaroo TheKangaroo force-pushed the feat/advanced-datapath-observability branch from 60ae18d to 32a8ae3 Compare October 24, 2023 08:37
@TheKangaroo
Copy link
Contributor Author

Hi @ericyz , WDYT about the variable names? I shortened them to

monitoring_enable_observability_metrics = false
monitoring_observability_metrics_relay_mode = null

The default for the mode variable is INTERNAL_VPC_LB in my private cluster, but I suspect this is different for different cluster types. I cannot verify this though. Anyway I think a default of null makes sense?

@TheKangaroo
Copy link
Contributor Author

@ericyz @apeabody could you have a look at this?

@apeabody
Copy link
Collaborator

/gcbrun

@TheKangaroo
Copy link
Contributor Author

I had to merge the latest master.
However, the pipeline status was green before.
I don't think I can trigger /gcbrun myself, can I?

@apeabody
Copy link
Collaborator

/gcbrun

@TheKangaroo
Copy link
Contributor Author

TheKangaroo commented Nov 3, 2023

@apeabody I have rebeased merged master again. Last time the pipeline status was red. Did the PR break something?

@apeabody
Copy link
Collaborator

apeabody commented Nov 3, 2023

/gcbrun

@apeabody
Copy link
Collaborator

apeabody commented Nov 3, 2023

@apeabody I have rebeased merged master again. Last time the pipeline status was red. Did the PR break something?

Hi @TheKangaroo - For terraform-google-kubernetes-engine-int-trigger a maintainer needs to comment gcbrun after the latest commits, otherwise the check won't actually run in CloudBuild and will (confusingly) be marked as failed in GitHub.

I just triggered it now. If the previous result was red from an actual run, it's very possible that result was a flake.

@TheKangaroo
Copy link
Contributor Author

Thanks for the explanation @apeabody.
Looks good now, doesn't it?

Copy link
Collaborator

@apeabody apeabody left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @TheKangaroo!

@apeabody apeabody merged commit 90e9bdf into terraform-google-modules:master Nov 6, 2023
4 checks passed
@Markieta
Copy link

Trying to disable these does not seem to take effect:

  monitoring_enable_observability_metrics = false
  monitoring_observability_metrics_relay_mode = "DISABLED"

Not sure if the problems lies in this module or the underlying google_container_cluster resource.

@TheKangaroo
Copy link
Contributor Author

@Markieta
Hmm, thats strange, I run all my clusters with:

  monitoring_enable_observability_metrics     = true
  monitoring_observability_metrics_relay_mode = "INTERNAL_VPC_LB"

but changed one to test this to

  monitoring_enable_observability_metrics     = false
  monitoring_observability_metrics_relay_mode = "INTERNAL_VPC_LB"

and I think everyting works as expected.

Terraform needed like half an hour to complete, but after that, everything seems to be fine.

The only thing that was a bit suprising to me ist that the cluster state show true if enalbed and nothing if disabled:
Enabled (before terraform change):

> gcloud container clusters describe <name> | grep monitoringConfig -A 3
monitoringConfig:
  advancedDatapathObservabilityConfig:
    enableMetrics: true
    relayMode: INTERNAL_VPC_LB

Disabled (after terraform change):

> gcloud container clusters describe <name> | grep monitoringConfig -A 3
monitoringConfig:
  advancedDatapathObservabilityConfig:
    relayMode: INTERNAL_VPC_LB
  componentConfig: {}

I think it would be better if you opened a new issue describing exactly what is happening.

@Markieta
Copy link

Markieta commented Mar 1, 2024

Thanks, perhaps it's only an issue for existing cluster (modules) that were created before adding these inputs? I've created an issue.

CPL-markus pushed a commit to WALTER-GROUP/terraform-google-kubernetes-engine that referenced this pull request Jul 15, 2024
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.

Support advanced_datapath_observability_config
3 participants