PO to GMP Migration Tool: Service Resolution Functions - #2002
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions in pkg/migrate/helpers.go along with comprehensive unit tests in pkg/migrate/helpers_test.go to find Services by selector, resolve Service ports to target ports, and convert Service target labels to static metric relabeling rules. The reviewer provided valuable feedback, suggesting a nil check for the ResourceCache receiver to prevent panics, an explicit check for empty port strings to avoid accidental matches with unnamed ports, and handling additional numeric types (float64 and int) in the targetPort type switch to ensure robust parsing of unstructured objects.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions in pkg/migrate/helpers.go—including findServicesBySelector, resolveServicePort, and convertServiceTargetLabels—along with comprehensive unit tests to support Kubernetes Service resolution and label mapping during migration. The review feedback highlights that resolveServicePort uses a fragile type assertion for port numbers in unstructured objects and incorrectly propagates fatal errors instead of logging warnings and falling back to the port string. Consequently, the reviewer suggests refactoring resolveServicePort to handle type coercion safely and return fallbacks, and updating the unit tests accordingly.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds helper functions to find services by selector, resolve service ports to target ports, and convert service target labels to relabeling rules, along with comprehensive unit tests. The feedback suggests improving the robustness of the service port resolution by logging a warning and using a placeholder instead of returning a fatal error when encountering a malformed port, which prevents the entire migration process from failing.
6d133c9 to
648b3e1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces helper functions to find services by selector, resolve service ports to target ports, and map service labels to metric relabeling rules, along with their corresponding unit tests. Feedback suggests modifying resolveServicePort to log a warning and inject a placeholder port instead of returning a fatal error when encountering a malformed port entry, preventing the entire migration process from failing.
648b3e1 to
8c87394
Compare
60e7cdc to
436c631
Compare
436c631 to
4bdfc9f
Compare
| return intstr.IntOrString{}, fmt.Errorf("failed to read port field: %w", err) | ||
| } | ||
| if !foundField { | ||
| return intstr.IntOrString{}, errors.New("service port spec is missing the port number") |
There was a problem hiding this comment.
In resolveServicePort, encountering a port entry missing port or with an invalid type returns a fatal error immediately. If a Service has multiple ports and an earlier entry is malformed, resolving a valid target port later in the slice fails. Consider skipping malformed entries and returning an error only if no matching port is resolved.
There was a problem hiding this comment.
Hmmm... This seems good, but we definitely need to warn about dropped ports
There was a problem hiding this comment.
I realize there might be a fundamental misunderstanding here. resolveServicePort is looking for a specific port on the Service I.e the one specified by the ServiceMonitor. So if any of the non-targeted ports have an issue, before we errored out, and after the change, we now log that there is an issue and we skip (even though we skip regardless since the port is irrelevant to us, we're basically just logging about an endpoint we don't care about).
Instead we should only check for malformations if the port matches the one we specified. Now the design choice here is whether we want to
- Error -> Fail the migration of this ServiceMonitor immediately.
- Warn and Drop -> Log a warning to skip this specific Service/endpoint, and continue migrating any other healthy Services/endpoints.
1c46b70 to
3b0235d
Compare
3b0235d to
3c2d321
Compare
b205e75 to
279a454
Compare
2 participants
This PR implements the core service and port resolution helper functions in
pkg/migrate/helpers.gorequired for the upcoming ServiceMonitor migration work.Key additions:
findServicesBySelector: Traverses the ResourceCache to return Service CRs matching a specified LabelSelector within target namespaces.resolveServicePort: Inspects a Service'sspec.portslist to map a target port reference (either a string name or a port number) to the Pod's actual container targetPort value.convertServiceTargetLabels: Maps Service-level labels (as defined inspec.targetLabelsof ServiceMonitor) to static metric relabeling rules (action: replace), appending the "exported_" prefix if it clashes with protected Prometheus labels.helpers_test.goverifying all helper behaviors, error paths, and edge cases.