refactor(longhorn): cleanup pass following Go best practices

- Drop dead BuildHelmValues exported wrapper. Tests in the same package
  use the unexported buildHelmValues directly; the public alias was
  added under a misleading 'for tests' comment with no external caller.
- Skip ensureISCSI on the upgrade path. The iSCSI DaemonSet runs once
  per cluster on first install; re-running it on every helm upgrade
  burned a 3-minute readiness wait for nothing. Now Install consults
  the Helm release history first and only provisions iSCSI when the
  release is absent.
- Fix Config struct doc. The previous comment claimed 'Enabled defaults
  to true so providers that ship a Longhorn block get the install for
  free' which contradicts IsEnabled() (nil Config = do not install).
  Reword to match the actual semantics.
- Drop the conda-store reference from existing/hetzner config comments.
  Nebari no longer ships conda-store; jupyterhub shared-storage for
  group dirs is the relevant RWX use case.

Author: aktech

pkg/provider/aws/config.go

@@ -95,4 +95,3 @@ func (c *Config) EFSStorageClassName() string {
 	}
 	return c.EFS.StorageClassName
 }
-

pkg/provider/existing/config.go

@@ -30,8 +30,8 @@ type Config struct {
 	// distributed/replicated block + RWX storage. The block is required to
 	// opt-in (nil means "do not install"). Use this on bare-metal / hetzner-k3s
 	// clusters that lack a managed RWX StorageClass — without it, charts that
-	// need RWX (jupyterhub shared-storage, conda-store) have to fall back to
-	// in-cluster NFS-on-RWO hacks.
+	// need RWX (e.g. jupyterhub shared-storage for group dirs) fall back to
+	// the in-cluster NFS-on-RWO workaround.
 	Longhorn *longhorn.Config `yaml:"longhorn,omitempty"`
 }
 

pkg/provider/hetzner/config.go

@@ -35,8 +35,8 @@ type Config struct {
 
 	// Longhorn opts the Hetzner provider into installing Longhorn distributed
 	// block storage on the cluster after creation. Hetzner's hcloud-volumes
-	// CSI is RWO-only; charts that need RWX (jupyterhub shared-storage,
-	// conda-store, etc.) require Longhorn (or another RWX provider) to avoid
+	// CSI is RWO-only; charts that need RWX (e.g. jupyterhub shared-storage
+	// for group dirs) require Longhorn — or another RWX provider — to avoid
 	// the in-cluster NFS-on-RWO workaround. Omit the block to skip the
 	// install; explicit `enabled: false` also disables.
 	Longhorn *longhorn.Config `yaml:"longhorn,omitempty"`

pkg/storage/longhorn/config.go

@@ -28,10 +28,10 @@ const (
 
 // Config carries the user-tunable Longhorn settings shared across providers.
 //
-// All fields are optional — Enabled defaults to true so providers that ship a
-// Longhorn block in their config get the install for free, and ReplicaCount
-// defaults to 2 (sensible for small clusters; cloud production deploys can
-// raise it).
+// A nil *Config means "do not install" (see IsEnabled). When the user supplies
+// a non-nil Config, Enabled defaults to true so an empty block (`longhorn: {}`)
+// is the minimal opt-in. ReplicaCount defaults to 2 — appropriate for small
+// clusters; production deploys should raise it.
 type Config struct {
 	Enabled        *bool             `yaml:"enabled,omitempty"`
 	ReplicaCount   int               `yaml:"replica_count,omitempty"`

pkg/storage/longhorn/install.go

@@ -19,12 +19,15 @@ import (
 const installTimeout = 10 * time.Minute
 
 // Install installs (or upgrades, if a release exists) Longhorn on the cluster
-// the kubeconfigBytes connect to. iSCSI prerequisites are deployed first as a
-// DaemonSet and waited on before the Helm install runs.
+// the kubeconfigBytes connect to.
 //
-// Install is idempotent: re-running it on a cluster with an existing
-// "longhorn" release in the longhorn-system namespace upgrades it in place
-// using the same Helm values.
+// On a fresh cluster, the iSCSI prerequisite DaemonSet is deployed and waited
+// on before the Helm install. On clusters with an existing release, the iSCSI
+// step is skipped (it was already provisioned on first install) and the Helm
+// release is upgraded in place.
+//
+// Install is idempotent: re-running on an installed cluster is a no-op modulo
+// any Config changes that would shift the rendered Helm values.
 func Install(ctx context.Context, kubeconfigBytes []byte, cfg *Config) error {
 	tracer := otel.Tracer("nebari-infrastructure-core")
 	ctx, span := tracer.Start(ctx, "longhorn.Install")
@@ -36,11 +39,6 @@ func Install(ctx context.Context, kubeconfigBytes []byte, cfg *Config) error {
 		attribute.Bool("dedicated_nodes", cfg != nil && cfg.DedicatedNodes),
 	)
 
-	if err := ensureISCSI(ctx, kubeconfigBytes); err != nil {
-		span.RecordError(err)
-		return fmt.Errorf("failed to install iSCSI prerequisites: %w", err)
-	}
-
 	kubeconfigPath, cleanup, err := helm.WriteTempKubeconfig(kubeconfigBytes)
 	if err != nil {
 		span.RecordError(err)
@@ -62,12 +60,19 @@ func Install(ctx context.Context, kubeconfigBytes []byte, cfg *Config) error {
 	histClient := action.NewHistory(actionConfig)
 	histClient.Max = 1
 	if _, err := histClient.Run(ReleaseName); err == nil {
+		// Release exists — iSCSI was provisioned on initial install, so skip
+		// the 3-minute DaemonSet readiness wait and go straight to upgrade.
 		status.Send(ctx, status.NewUpdate(status.LevelInfo, "Longhorn already installed, upgrading").
 			WithResource("longhorn").
 			WithAction("upgrading"))
 		return upgrade(ctx, actionConfig, cfg)
 	}
 
+	if err := ensureISCSI(ctx, kubeconfigBytes); err != nil {
+		span.RecordError(err)
+		return fmt.Errorf("failed to install iSCSI prerequisites: %w", err)
+	}
+
 	helmValues := buildHelmValues(cfg)
 
 	status.Send(ctx, status.NewUpdate(status.LevelProgress, "Installing Longhorn storage").
@@ -161,7 +166,7 @@ func loadChart(chartPathOptions action.ChartPathOptions) (*chart.Chart, error) {
 }
 
 // buildHelmValues turns a Config into the values map passed to the Longhorn
-// Helm chart. Exported as helmValues() via BuildHelmValues for tests.
+// Helm chart.
 func buildHelmValues(cfg *Config) map[string]any {
 	persistence := map[string]any{
 		"defaultClass":             true,
@@ -207,7 +212,3 @@ func buildHelmValues(cfg *Config) map[string]any {
 
 	return values
 }
-
-// BuildHelmValues exposes buildHelmValues for cross-package tests that want
-// to assert the values map shape without actually running Helm.
-func BuildHelmValues(cfg *Config) map[string]any { return buildHelmValues(cfg) }