feat: add more neutron metrics #64
feat: add more neutron metrics #64Rosan Shanmuganathan (na50r) wants to merge 9 commits intovexxhost:mainfrom
Conversation
6d70d32 to
972f94b
Compare
Signed-off-by: na50r <shanrosan@gmail.com>
972f94b to
0c91bac
Compare
Signed-off-by: na50r <shanrosan@gmail.com>
Signed-off-by: na50r <shanrosan@gmail.com>
Signed-off-by: na50r <shanrosan@gmail.com>
Signed-off-by: na50r <shanrosan@gmail.com>
391f403 to
924c768
Compare
Signed-off-by: na50r <shanrosan@gmail.com>
Signed-off-by: na50r <shanrosan@gmail.com>
99b1b64 to
7b8e5c6
Compare
Signed-off-by: na50r <shanrosan@gmail.com>
7b8e5c6 to
9e84239
Compare
|
Rosan Shanmuganathan (@na50r) we approve your pipeline for a new features later. Thank you for your patience. |
There was a problem hiding this comment.
Pull request overview
Adds additional Neutron-related Prometheus metrics by expanding the Neutron SQL schema/queries, generating db accessors, and introducing new collectors + tests under the Neutron subsystem.
Changes:
- Expanded Neutron SQL schema and added multiple new SQL queries (routers, floating IPs, networks, ports, subnets, subnet pools, IP availability).
- Added new Neutron collectors (networking umbrella collector + per-resource collectors) and corresponding unit tests.
- Updated generated Go query bindings and added supporting model types.
Reviewed changes
Copilot reviewed 21 out of 22 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
| sql/neutron/schema.sql | Adds Neutron tables needed for new metrics/queries. |
| sql/neutron/queries.sql | Adds SQL queries powering the new collectors. |
| internal/db/neutron/queries.sql.go | Generates Go bindings for the new SQL queries. |
| internal/db/neutron/models.go | Adds model / enum scan helpers for Neutron types. |
| internal/collector/neutron/networking.go | Adds an umbrella collector emitting a single neutron_up and delegating to sub-collectors. |
| internal/collector/neutron/networks.go | Adds network metrics collector. |
| internal/collector/neutron/routers.go | Adds router metrics collector; updates HA router-agent binding collector to return errors. |
| internal/collector/neutron/ports.go | Adds port metrics collector. |
| internal/collector/neutron/floating_ips.go | Adds floating IP metrics collector. |
| internal/collector/neutron/subnets.go | Adds subnet + subnet pool capacity metrics collector. |
| internal/collector/neutron/security_groups.go | Adds security group count collector. |
| internal/collector/neutron/misc.go | Adds IP availability metrics and related helpers. |
| internal/collector/neutron/*.go tests | Adds tests for the new collectors and umbrella neutron_up behavior. |
| internal/collector/neutron/neutron.go | Registers the new umbrella collector instead of the single HA collector. |
| go.mod | Adds go4.org/netipx dependency for subnet math. |
Comments suppressed due to low confidence (1)
internal/collector/neutron/subnets.go:1
calculateFreeSubnetsrebuilds an IP set (add pool, remove all subnets, build) for everyprefixLen, which can be expensive for large deployments. Consider building the remaining free IP set once per pool prefix and deriving counts for eachprefixLenfrom that (or caching intermediate results) to reduce repeated work.
package neutron
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for i, pool := range pools { | ||
| result[i] = subnetpoolWithSubnets{&pool, subnetPrefixes[pool.ID]} |
There was a problem hiding this comment.
This takes the address of the range loop variable (&pool), which is reused each iteration, so every subnetPool pointer in result ends up pointing to the same (last) pool. Use a stable address (e.g., &pools[i]) or create a new variable per iteration before taking its address.
| for i, pool := range pools { | |
| result[i] = subnetpoolWithSubnets{&pool, subnetPrefixes[pool.ID]} | |
| for i := range pools { | |
| pool := &pools[i] | |
| result[i] = subnetpoolWithSubnets{pool, subnetPrefixes[pool.ID]} |
| func (e *SubnetsIpv6AddressMode) Scan(src interface{}) error { | ||
| switch s := src.(type) { | ||
| case []byte: | ||
| *e = SubnetsIpv6AddressMode(s) | ||
| case string: | ||
| *e = SubnetsIpv6AddressMode(s) | ||
| default: | ||
| return fmt.Errorf("unsupported scan type for SubnetsIpv6AddressMode: %T", src) | ||
| } |
There was a problem hiding this comment.
Converting []byte directly to a defined string type won’t compile. Convert via string(s) first (same issue also applies to SubnetsIpv6RaMode.Scan).
There was a problem hiding this comment.
This is generated code, if you want it to be different, either complain to those who made sqlc or change the SQL query that generated this. And you are wrong, this code DOES compile.
| s.gateway_ip, | ||
| s.network_id, | ||
| s.project_id, | ||
| s.enable_dhcp; |
There was a problem hiding this comment.
s.subnetpool_id is selected but not included in GROUP BY and is not aggregated. With MySQL/MariaDB ONLY_FULL_GROUP_BY enabled, this query will error. Either add s.subnetpool_id to the GROUP BY list, or aggregate it (e.g., MAX(s.subnetpool_id) if appropriate).
| s.enable_dhcp; | |
| s.enable_dhcp, | |
| s.subnetpool_id; |
| sp.ip_version, | ||
| sp.max_prefixlen, | ||
| sp.min_prefixlen, | ||
| sp.default_prefixlen; |
There was a problem hiding this comment.
sp.project_id and sp.name are selected but not included in GROUP BY and not aggregated. This will fail under ONLY_FULL_GROUP_BY (and is nondeterministic otherwise). Add those columns to GROUP BY or use an aggregate/ANY_VALUE() depending on your DB support.
| sp.default_prefixlen; | |
| sp.default_prefixlen, | |
| sp.project_id, | |
| sp.name; |
| func (t *testHARouterAgentPortBindingCollector) Collect(ch chan<- prometheus.Metric) { | ||
| _ = t.HARouterAgentPortBindingCollector.Collect(ch) | ||
| } |
There was a problem hiding this comment.
The test wrapper discards errors from Collect, which can make the test pass even if the collector fails. Match the pattern used in other tests (panic/fail the test when Collect returns an error).
| func (t *testRouterCollector) Collect(ch chan<- prometheus.Metric) { | ||
| _ = t.RouterCollector.Collect(ch) | ||
| } |
There was a problem hiding this comment.
Same issue as above: ignoring the returned error can hide failures. Fail the test if Collect returns an error.
| "02e67ccf829b44438e1c0397b2444a0f", | ||
| "bd221f59-33c9-4d67-8f9c-ccaea76b05c5", | ||
| "ACTIVE", | ||
| "", |
There was a problem hiding this comment.
Using an empty string for fixed_ip_address makes sql.NullString.Valid true, which changes the collector behavior that checks .Valid to determine association. If you intend to represent SQL NULL here, use nil so the test matches real DB semantics.
| "", | |
| nil, |
| `device_id` varchar(255) NOT NULL, | ||
| `device_owner` varchar(255) NOT NULL, | ||
| `standard_attr_id` bigint NOT NULL, | ||
| `ip_allocation` varchar(16) DEFAULT NULL |
There was a problem hiding this comment.
The ports table definition lacks a primary key. Neutron’s ports.id is typically the primary key; missing it can break assumptions in tests/tools and can affect join correctness/deduplication when developing against this schema. Consider adding PRIMARY KEY (id) (and any other required indexes) to match the real schema more closely.
| `ip_allocation` varchar(16) DEFAULT NULL | |
| `ip_allocation` varchar(16) DEFAULT NULL, | |
| PRIMARY KEY (`id`) |
| github.com/prometheus/procfs v0.16.1 // indirect | ||
| github.com/xhit/go-str2duration/v2 v2.1.0 // indirect | ||
| go.yaml.in/yaml/v2 v2.4.3 // indirect | ||
| go4.org/netipx v0.0.0-20231129151722-fdeea329fbba // indirect |
There was a problem hiding this comment.
go4.org/netipx is imported directly (e.g., in internal/collector/neutron/subnets.go), so it should not be marked // indirect. Running go mod tidy should correct this and keep the module metadata consistent.
| go4.org/netipx v0.0.0-20231129151722-fdeea329fbba // indirect | |
| go4.org/netipx v0.0.0-20231129151722-fdeea329fbba |
Adds neutron metrics to the vexxhost openstack database exporter
TODO:
Implement tests (unsure how thorough...)
Implement missing metrics (Niedbalski 1.6.0)
Comment
openstack_neutron_agent_statefrom the DB. See: codeTODO: