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

More defensive approach to retrieving Node labels for a node #2534

Merged
merged 1 commit into from Feb 20, 2024

Conversation

thomasvn
Copy link
Contributor

@thomasvn thomasvn commented Feb 14, 2024

What does this PR change?

  • Add NodeLabels one by one, instead of assigning the map of labels directly to the result.
  • This protects against the case where there exist multiple kube_node_labels metrics for a single node.

Does this PR relate to any other PRs?

  • N/A

How will this PR impact users?

  • Fixes a bug for users who have more than one kube_node_labels per node in their Prometheus environment.

Does this PR address any GitHub or Zendesk issues?

  • N/A

How was this PR tested?

  • Ran locally in an environment that does not have duplicated kube_node_labels. Functioning as expected when querying the Assets API.

Does this PR require changes to documentation?

  • N/A

Have you labeled this PR and its corresponding Issue as "next release" if it should be part of the next OpenCost release? If not, why not?

Copy link

vercel bot commented Feb 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
opencost ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 20, 2024 5:27pm

@AjayTripathy
Copy link
Contributor

This was really impactful. I'd like to get this change into the agent

Copy link
Contributor

@michaelmdresser michaelmdresser left a comment

Choose a reason for hiding this comment

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

Really, really nice fix. How in the world did you discover it?

m[key] = map[string]string{}
}
for k, l := range result.GetLabels() {
m[key][k] = l
Copy link
Contributor

Choose a reason for hiding this comment

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

This is way better, but will still result in one value for a key "winning" in the event of a conflict. It's not really possible to avoid that. I'm wondering if, in the future, there is a way for us to preserve conflicts instead of picking one winner silently. Not for this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, good catch! If there are two kube_node_labels metrics describing the same node and using the same labels, the second label read wins (example below). I can't think of a simple solution right now to resolve those conflicts, but we may be able to implement something similar to the honor_labels Prometheus config (docs ref).

kube_node_labels{node="gke-qa-gcp1-default-pool-169fb9bb-0zz6", label_topology_gke_io_zone="us-east1-d"}
kube_node_labels{node="gke-qa-gcp1-default-pool-169fb9bb-0zz6", label_topology_gke_io_zone="us-east1-a"}

@thomasvn
Copy link
Contributor Author

@AjayTripathy @michaelmdresser Appreciate the reviews! I don't have write access so if either of you could merge that'd be great!

directly to the result. This protects against the case where there exist
multiple `kube_node_labels` metrics for a single node.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>
@michaelmdresser michaelmdresser merged commit cf0db80 into opencost:develop Feb 20, 2024
5 checks passed
Copy link

sonarcloud bot commented Feb 20, 2024

@thomasvn thomasvn deleted the thomasn/nodelabels branch February 20, 2024 17:31
michaelmdresser pushed a commit that referenced this pull request Feb 20, 2024
)

directly to the result. This protects against the case where there exist
multiple `kube_node_labels` metrics for a single node.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
michaelmdresser pushed a commit that referenced this pull request Feb 20, 2024
)

directly to the result. This protects against the case where there exist
multiple `kube_node_labels` metrics for a single node.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
michaelmdresser added a commit that referenced this pull request Feb 20, 2024
) (#2552)

directly to the result. This protects against the case where there exist
multiple `kube_node_labels` metrics for a single node.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Co-authored-by: Thomas Nguyen <thomasvn.dev@gmail.com>
michaelmdresser added a commit that referenced this pull request Feb 20, 2024
) (#2551)

directly to the result. This protects against the case where there exist
multiple `kube_node_labels` metrics for a single node.

Signed-off-by: thomasvn <thomasnguyen96@gmail.com>
Signed-off-by: Michael Dresser <michaelmdresser@gmail.com>
Co-authored-by: Thomas Nguyen <thomasvn.dev@gmail.com>
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.

None yet

3 participants