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

Add ClusterCIDR name to MultiCIDRSet to track metrics correctly. #47

Merged
merged 3 commits into from
Jan 3, 2025

Conversation

mneverov
Copy link
Member

@mneverov mneverov commented Jan 2, 2025

With this change the metrics will have two labels: cidr and cidr name.

Fixes #44

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mneverov

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 2, 2025
@aojea
Copy link
Contributor

aojea commented Jan 2, 2025

sounds simple and easy way to deambiguate and in theory should be compatible, right?
@mneverov there is a panic

2025-01-02T11:17:03.400562679Z stderr F I0102 11:17:03.400472       1 multi_cidr_range_allocator.go:219] "Regenerating existing ClusterCIDR" clusterCIDR={"kind":"ClusterCIDR","apiVersion":"networking.x-k8s.io/v1","metadata":{"name":"clustercidr-test","uid":"1131e4c9-300b-4433-934a-beccd648f9ff","resourceVersion":"534","generation":1,"creationTimestamp":"2025-01-02T11:16:14Z","managedFields":[{"manager":"kubectl-create","operation":"Update","apiVersion":"networking.x-k8s.io/v1","time":"2025-01-02T11:16:14Z","fieldsType":"FieldsV1","fieldsV1":{"f:spec":{".":{},"f:ipv4":{},"f:perNodeHostBits":{}}}}]},"spec":{"perNodeHostBits":8,"ipv4":"10.244.0.0/16"}}
2025-01-02T11:17:03.403781443Z stderr F panic: inconsistent label cardinality: expected 1 label values but got 2 in []string{"10.244.0.0/16", "clustercidr-test"}
2025-01-02T11:17:03.403793535Z stderr F 
2025-01-02T11:17:03.403799536Z stderr F goroutine 26 [running]:
2025-01-02T11:17:03.403804866Z stderr F github.com/prometheus/client_golang/prometheus.(*GaugeVec).WithLabelValues(...)
2025-01-02T11:17:03.403810337Z stderr F 	/go/pkg/mod/github.com/prometheus/[email protected]/prometheus/gauge.go:238
2025-01-02T11:17:03.403815787Z stderr F sigs.k8s.io/node-ipam-controller/pkg/controller/ipam/multicidrset.NewMultiCIDRSet({0xc0000d60f0, 0x10}, 0xc0000bd800, 0x8)
2025-01-02T11:17:03.403824683Z stderr F 	/workspace/pkg/controller/ipam/multicidrset/multi_cidr_set.go:148 +0x3fe
2025-01-02T11:17:03.403831075Z stderr F sigs.k8s.io/node-ipam-controller/pkg/controller/ipam.(*multiCIDRRangeAllocator).createClusterCIDRSet(0xc000595860?, 0xc0001b6140, 0x0)
2025-01-02T11:17:03.403836265Z stderr F 	/workspace/pkg/controller/ipam/multi_cidr_range_allocator.go:1184 +0x108
2025-01-02T11:17:03.403842216Z stderr F sigs.k8s.io/node-ipam-controller/pkg/controller/ipam.(*multiCIDRRangeAllocator).createClusterCIDR(0xc0000c6420, {0x1d061f8, 0xc00024c1e0}, 0xc0001b6140, 0x25?)
2025-01-02T11:17:03.403848027Z stderr F 	/workspace/pkg/controller/ipam/multi_cidr_range_allocator.go:1134 +0xc5
2025-01-02T11:17:03.403853167Z stderr F sigs.k8s.io/node-ipam-controller/pkg/controller/ipam.(*multiCIDRRangeAllocator).reconcileBootstrap(0xc0000c6420, {0x1d061f8, 0xc00024c1e0}, 0xc0001b6140)
2025-01-02T11:17:03.403865979Z stderr F 	/workspace/pkg/controller/ipam/multi_cidr_range_allocator.go:1119 +0x29c
2025-01-02T11:17:03.403875386Z stderr F sigs.k8s.io/node-ipam-controller/pkg/controller/ipam.NewMultiCIDRRangeAllocator({0x1d061f8, 0xc00024c1e0}, {0x1d1c228, 0xc00049b040}, {0x1d0bbd0?, 0xc000051da0?}, {0x1cf4fb0, 0xc0003de588}, {0x1cf4dd0, 0xc0003de5a0}, ...)
2025-01-02T11:17:03.403880666Z stderr F 	/workspace/pkg/controller/ipam/multi_cidr_range_allocator.go:222 +0xc18
2025-01-02T11:17:03.403889382Z stderr F main.runControllers({0x1d061f8, 0xc00024c1e0}, {0x1d1c228, 0xc00049b040}, 0x0?, {{0x1d08a30?, 0xc00011eb40?}, 0x0?})
2025-01-02T11:17:03.403898369Z stderr F 	/workspace/main.go:149 +0x2e5
2025-01-02T11:17:03.403905352Z stderr F sigs.k8s.io/node-ipam-controller/pkg/leaderelection.StartLeaderElection.func1({0x1d061f8, 0xc00024c1e0})
2025-01-02T11:17:03.40403858Z stderr F 	/workspace/pkg/leaderelection/leaderelection.go:74 +0x16a
2025-01-02T11:17:03.404044651Z stderr F created by k8s.io/client-go/tools/leaderelection.(*LeaderElector).Run in goroutine 1
2025-01-02T11:17:03.40404935Z stderr F 	/go/pkg/mod/k8s.io/[email protected]/tools/leaderelection/leaderelection.go:213 +0xf6

