fix: orb-agent docker image restart #271
Conversation
|
@codex review |
|
Go test coverage
Total coverage: 60.1% |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6271f330f5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This pull request introduces reliability improvements to the orb-agent, focusing on Docker container restart handling, backend timeout adjustments, and temporary file cleanup. The changes aim to make the agent more resilient to restarts and reduce downtime during container lifecycle events.
Changes:
- Increased
readinessBackofftimeout from 10 to 12 seconds across all backend modules to improve startup reliability - Enhanced pktvisor backend to clean up temporary configuration files during shutdown and reconfiguration
- Improved agent entry script to handle stale PID files gracefully instead of exiting the container
- Added SO_REUSEADDR socket option to OTLP bridge server for faster port reuse after restarts
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| agent/otlpbridge/server.go | Implements SO_REUSEADDR using ListenConfig to enable faster port reuse after agent restarts |
| agent/docker/orb-agent-entry.sh | Refactored PID file handling logic to detect and clean up stale PID files instead of exiting |
| agent/backend/worker/worker.go | Increased readinessBackoff timeout from 10 to 12 seconds |
| agent/backend/snmpdiscovery/snmp_discovery.go | Increased readinessBackoff timeout from 10 to 12 seconds |
| agent/backend/pktvisor/pktvisor.go | Added temp file cleanup in Stop() and Configure(), increased readinessBackoff to 12 seconds |
| agent/backend/opentelemetryinfinity/opentelemetry_infinity.go | Increased readinessBackoff timeout from 10 to 12 seconds |
| agent/backend/networkdiscovery/network_discovery.go | Increased readinessBackoff timeout from 10 to 12 seconds |
| agent/backend/devicediscovery/device_discovery.go | Increased readinessBackoff timeout from 10 to 12 seconds |
Comments suppressed due to low confidence (1)
agent/backend/pktvisor/pktvisor.go:461
- After Stop() deletes the temporary config file, FullReset() calls Start() which will attempt to use the now-deleted config file path (line 118 in Start method uses p.configFile). This will likely cause pktvisord to fail to start because it's passed a non-existent config file path via the --config flag. Consider either: 1) not deleting p.configFile in Stop() (just let it be cleaned up on reconfiguration), or 2) clearing p.configFile in Stop() so Start() doesn't try to use a deleted file, or 3) having FullReset() call Configure() to create a new temp file before calling Start().
// Note: Stop() already cleaned up temp file, but kept path for restart
// for each policy, restart the scraper
backendCtx, cancelFunc := context.WithCancel(context.WithValue(ctx, config.ContextKey("routine"), "pktvisor"))
// start it
if err := p.Start(backendCtx, cancelFunc); err != nil {
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f27555c387
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7c9f4cf894
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
leoparente
requested review from
MicahParks,
grant-nbl,
jajeffries,
manrodrigues,
mfiedorowicz and
paulstuart
as code owners
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 395db7e49b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
agent/backend/pktvisor/pktvisor.go:354
- The temporary file created by os.CreateTemp is never closed before being written to with os.WriteFile. While os.WriteFile handles opening the file, the file descriptor from CreateTemp should be closed to avoid resource leaks. Add tmpFile.Close() after creating the file and before writing to it with os.WriteFile, or use defer tmpFile.Close() immediately after successful creation.
tmpFile, err := os.CreateTemp(tmpDir, "pktvisor-*.yaml")
if err != nil {
return fmt.Errorf("failed to create pktvisor temp config file: %w", err)
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Codex Review: Didn't find any major issues. Delightful! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
This pull request introduces several improvements focused on container signal handling, resource cleanup, and platform-specific socket configuration for faster and more reliable agent restarts. The most significant changes are grouped below:
Container entrypoint and signal handling improvements:
tinias the entrypoint, ensuring proper signal forwarding and reaping of zombie processes. Theorb-agent-entry.shscript was simplified to directlyexecthe agent binary, removing custom signal traps and therun-agent.shhelper script. This makes shutdown and restart behavior more robust in container environments. [1] [2] [3] [4] [5]Platform-specific socket options for fast port reuse:
SO_REUSEADDRenabled to allow fast port reuse (important for Docker restarts); on Windows, standard socket creation is used. This is implemented via newlistenhelper functions insocket_unix.goandsocket_windows.go, and integrated into the server startup. [1] [2] [3]Resource and process cleanup:
Dependency updates:
golang.org/x/sysdependency is now a direct requirement ingo.modto support the new socket configuration code. [1] [2]Minor code cleanup: