Skip to content

Commit 64b7ce9

Browse files
committed
nvmeof: refactor gateway connection pattern for ControllerModifyVolume
Extracted common gateway connection logic into `withGatewayConnection()` helper to avoid code duplication between QoS and host management operations (in the next commit). The helper manages: - Secret retrieval - NVMe-oF metadata extraction from volume - Gateway connection with automatic cleanup - Operation execution via callback function Refactored modifyNVMeoFQoS to use the new helper. Signed-off-by: gadi-didi <gadi.didi@ibm.com>
1 parent e6ca34a commit 64b7ce9

1 file changed

Lines changed: 48 additions & 31 deletions

File tree

internal/nvmeof/controller/controllerserver.go

Lines changed: 48 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -353,7 +353,9 @@ func (cs *Server) ControllerModifyVolume(
353353
return nil, status.Errorf(codes.InvalidArgument, "failed to parse QoS parameters: %v", err)
354354
}
355355
if nvmeofQoS != nil {
356-
return cs.modifyNVMeoFQoS(ctx, req, nvmeofQoS)
356+
if err := cs.modifyNVMeoFQoS(ctx, req, nvmeofQoS); err != nil {
357+
return nil, err
358+
}
357359
}
358360

359361
return &csi.ControllerModifyVolumeResponse{}, nil
@@ -568,34 +570,32 @@ func parseQoSParameters(params map[string]string) (*nvmeof.NVMeoFQosVolume, erro
568570
return qos, nil
569571
}
570572

