-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
expose the PodCIDR in node info #963
Conversation
Welcome @jacobstr! |
6027ea0
to
5568b22
Compare
It can be helpful to know the PodCIDR of nodes in order to predict IP space utilization. In GKE for example, one typically uses a Secondary Alias IP range that doles out PodCIDRs dynamically. When this is exhausted, nodes will fail to boot with messages such as: ``` Instance 'gke-test-ingre-43fd70d6-9v77' creation failed: IP space of 'projects/network-project/regions/us-central1/subnetworks/pod-alias-b85f82b0bca36afa' is exhausted. ``` Although not comprehensive, on could use this label to construct a query such as: ``` count(kube_node_info(pod_cidr~="/24")) * 256 ```
5568b22
to
a374b05
Compare
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.
Thanks for your PR!
@@ -59,6 +60,7 @@ var ( | |||
n.Status.NodeInfo.KubeletVersion, | |||
n.Status.NodeInfo.KubeProxyVersion, | |||
n.Spec.ProviderID, | |||
n.Spec.PodCIDR, |
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.
This is an optional label, not sure how we usually handle optional labels cc @brancz @tariq1890
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.
It’s not a pointer so at worst this is the empty string in which case Prometheus just drops the label so it’s fine as is.
@@ -46,6 +46,7 @@ func TestNodeStore(t *testing.T) { | |||
}, | |||
Spec: v1.NodeSpec{ | |||
ProviderID: "provider://i-uniqueid", | |||
PodCIDR: "172.24.10.0/24", |
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.
Since this can be empty or not set, lets have one test where this is not set.
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.
I'm adding a general test case with an empty v1.Node
and verifying all empty strings. Not sure if you have preferences over the ordering of test cases but I'm happy to change that.
Note: I'm not committing a spurious
That shows up in my local diff. I don't expect any changes to |
Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: brancz, jacobstr The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
What this PR does / why we need it:
It can be helpful to know the PodCIDR of nodes in order to predict IP
space utilization. In GKE for example, one typically uses a Secondary
Alias IP range that doles out PodCIDRs dynamically. When this is
exhausted, nodes will fail to boot with messages such as:
Although not comprehensive, on could use this label to construct a query
such as: