diff --git a/.dockerignore b/.dockerignore new file mode 100644 index 0000000..a12dbe3 --- /dev/null +++ b/.dockerignore @@ -0,0 +1,6 @@ +# More info: https://docs.docker.com/engine/reference/builder/#dockerignore-file +# Ignore all files which are not go type or shell scripts +!**/*.go +!**/*.mod +!**/*.sum +!**/*.sh \ No newline at end of file diff --git a/Dockerfile b/Dockerfile index be21ebe..bfdf9c8 100644 --- a/Dockerfile +++ b/Dockerfile @@ -15,18 +15,13 @@ # See the License for the specific language governing permissions and # limitations under the License. -FROM golang:1.22 AS builder +FROM golang:1.23 AS builder WORKDIR /workspace -# Copy the Go Modules manifests -COPY go.mod go.mod -COPY go.sum go.sum - -# Copy the Go source tree -COPY cmd/ cmd/ -COPY internal/ internal/ -COPY pkg/ pkg/ +# Copy all source files, avoid producing multiple layers +# See .dockerignore for exclusions +COPY . . # Build RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o nnf-ec ./cmd/nnf_ec.go diff --git a/Makefile b/Makefile index 30d6fcc..726671b 100644 --- a/Makefile +++ b/Makefile @@ -21,62 +21,105 @@ PROD_VERSION=$(shell sed 1q .version) DEV_IMGNAME=nnf-ec DTR_IMGPATH=ghcr.io/nearnodeflash/$(DEV_IMGNAME) -all: image - -vendor: +.DEFAULT_GOAL := help + +## Display this help message +help: + @echo "NNF Element Controller (nnf-ec) Makefile" + @echo "" + @echo "Available targets:" + @echo "" + @awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_-]+:.*?## / {printf " \033[36m%-20s\033[0m %s\n", $$1, $$2}' $(MAKEFILE_LIST) + @echo "" + @echo "Docker cleanup targets:" + @echo " clean-images Remove all project-related Docker images" + @echo " clean-docker-all Remove ALL unused Docker resources (aggressive)" + @echo " clean-docker-images Remove unused Docker images only" + @echo " clean-docker-cache Remove Docker build cache" + @echo " clean-project-docker Remove project Docker images and unused resources" + @echo "" + +all: image ## Build the default Docker image + +vendor: ## Download Go module dependencies go mod vendor -vet: +vet: ## Run Go vet on all packages go vet `go list -f {{.Dir}} ./...` -fmt: +fmt: ## Format Go source code go fmt `go list -f {{.Dir}} ./...` -generate: +generate: ## Generate code and redfish/swordfish message registries ( cd ./pkg/manager-message-registry/generator && go build msgenerator.go ) go generate ./... go fmt ./pkg/manager-message-registry/registries -test: +test: ## Run Go unit tests locally go test -v ./... -linux: +linux: ## Build Linux binary env CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o ${DEV_IMGNAME} ./cmd/nnf_ec.go -image: +image: ## Build Docker image docker build --file Dockerfile --label $(DTR_IMGPATH):$(PROD_VERSION) --tag $(DTR_IMGPATH):$(PROD_VERSION) . -container-unit-test: +container-unit-test: ## Run containerized unit tests docker build -f Dockerfile --label $(DTR_IMGPATH)-$@:$(PROD_VERSION)-$@ -t $(DTR_IMGPATH)-$@:$(PROD_VERSION) --target $@ . docker run --rm -t --name $@ -v $(PWD):$(PWD):rw,z $(DTR_IMGPATH)-$@:$(PROD_VERSION) # This and the corresponding clean-lint should go away and move to git pre-commit hook -lint: +lint: ## Run linting checks in Docker container docker build -f Dockerfile --label $(DTR_IMGPATH)-$@:$(PROD_VERSION)-$@ -t $(DTR_IMGPATH)-$@:$(PROD_VERSION) --target $@ . docker run --rm -t --name $@ -v $(PWD):$(PWD):rw,z $(DTR_IMGPATH)-$@:$(PROD_VERSION) # This and the cooresponding clean-codestyle should go away and move to git pre-commit hook -codestyle: +codestyle: ## Run code style checks in Docker container docker build -f Dockerfile --label $(DTR_IMGPATH)-$@:$(PROD_VERSION) -t $(DTR_IMGPATH)-$@:$(PROD_VERSION) --target $@ . docker run --rm -t --name $@ -v $(PWD):$(PWD):rw,z $(DTR_IMGPATH)-$@:$(PROD_VERSION) -clean-lint: +clean-lint: ## Clean lint Docker images and containers docker rmi $(DTR_IMGPATH)-lint:$(PROD_VERSION) || true + docker container rm lint || true -clean-codestyle: +clean-codestyle: ## Clean codestyle Docker images and containers docker rmi $(DTR_IMGPATH)-codestyle:$(PROD_VERSION) || true + docker container rm codestyle || true + +clean-container-unit-test: ## Clean container-unit-test Docker images and containers + docker rmi $(DTR_IMGPATH)-container-unit-test:$(PROD_VERSION) || true + docker container rm container-unit-test || true # push: # docker push $(DTR_IMGPATH):$(PROD_VERSION) -kind-push: image +kind-push: image ## Load Docker image into kind cluster kind load docker-image $(DTR_IMGPATH):$(PROD_VERSION) -clean: clean-db +clean: clean-db ## Clean build artifacts and Docker resources docker container prune --force docker image prune --force - docker rmi $(DTR_IMGPATH):$(PROD_VERSION) + docker rmi $(DTR_IMGPATH):$(PROD_VERSION) || true go clean all -clean-db: +clean-images: ## Clean all Docker images related to this project + docker rmi $(DTR_IMGPATH):$(PROD_VERSION) || true + docker rmi $(DTR_IMGPATH)-container-unit-test:$(PROD_VERSION) || true + docker rmi $(DTR_IMGPATH)-lint:$(PROD_VERSION) || true + docker rmi $(DTR_IMGPATH)-codestyle:$(PROD_VERSION) || true + +clean-docker-all: ## Remove ALL unused Docker resources (aggressive) + docker system prune -a --volumes --force + +clean-docker-images: ## Remove unused Docker images only + docker image prune --force + +clean-docker-cache: ## Remove Docker build cache + docker builder prune --force + +clean-project-docker: clean-images ## Remove project Docker images and unused resources + docker container prune --force + docker image prune --force + +clean-db: ## Remove all database files find . -name "*.db" -type d -exec rm -rf {} + diff --git a/go.mod b/go.mod index dc4cdf1..fad46ed 100644 --- a/go.mod +++ b/go.mod @@ -1,8 +1,6 @@ module github.com/NearNodeFlash/nnf-ec -go 1.22.0 - -toolchain go1.22.5 +go 1.23.9 require ( github.com/HewlettPackard/structex v1.0.4 diff --git a/kubernetes/nnf-ec/Chart.yaml b/kubernetes/nnf-ec/Chart.yaml deleted file mode 100644 index f9ad5ae..0000000 --- a/kubernetes/nnf-ec/Chart.yaml +++ /dev/null @@ -1,5 +0,0 @@ -apiVersion: v1 -appVersion: "1.0" -description: A Helm chart for Kubernetes -name: nnf-ec -version: 1.0.0 diff --git a/kubernetes/nnf-ec/charts/cray-service-2.0.0.tgz b/kubernetes/nnf-ec/charts/cray-service-2.0.0.tgz deleted file mode 100644 index 0a69ccf..0000000 Binary files a/kubernetes/nnf-ec/charts/cray-service-2.0.0.tgz and /dev/null differ diff --git a/kubernetes/nnf-ec/templates/NOTES.txt b/kubernetes/nnf-ec/templates/NOTES.txt deleted file mode 100644 index e69de29..0000000 diff --git a/kubernetes/nnf-ec/templates/_helpers.tpl b/kubernetes/nnf-ec/templates/_helpers.tpl deleted file mode 100644 index 3b2fe98..0000000 --- a/kubernetes/nnf-ec/templates/_helpers.tpl +++ /dev/null @@ -1,45 +0,0 @@ -{{/* vim: set filetype=mustache: */}} -{{/* -Expand the name of the chart. -*/}} -{{- define "csds-dpapi.name" -}} -{{- default .Chart.Name .Values.nameOverride | trunc 63 | trimSuffix "-" -}} -{{- end -}} - -{{/* -Create a default fully qualified app name. -We truncate at 63 chars because some Kubernetes name fields are limited to this (by the DNS naming spec). -If release name contains chart name it will be used as a full name. -*/}} -{{- define "csds-dpapi.fullname" -}} -{{- if .Values.fullnameOverride -}} -{{- .Values.fullnameOverride | trunc 63 | trimSuffix "-" -}} -{{- else -}} -{{- $name := default .Chart.Name .Values.nameOverride -}} -{{- if contains $name .Release.Name -}} -{{- .Release.Name | trunc 63 | trimSuffix "-" -}} -{{- else -}} -{{- printf "%s-%s" .Release.Name $name | trunc 63 | trimSuffix "-" -}} -{{- end -}} -{{- end -}} -{{- end -}} - -{{/* -Create chart name and version as used by the chart label. -*/}} -{{- define "csds-dpapi.chart" -}} -{{- printf "%s-%s" .Chart.Name .Chart.Version | replace "+" "_" | trunc 63 | trimSuffix "-" -}} -{{- end -}} - -{{/* -Common labels -*/}} -{{- define "csds-api.labels" -}} -app.kubernetes.io/name: {{ include "csds-dpapi.name" . }} -helm.sh/chart: {{ include "csds-dpapi.chart" . }} -app.kubernetes.io/instance: {{ .Release.Name }} -{{- if .Chart.AppVersion }} -app.kubernetes.io/version: {{ .Chart.AppVersion | quote }} -{{- end }} -app.kubernetes.io/managed-by: {{ .Release.Service }} -{{- end -}} diff --git a/kubernetes/nnf-ec/templates/role.yaml b/kubernetes/nnf-ec/templates/role.yaml deleted file mode 100644 index be81d4e..0000000 --- a/kubernetes/nnf-ec/templates/role.yaml +++ /dev/null @@ -1,32 +0,0 @@ -apiVersion: rbac.authorization.k8s.io/v1 -kind: ClusterRole -metadata: - name: {{ include "csds-dpapi.fullname" . }} - labels: - app.kubernetes.io/name: {{ include "cray-service.name" . }} - {{- include "cray-service.common-labels" . | nindent 4 }} - annotations: - {{- include "cray-service.common-annotations" . | nindent 4 }} -rules: -- apiGroups: [""] - resources: ["persistentvolumeclaims", "events", "secrets", "pods"] - verbs: ["*"] -- apiGroups: [""] - resources: ["namespaces"] - verbs: ["get"] -- apiGroups: [""] - resources: ["configmaps", "services"] - verbs: ["get", "list", "create", "delete", "update"] -- apiGroups: ["apps"] - resources: ["daemonsets"] - verbs: ["get", "list"] -- apiGroups: ["monitoring.coreos.com"] - resources: ["servicemonitors"] - verbs: ["get", "create"] -- apiGroups: ["apps"] - resourceNames: ["csds-dpapi"] - resources: ["deployments/finalizers"] - verbs: ["update"] -- apiGroups: ["dpm.cray.com"] - resources: ["*"] - verbs: ["*"] diff --git a/kubernetes/nnf-ec/templates/role_binding.yaml b/kubernetes/nnf-ec/templates/role_binding.yaml deleted file mode 100644 index d6f658b..0000000 --- a/kubernetes/nnf-ec/templates/role_binding.yaml +++ /dev/null @@ -1,17 +0,0 @@ -kind: ClusterRoleBinding -apiVersion: rbac.authorization.k8s.io/v1 -metadata: - name: {{ include "csds-dpapi.fullname" . }} - labels: - app.kubernetes.io/name: {{ include "cray-service.name" . }} - {{- include "cray-service.common-labels" . | nindent 4 }} - annotations: - {{- include "cray-service.common-annotations" . | nindent 4 }} -subjects: -- kind: ServiceAccount - name: {{ index .Values "cray-service" "serviceAccountName" }} - namespace: {{ .Release.Namespace }} -roleRef: - kind: ClusterRole - name: {{ include "csds-dpapi.fullname" . }} - apiGroup: rbac.authorization.k8s.io diff --git a/kubernetes/nnf-ec/templates/service_account.yaml b/kubernetes/nnf-ec/templates/service_account.yaml deleted file mode 100644 index ea7dab0..0000000 --- a/kubernetes/nnf-ec/templates/service_account.yaml +++ /dev/null @@ -1,10 +0,0 @@ -apiVersion: v1 -kind: ServiceAccount -metadata: - name: {{ index .Values "cray-service" "serviceAccountName" }} - namespace: {{ .Release.Namespace }} - labels: - app.kubernetes.io/name: {{ include "cray-service.name" . }} - {{- include "cray-service.common-labels" . | nindent 4 }} - annotations: - {{- include "cray-service.common-annotations" . | nindent 4 }} diff --git a/kubernetes/nnf-ec/values.yaml b/kubernetes/nnf-ec/values.yaml deleted file mode 100644 index 797ad18..0000000 --- a/kubernetes/nnf-ec/values.yaml +++ /dev/null @@ -1,53 +0,0 @@ -# Note that cray-service.containers[*].image and cray-service.initContainers[*].image map values are one of the only structures that -# differ from the standard kubernetes container spec: -# image: -# repository: "" -# tag: "" (default = "latest") -# pullPolicy: "" (default = "IfNotPresent") -cray-service: - # imagesHost: "s-lmo-anastl:5000" - nameOverride: nnf-ec - type: DaemonSet - service: - enabled: False - replicaCount: 1 - serviceAccountName: nnf-ec - tolerations: - - key: "node-type" - operator: "Equal" - value: "storage" - effect: "NoSchedule" - containers: - cray-nnf-ec: - name: cray-nnf-ec - image: - repository: cray/cray-nnf-ec - tag: latest - pullPolicy: Never - command: ["nnf-ec"] - args: ["--http", "--port=8080", "--mock", "--log", "--verbose"] - ports: - - name: nnf-api-service - containerPort: 8080 - securityContext: - privileged: true - volumeMounts: - - mountPath: /dev - name: dev-dir - - mountPath: /mnt - name: mnt-dir - mountPropagation: Bidirectional - volumes: - - name: dev-dir - hostPath: - path: /dev - - name: mnt-dir - hostPath: - path: /mnt - nodeSelector: - cray.dpm.dg.dpm-nnf-node: "true" - -global: - chart: - name: "" # set at deploy time automatically, no need to ever set explicitly - version: "" # set at deploy time automatically, no need to ever set explicitly diff --git a/pkg/manager-nnf/allocation_policy.go b/pkg/manager-nnf/allocation_policy.go index da0d3d8..636ece2 100644 --- a/pkg/manager-nnf/allocation_policy.go +++ b/pkg/manager-nnf/allocation_policy.go @@ -161,8 +161,9 @@ func (p *SpareAllocationPolicy) CheckAndAdjustCapacity() error { if p.compliance != RelaxedAllocationComplianceType { - if len(p.storage) < SpareAllocationPolicyMinimumDriveCount { - return fmt.Errorf("Insufficient drive count. Required: %d Available: %d", SpareAllocationPolicyMinimumDriveCount, len(p.storage)) + driveCount := uint64(len(p.storage)) + if driveCount < SpareAllocationPolicyMinimumDriveCount { + return fmt.Errorf("Insufficient drive count. Required: %d Available: %d", SpareAllocationPolicyMinimumDriveCount, driveCount) } roundUpToMultiple := func(n, m uint64) uint64 { // Round 'n' up to a multiple of 'm' @@ -170,8 +171,8 @@ func (p *SpareAllocationPolicy) CheckAndAdjustCapacity() error { } // Validate each drive can contribute sufficient capacity towards the entire pool. - poolCapacityBytes := roundUpToMultiple(p.capacityBytes, SpareAllocationPolicyMinimumDriveCount) - driveCapacityBytes := roundUpToMultiple(poolCapacityBytes/SpareAllocationPolicyMinimumDriveCount, 4096) + poolCapacityBytes := roundUpToMultiple(p.capacityBytes, driveCount) + driveCapacityBytes := roundUpToMultiple(poolCapacityBytes/driveCount, 4096) for _, s := range p.storage { if driveCapacityBytes > s.UnallocatedBytes() { @@ -180,7 +181,7 @@ func (p *SpareAllocationPolicy) CheckAndAdjustCapacity() error { } // Adjust the pool's capacity such that it is a multiple of the number drives. - p.capacityBytes = driveCapacityBytes * uint64(len(p.storage)) + p.capacityBytes = driveCapacityBytes * driveCount } if availableBytes < p.capacityBytes { @@ -193,7 +194,8 @@ func (p *SpareAllocationPolicy) CheckAndAdjustCapacity() error { // Allocate - allocate the storage func (p *SpareAllocationPolicy) Allocate() ([]nvme.ProvidingVolume, error) { - perStorageCapacityBytes := p.capacityBytes / uint64(len(p.storage)) + driveCount := uint64(len(p.storage)) + perStorageCapacityBytes := p.capacityBytes / driveCount remainingCapacityBytes := p.capacityBytes volumes := []nvme.ProvidingVolume{} @@ -204,7 +206,7 @@ func (p *SpareAllocationPolicy) Allocate() ([]nvme.ProvidingVolume, error) { // Leftover bytes are placed on trailing volume; note that this // is never the case for strict allocation in which the requested // allocation must be a multiple of the storage size. - if idx == len(p.storage)-1 { + if idx == int(driveCount-1) { capacityBytes = remainingCapacityBytes } diff --git a/pkg/manager-nnf/manager.go b/pkg/manager-nnf/manager.go index 12f0092..bd49fef 100644 --- a/pkg/manager-nnf/manager.go +++ b/pkg/manager-nnf/manager.go @@ -500,6 +500,7 @@ func (s *StorageService) Initialize(log ec.Logger, ctrl NnfControllerInterface) // Create the key-value storage database { path := "nnf.db" + persistent.SetLogger(log) s.store, err = persistent.Open(path, false) if err != nil { log.Error(err, "Unable to open database", "path", path) diff --git a/pkg/manager-nnf/storage_pool.go b/pkg/manager-nnf/storage_pool.go index b7d3af0..09e3a62 100644 --- a/pkg/manager-nnf/storage_pool.go +++ b/pkg/manager-nnf/storage_pool.go @@ -49,7 +49,7 @@ type StoragePool struct { missingVolumes []storagePoolPersistentVolumeInfo // Original persistent volume information from KV store - persistentVolumes []storagePoolPersistentVolumeInfo + persistedVolumes []storagePoolPersistentVolumeInfo storageGroupIds []string fileSystemId string @@ -123,17 +123,34 @@ func (p *StoragePool) findStorageGroupByEndpoint(endpoint *Endpoint) *StorageGro return nil } +// recoverVolumes attempts to restore the state of volumes in the storage pool based on +// the provided slice of persisted volume information. It updates the pool's internal +// lists of persisted, missing, and providing volumes by: +// - Skipping and marking as missing any volumes with an invalid namespace ID. +// - Attempting to locate the corresponding storage device and namespace for each volume. +// - Marking volumes as missing if the storage device or namespace cannot be found. +// - Adding successfully located volumes to the providingVolumes list. +// +// Finally, it updates the allocatedVolume field to reflect the current pool capacity. func (p *StoragePool) recoverVolumes(volumes []storagePoolPersistentVolumeInfo) error { log := p.storageService.log.WithValues(storagePoolIdKey, p.id) log.Info("recover volumes") - // Store the persistent volumes information for later use - p.persistentVolumes = make([]storagePoolPersistentVolumeInfo, len(volumes)) - copy(p.persistentVolumes, volumes) + // Store the persisted volumes information for later use + p.persistedVolumes = make([]storagePoolPersistentVolumeInfo, len(volumes)) + copy(p.persistedVolumes, volumes) for _, volumeInfo := range volumes { log := log.WithValues("serialNumber", volumeInfo.SerialNumber, "namespaceId", volumeInfo.NamespaceID) + // Consider volumes that have been invalidated (marked with invalid namespace ID) as missing. + // They need a replacement. + if volumeInfo.NamespaceID == invalidNamespaceID { + log.V(2).Info("skipping invalidated volume during recovery") + p.missingVolumes = append(p.missingVolumes, volumeInfo) + continue + } + // Locate the NVMe Storage device by Serial Number storage := p.storageService.findStorage(volumeInfo.SerialNumber) if storage == nil { @@ -165,16 +182,31 @@ func (p *StoragePool) recoverVolumes(volumes []storagePoolPersistentVolumeInfo) return nil } -// Rescan namespaces associated to set the missing volumes list +// checkVolumes validates the current state of volumes in the storage pool by rescanning +// associated storage devices and updating the pool's volume tracking lists. +// +// This method performs the following operations: +// 1. Rescans all storage devices that should contain volumes for this pool to refresh +// their namespace information and detect any changes in device state +// 2. Clears and rebuilds both the missingVolumes and providingVolumes lists to ensure +// they accurately reflect the current state +// 3. Attempts to recover volumes from persistent storage information, identifying +// which volumes are still available and which are missing or invalid +// 4. Updates the pool's internal state to reflect any volumes that may have become +// unavailable due to device failures, disconnections, or other issues +// +// The method is typically called during pool initialization, recovery operations, +// or when storage device states may have changed. It ensures the pool maintains +// an accurate view of its available storage resources. func (p *StoragePool) checkVolumes() error { log := p.storageService.log.WithValues(storagePoolIdKey, p.id) log.Info("check volumes") // Rescan the storages to ensure our namespace information is up to date - volumes := make([]storagePoolPersistentVolumeInfo, len(p.persistentVolumes)) - copy(volumes, p.persistentVolumes) + volumesInPool := make([]storagePoolPersistentVolumeInfo, len(p.persistedVolumes)) + copy(volumesInPool, p.persistedVolumes) - for _, pv := range volumes { + for _, pv := range volumesInPool { log := log.WithValues("serialNumber", pv.SerialNumber, "namespaceId", pv.NamespaceID) storage := p.storageService.findStorage(pv.SerialNumber) if storage == nil { @@ -187,7 +219,7 @@ func (p *StoragePool) checkVolumes() error { p.missingVolumes = p.missingVolumes[:0] // Clear the missing volumes list p.providingVolumes = p.providingVolumes[:0] // Clear the providing volumes list - if err := p.recoverVolumes(volumes); err != nil { + if err := p.recoverVolumes(volumesInPool); err != nil { log.Error(err, "Failed to recover volumes") return err } @@ -268,11 +300,11 @@ func (p *StoragePool) replaceMissingVolumes() error { pv.Storage.SerialNumber(), pv.VolumeId), p) - // TODO: Find the serialnumber/volumeid in any other storage pools and - // invalidate that volumeid to prevent reuse. Should probably store - // that new value some how too. - // OR - // Patch all the storage pools and don't allow a particular storeage pool to be patched in isolation + // Invalidate this volume in any other storage pools to prevent reuse + if err := p.invalidateVolumeInOtherPools(pv.Storage.SerialNumber(), pv.Storage.FindVolume(pv.VolumeId).GetNamespaceId()); err != nil { + log.Error(err, "Failed to invalidate volume in other storage pools", "serialNumber", pv.Storage.SerialNumber(), "namespaceId", pv.Storage.FindVolume(pv.VolumeId).GetNamespaceId()) + // This is not a fatal error, continue with the replacement + } } // We've replaced all the missing volumes, so clear the list @@ -281,6 +313,45 @@ func (p *StoragePool) replaceMissingVolumes() error { return nil } +// invalidateVolumeInOtherPools finds and invalidates the specified volume in any other storage pools +// to prevent reuse of the same volume in multiple pools. It replaces the namespace ID with a +// nonsense value (0xFFFFFFFF) in the persistent volume information of other pools. +func (p *StoragePool) invalidateVolumeInOtherPools(serialNumber string, namespaceID nvme2.NamespaceIdentifier) error { + log := p.storageService.log.WithValues(storagePoolIdKey, p.id, "targetSerialNumber", serialNumber, "targetNamespaceId", namespaceID) + log.V(2).Info("invalidating volume in other storage pools") + + invalidatedPools := 0 + + // Check all other storage pools in the service + for i := range p.storageService.pools { + otherPool := &p.storageService.pools[i] + if otherPool.id == p.id { + continue // Skip self + } + + poolModified := false + + // Check persistent volumes + for j := range otherPool.persistedVolumes { + persistentVol := &otherPool.persistedVolumes[j] + if persistentVol.SerialNumber == serialNumber && persistentVol.NamespaceID == namespaceID { + log.Info("invalidating persistent volume in storage pool", "otherPoolId", otherPool.id, "originalNamespaceId", persistentVol.NamespaceID) + + // Replace with a nonsense namespace ID to prevent reuse + persistentVol.NamespaceID = invalidNamespaceID + poolModified = true + } + } + + if poolModified { + invalidatedPools++ + } + } + + log.V(2).Info("volume invalidation complete", "invalidatedPools", invalidatedPools) + return nil +} + // Locate Storages not providing a volume for this pool func (p *StoragePool) locateUnusedStorage() []*nvme.Storage { log := p.storageService.log.WithValues(storagePoolIdKey, p.id) @@ -356,6 +427,9 @@ func (p *StoragePool) deallocateVolumes() error { const storagePoolRegistryPrefix = "SP" +// Invalid namespace ID used to mark volumes as unusable +const invalidNamespaceID = nvme2.NamespaceIdentifier(0xFFFFFFFF) + const ( // Allocation Log Entry is recorded after the storage pool successfully allocates the backing storage resources (i.e. NVMe Namespaces) storagePoolStorageCreateStartLogEntryType uint32 = iota @@ -430,8 +504,8 @@ func (p *StoragePool) GenerateStateData(state uint32) ([]byte, error) { } // Store the persistent volumes information for later use - p.persistentVolumes = make([]storagePoolPersistentVolumeInfo, len(entry.Volumes)) - copy(p.persistentVolumes, entry.Volumes) + p.persistedVolumes = make([]storagePoolPersistentVolumeInfo, len(entry.Volumes)) + copy(p.persistedVolumes, entry.Volumes) return json.Marshal(entry) } diff --git a/pkg/manager-nvme/manager.go b/pkg/manager-nvme/manager.go index efde722..fe37caf 100644 --- a/pkg/manager-nvme/manager.go +++ b/pkg/manager-nvme/manager.go @@ -1,5 +1,5 @@ /* - * Copyright 2020-2024 Hewlett Packard Enterprise Development LP + * Copyright 2020-2025 Hewlett Packard Enterprise Development LP * Other additional copyright holders may be indicated within. * * The entirety of this work is licensed under the Apache License, @@ -364,6 +364,7 @@ func (s *Storage) initialize() error { ctrl, err := s.device.IdentifyController(0) if err != nil { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) return fmt.Errorf("Initialize Storage %s: Failed to identify common controller: Error: %w", s.id, err) } @@ -420,6 +421,8 @@ func (s *Storage) initialize() error { ns, err = s.device.IdentifyNamespace(CommonNamespaceIdentifier) if err != nil { if retryCount >= 2 { + // After retries, this is likely a system-level failure + s.notify(sf.UNAVAILABLE_OFFLINE_RST) return fmt.Errorf("Initialize Storage %s: Failed to identify common namespace, retried %d times: Error: %w", s.id, retryCount, err) } @@ -464,11 +467,19 @@ func (s *Storage) purge() error { namespaces, err := s.device.ListNamespaces(0) if err != nil { + // System-level failure when listing namespaces + if isSystemLevelError(err) { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } for _, nsid := range namespaces { if err := s.device.DeleteNamespace(nsid); err != nil { + // System-level failure when deleting namespace during purge + if isSystemLevelError(err) { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } } @@ -540,19 +551,23 @@ func (s *Storage) createVolume(desiredCapacityInBytes uint64) (*Volume, error) { actualCapacityBytes := roundUpToMultiple(desiredCapacityInBytes, s.blockSizeBytes) s.log.V(2).Info("Creating namespace", "capacityInBytes", actualCapacityBytes, "formatIndex", s.lbaFormatIndex) - namespaceId, guid, err := s.device.CreateNamespace(actualCapacityBytes/s.blockSizeBytes, s.lbaFormatIndex) + namespaceID, guid, err := s.device.CreateNamespace(actualCapacityBytes/s.blockSizeBytes, s.lbaFormatIndex) if err != nil { + // Only publish error events for system-level failures + if isSystemLevelError(err) { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return nil, err } - id := strconv.Itoa(int(namespaceId)) + id := strconv.Itoa(int(namespaceID)) - log := s.log.WithName(id).WithValues(namespaceIdKey, namespaceId) + log := s.log.WithName(id).WithValues(namespaceIdKey, namespaceID) log.V(1).Info("Created namespace") volume := Volume{ id: id, - namespaceId: namespaceId, + namespaceId: namespaceID, guid: guid, capacityBytes: actualCapacityBytes, storage: s, @@ -575,6 +590,10 @@ func (s *Storage) deleteVolume(volumeId string) error { log.V(2).Info("Deleting namespace") err := s.device.DeleteNamespace(volume.namespaceId) if err != nil { + // Only publish error events for system-level failures + if isSystemLevelError(err) { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) + } log.Error(err, "Delete namespace failed, removing from in-memory state anyway", "capacity", volume.capacityBytes) } else { log.V(1).Info("Deleted namespace", "capacity", volume.capacityBytes) @@ -599,6 +618,10 @@ func (s *Storage) formatVolume(volumeID string) error { log.V(2).Info("Format namespace") if err := s.device.FormatNamespace(volume.namespaceId); err != nil { + // Only publish error events for system-level failures + if isSystemLevelError(err) { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) + } log.Error(err, "Format namespace failure") return err } @@ -614,6 +637,10 @@ func (s *Storage) formatVolume(volumeID string) error { func (s *Storage) recoverStorageVolumes() error { namespaces, err := s.device.ListNamespaces(0) if err != nil { + // Only publish error events for system-level failures + if isSystemLevelError(err) { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) + } s.log.Error(err, "Failed to list device namespaces") } @@ -623,6 +650,10 @@ func (s *Storage) recoverStorageVolumes() error { ns, err := s.device.IdentifyNamespace(nsid) if err != nil { + // Only publish error events for system-level failures + if isSystemLevelError(err) { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) + } log.Error(err, "Failed to identify namespace") continue } @@ -649,6 +680,66 @@ func (s *Storage) recoverStorageVolumes() error { return nil } +// isSystemLevelError determines if an error represents a system-level failure +// (hardware, communication, etc.) rather than an expected operational failure +// (insufficient capacity, namespace already exists, etc.). +// System-level errors should trigger ResourceState event publishing. +func isSystemLevelError(err error) bool { + if err == nil { + return false + } + + // Check if it's an NVMe command error with a specific status code + var cmdErr *nvme.CommandError + if errors.As(err, &cmdErr) { + switch cmdErr.StatusCode { + // Expected operational failures - don't publish events + case nvme.NamespaceAlreadyAttached: + return false + case nvme.NamespaceNotAttached: + return false + case 0x0A: // Namespace Insufficient Capacity + return false + case 0x0B: // Namespace Identifier Unavailable + return false + case 0x0C: // Namespace Already Exists + return false + case 0x15: // Invalid Namespace Identifier (operational - bad request) + return false + case 0x02: // Invalid Field in Command (operational - bad parameters) + return false + default: + // Unknown status codes are considered system-level errors + // This includes hardware failures, communication timeouts, etc. + return true + } + } + + // Check for mock device operational errors + errMsg := err.Error() + if strings.Contains(errMsg, "Insufficient capacity") || + strings.Contains(errMsg, "Could not find free namespace") || + strings.Contains(errMsg, "Namespace Already Exists") || + strings.Contains(errMsg, "already has controller") || + strings.Contains(errMsg, "already has controller") || + strings.Contains(errMsg, "not found") { + return false + } + + // Check for CLI parsing errors (not system-level) + if strings.Contains(errMsg, "NSID not found in response output") { + return false + } + + // All other errors are considered system-level failures: + // - Device communication errors + // - Hardware failures + // - Timeout errors + // - Permission/access errors + // - Unknown NVMe status codes + return true +} + func (s *Storage) findVolume(volumeId string) *Volume { for idx, v := range s.volumes { if v.id == volumeId { @@ -727,6 +818,10 @@ func (v *Volume) WaitFormatComplete() error { log.V(2).Info("Wait for format completion") ns, err := v.storage.device.IdentifyNamespace(v.namespaceId) if err != nil { + // System-level failure when identifying namespace during format wait + if isSystemLevelError(err) { + v.storage.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } if ns == nil { @@ -746,6 +841,10 @@ func (v *Volume) WaitFormatComplete() error { ns, err = v.storage.device.IdentifyNamespace(v.namespaceId) if err != nil { + // System-level failure when re-identifying namespace during format wait + if isSystemLevelError(err) { + v.storage.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } if ns == nil { @@ -808,9 +907,17 @@ func (v *Volume) attach(controllerIndex uint16) error { var cmdErr *nvme.CommandError if errors.As(err, &cmdErr) { if cmdErr.StatusCode != nvme.NamespaceAlreadyAttached { + // Only publish error events for system-level failures + if isSystemLevelError(err) { + v.storage.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } } else { + // Non-CommandError types - check if system-level + if isSystemLevelError(err) { + v.storage.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } } @@ -834,9 +941,17 @@ func (v *Volume) detach(controllerIndex uint16) error { var cmdErr *nvme.CommandError if errors.As(err, &cmdErr) { if cmdErr.StatusCode != nvme.NamespaceNotAttached { + // Only publish error events for system-level failures + if isSystemLevelError(err) { + v.storage.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } } else { + // Non-CommandError types - check if system-level + if isSystemLevelError(err) { + v.storage.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } } @@ -866,6 +981,10 @@ func (v *Volume) AttachControllerIfUnattached(controllerIndex uint16) error { attachedControllers, err := v.listAttachedControllers() if err != nil { log.Error(err, "failed to list attached controllers") + // System-level failure when listing attached controllers + if isSystemLevelError(err) { + v.storage.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return err } @@ -1332,6 +1451,10 @@ func (mgr *Manager) StorageIdVolumeIdGet(storageId, volumeId string, model *sf.V ns, err := s.device.IdentifyNamespace(nvme.NamespaceIdentifier(v.namespaceId)) if err != nil { v.log.Error(err, "Identify Namespace Failed") + // System-level failure when identifying namespace for volume retrieval + if isSystemLevelError(err) { + s.notify(sf.UNAVAILABLE_OFFLINE_RST) + } return ec.NewErrInternalServerError() } if ns == nil { diff --git a/pkg/persistent/storage_api.go b/pkg/persistent/storage_api.go index b6c69ed..24812dc 100644 --- a/pkg/persistent/storage_api.go +++ b/pkg/persistent/storage_api.go @@ -1,5 +1,5 @@ /* - * Copyright 2022 Hewlett Packard Enterprise Development LP + * Copyright 2022-2025 Hewlett Packard Enterprise Development LP * Other additional copyright holders may be indicated within. * * The entirety of this work is licensed under the Apache License, @@ -17,15 +17,37 @@ * limitations under the License. */ +// Package persistent provides persistent storage functionality for key-value operations. +// This package implements durable storage mechanisms with support for various backends. package persistent +import ( + "github.com/NearNodeFlash/nnf-ec/pkg/ec" + "github.com/go-logr/logr" +) + +// Package-level logger that can be configured +var packageLogger ec.Logger = logr.Discard() + +// SetLogger configures the package-level logger for all persistent storage operations +func SetLogger(log ec.Logger) { + packageLogger = log.WithName("persistent") +} + +// GetLogger returns the current package logger +func GetLogger() ec.Logger { + return packageLogger +} + +// StorageProvider is the default storage provider instance for the package var StorageProvider = NewLocalPersistentStorageProvider() +// PersistentStorageProvider provides methods for creating persistent storage interfaces type PersistentStorageProvider interface { NewPersistentStorageInterface(path string, readOnly bool) (PersistentStorageApi, error) } -// Persistent Storage API provides an interface for interacting with persistent storage +// PersistentStorageApi provides an interface for interacting with persistent storage. type PersistentStorageApi interface { View(func(txn PersistentStorageTransactionApi) error) error Update(func(txn PersistentStorageTransactionApi) error) error @@ -34,7 +56,7 @@ type PersistentStorageApi interface { Close() error } -// Persistent Storage Transaction API provides an interface for interacting with persistent storage transactions +// PersistentStorageTransactionApi provides an interface for interacting with persistent storage transactions type PersistentStorageTransactionApi interface { NewIterator(prefix string) PersistentStorageIteratorApi Set(key string, value []byte) error diff --git a/pkg/persistent/storage_json.go b/pkg/persistent/storage_json.go index 1f9e5f8..dc53573 100644 --- a/pkg/persistent/storage_json.go +++ b/pkg/persistent/storage_json.go @@ -1,5 +1,5 @@ /* - * Copyright 2022 Hewlett Packard Enterprise Development LP + * Copyright 2022-2025 Hewlett Packard Enterprise Development LP * Other additional copyright holders may be indicated within. * * The entirety of this work is licensed under the Apache License, @@ -22,7 +22,6 @@ package persistent import ( "encoding/json" "io/ioutil" - ) func NewJsonFilePersistentStorageProvider(filename string) PersistentStorageProvider { @@ -34,16 +33,22 @@ type jsonFilePersisentStorageProvider struct { } func (p *jsonFilePersisentStorageProvider) NewPersistentStorageInterface(name string, readOnly bool) (PersistentStorageApi, error) { + log := GetLogger() + log.Info("Opening JSON file storage", "file", p.filename, "name", name, "readOnly", readOnly) + content, err := ioutil.ReadFile(p.filename) if err != nil { + log.Error(err, "Failed to read JSON file", "file", p.filename) return nil, err } var payload map[string]map[string]string if err := json.Unmarshal(content, &payload); err != nil { + log.Error(err, "Failed to unmarshal JSON content", "file", p.filename) return nil, err } + log.Info("Successfully opened JSON file storage", "name", name) return &jsonPersistentStorageInterface{data: payload[name]}, nil } @@ -66,7 +71,3 @@ func (*jsonPersistentStorageInterface) Delete(key string) error { func (*jsonPersistentStorageInterface) Close() error { panic("unimplemented") } - - - - diff --git a/pkg/persistent/storage_local.go b/pkg/persistent/storage_local.go index 8a788d3..60676bd 100644 --- a/pkg/persistent/storage_local.go +++ b/pkg/persistent/storage_local.go @@ -1,5 +1,5 @@ /* - * Copyright 2022 Hewlett Packard Enterprise Development LP + * Copyright 2022-2025 Hewlett Packard Enterprise Development LP * Other additional copyright holders may be indicated within. * * The entirety of this work is licensed under the Apache License, @@ -20,9 +20,13 @@ package persistent import ( + "time" + "github.com/dgraph-io/badger/v3" ) +const garbageCollectPeriod = 24 * time.Hour + func NewLocalPersistentStorageProvider() PersistentStorageProvider { return &localPersistentStorageProvider{} } @@ -40,27 +44,51 @@ type localPersistentStorage struct { } func (s *localPersistentStorage) open(path string, readOnly bool) (err error) { + log := GetLogger().WithValues("path", path, "readOnly", readOnly) + log.Info("BadgerDB: Opening database") + opts := badger.DefaultOptions(path) opts.SyncWrites = true - //opts.ReadOnly = readOnly // Causes ErrLogTruncate opts.BypassLockGuard = readOnly + opts.VerifyValueChecksum = true - // Shrink the in-memory and on-disk size to a more manageable 8 MiB and 16 MiB, respectively; + // Shrink the in-memory and on-disk size to a more manageable 8 MiB and 32 MiB, respectively; // We use very little data and the 64 MiB and 256 MiB defaults will cause OOM issues in kubernetes. - // 8MiB seems to be the lower limit within badger, anything smaller and badger will complain with + // 8MiB seems to be the lower limit within badger, anything smaller and badger complains with // """ // Valuethreshold 1048576 greater than max batch size of 629145. Either reduce opt.ValueThreshold // or increase opt.MaxTableSize. // """ opts.MemTableSize = 8 << 20 - opts.BlockCacheSize = 16 << 20 + opts.BlockCacheSize = 32 << 20 // Increased to 32 MiB for better cache hit ratio s.DB, err = badger.Open(opts) - return err + if err != nil { + log.Error(err, "BadgerDB: Failed to open database") + return err + } + + log.WithValues("mem_table_size", opts.MemTableSize, "block_cache_size", opts.BlockCacheSize).Info("BadgerDB: Database opened successfully") + + // Run garbage collection on existing database during initialization + // Skip GC for read-only databases to avoid potential issues + if !readOnly { + s.RunPeriodicGC(garbageCollectPeriod) + } + + return nil } func (s *localPersistentStorage) Close() error { - return s.DB.Close() + log := GetLogger() + log.Info("BadgerDB: Closing database") + err := s.DB.Close() + if err != nil { + log.Error(err, "BadgerDB: Failed to close database") + } else { + log.Info("BadgerDB: Database closed successfully") + } + return err } func (s *localPersistentStorage) View(fn func(PersistentStorageTransactionApi) error) error { @@ -84,6 +112,38 @@ func (s *localPersistentStorage) Delete(key string) error { return txn.Commit() } +func (s *localPersistentStorage) RunGC() error { + log := GetLogger().WithName("gc") + log.Info("BadgerDB: Starting garbage collection") + + err := s.DB.RunValueLogGC(0.5) + if err != nil { + if err == badger.ErrNoRewrite { + log.Info("BadgerDB: GC completed - no rewrite needed") + return nil + } + log.Error(err, "BadgerDB: GC failed") + return err + } + log.Info("BadgerDB: GC completed successfully") + return nil +} + +func (s *localPersistentStorage) RunPeriodicGC(interval time.Duration) { + log := GetLogger().WithName("periodic-gc").WithValues("interval", interval) + log.Info("BadgerDB: Starting periodic GC") + + ticker := time.NewTicker(interval) + go func() { + defer ticker.Stop() + for range ticker.C { + if err := s.RunGC(); err != nil { + log.Error(err, "BadgerDB: Periodic GC encountered error") + } + } + }() +} + type localPersistentStorageTransaction struct { *badger.Txn }