-
Notifications
You must be signed in to change notification settings - Fork 31
Add runtime log level management API with automatic restoration #2897
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
bbockelm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot - please start addressing the requested changes.
cmd/logging.go
Outdated
|
|
||
| func init() { | ||
| // Add the server URL as a persistent flag to all subcommands of loggingCmd | ||
| loggingCmd.PersistentFlags().StringVarP(&loggingServerURLStr, "server", "s", "", "Web URL of the Pelican server (e.g. https://my-origin.com:8447)") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take this information from the config file if not specified by the CLI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a567fdd. Server URL now defaults to Server.ExternalWebUrl from config if --server flag is not provided.
cmd/logging_set_level.go
Outdated
| Valid log levels: debug, info, warn, error, fatal, panic | ||
| Examples: | ||
| pelican logging set-level debug 300 -s https://my-origin.com:8447 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having a top-level command for just the logging seems overkill. Maybe have a server command and set-logging-level can be the sub-command?
@patrickbrophy - for the API token work you're considering, we should align the "top level" command. Manipulating API tokens and manipulating server log levels seems to have some parallel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in fc53ca0. Restructured to pelican server set-logging-level command structure. Created new server command as parent for server management operations.
cmd/logging_set_level.go
Outdated
| } | ||
| req.Header.Set("Content-Type", "application/json") | ||
| req.Header.Set("Authorization", "Bearer "+tok) | ||
| req.AddCookie(&http.Cookie{Name: "login", Value: tok}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cookie shouldn't be needed if the Authorization is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a567fdd. Removed cookie - Authorization header is sufficient.
cmd/logging_set_level.go
Outdated
| } | ||
|
|
||
| // Parse response | ||
| var response map[string]interface{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a structured response, don't just print out JSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a567fdd. Now outputs structured response:
Log level successfully changed to 'debug' for 300 seconds
Change ID: 550e8400-e29b-41d4-a716-446655440000
Will revert at: 2025-12-19T19:15:00Z
cmd/logging_set_level.go
Outdated
| return targetURL, nil | ||
| } | ||
|
|
||
| func trimSuffix(s, suffix string) string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a standard library equivalent, use that. This seems fairly basic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in a567fdd. Now using strings.TrimSuffix from standard library.
docs/scopes.yaml
Outdated
| acceptedBy: ["*"] | ||
| --- | ||
| ############################ | ||
| # Pelican Scopes # |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Call this "Logging Management Scopes"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a567fdd. Section renamed to "Logging Management Scopes".
logging/level_manager.go
Outdated
| type ( | ||
| // LogLevelChange represents a temporary change to a log level | ||
| LogLevelChange struct { | ||
| Level log.Level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Log level changes should not be only the global level but also the sub-levels, including those for XRootD.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Acknowledged. The current implementation supports global log level changes. Support for module-specific levels including XRootD will require additional design work to:
- Identify and integrate with XRootD's logging system
- Extend LogLevelChange structure for module-specific configurations
- Modify applyChanges() to handle per-module level settings
- Update API to accept module names
This is reserved for a follow-up PR given the scope. The ModuleName field is already present in the data structures as a placeholder for this future enhancement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the original draft was done, we added the ability for xrootd to restart. This is now plausible.
logging/level_manager.go
Outdated
| setLevelFunc: setFunc, | ||
| getLevelFunc: getFunc, | ||
| } | ||
| go globalManager.manageLogLevels() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should take a context.Context and an errgroup.ErrorGroup to allow for a clean shutdown where all background goroutines are stopped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in a567fdd. LogLevelManager now accepts context.Context and errgroup.Group, with nil-safe fallbacks for backward compatibility.
bbockelm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only got partly through the file. Here's my first piece of input thoguh.
cmd/logging_set_level.go
Outdated
| // Well-known locations for the address file | ||
| // These should match the locations where the server drops the address file | ||
| possiblePaths := []string{ | ||
| "/run/pelican/server-web-url", // systemd standard location |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relevant logic is in config/address_file.go. The name of the file is pelican.addresses.
cmd/logging_set_level.go
Outdated
| possiblePaths := []string{ | ||
| "/run/pelican/server-web-url", // systemd standard location | ||
| "/var/run/pelican/server-web-url", // alternative systemd location | ||
| filepath.Join(os.TempDir(), "pelican", "server-web-url"), // temp directory |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this make sense? You're looking for a common directory that the daemon is using; generating a temporary directory won't help find the file.
cmd/logging_set_level.go
Outdated
| } | ||
|
|
||
| // Also check in the config directory if available | ||
| configDir := viper.GetString("ConfigDir") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use viper directly. It'd be the runtime directory anyway.
cmd/logging_set_level.go
Outdated
| // Search for the address file | ||
| for _, path := range possiblePaths { | ||
| if data, err := os.ReadFile(path); err == nil { | ||
| urlStr := strings.TrimSpace(string(data)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not the right format. Refer to config/address_file.go.
logging/level_manager.go
Outdated
| type ( | ||
| // LogLevelChange represents a temporary change to a log level | ||
| LogLevelChange struct { | ||
| Level log.Level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the original draft was done, we added the ability for xrootd to restart. This is now plausible.
web_ui/logging_api.go
Outdated
| } | ||
|
|
||
| // Module-specific log levels are not yet supported | ||
| if req.ModuleName != "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These will be added soon, I think.
5aedafa to
94ccfd5
Compare
c671897 to
56f6af5
Compare
- Added pelican.logging_modify token scope - Implemented LogLevelManager for tracking temporary log level changes - Added web API endpoints for setting, getting, and deleting log level changes - Created CLI command 'pelican logging set-level' for invoking the API - Includes background goroutine for automatic log level restoration
The test was experiencing a race condition where HTTP requests were made through the broker before the Director had processed the Origin's advertisement and registered the broker endpoint. Changes: - Add HasBrokerEndpoint() method to broker.BrokerDialer to check if an endpoint is registered - Add HasBrokerForAddr() function to director package to expose the check - Update TestBrokerApi to poll until the Director has registered the origin's broker endpoint before proceeding with the test This ensures the broker connection infrastructure is fully set up before the test makes requests, eliminating the race condition that caused sporadic TLS handshake timeout failures in CI (20% failure rate).
56f6af5 to
16ec843
Compare
|
Alright, this is all ready! |
Implements a web API and CLI to temporarily change server log levels at runtime without restart or config modification. Log levels automatically revert after a specified duration.
Implementation
Core Manager (
logging/level_manager.go)LogLevelManagertracks active temporary changes with expiration timescontext.Contextanderrgroup.Groupfor clean shutdownserver_utils.ResetTestState()for proper test cleanupWeb API (
/api/v1.0/logging/level)POST- Create temporary change (max 24h duration)GET- Query current status and active changesDELETE /:changeId- Cancel change earlyAuthHandler/AdminAuthHandlerswagger/pelican-swagger.yamlCLI Command
servercommand for server management operations--serverflag if providedServer.ExternalWebUrlconfig if flag not provided0, automatically discovers actual server address from file/run/pelican/server-web-url,/var/run/pelican/server-web-url, temp directory, config directory--tokenflag for custom credentialsToken Scope
pelican.logging_modifyscope under "Logging Management Scopes" indocs/scopes.yamlExample Usage
CLI output:
API response shows current level and active changes:
{ "currentLevel": "debug", "baseLevel": "info", "activeChanges": [{ "changeId": "550e8400-e29b-41d4-a716-446655440000", "level": "debug", "endTime": "2025-12-19T19:15:00Z", "remainingSeconds": 245 }] }Notes
discoverLocalServerURL()for testability and clarityOriginal prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.