@ugur99 can you validate this approach works for your environment and solves the problem (we need to solve the panic first 😄 )?

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 2, 2025
@ugur99
Copy link
Contributor

ugur99 commented Jan 2, 2025

I have tested this change on my local test cluster and I think the metrics look better and the fluctuating issue seems to be fixed.
Screenshot 2025-01-02 at 19 26 35

@aojea
Copy link
Contributor

aojea commented Jan 2, 2025

Great, some code need to be updated to use the new signature

Error: : # sigs.k8s.io/node-ipam-controller/pkg/controller/ipam [sigs.k8s.io/node-ipam-controller/pkg/controller/ipam.test]
  Error: pkg/controller/ipam/multi_cidr_priority_queue_test.go:157:58: not enough arguments in call to multicidrset.NewMultiCIDRSet
  	have (*"net".IPNet, int)
  	want (string, *"net".IPNet, int) (typecheck)
  Error: : # sigs.k8s.io/node-ipam-controller/pkg/controller/ipam/multicidrset [sigs.k8s.io/node-ipam-controller/pkg/controller/ipam/multicidrset.test]
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:62:42: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, int)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:213:42: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, int)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:243:42: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, number)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:[30](https://github.com/kubernetes-sigs/node-ipam-controller/actions/runs/12586168465/job/35079465078?pr=47#step:4:32)3:42: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, number)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:415:41: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, number)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:578:43: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, int)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:639:43: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, int)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:680:41: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, number)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:7[40](https://github.com/kubernetes-sigs/node-ipam-controller/actions/runs/12586168465/job/35079465078?pr=47#step:4:42):41: not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, number)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:788:[43](https://github.com/kubernetes-sigs/node-ipam-controller/actions/runs/12586168465/job/35079465078?pr=47#step:4:45): not enough arguments in call to NewMultiCIDRSet
  	have (*"net".IPNet, number)
  	want (string, *"net".IPNet, int)
  Error: pkg/controller/ipam/multicidrset/multi_cidr_set_test.go:788:43: too many errors (typecheck)

@mneverov
Copy link
Member Author

mneverov commented Jan 3, 2025

@aojea ptal, fixed tests.
I also tested it locally with 5 nodes and two shared IP ranges (different CIDRs). Got correct metrics per CIDR.

@aojea
Copy link
Contributor

aojea commented Jan 3, 2025

/lgtm

Thanks

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 3, 2025
@k8s-ci-robot k8s-ci-robot merged commit e2b5a7e into kubernetes-sigs:main Jan 3, 2025
8 of 9 checks passed
@ugur99
Copy link
Contributor

ugur99 commented Jan 3, 2025

Thanks a lot @mneverov @aojea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fluctuating multicidrset_usage_cidrs metric
4 participants