Add passthrough handlers and mcl setup to placement shim#692
Open
PhilippMatthes wants to merge 17 commits intomainfrom
Open
Add passthrough handlers and mcl setup to placement shim#692PhilippMatthes wants to merge 17 commits intomainfrom
PhilippMatthes wants to merge 17 commits intomainfrom
Conversation
PhilippMatthes
changed the title
This comment was marked as spam.
This comment was marked as spam.
PhilippMatthes
force-pushed
the
cortex-shim-handlers
branch
from
960bfdc to
af9841c
Compare
PhilippMatthes
changed the title
PhilippMatthes
requested review from
SoWieMarkus,
auhlig,
juliusclausnitzer,
mblos and
umswmayj
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
Member
Author
|
^ Ignored the comments already discussed in #691 or related to the kubebuilder scaffold. |
PhilippMatthes
changed the title
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/shim/main.go`:
- Around line 65-66: The code advertises metrics-bind-address via the
metricsAddr flag while the shim API is hardcoded to ":8080", causing port
collisions; add a new flag (e.g., apiBindAddr) and replace the hardcoded ":8080"
used when starting the shim API server with that variable (use flag.StringVar to
declare apiBindAddr), then in main validate that if metricsAddr != "0" and
metricsAddr == apiBindAddr you call processLogger.Fatal / log.Fatalf to reject
overlapping addresses (or adjust one of the addresses), ensuring the symbols to
change are metricsAddr/metrics-bind-address and the hardcoded ":8080" used to
start the shim API server.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b32b81a9-724c-4dd5-83c5-b17403fb4128
📒 Files selected for processing (2)
cmd/shim/main.gointernal/shim/placement/shim.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/shim/placement/shim.go
Contributor
Test Coverage ReportTest Coverage 📊: 69.1% |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
cmd/shim/main.go (2)
281-293:⚠️ Potential issue | 🟡 MinorHandle API server failures in the main goroutine.
This still calls
os.Exit(1)from a goroutine, so deferred cleanup inmainis skipped and shutdown is not coordinated withmgr.Start(ctx). Bubble the API server error back to the main goroutine via the channel or anerrgroup, then cancel/return there instead.#!/bin/bash # Show the API server error path and confirm os.Exit still happens inside a goroutine. rg -n -C3 'errchan|os\.Exit\(1\)|ListenAndServeContext' cmd/shim/main.go🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/shim/main.go` around lines 281 - 293, The API server goroutine currently calls os.Exit(1) inside a goroutine which skips main's deferred cleanup and mgr.Start(ctx) shutdown coordination; change the pattern so the goroutine running httpext.ListenAndServeContext sends any error to a channel (errchan) and does NOT call os.Exit itself, then in main select or receive on errchan (or use an errgroup) to log the error via setupLog, cancel the root context (or call the main cancel function) to coordinate shutdown with mgr.Start(ctx), and finally return/exit from main with a non-zero status; update the goroutine that calls httpext.ListenAndServeContext and the main error-handling receiver to implement this bubbling behavior.
66-67:⚠️ Potential issue | 🟡 MinorFix the metrics flag help text.
The help string still recommends
:8080for HTTP metrics, but the new default--api-bind-address=:8080makes that example invalid and immediately trips the overlap check on Line 100. Please remove the:8080example or say explicitly that the two bind addresses must differ.♻️ Proposed fix
flag.StringVar(&metricsAddr, "metrics-bind-address", "0", "The address the metrics endpoint binds to. "+ - "Use :8443 for HTTPS or :8080 for HTTP, or leave as 0 to disable the metrics service.") + "Use :8443 for HTTPS, or leave as 0 to disable the metrics service. Must differ from --api-bind-address.")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/shim/main.go` around lines 66 - 67, The metrics flag help text (the flag.StringVar call that sets metricsAddr) currently suggests ":8080" which conflicts with the default --api-bind-address; update the help string in cmd/shim/main.go for the metricsAddr flag to either remove the ":8080" example or explicitly state that the metrics bind address must be different from --api-bind-address, e.g. replace the example with a neutral suggestion (or add "must differ from --api-bind-address") so the documented example no longer triggers the overlap check in the startup validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/shim/main.go`:
- Around line 84-85: The placement shim flag is currently defaulting to false
which starts the API server with no routes (mux empty) causing 404s; update the
flag BoolVar for enablePlacementShim in cmd/shim/main.go to default to true
(i.e., flag.BoolVar(&enablePlacementShim, "placement-shim", true, ...)) or,
alternatively, add a guard before starting the API server so that if
enablePlacementShim is false you do not start the server at all; apply the same
change to the other flag declarations/uses of enablePlacementShim and the
corresponding server startup/route-registration logic (the mux registration code
and server start path) so they are consistent.
---
Duplicate comments:
In `@cmd/shim/main.go`:
- Around line 281-293: The API server goroutine currently calls os.Exit(1)
inside a goroutine which skips main's deferred cleanup and mgr.Start(ctx)
shutdown coordination; change the pattern so the goroutine running
httpext.ListenAndServeContext sends any error to a channel (errchan) and does
NOT call os.Exit itself, then in main select or receive on errchan (or use an
errgroup) to log the error via setupLog, cancel the root context (or call the
main cancel function) to coordinate shutdown with mgr.Start(ctx), and finally
return/exit from main with a non-zero status; update the goroutine that calls
httpext.ListenAndServeContext and the main error-handling receiver to implement
this bubbling behavior.
- Around line 66-67: The metrics flag help text (the flag.StringVar call that
sets metricsAddr) currently suggests ":8080" which conflicts with the default
--api-bind-address; update the help string in cmd/shim/main.go for the
metricsAddr flag to either remove the ":8080" example or explicitly state that
the metrics bind address must be different from --api-bind-address, e.g. replace
the example with a neutral suggestion (or add "must differ from
--api-bind-address") so the documented example no longer triggers the overlap
check in the startup validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9e18830-b1f4-407f-94be-55c18576dcbb
📒 Files selected for processing (2)
cmd/shim/main.gohelm/library/cortex-shim/values.yaml
✅ Files skipped from review due to trivial changes (1)
- helm/library/cortex-shim/values.yaml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In #691 we scaffolded the helm chart and entrypoint for the placement api shim. Now, we add a
type Shim structwhich carries all http handlers needed to serve the full placement api. This shim needs to talk in two directions: 1. to kubernetes using a multicluster controller-runtime client to access the hypervisor crd and 2. to real placement via a http client.To support (1), a multicluster controller-runtime client is now initialized wired with
type Shim structsuch that the hypervisor crd informer-cache gets initialized. The easiest way to integrate this with our multicluster setup is to implementtype Shim structas a controller. To implement (2), a http client is initialized with proper transport parameters to avoid foreseeable issues like hanging handlers, using timeouts and increasing the number of parallel connections. The connection is initialized usingconf.placementURLon controller startup and then tested on placement's/endpoint.Includes changes from unmerged #691 and needs rebase afterward.