[hamistao]

This aims to fix the currently failing TestVolumeEncryption and TestMonitoringIntegration.

TestVolumeEncryption is fixed by restoring an encrypted backup onto an encrypted volume and TestMonitoringIntegration is fixed by installing Longhorn without the network policies that prevent Prometheus to scrape Longhorn's data. TestMonitoringIntegration now also issues a warning if a pre-installed Longhorn includes such policies.

Also includes other minor improvements such as improved error handling in TestVolumeEncryption, fixed backup cleanup and added workload waits when installing the monitoring chart.

[khushboo-rancher]

In general LGTM, just one question.

[khushboo-rancher]

Why do we need this setting? Is it something mandatory for Longhorn chart now?

[hamistao]

This is the core of this PR's fix. It allows us to install Longhorn without network policies that prevents Prometheus from scraping data from the longhorn-system namespace. Creating the namespaces became the default since 1.12.

[hamistao]

@khushboo-rancher

[lscalabrini01]

LGTM. one question

[lscalabrini01]

Should us verify if the policies.Data is empty?

[hamistao]

@lscalabrini01 Not sure I get the point. If it is empty then we just don't issue any warning.

[floatingman]

Review verdict: Comment, nothing blocking.

Before flagging anything, the risky-looking parts were verified against upstream sources (longhorn-manager, shepherd, rancher/charts):

