Embed an HTTP Server in ci-operator#4958
Embed an HTTP Server in ci-operator#4958danilo-gemoli wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
WalkthroughAdds an HTTP server lifecycle to the operator Run flow (startup, mux injection into Config, and graceful shutdown), changes Run to return an aggregated error slice, and updates configuration and podspec to expose/use the HTTP server IP environment variable constant. Changes
Sequence Diagram(s)sequenceDiagram
participant Run as "Operator Run"
participant HTTP as "HTTP Server\n(http.Server + ServeMux)"
participant Components as "Downstream Components\n(config consumers)"
participant Cleanup as "Cleanup/Shutdown"
Run->>HTTP: create ServeMux, construct http.Server with BaseContext(ctx)
Run->>Components: set cfg.HTTPServerMux = srvMux
Run->>HTTP: start server (goroutine) -> ListenAndServe on IP:Port
Components->>HTTP: register handlers on ServeMux
HTTP-->>Run: unexpected server error -> cancel main context / append to errs
Run->>Cleanup: defer server.Close(), aggregate close errors into errs
Run->>Run: execute initialization, graph, steps -> append errors to errs
Run->>Cleanup: return accumulated errs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
1 similar comment
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/ci-operator/main.go`:
- Around line 1076-1093: The code is checking the wrong error variable (errs)
after nodes.TopologicalSort() and calculateGraph(); change those checks to
inspect sortErrs and calculateGraphErrs respectively: after calling
nodes.TopologicalSort() check if sortErrs != nil, append a descriptive error
(e.g. results.ForReason("building_graph").ForError(errors.New("could not sort
nodes"))) and then append sortErrs into errs and return; similarly after
calculateGraph(stepList) check if calculateGraphErrs != nil, append
calculateGraphErrs into errs and return. Update the error-append logic around
stepList/sortErrs and graph/calculateGraphErrs so graph errors are not silently
ignored.
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/ci-operator/main.go`:
- Around line 1076-1093: The code is checking the wrong error variable (errs)
after TopologicalSort and calculateGraph, so sortErrs and calculateGraphErrs are
ignored; update the TopologicalSort handling to check if sortErrs != nil (append
results.ForReason("building_graph").ForError(errors.New("could not sort nodes"))
plus sortErrs to errs and return) and similarly check if calculateGraphErrs !=
nil (append calculateGraphErrs to errs and return) after calling
calculateGraph(stepList), referencing variables stepList, sortErrs,
calculateGraphErrs, nodes.TopologicalSort(), and calculateGraph() to locate
where to change.
danilo-gemoli
force-pushed
the
feat/ci-operator/embed-http-server
branch
from
a9eefa0 to
c552404
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/ci-operator/main.go (1)
988-996: Consider usingsrv.Shutdown()for graceful HTTP server shutdown.
srv.Close()immediately closes all active connections without waiting for in-flight requests to complete. If the lease proxy server (mentioned in PR objectives) has requests in progress, they will be abruptly terminated.Using
srv.Shutdown(ctx)with a timeout context would allow active requests to complete gracefully before closing.♻️ Suggested change for graceful shutdown
defer func() { logrus.Infof("Ran for %s", time.Since(start).Truncate(time.Second)) o.metricsAgent.Stop() if srv != nil { - if err := srv.Close(); err != nil { + shutdownCtx, shutdownCancel := context.WithTimeout(context.Background(), 5*time.Second) + defer shutdownCancel() + if err := srv.Shutdown(shutdownCtx); err != nil { errs = append(errs, fmt.Errorf("close http server: %w", err)) } } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ci-operator/main.go` around lines 988 - 996, The defer currently calls srv.Close() which force-closes active connections; replace this with a graceful shutdown using srv.Shutdown(ctx): create a timeout context (e.g., context.WithTimeout) before calling srv.Shutdown(ctx), call o.metricsAgent.Stop() after Shutdown completes, and append any Shutdown error to errs (similar to the existing Close error handling). Ensure you cancel the context and handle/format the error as fmt.Errorf("shutdown http server: %w", err) so in-flight requests are given time to finish.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cmd/ci-operator/main.go`:
- Around line 988-996: The defer currently calls srv.Close() which force-closes
active connections; replace this with a graceful shutdown using
srv.Shutdown(ctx): create a timeout context (e.g., context.WithTimeout) before
calling srv.Shutdown(ctx), call o.metricsAgent.Stop() after Shutdown completes,
and append any Shutdown error to errs (similar to the existing Close error
handling). Ensure you cancel the context and handle/format the error as
fmt.Errorf("shutdown http server: %w", err) so in-flight requests are given time
to finish.
|
@danilo-gemoli: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Run an HTTP in
ci-operatorthat will host routes for the lease proxy server, see #4877.Add no routes so far, but add the
http.ServeMuxintoConfigso that is can be used bystepLeaseProxyServer.Summary by CodeRabbit
New Features
Improvements