571-
// modifyNVMeoFQoS handles NVMe-oF gateway QoS modification.
572-
func (cs *Server) modifyNVMeoFQoS(
573+
// withGatewayConnection is a helper that manages the common pattern of:
574+
// 1. Getting secrets (with fallback to k8s secret)
575+
// 2. Getting NVMe-oF metadata
576+
// 3. Connecting to gateway with proper cleanup
577+
// 4. Executing the provided operation function.
578+
func (cs *Server) withGatewayConnection(
573579
ctx context.Context,
574580
req *csi.ControllerModifyVolumeRequest,
575-
qos *nvmeof.NVMeoFQosVolume,
576-
) (*csi.ControllerModifyVolumeResponse, error) {
577-
volumeID := req.GetVolumeId()
578-
581+
volumeID string,
582+
fn func(context.Context, *nvmeof.GatewayRpcClient, *nvmeof.NVMeoFVolumeData) error,
583+
) error {
579584
// Step 1: Get secrets
580-
581-
// Since ControllerModifyVolume doesn't receive volume context and dont have option to take secrets
582-
// because there is no "csi.storage.k8s.io/controller-modify-secret-name" field in the SC !,
583-
// the full solution for it is to use GetControllerExpandSecretRef but there is no such function yet.
584-
// TODO: change the call to GetControllerExpandSecretRef once it is implemented.
585585
secrets := req.GetSecrets()
586586
if secrets == nil {
587587
secretName, secretNamespace, err := util.GetControllerPublishSecretRef(volumeID, util.RBDType)
588588
if err != nil {
589589
log.ErrorLog(ctx, "Failed to get secret reference: %v", err)
590590

591-
return nil, status.Errorf(codes.Internal, "failed to get secret reference: %v", err)
591+
return status.Errorf(codes.Internal, "failed to get secret reference: %v", err)
592592
}
593593

594594
secrets, err = k8s.GetSecret(secretName, secretNamespace)
595595
if err != nil {
596596
log.ErrorLog(ctx, "Failed to get secret from k8s: %v", err)
597597

598-
return nil, status.Errorf(codes.Internal, "failed to get secret: %v", err)
598+
return status.Errorf(codes.Internal, "failed to get secret: %v", err)
599599
}
600600
}
601601

@@ -604,7 +604,7 @@ func (cs *Server) modifyNVMeoFQoS(
604604
if err != nil {
605605
log.ErrorLog(ctx, "Failed to get NVMe-oF metadata: %v", err)
606606

607-
return nil, nvmeoferrors.ToGRPCError(err)
607+
return nvmeoferrors.ToGRPCError(err)
608608
}
609609

610610
// Step 3: Connect to gateway
@@ -616,35 +616,52 @@ func (cs *Server) modifyNVMeoFQoS(
616616
if err != nil {
617617
log.ErrorLog(ctx, "Gateway connection failed: %v", err)
618618

619-
return nil, status.Errorf(codes.Unavailable, "gateway connection failed: %v", err)
619+
return status.Errorf(codes.Unavailable, "gateway connection failed: %v", err)
620620
}
621621
defer func() {
622622
if closeErr := gateway.Destroy(); closeErr != nil {
623623
log.ErrorLog(ctx, "Failed to close gateway connection: %v", closeErr)
624624
}
625625
}()
626626

627-
// Step 4: Apply NVMe-oF QoS via gateway
628-
log.DebugLog(ctx, "Setting QoS for subsystem=%s, nsid=%d", nvmeofData.SubsystemNQN, nvmeofData.NamespaceID)
627+
// Step 4: Execute the operation
628+
return fn(ctx, gateway, nvmeofData)
629+
}
629630

630-
err = gateway.SetQoSLimitsForNamespace(ctx, nvmeofData.SubsystemNQN, nvmeofData.NamespaceID, *qos)
631-
if err != nil {
632-
// Check if error is EEXIST (RBD QoS already set)
633-
if errors.Is(err, nvmeoferrors.ErrRbdQoSExists) {
634-
log.ErrorLog(ctx, "RBD QoS already configured on volume")
631+
// modifyNVMeoFQoS handles NVMe-oF gateway QoS modification.
632+
func (cs *Server) modifyNVMeoFQoS(
633+
ctx context.Context,
634+
req *csi.ControllerModifyVolumeRequest,
635+
qos *nvmeof.NVMeoFQosVolume,
636+
) error {
637+
volumeID := req.GetVolumeId()
635638

636-
return nil, status.Error(codes.InvalidArgument,
637-
"RBD QoS already configured on this volume, cannot set NVMe-oF gateway QoS")
638-
}
639+
return cs.withGatewayConnection(ctx, req, volumeID, func(
640+
ctx context.Context,
641+
gateway *nvmeof.GatewayRpcClient,
642+
nvmeofData *nvmeof.NVMeoFVolumeData,
643+
) error {
644+
log.DebugLog(ctx, "Setting QoS for subsystem=%s, nsid=%d", nvmeofData.SubsystemNQN, nvmeofData.NamespaceID)
639645

640-
log.ErrorLog(ctx, "Failed to set QoS limits: %v", err)
646+
err := gateway.SetQoSLimitsForNamespace(ctx, nvmeofData.SubsystemNQN, nvmeofData.NamespaceID, *qos)
647+
if err != nil {
648+
// Check if error is EEXIST (RBD QoS already set)
649+
if errors.Is(err, nvmeoferrors.ErrRbdQoSExists) {
650+
log.ErrorLog(ctx, "RBD QoS already configured on volume")
641651

642-
return nil, status.Errorf(codes.Internal, "failed to set QoS limits: %v", err)
643-
}
652+
return status.Error(codes.InvalidArgument,
653+
"RBD QoS already configured on this volume, cannot set NVMe-oF gateway QoS")
654+
}
644655

645-
log.DebugLog(ctx, "Successfully modified NVMe-oF QoS for volume %s", volumeID)
656+
log.ErrorLog(ctx, "Failed to set QoS limits: %v", err)
646657

647-
return &csi.ControllerModifyVolumeResponse{}, nil
658+
return status.Errorf(codes.Internal, "failed to set QoS limits: %v", err)
659+
}
660+
661+
log.DebugLog(ctx, "Successfully modified NVMe-oF QoS for volume %s", volumeID)
662+
663+
return nil
664+
})
648665
}
649666

650667
// ensureSubsystem checks if the subsystem exists, and creates it if not.

0 commit comments

Comments
 (0)