• Cleanup fix (backup.Name via backupController): the old code deleted a Backup name that never existed (snapshot and backup names are independently randomized, guaranteed 404 at teardown) while leaking the real Backup CR; the snapshot keeps its own separate cleanup. Strict fix.
• backup.Status.Labels as the encrypted-flag source: the longhorn-manager backup controller propagates volume-encrypted: enabled into Status.Labels in the same sync that sets VolumeName, so it is populated once the backup waits complete. ObjectMeta labels or Spec.Labels would have been the wrong source here.
• Restore fail-fast: the failure condition is checked before the done-return on every poll iteration, RestoreFailure is only set on the faulted path (in-progress uses RestoreInProgress), and the poll aborts on first error, so no false positives and no missed terminal failures.
• Timeout bumps (10s interval, 15m for the BackupVolume to appear on a fresh S3 target): proportionate to repo wait conventions.
• ServiceMonitor move to RancherMonitoringNamespace: selection-neutral (the chart's serviceMonitor selectors are {} and the spec keeps NamespaceSelector: [longhorn-system]), RBAC holds for the cluster-owner standard user, cleanup follows the same variable.
• NetworkPolicy warning helper: premise matches the chart (the longhorn-manager policy exists iff restrictInternalTraffic is on, and it blocks port 9500); List(nil) and the steve type are safe.

Also checked and clean: const adoption complete and value-preserving, v1 alias and map[string]any changes are no-ops, uninstall payload flatten equivalent, empty workload lists return immediately so component-disabled installs will not hang.

Five minor/nit findings inline; one open question that has no inline anchor:

Upgrade path: only InstallRancherMonitoringChart gains the workload-readiness waits; UpgradeRancherMonitoringChart still returns once App state is deployed. If the Prometheus-not-yet-scraping race can also bite after an upgrade, the same three waits belong there. If install-only coverage is deliberate, no action needed.

[floatingman]

minor: The comment above ("If no specific value for setting deletingConfirmationFlag was provided...") no longer matches the code: this now sets the flag unconditionally, silently overriding a caller that explicitly passed deletingConfirmationFlag: false, and mutating their map in place. If the force is intended, the comment should say so:

Suggested change

	defaultSettings["deletingConfirmationFlag"] = true
	// Force deletingConfirmationFlag so chart cleanup always works, and force
	// restrictInternalTraffic off so it cannot block Prometheus scraping.
	defaultSettings["deletingConfirmationFlag"] = true

If caller intent should win, the conditional set was lost in the edit: wrap it in if _, ok := defaultSettings["deletingConfirmationFlag"]; !ok { ... }.

[floatingman]

minor: Asymmetric validation between the two forced values. A wrong-typed defaultSettings returns an explicit error, but a wrong-typed networkPolicies (e.g. a string or map[string]string) is silently discarded and replaced with an empty map, so the caller's value vanishes with no signal. The sibling keys should behave the same:

Suggested change

	networkPolicies = map[string]any{}
	networkPolicies, ok := values["networkPolicies"].(map[string]any)
	if !ok {
	if _, exists := values["networkPolicies"]; exists {
	return errors.New(`Provided values map has invalid value for "networkPolicies"`)
	}
	networkPolicies = map[string]any{}
	values["networkPolicies"] = networkPolicies
	}

[floatingman]

minor: All three waits pass bare metav1.ListOptions{}, so they gate on every Deployment, DaemonSet, and StatefulSet in cattle-monitoring-system, not just this release. Any unrelated or leftover workload in that namespace (failed prior cleanup, another chart's components) that never becomes ready will stall the install until the per-workload watch timeout. Scoping to the release costs one line and makes the wait honest:

Suggested change

	err = shepherdCharts.WatchAndWaitDeployments(client, installOptions.Cluster.ID, RancherMonitoringNamespace, metav1.ListOptions{})
	releaseSelector := metav1.ListOptions{LabelSelector: "app.kubernetes.io/instance=" + RancherMonitoringName}
	err = shepherdCharts.WatchAndWaitDeployments(client, installOptions.Cluster.ID, RancherMonitoringNamespace, releaseSelector)

(and the same selector for the DaemonSet and StatefulSet calls).

[floatingman]

minor: This readiness gate is weaker than it looks: shepherd's WatchAndWaitDeployments/DaemonSets/StatefulSets discard WatchWait's return value, so a workload that never becomes ready produces a silent nil after the watch timeout rather than an error. The new code guarantees "waited up to the timeout", not "ready". Worth a one-line call-site comment (and ideally a shepherd fix) so future callers are not lulled by the names:

Suggested change

	return shepherdCharts.WatchAndWaitStatefulSets(client, installOptions.Cluster.ID, RancherMonitoringNamespace, metav1.ListOptions{})
	// Note: shepherd WatchAndWait* swallow per-workload watch timeouts; a stuck
	// workload delays but does not fail this install.
	return shepherdCharts.WatchAndWaitStatefulSets(client, installOptions.Cluster.ID, RancherMonitoringNamespace, releaseSelector)

[floatingman]

nit: This is the one remaining nested InstallOptions: InstallOptions{...} construction; the PR flattens the same shape everywhere else (e.g. UninstallLonghornChart). Both compile, purely a consistency follow-through of the PR's own flattening.

[Copilot]

🟡 Changes recommended

Multiple critical compile errors and moderate test logic issues must be corrected before approval.

10 open issues

• Initialize embedded InstallOptions for Cluster · New
• Initialize Secret metadata through ObjectMeta · New
• Initialize PayloadOpts fields through InstallOptions · New
• Initialize Service metadata through ObjectMeta · New
• Initialize PayloadOpts through embedded InstallOptions · New
• Initialize PayloadOpts fields through InstallOptions · New
• Initialize Secret metadata through ObjectMeta · New
• Invert alerting chart installation skip condition · New
• Refresh catalog before resolving monitoring version · New
• Refresh catalog before resolving chart version · New

What changed in this PR

This pull request fixes Longhorn encryption and monitoring integration tests by improving encrypted backup restoration, chart installation behavior, repository refreshes, cleanup, and workload readiness checks.

Changes:

• Restores encrypted Longhorn backups into encrypted volumes with improved error handling and cleanup.
• Disables restrictive Longhorn network policies for new installations and improves monitoring setup.
• Adds chart repository refresh support and monitoring workload readiness waits.
• Updates related chart installation tests and diagnostics.

File	Reviewed changes and findings
validation/​longhorn/​longhorn_test.go	Updates Longhorn installation, encrypted backup restoration, and monitoring integration. Critical (2 votes): PayloadOpts uses promoted embedded fields in a composite literal. Critical (2 votes): corev1.Secret uses promoted metadata fields directly.
validation/​longhorn/​chartinstall/​installation_test.go	Removes an outdated replica-default comment.
validation/​charts/​neuvector_test.go	Refreshes the chart repository before installation. Critical (2 votes): PayloadOpts uses promoted embedded fields directly. Moderate (2 votes): the repository refresh occurs after the chart version is resolved.
validation/​charts/​monitoring_test.go	Refreshes the repository and updates monitoring resources. Moderate (2 votes): refresh occurs after the version lookup. Critical (2 votes): corev1.Service uses promoted metadata fields directly.
validation/​charts/​logging_test.go	Refreshes the repository and updates Longhorn payload construction. Critical (2 votes): promoted embedded fields are used directly in a PayloadOpts literal.
validation/​charts/​installations_test.go	Refreshes the Rancher chart repository during suite setup.
validation/​charts/​alerting_test.go	Adds conditional Alerting installation. Moderate (4 votes): the installation guard is inverted, preventing installation on a clean cluster.
actions/​kubeapi/​longhorn/​longhorn.go	Improves backup cleanup, encrypted-volume restoration, and restore polling. Critical (2 votes): corev1.Secret uses promoted metadata fields directly at multiple locations.
actions/​charts/​repo.go	Adds functionality to force and wait for chart repository updates.
actions/​charts/​ranchermonitoring.go	Waits for monitoring Deployments, DaemonSets, and StatefulSets after installation.
actions/​charts/​longhorn.go	Adds Longhorn network policy defaults and updates CRD handling. Critical (2 votes): PayloadOpts uses Cluster as a direct keyed field despite the embedded InstallOptions.
actions/​charts/​action_diagnostics_test.go	Uses shared monitoring namespace constants and explicit import aliases.

_{Review effort: Lite}