From 45374ad264d5ea52610dab7f482989db21e74aa7 Mon Sep 17 00:00:00 2001 From: "C, Amarnath" Date: Sat, 6 Jun 2026 09:24:10 +0530 Subject: [PATCH] speed up system fetch 1. Use one WS-Man setup per system request to remove extra overhead. 2. Cache static hardware and version data for 5 minutes, while keeping power, features, and boot data live. Signed-off-by: C, Amarnath --- internal/usecase/devices/system_data.go | 216 ++++++++++++++++++ internal/usecase/devices/usecase.go | 20 +- redfish/internal/entity/v1/computer_system.go | 10 +- redfish/internal/usecase/computer_system.go | 17 +- redfish/internal/usecase/wsman_repo.go | 82 ++++--- 5 files changed, 289 insertions(+), 56 deletions(-) create mode 100644 internal/usecase/devices/system_data.go diff --git a/internal/usecase/devices/system_data.go b/internal/usecase/devices/system_data.go new file mode 100644 index 000000000..0e60dcada --- /dev/null +++ b/internal/usecase/devices/system_data.go @@ -0,0 +1,216 @@ +package devices + +import ( + "context" + "time" + + "github.com/device-management-toolkit/go-wsman-messages/v2/pkg/wsman/amt/boot" + + "github.com/device-management-toolkit/console/internal/entity" + dto "github.com/device-management-toolkit/console/internal/entity/dto/v1" + dtov2 "github.com/device-management-toolkit/console/internal/entity/dto/v2" + wsmanAPI "github.com/device-management-toolkit/console/internal/usecase/devices/wsman" +) + +// systemStaticDataCacheTTL is how long the cached static per-device data +// (hardware inventory and AMT control-mode/version) is reused before it is +// re-fetched from the device. Hardware inventory is effectively immutable and +// the AMT control mode only changes on (de)activation, so a short-to-moderate +// TTL trades a small window of control-mode staleness for skipping ~3.5s of +// WS-Man round-trips on repeat ComputerSystem requests. +const systemStaticDataCacheTTL = 5 * time.Minute + +// systemStaticCacheEntry holds the cached static data for a single device. +type systemStaticCacheEntry struct { + hardwareInfo dto.HardwareInfo + version dto.Version + expiresAt time.Time +} + +// SystemData aggregates the per-device information required to render a Redfish +// ComputerSystem resource. It is populated using a single WS-Man client setup so +// that the per-call request-queue overhead (and redundant authentication/DB +// lookups) are paid only once instead of once per data point. +// +// Only the data actually consumed by the Redfish ComputerSystem response is +// fetched: the more expensive WS-Man round-trips that the response never reads +// (OS power-saving state, AMT software identity and One-Click-Recovery boot +// data) are intentionally skipped to keep the request fast. +type SystemData struct { + Device entity.Device + PowerState dto.PowerState + HardwareInfo dto.HardwareInfo + FeaturesV2 dtov2.Features + Version dto.Version + BootData boot.BootSettingDataResponse + + // FeaturesErr, VersionErr and BootErr capture best-effort failures so the + // caller can preserve the previous behaviour of returning a partial system + // when these optional sections cannot be retrieved. + FeaturesErr error + VersionErr error + BootErr error +} + +// GetSystemData retrieves the power state, hardware info, features, version and +// boot data for a device using a single WS-Man client setup. Power state and +// hardware info are mandatory (an error is returned if they fail), while +// features, version and boot data are best-effort and their failures are +// reported via the corresponding error fields on the returned SystemData. +// +// The effectively static data (hardware info and AMT control-mode/version) is +// served from a short-lived per-device cache (see systemStaticDataCacheTTL) so +// that repeat requests skip those round-trips; power state, features and boot +// data are always fetched fresh. +func (uc *UseCase) GetSystemData(c context.Context, guid string) (SystemData, error) { + item, err := uc.repo.GetByID(c, guid, "") + if err != nil { + return SystemData{}, err + } + + if item == nil || item.GUID == "" { + return SystemData{}, ErrNotFound + } + + device, err := uc.device.SetupWsmanClient(c, *item, false, true) + if err != nil { + return SystemData{}, err + } + + result := SystemData{Device: *item} + + // Power state is mandatory and dynamic, so it is always fetched fresh. Only + // the CIM power state is needed by Redfish; the OS power-saving state (an + // extra round-trip) is intentionally skipped. + powerState, err := uc.powerStateForRedfish(device) + if err != nil { + return SystemData{}, err + } + + result.PowerState = powerState + + // Hardware info and version (AMT control mode) are effectively static, so + // they are served from a short-lived per-device cache to avoid their ~3.5s + // of WS-Man round-trips on repeat requests. On a cache miss they are fetched + // and, when both succeed, stored for subsequent requests. + if cached, ok := uc.getCachedStaticData(guid); ok { + result.HardwareInfo = cached.hardwareInfo + result.Version = cached.version + } else { + // Hardware info is mandatory. + hwResults, hwErr := device.GetHardwareInfo() + if hwErr != nil { + return SystemData{}, hwErr + } + + hwInfo := uc.hardwareInfoToDTO(hwResults) + result.HardwareInfo = hwInfo + + // Version is best-effort and only used to derive the AMT control mode, + // which comes from the setup-and-configuration service; the AMT software + // identity (an extra round-trip) is intentionally skipped. + result.Version, result.VersionErr = uc.controlModeVersionFromDevice(device) + + if result.VersionErr == nil { + uc.setCachedStaticData(guid, hwInfo, result.Version) + } + } + + // Features are best-effort and dynamic (user-consent/opt-in state can change + // mid-session), so they are always fetched fresh. Only redirection, + // user-consent and KVM state are needed for the Graphical/Serial console; + // the One-Click-Recovery data (4 extra round-trips) is intentionally skipped. + result.FeaturesV2, result.FeaturesErr = redfishFeaturesFromDevice(device) + + // Boot data is best-effort and dynamic. + result.BootData, result.BootErr = device.GetBootData() + + return result, nil +} + +// getCachedStaticData returns the cached static data for a device if present and +// not expired. Reading a nil cache map is safe and reports a miss. +func (uc *UseCase) getCachedStaticData(guid string) (systemStaticCacheEntry, bool) { + uc.systemStaticMutex.RLock() + entry, ok := uc.systemStaticCache[guid] + uc.systemStaticMutex.RUnlock() + + if !ok || time.Now().After(entry.expiresAt) { + return systemStaticCacheEntry{}, false + } + + return entry, true +} + +// setCachedStaticData stores the static data for a device with a fresh TTL. The +// cache map is lazily initialised so the method is safe even when the UseCase +// was constructed without New (e.g. in tests). +func (uc *UseCase) setCachedStaticData(guid string, hardwareInfo dto.HardwareInfo, version dto.Version) { + uc.systemStaticMutex.Lock() + defer uc.systemStaticMutex.Unlock() + + if uc.systemStaticCache == nil { + uc.systemStaticCache = make(map[string]systemStaticCacheEntry) + } + + uc.systemStaticCache[guid] = systemStaticCacheEntry{ + hardwareInfo: hardwareInfo, + version: version, + expiresAt: time.Now().Add(systemStaticDataCacheTTL), + } +} + +// powerStateForRedfish fetches only the CIM power state (skipping the OS +// power-saving state round-trip) since the Redfish response uses only the +// former. +func (uc *UseCase) powerStateForRedfish(device wsmanAPI.Management) (dto.PowerState, error) { + state, err := device.GetPowerState() + if err != nil { + return dto.PowerState{}, err + } + + if len(state) == 0 { + return dto.PowerState{}, ErrDeviceUseCase.Wrap("GetPowerState", "device.GetPowerState returned empty state", nil) + } + + return dto.PowerState{PowerState: int(state[0].PowerState)}, nil +} + +// redfishFeaturesFromDevice retrieves only the feature settings consumed by the +// Redfish Graphical/Serial console (redirection, user consent and KVM), +// skipping the One-Click-Recovery boot queries that the response never reads. +func redfishFeaturesFromDevice(device wsmanAPI.Management) (dtov2.Features, error) { + var features dtov2.Features + + if err := getRedirectionService(&features, device); err != nil { + return features, err + } + + if err := getUserConsent(&features, device); err != nil { + return features, err + } + + if err := getKVM(&features, device); err != nil { + return features, err + } + + return features, nil +} + +// controlModeVersionFromDevice fetches only the setup-and-configuration service +// response needed to derive the AMT control mode, skipping the AMT software +// identity round-trip. +func (uc *UseCase) controlModeVersionFromDevice(device wsmanAPI.Management) (dto.Version, error) { + data, err := device.GetSetupAndConfiguration() + if err != nil { + return dto.Version{}, err + } + + version := dto.Version{} + if len(data) > 0 { + resp := uc.setupAndConfigurationServiceResponseEntityToDTO(&data[0]) + version.AMTSetupAndConfigurationService = dto.SetupAndConfigurationServiceResponses{Response: *resp} + } + + return version, nil +} diff --git a/internal/usecase/devices/usecase.go b/internal/usecase/devices/usecase.go index a61fb361a..01c9c5b16 100644 --- a/internal/usecase/devices/usecase.go +++ b/internal/usecase/devices/usecase.go @@ -45,6 +45,13 @@ type UseCase struct { redirMutex sync.RWMutex // Protects redirConnections map log logger.Interface safeRequirements security.Cryptor + + // systemStaticCache caches the per-device data that is effectively static + // (hardware inventory and AMT control-mode/version) so repeat Redfish + // ComputerSystem requests can skip those WS-Man round-trips. Entries expire + // after systemStaticDataCacheTTL. Protected by systemStaticMutex. + systemStaticCache map[string]systemStaticCacheEntry + systemStaticMutex sync.RWMutex } var ErrAMT = AMTError{Console: consoleerrors.CreateConsoleError("DevicesUseCase")} @@ -52,12 +59,13 @@ var ErrAMT = AMTError{Console: consoleerrors.CreateConsoleError("DevicesUseCase" // New -. func New(r Repository, d WSMAN, redirection Redirection, log logger.Interface, safeRequirements security.Cryptor) *UseCase { uc := &UseCase{ - repo: r, - device: d, - redirection: redirection, - redirConnections: make(map[string]*DeviceConnection), - log: log, - safeRequirements: safeRequirements, + repo: r, + device: d, + redirection: redirection, + redirConnections: make(map[string]*DeviceConnection), + systemStaticCache: make(map[string]systemStaticCacheEntry), + log: log, + safeRequirements: safeRequirements, } // start up the worker go d.Worker() diff --git a/redfish/internal/entity/v1/computer_system.go b/redfish/internal/entity/v1/computer_system.go index a87d9887a..db970acb6 100644 --- a/redfish/internal/entity/v1/computer_system.go +++ b/redfish/internal/entity/v1/computer_system.go @@ -1,6 +1,8 @@ // Package redfish provides entity definitions for Redfish computer systems. package redfish +import "github.com/device-management-toolkit/console/redfish/internal/controller/http/v1/generated" + // ComputerSystem represents a Redfish Computer System entity. type ComputerSystem struct { ID string `json:"Id"` @@ -18,8 +20,12 @@ type ComputerSystem struct { Status *Status `json:"Status,omitempty"` MemorySummary *ComputerSystemMemorySummary `json:"MemorySummary,omitempty"` ProcessorSummary *ComputerSystemProcessorSummary `json:"ProcessorSummary,omitempty"` - ODataID string `json:"@odata.id"` - ODataType string `json:"@odata.type"` + // Boot holds the boot configuration fetched alongside the rest of the + // system data so a separate WS-Man round-trip can be avoided. It may be nil + // when boot data was not retrieved (callers should fall back accordingly). + Boot *generated.ComputerSystemBoot `json:"Boot,omitempty"` + ODataID string `json:"@odata.id"` + ODataType string `json:"@odata.type"` } // ComputerSystemHostGraphicalConsole represents graphical console (KVM) capabilities for a system. diff --git a/redfish/internal/usecase/computer_system.go b/redfish/internal/usecase/computer_system.go index 9ceb4314e..feaa296ee 100644 --- a/redfish/internal/usecase/computer_system.go +++ b/redfish/internal/usecase/computer_system.go @@ -186,12 +186,19 @@ func (uc *ComputerSystemUseCase) GetComputerSystem(ctx context.Context, systemID } } - // Fetch boot settings - boot, err := uc.Repo.GetBootSettings(ctx, systemID) - if err != nil { - // Log error but don't fail the entire request - boot settings may not be available - boot = nil + // Use boot settings retrieved alongside the rest of the system data when + // available; otherwise fetch them separately to preserve backward behaviour. + boot := system.Boot + if boot == nil { + var bootErr error + + boot, bootErr = uc.Repo.GetBootSettings(ctx, systemID) + if bootErr != nil { + // Log error but don't fail the entire request - boot settings may not be available + boot = nil + } } + // Create Actions for this system using the generated Actions type actions := uc.createActionsStruct(systemID) diff --git a/redfish/internal/usecase/wsman_repo.go b/redfish/internal/usecase/wsman_repo.go index 1c31df442..08232de17 100644 --- a/redfish/internal/usecase/wsman_repo.go +++ b/redfish/internal/usecase/wsman_repo.go @@ -297,18 +297,12 @@ func NewWsmanComputerSystemRepo(uc *devices.UseCase, log logger.Interface) *Wsma } } -// getCIMProperties extracts multiple CIM properties in a single call. -func (r *WsmanComputerSystemRepo) getCIMProperties(ctx context.Context, systemID string, configs []CIMPropertyConfig) (map[string]interface{}, dto.HardwareInfo, error) { +// extractCIMProperties builds the CIM property map from pre-fetched hardware +// info, avoiding an additional WSMAN call when the hardware info is already +// available (e.g. via a combined GetSystemData fetch). +func (r *WsmanComputerSystemRepo) extractCIMProperties(hwInfo dto.HardwareInfo, configs []CIMPropertyConfig) map[string]interface{} { results := make(map[string]interface{}) - // Get hardware info only once to avoid multiple WSMAN calls - hwInfo, err := r.usecase.GetHardwareInfo(ctx, systemID) - if err != nil { - r.log.Error("Failed to get hardware info", "systemID", systemID, "error", err) - - return results, hwInfo, err - } - for _, config := range configs { if value := r.extractPropertyFromHardwareInfo(hwInfo, config); value != nil { // Use StructField as key if specified, otherwise use CIMProperty @@ -321,7 +315,7 @@ func (r *WsmanComputerSystemRepo) getCIMProperties(ctx context.Context, systemID } } - return results, hwInfo, nil + return results } // extractPropertyFromHardwareInfo extracts a single property from pre-fetched hardware info. @@ -877,8 +871,9 @@ func (r *WsmanComputerSystemRepo) GetAll(ctx context.Context) ([]string, error) // GetByID retrieves a computer system by its ID from the WSMAN backend. func (r *WsmanComputerSystemRepo) GetByID(ctx context.Context, systemID string) (*redfishv1.ComputerSystem, error) { - // Verify device exists first - device, err := r.usecase.GetByID(ctx, systemID, "", true) + // Fetch power state, hardware info, features, version and boot data using a + // single WS-Man client setup to avoid the per-call request-queue overhead. + systemData, err := r.usecase.GetSystemData(ctx, systemID) if r.isDeviceNotFoundError(err) { return nil, ErrSystemNotFound } @@ -887,45 +882,39 @@ func (r *WsmanComputerSystemRepo) GetByID(ctx context.Context, systemID string) return nil, err } - if device == nil { - return nil, ErrSystemNotFound - } - - // Get power state from devices use case - powerState, err := r.usecase.GetPowerState(ctx, systemID) - if r.isDeviceNotFoundError(err) { - return nil, ErrSystemNotFound - } + // Map the integer power state to Redfish PowerState + redfishPowerState := r.mapCIMPowerStateToRedfish(systemData.PowerState.PowerState) - if err != nil { - return nil, err - } + // Extract CIM data from the pre-fetched hardware info. + cimData := r.extractCIMProperties(systemData.HardwareInfo, allCIMConfigs) - // Map the integer power state to Redfish PowerState - redfishPowerState := r.mapCIMPowerStateToRedfish(powerState.PowerState) + // Build the complete ComputerSystem using CIM data and hardware info + system := r.buildComputerSystemFromCIMData(systemID, redfishPowerState, cimData, systemData.HardwareInfo) - // Extract CIM data using the global configuration with static transformers - cimData, hwInfo, err := r.getCIMProperties(ctx, systemID, allCIMConfigs) - if err != nil { - return nil, err + // Attach boot settings (best-effort) so a separate WS-Man round-trip is avoided. + if systemData.BootErr != nil { + r.log.Warn("Failed to get boot data from device", "systemID", systemID, "error", systemData.BootErr) + } else { + system.Boot = mapBootDataToBoot(systemData.BootData) } - // Build and return the complete ComputerSystem using CIM data and hardware info - system := r.buildComputerSystemFromCIMData(systemID, redfishPowerState, cimData, hwInfo) - - // Fetch GraphicalConsole data best-effort, but avoid returning guessed values - // when feature retrieval fails. - _, featuresV2, err := r.usecase.GetFeatures(ctx, systemID) - if err != nil { - r.log.Warn("Failed to retrieve KVM features for GraphicalConsole", "systemID", systemID, "error", err) + // Populate GraphicalConsole/SerialConsole best-effort, but avoid returning + // guessed values when feature retrieval failed. + if systemData.FeaturesErr != nil { + r.log.Warn("Failed to retrieve KVM features for GraphicalConsole", "systemID", systemID, "error", systemData.FeaturesErr) return system, nil } - controlMode := r.getAMTControlMode(ctx, systemID) + controlMode := "" + if systemData.VersionErr != nil { + r.log.Warn("Failed to retrieve AMT version for control mode", "systemID", systemID, "error", systemData.VersionErr) + } else { + controlMode = mapControlModeFromVersion(systemData.Version) + } - system.GraphicalConsole = r.buildGraphicalConsole(device.UseTLS, featuresV2, controlMode) - system.SerialConsole = r.buildSerialConsole(systemID, featuresV2, controlMode) + system.GraphicalConsole = r.buildGraphicalConsole(systemData.Device.UseTLS, systemData.FeaturesV2, controlMode) + system.SerialConsole = r.buildSerialConsole(systemID, systemData.FeaturesV2, controlMode) return system, nil } @@ -1306,6 +1295,13 @@ func (r *WsmanComputerSystemRepo) GetBootSettings(ctx context.Context, systemID } // Map AMT boot data to Redfish Boot structure + return mapBootDataToBoot(bootData), nil +} + +// mapBootDataToBoot converts AMT boot settings data into the Redfish Boot +// structure. It is shared by GetBootSettings and the combined GetByID fetch so +// both produce identical results. +func mapBootDataToBoot(bootData amtBoot.BootSettingDataResponse) *generated.ComputerSystemBoot { boot := &generated.ComputerSystemBoot{} // Map BootSourceOverrideEnabled @@ -1352,7 +1348,7 @@ func (r *WsmanComputerSystemRepo) GetBootSettings(ctx context.Context, systemID boot.BootSourceOverrideMode = &mode - return boot, nil + return boot } // UpdateBootSettings updates the boot configuration for a system.