Operator envs#1
Conversation
Reviewer's GuideThis PR introduces configurable timing parameters for the operator by parsing new environment variables and exposing them in deployment templates, refactors environment variable parsing logic for increased robustness, and unifies controller-runtime manager options setup using the common operator library with standardized imports. Sequence Diagram: Operator Manager Configuration with lib-commonsequenceDiagram
participant main as "main() (operator/main.go or main.go)"
participant libCommonOp as "lib-common/operator.SetManagerOptions"
participant ctrlOptions as "ctrl.Options"
participant ctrlMgr as "ctrl.NewManager"
main->>main: Initialize ctrl.Options (options)
note left of main: SetupEnv() has already parsed env vars like LEASE_DURATION into global variables
main->>libCommonOp: SetManagerOptions(&options, setupLog)
libCommonOp->>ctrlOptions: Configure leader election (LeaseDuration, RenewDeadline, RetryPeriod, etc.)
note right of libCommonOp: Uses values from env vars (read by SetupEnv()) either directly or indirectly if SetManagerOptions accesses them or they are passed through options struct setup before this call.
main->>ctrlMgr: NewManager(config, options)
Sequence Diagram: Refactored Environment Variable Parsing in SetupEnvsequenceDiagram
participant osEnviron as "os.Environ()"
participant SetupEnv as "SetupEnv()"
participant GlobalConfigs as "Global Config Variables"
SetupEnv->>osEnviron: Get all environment variables
loop For each environment variable
SetupEnv->>SetupEnv: Parse key & value (using strings.SplitN)
alt key is "LEASE_DURATION"
SetupEnv->>GlobalConfigs: operatorLeaseDuration = value
else key is "RENEW_DEADLINE"
SetupEnv->>GlobalConfigs: operatorRenewDeadline = value
else key is "RETRY_PERIOD"
SetupEnv->>GlobalConfigs: operatorRetryPeriod = value
else key matches "*_OPERATOR_MANAGER_IMAGE_URL"
SetupEnv->>GlobalConfigs: envRelatedOperatorImages[operatorName] = value
else key matches "RELATED_IMAGE_*"
SetupEnv->>GlobalConfigs: envRelatedOpenStackServiceImages[key] = value
else Other specific keys (KUBE_RBAC_PROXY, etc.)
SetupEnv->>GlobalConfigs: (assigns to respective global var)
end
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @stuggi - I've reviewed your changes - here's some feedback:
- Provide in-code default values for LEASE_DURATION, RENEW_DEADLINE, and RETRY_PERIOD to mirror your template defaults and avoid running with empty settings when those env vars aren’t set.
- Simplify SetupEnv by using os.LookupEnv (or a map of expected env‐vars) instead of iterating over all os.Environ and splitting each entry, to make the parsing clearer and more efficient.
- Since you renamed the CRD field frrConfigurationNamespace to nodeName, consider adding a conversion webhook or migration logic to handle existing instances that still use the old field name.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Hey @stuggi - I've reviewed your changes - here's some feedback:
- In SetupEnv you’re taking the address of the reused loop variable
value, so every map entry will point to the same memory—capture a new local variable (e.g.val := value) per case before storing its pointer. - The CRD change renaming
frrConfigurationNamespacetonodeNameis a breaking change—consider bumping the CRD version or adding conversion logic to preserve compatibility.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @stuggi - I've reviewed your changes - here's some feedback:
- In SetupEnv, use strings.SplitN(env, "=", 2) instead of strings.Split so you correctly capture values containing “=”.
- Add validation or fallback defaults for LEASE_DURATION, RENEW_DEADLINE, and RETRY_PERIOD to avoid starting the operator with empty or invalid timing values.
- The manager options initialization is duplicated in cmd/operator/main.go and main.go—consider extracting it into a shared helper to reduce duplication.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - name: LEASE_DURATION | ||
| value: "137" | ||
| - name: RENEW_DEADLINE | ||
| value: "107" | ||
| - name: RETRY_PERIOD |
There was a problem hiding this comment.
suggestion: Hardcoded defaults may drift without context
Consider centralizing the default values (137, 107, 26) or adding a comment explaining their choice to avoid them drifting out of sync.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @stuggi - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Changes: 1. Controller architecture: Two controllers (Backup and Restore) - BackupController handles both ControlPlane/DataPlane backups - RestoreController handles both ControlPlane/DataPlane restores - Different responsibilities justify separation 2. PVC management: BackupController creates PVC - Removes user prerequisite to pre-create PVC - Controller ensures PVC exists with correct labels - User can optionally specify size and storageClass 3. Fixed operator location references - Changed "infra-operator" to "openstack-operator" - Consistent with Decision #1 4. Removed retentionPolicy from CRD specs - OADP TTL handles retention (per Decision openstack-k8s-operators#3) - Added oadp.ttl and oadp.snapshotMoveData to specs 5. Updated "Separation of Concerns" to "Related Components" - Reflects two-controller design - Clarifies relationship with GaleraBackup, test-operator 6. Updated implementation plan Phase 3 - Changed "retention policy" to "OADP TTL configuration" 7. Updated discussion topics to open implementation questions - Removed answered questions - Added relevant implementation details Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes: 1. Controller architecture: Two controllers (Backup and Restore) - BackupController handles both ControlPlane/DataPlane backups - RestoreController handles both ControlPlane/DataPlane restores - Different responsibilities justify separation 2. PVC management: BackupController creates PVC - Removes user prerequisite to pre-create PVC - Controller ensures PVC exists with correct labels - User can optionally specify size and storageClass 3. Fixed operator location references - Changed "infra-operator" to "openstack-operator" - Consistent with Decision #1 4. Removed retentionPolicy from CRD specs - OADP TTL handles retention (per Decision openstack-k8s-operators#3) - Added oadp.ttl and oadp.snapshotMoveData to specs 5. Updated "Separation of Concerns" to "Related Components" - Reflects two-controller design - Clarifies relationship with GaleraBackup, test-operator 6. Updated implementation plan Phase 3 - Changed "retention policy" to "OADP TTL configuration" 7. Updated discussion topics to open implementation questions - Removed answered questions - Added relevant implementation details Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolves compatibility topic #1 (Label Naming Convention). Changes: 1. Added comprehensive PVC Labeling Strategy section explaining: - openstack.org/backup: "true" - Include PVC in backup snapshot - openstack.org/backup-restore: "true" - Include PVC in restore - Three scenarios: backup+restore, backup-only, skip-backup 2. Clarified backup approach: - All CRs/Secrets/ConfigMaps backed up without label selector - PVCs selectively backed up using openstack.org/backup label - Individual PVCs can be excluded via backup.velero.io/backup-volumes annotation 3. Updated examples to show label selectors for PVC backup/restore 4. Removed PVC Labeling from Open Questions (resolved) Decision: Most PVCs will have BOTH labels, but backup-only label enables backup-without-restore scenarios (logs, audit data, test data). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes: 1. Controller architecture: Two controllers (Backup and Restore) - BackupController handles both ControlPlane/DataPlane backups - RestoreController handles both ControlPlane/DataPlane restores - Different responsibilities justify separation 2. PVC management: BackupController creates PVC - Removes user prerequisite to pre-create PVC - Controller ensures PVC exists with correct labels - User can optionally specify size and storageClass 3. Fixed operator location references - Changed "infra-operator" to "openstack-operator" - Consistent with Decision #1 4. Removed retentionPolicy from CRD specs - OADP TTL handles retention (per Decision openstack-k8s-operators#3) - Added oadp.ttl and oadp.snapshotMoveData to specs 5. Updated "Separation of Concerns" to "Related Components" - Reflects two-controller design - Clarifies relationship with GaleraBackup, test-operator 6. Updated implementation plan Phase 3 - Changed "retention policy" to "OADP TTL configuration" 7. Updated discussion topics to open implementation questions - Removed answered questions - Added relevant implementation details Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolves compatibility topic #1 (Label Naming Convention). Changes: 1. Added comprehensive PVC Labeling Strategy section explaining: - openstack.org/backup: "true" - Include PVC in backup snapshot - openstack.org/backup-restore: "true" - Include PVC in restore - Three scenarios: backup+restore, backup-only, skip-backup 2. Clarified backup approach: - All CRs/Secrets/ConfigMaps backed up without label selector - PVCs selectively backed up using openstack.org/backup label - Individual PVCs can be excluded via backup.velero.io/backup-volumes annotation 3. Updated examples to show label selectors for PVC backup/restore 4. Removed PVC Labeling from Open Questions (resolved) Decision: Most PVCs will have BOTH labels, but backup-only label enables backup-without-restore scenarios (logs, audit data, test data). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Changes: 1. Controller architecture: Two controllers (Backup and Restore) - BackupController handles both ControlPlane/DataPlane backups - RestoreController handles both ControlPlane/DataPlane restores - Different responsibilities justify separation 2. PVC management: BackupController creates PVC - Removes user prerequisite to pre-create PVC - Controller ensures PVC exists with correct labels - User can optionally specify size and storageClass 3. Fixed operator location references - Changed "infra-operator" to "openstack-operator" - Consistent with Decision #1 4. Removed retentionPolicy from CRD specs - OADP TTL handles retention (per Decision openstack-k8s-operators#3) - Added oadp.ttl and oadp.snapshotMoveData to specs 5. Updated "Separation of Concerns" to "Related Components" - Reflects two-controller design - Clarifies relationship with GaleraBackup, test-operator 6. Updated implementation plan Phase 3 - Changed "retention policy" to "OADP TTL configuration" 7. Updated discussion topics to open implementation questions - Removed answered questions - Added relevant implementation details Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Resolves compatibility topic #1 (Label Naming Convention). Changes: 1. Added comprehensive PVC Labeling Strategy section explaining: - openstack.org/backup: "true" - Include PVC in backup snapshot - openstack.org/backup-restore: "true" - Include PVC in restore - Three scenarios: backup+restore, backup-only, skip-backup 2. Clarified backup approach: - All CRs/Secrets/ConfigMaps backed up without label selector - PVCs selectively backed up using openstack.org/backup label - Individual PVCs can be excluded via backup.velero.io/backup-volumes annotation 3. Updated examples to show label selectors for PVC backup/restore 4. Removed PVC Labeling from Open Questions (resolved) Decision: Most PVCs will have BOTH labels, but backup-only label enables backup-without-restore scenarios (logs, audit data, test data). Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Summary by Sourcery
Introduce configurable leader election parameters for the operator by wiring new environment variables through the setup and deployment, and refactor application startup to leverage lib-common for manager options.
New Features:
Enhancements:
Deployment: