Feature/ha service self enable and disable#308
Conversation
|
Warning Review limit reached
More reviews will be available in 42 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThe PR removes the internal ChangesExternalize HA service management
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PaulPickhardt
force-pushed
the
feature/ha-service-self-enable-and-disable
branch
from
7b2f4f8 to
1663db8
Compare
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/controller/hypervisor_maintenance_controller.go (1)
117-135:⚠️ Potential issue | 🟠 Major | ⚡ Quick winLine 123 early-return can skip clearing
forced_downinMaintenanceUnset.If
ConditionTypeHypervisorDisabledis already false, reconciliation returns before the Nova update call, soforced_down=falsemay never be sent.Proposed fix
- if !meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ + _ = meta.SetStatusCondition(&hv.Status.Conditions, metav1.Condition{ Type: kvmv1.ConditionTypeHypervisorDisabled, Status: metav1.ConditionFalse, Message: "Hypervisor is enabled", Reason: kvmv1.ConditionReasonSucceeded, - }) { - // Spec matches status - return nil - } + })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/hypervisor_maintenance_controller.go` around lines 117 - 135, The current early-return when SetStatusCondition(&hv.Status.Conditions, metav1.Condition{Type: kvmv1.ConditionTypeHypervisorDisabled, Status: metav1.ConditionFalse, ...}) returns false causes the Nova Update (services.Update using enableService) to be skipped and thus forced_down may never be cleared; in MaintenanceUnset, remove or change the early return so the enableService update (services.Update(ctx, hec.computeClient, serviceId, enableService).Extract()) always runs even if the status condition was already false — i.e., ensure the call that sets Status: services.ServiceEnabled and ForcedDown: &falseVal executes unconditionally (or move it below the condition block) while still avoiding duplicate status writes to hv.Status.Conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/openstack-hypervisor-operator/templates/ha-service-rbac.yaml`:
- Line 4: The ClusterRole name using the Helm template include
"openstack-hypervisor-operator.fullname" (the value that produces "<include
...>-ha-service-role") must be quoted to avoid YAML parse/lint errors; update
the metadata name in ha-service-rbac.yaml (the resource that sets name: {{
include "openstack-hypervisor-operator.fullname" . }}-ha-service-role) to wrap
the entire templated expression in quotes so linters see a valid YAML string.
---
Outside diff comments:
In `@internal/controller/hypervisor_maintenance_controller.go`:
- Around line 117-135: The current early-return when
SetStatusCondition(&hv.Status.Conditions, metav1.Condition{Type:
kvmv1.ConditionTypeHypervisorDisabled, Status: metav1.ConditionFalse, ...})
returns false causes the Nova Update (services.Update using enableService) to be
skipped and thus forced_down may never be cleared; in MaintenanceUnset, remove
or change the early return so the enableService update (services.Update(ctx,
hec.computeClient, serviceId, enableService).Extract()) always runs even if the
status condition was already false — i.e., ensure the call that sets Status:
services.ServiceEnabled and ForcedDown: &falseVal executes unconditionally (or
move it below the condition block) while still avoiding duplicate status writes
to hv.Status.Conditions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8f6e7348-4b8c-4ef3-a17e-4547b9c8d9b1
📒 Files selected for processing (7)
charts/openstack-hypervisor-operator/templates/ha-service-rbac.yamlcmd/main.gointernal/controller/hypervisor_instance_ha_controller.gointernal/controller/hypervisor_instance_ha_controller_test.gointernal/controller/hypervisor_maintenance_controller.gointernal/controller/hypervisor_maintenance_controller_test.gointernal/controller/utils.go
💤 Files with no reviewable changes (4)
- internal/controller/hypervisor_instance_ha_controller_test.go
- cmd/main.go
- internal/controller/hypervisor_instance_ha_controller.go
- internal/controller/utils.go
| apiVersion: rbac.authorization.k8s.io/v1 | ||
| kind: ClusterRole | ||
| metadata: | ||
| name: {{ include "openstack-hypervisor-operator.fullname" . }}-ha-service-role |
There was a problem hiding this comment.
Quote the templated ClusterRole name to avoid YAML parse/lint failure.
Line 4 is parsed as invalid YAML by linters before Helm rendering.
Proposed fix
- name: {{ include "openstack-hypervisor-operator.fullname" . }}-ha-service-role
+ name: '{{ include "openstack-hypervisor-operator.fullname" . }}-ha-service-role'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: {{ include "openstack-hypervisor-operator.fullname" . }}-ha-service-role | |
| name: '{{ include "openstack-hypervisor-operator.fullname" . }}-ha-service-role' |
🧰 Tools
🪛 YAMLlint (1.38.0)
[error] 4-4: syntax error: expected , but found ''
(syntax)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/openstack-hypervisor-operator/templates/ha-service-rbac.yaml` at line
4, The ClusterRole name using the Helm template include
"openstack-hypervisor-operator.fullname" (the value that produces "<include
...>-ha-service-role") must be quoted to avoid YAML parse/lint errors; update
the metadata name in ha-service-rbac.yaml (the resource that sets name: {{
include "openstack-hypervisor-operator.fullname" . }}-ha-service-role) to wrap
the entire templated expression in quotes so linters see a valid YAML string.
PaulPickhardt
force-pushed
the
feature/ha-service-self-enable-and-disable
branch
2 times, most recently
from
74d7082 to
84153a4
Compare
The kvm-ha-service now owns the HA lifecycle for hypervisors,
including setting spec.maintenance=ha and the ConditionTypeHaEnabled
and setting ConditionTypeHypervisorDisabled status condition after
the compute service got disabled.
Changes:
- Delete HypervisorInstanceHaController and its tests
- Remove enableInstanceHA, disableInstanceHA, updateInstanceHA,
InstanceHaUrl from utils.go
- Exclude MaintenanceHA from the maintenance controller's compute
service disable logic, as the ha-service handles that case
When maintenance: ha is removed, the kvm-ha-service have set forced_down=true on the nova-compute service. The operator's MaintenanceUnset path previously only re-enabled the service status but did not clear forced_down, leaving the compute service unable to accept workloads despite being marked enabled.
PaulPickhardt
force-pushed
the
feature/ha-service-self-enable-and-disable
branch
from
84153a4 to
86e5849
Compare
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
Merging this branch will decrease overall coverage
Coverage by fileChanged files (no unit tests)
Please note that the "Total", "Covered", and "Missed" counts above refer to code statements instead of lines of code. The value in brackets refers to the test coverage of that file in the old version of the code. Changed unit test files
|
4 participants
Summary by CodeRabbit
Release Notes