[controller] Implement rvr-status-config-node-id-controller #341
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Implement controller for assigning unique nodeId (0-7) to RVR replicas - Add request types and reconciler with conflict retry handling - Add comprehensive unit tests covering all scenarios - Add SPEC_COMPLIANCE documentation - Register controller in registry
- Use standard reconcile.Reconciler and .For() - Use structured logging - Remove custom Request types - Add CONTROLLER_STYLE_GUIDE.md
asergunov
reviewed
Nov 27, 2025
- Remove unused fields from Reconciler struct (rdr, sch) - Export Reconciler fields (Cl, Log, LogAlt) for testability - Remove NewReconciler constructor, create struct directly in BuildController - Update all field usages to exported fields (r.cl -> r.Cl, r.logAlt -> r.LogAlt) - Update tests to use exported fields directly - Remove unused runtime import Follows updated CONTROLLER_STYLE_GUIDE.md standards for simplified controller structure.
…d-controller - Replace standard testing package with Ginkgo/Gomega framework - Refactor all 9 tests to use Describe/It/BeforeEach pattern - Use GinkgoLogr for integrated logging with Ginkgo - Update SPEC_COMPLIANCE.md to reflect Ginkgo testing approach - Add ginkgo/v2 and gomega dependencies to go.mod
asergunov
requested changes
Nov 29, 2025
Comment on lines
+26
to
+51
| WithEventFilter(predicate.Funcs{ | ||
| CreateFunc: func(ce event.CreateEvent) bool { | ||
| rvr, ok := ce.Object.(*v1alpha3.ReplicatedVolumeReplica) | ||
| if !ok { | ||
| return false | ||
| } | ||
| // Trigger only if nodeID is not set | ||
| return rvr.Status == nil || rvr.Status.Config == nil || rvr.Status.Config.NodeId == nil | ||
| }, | ||
| UpdateFunc: func(_ event.UpdateEvent) bool { | ||
| // No-op: nodeID is immutable once set, so we only care about CREATE | ||
| return false | ||
| }, | ||
| DeleteFunc: func(_ event.DeleteEvent) bool { | ||
| // No-op: deletion doesn't require nodeID assignment | ||
| return false | ||
| }, | ||
| GenericFunc: func(ge event.GenericEvent) bool { | ||
| rvr, ok := ge.Object.(*v1alpha3.ReplicatedVolumeReplica) | ||
| if !ok { | ||
| return false | ||
| } | ||
| // Trigger only if nodeID is not set (for reconciliation on startup) | ||
| return rvr.Status == nil || rvr.Status.Config == nil || rvr.Status.Config.NodeId == nil | ||
| }, | ||
| }). |
| LogAlt logr.Logger | ||
| } | ||
|
|
||
| var _ reconcile.Reconciler = &Reconciler{} |
Contributor
There was a problem hiding this comment.
Suggested change
| var _ reconcile.Reconciler = &Reconciler{} | |
| var _ reconcile.Reconciler = (*Reconciler)(nil) |
Comment on lines
+21
to
+23
| Cl client.Client | ||
| Log *slog.Logger | ||
| LogAlt logr.Logger |
Comment on lines
+57
to
+64
| // Filter by replicatedVolumeName | ||
| filteredRVRs := make([]v1alpha3.ReplicatedVolumeReplica, 0) | ||
| for _, item := range rvrList.Items { | ||
| if item.Spec.ReplicatedVolumeName == rvr.Spec.ReplicatedVolumeName { | ||
| filteredRVRs = append(filteredRVRs, item) | ||
| } | ||
| } | ||
| rvrList.Items = filteredRVRs |
| for _, item := range rvrList.Items { | ||
| if item.Status != nil && item.Status.Config != nil && item.Status.Config.NodeId != nil { | ||
| nodeID := *item.Status.Config.NodeId | ||
| if nodeID >= minNodeID && nodeID <= maxNodeID { |
Contributor
There was a problem hiding this comment.
А в проверке выше это не проверялось. Это точно нужно?
| rvrList.Items = filteredRVRs | ||
|
|
||
| // Collect used nodeIDs | ||
| usedNodeIDs := make(map[uint]bool) |
Contributor
There was a problem hiding this comment.
Вместо bool тут правильнее использовать пустую структуру
Comment on lines
+80
to
+92
| // NOTE: Setting status condition is NOT in the spec. | ||
| // This was added to improve observability - administrators can see the problem | ||
| // in RVR status conditions instead of only in controller logs. | ||
| // To revert: remove the setNodeIDErrorCondition call and the function definition. | ||
| // The spec only requires returning an error, which we do below. | ||
| if err := r.setNodeIDErrorCondition(ctx, rvr, fmt.Sprintf( | ||
| "too many replicas for volume %s: %d (maximum is %d)", | ||
| rvr.Spec.ReplicatedVolumeName, | ||
| totalReplicas, | ||
| maxNodeID+1, | ||
| )); err != nil { | ||
| log.Info("failed to set error condition", "err", err) | ||
| } |
Contributor
There was a problem hiding this comment.
Надо прояснить этот момент и сделать как надо
| return reconcile.Result{}, fmt.Errorf("getting RVR for patch: %w", err) | ||
| } | ||
| if err := api.PatchStatusWithConflictRetry(ctx, r.Cl, freshRVR, func(currentRVR *v1alpha3.ReplicatedVolumeReplica) error { | ||
| // Check again if nodeID is already set (handles race condition where another worker set it during retry) |
Contributor
There was a problem hiding this comment.
Этого не может произойти. Одновременно два одинаковых объекта не рекосайлятся
Comment on lines
+35
to
+45
| func createRVR(name, volumeName, nodeName string) *v1alpha3.ReplicatedVolumeReplica { | ||
| return &v1alpha3.ReplicatedVolumeReplica{ | ||
| ObjectMeta: metav1.ObjectMeta{ | ||
| Name: name, | ||
| }, | ||
| Spec: v1alpha3.ReplicatedVolumeReplicaSpec{ | ||
| ReplicatedVolumeName: volumeName, | ||
| NodeName: nodeName, | ||
| }, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
То что внутри этой функции читаемее сигнатуры. Давай заинлайним
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Implements the
rvr-status-config-node-id-controlleras specified indocs/dev/spec_v1alpha3.md. The controller automatically assigns uniquenodeIdvalues (0-7) toReplicatedVolumeReplicaresources within the sameReplicatedVolume.Changes:
images/controller/internal/controllers/rvr_status_config_node_id/registry.godocs/dev/controllers/rvr_status_config_node_id/SPEC_COMPLIANCE.mdNo critical components affected: Only writes to RVR status subresource. No restarts or changes to existing components.
Why do we need it, and what problem does it solve?
DRBD requires each replica to have a unique
nodeId(0-7) for cluster identification. Without this controller,nodeIdassignment would be manual, error-prone, and not scalable.This controller automates the process, ensuring uniqueness, handling concurrent assignments safely, and providing error conditions when assignment fails (e.g., too many replicas).
Related spec:
docs/dev/spec_v1alpha3.md-rvr-status-config-node-id-controller[OK | priority: 5 | complexity: 2]What is the expected result?
After applying:
nodeIdinstatus.config.nodeId(range 0-7)ReplicatedVolumegets a uniquenodeIdConfigurationAdjusted=Falseif >8 replicas or all nodeIds usednodeIdHow to verify:
ReplicatedVolumeReplicawithoutnodeId→ check thatstatus.config.nodeIdis assignedMUST NOT change:
nodeIdremain unchangedChecklist