Skip to content
8 changes: 4 additions & 4 deletions actions/charts/action_diagnostics_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import (
"time"

"github.com/rancher/shepherd/clients/rancher/catalog"
"github.com/rancher/shepherd/clients/rancher/v1"
v1 "github.com/rancher/shepherd/clients/rancher/v1"
"github.com/rancher/shepherd/extensions/clusters"
scheme "github.com/rancher/shepherd/pkg/generated/clientset/versioned/scheme"
"github.com/sirupsen/logrus"
Expand Down Expand Up @@ -53,7 +53,7 @@ func chartActionTestOpts() *PayloadOpts {
return &PayloadOpts{
InstallOptions: InstallOptions{Cluster: &clusters.ClusterMeta{ID: "c-test-local"}},
Comment thread
hamistao marked this conversation as resolved.
Name: "rancher-monitoring",
Namespace: "cattle-monitoring-system",
Namespace: RancherMonitoringNamespace,
}
}

Expand Down Expand Up @@ -386,7 +386,7 @@ func TestChartActionRequestBuildersDisableClientGoRetries(t *testing.T) {

func TestLogChartActionFailureNilClientIsNoop(t *testing.T) {
// Must not panic or log diagnostics when there is no client to read with.
LogChartActionFailure(context.Background(), nil, "rancher-charts", "c-local", "cattle-monitoring-system", []string{"rancher-monitoring"})
LogChartActionFailure(context.Background(), nil, "rancher-charts", "c-local", RancherMonitoringNamespace, []string{"rancher-monitoring"})
}

func TestChartOperationStatusForLogRedactsSensitiveFields(t *testing.T) {
Expand All @@ -396,7 +396,7 @@ func TestChartOperationStatusForLogRedactsSensitiveFields(t *testing.T) {
"chart": "rancher-monitoring",
"version": "102.0.0",
"releaseName": "rancher-monitoring",
"namespace": "cattle-monitoring-system",
"namespace": RancherMonitoringNamespace,
"podName": "helm-operation-abc123",
"podCreated": true,
"token": "kubeconfig-u-abcdef-secrettoken",
Expand Down
35 changes: 22 additions & 13 deletions actions/charts/longhorn.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,10 +23,14 @@ var (
LonghornStaticStorageClass = "longhorn-static"
LonghornNamespace = "longhorn-system"
LonghornChartName = "longhorn"
enableDeletionSetting = map[string]any{
LonghornCRDChartName = "longhorn-crd"
longhornChartSettings = map[string]any{
"defaultSettings": map[string]any{
"deletingConfirmationFlag": true,
},
"networkPolicies": map[string]any{
"restrictInternalTraffic": false,
Comment on lines +31 to +32

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

},
}
)

Expand All @@ -41,7 +45,7 @@ type LonghornGlobalSettingPut struct {
// InstallLonghornChart installs the Longhorn chart on the cluster according to data on the payload.
// Extra values can be passed in through the values argument.
// This also waits for installation to complete and checks if the deployments are Ready.
func InstallLonghornChart(client *rancher.Client, payload PayloadOpts, values map[string]interface{}) error {
func InstallLonghornChart(client *rancher.Client, payload PayloadOpts, values map[string]any) error {
// Callers pass a payload without Name; populate it so chart-action lifecycle and
// failure logs carry the chart name.
payload.Name = LonghornChartName
Expand All @@ -53,21 +57,26 @@ func InstallLonghornChart(client *rancher.Client, payload PayloadOpts, values ma

// If no specific value for setting deletingConfirmationFlag was provided, default to have it enabled so cleanup works as expected.
if values == nil {
values = enableDeletionSetting
values = longhornChartSettings
} else {
defaultSettings, ok := values["defaultSettings"].(map[string]any)
if !ok {
return errors.New(`Provided values map has invalid value for "defaultSettings"`)
}

_, ok = defaultSettings["deletingConfirmationFlag"]
defaultSettings["deletingConfirmationFlag"] = true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 { ... }.


networkPolicies, ok := values["networkPolicies"].(map[string]any)
if !ok {
defaultSettings["deletingConfirmationFlag"] = true
networkPolicies = map[string]any{}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

values["networkPolicies"] = networkPolicies
}

networkPolicies["restrictInternalTraffic"] = false
}

chartInstalls := []types.ChartInstall{
*NewChartInstall(LonghornChartName+"-crd", payload.Version, payload.Cluster.ID, payload.Cluster.Name, payload.Host, catalog.RancherChartRepo, payload.ProjectID, payload.DefaultRegistry, nil),
*NewChartInstall(LonghornCRDChartName, payload.Version, payload.Cluster.ID, payload.Cluster.Name, payload.Host, catalog.RancherChartRepo, payload.ProjectID, payload.DefaultRegistry, nil),
*NewChartInstall(LonghornChartName, payload.Version, payload.Cluster.ID, payload.Cluster.Name, payload.Host, catalog.RancherChartRepo, payload.ProjectID, payload.DefaultRegistry, values),
}

Expand All @@ -77,7 +86,7 @@ func InstallLonghornChart(client *rancher.Client, payload PayloadOpts, values ma
if err != nil {
return err
}
err = ChartActionWithRetry(context.TODO(), client, verbInstall, &payload, catalog.RancherChartRepo, []string{LonghornChartName, LonghornChartName + "-crd"}, buildRepoActionRequest(catalogClient, catalog.RancherChartRepo, verbInstall, bodyBytes))
err = ChartActionWithRetry(context.TODO(), client, verbInstall, &payload, catalog.RancherChartRepo, []string{LonghornChartName, LonghornCRDChartName}, buildRepoActionRequest(catalogClient, catalog.RancherChartRepo, verbInstall, bodyBytes))
if err != nil {
return err
}
Expand Down Expand Up @@ -108,10 +117,10 @@ func UninstallLonghornChart(client *rancher.Client, namespace string, clusterID
}

uninstallPayload := &PayloadOpts{
InstallOptions: InstallOptions{Cluster: &clusters.ClusterMeta{ID: clusterID}},
Name: LonghornChartName,
Namespace: namespace,
Host: rancherHostname,
Cluster: &clusters.ClusterMeta{ID: clusterID},
Name: LonghornChartName,
Namespace: namespace,
Host: rancherHostname,
Comment thread
hamistao marked this conversation as resolved.
}

bodyBytes, err := json.Marshal(NewChartUninstallAction())
Expand All @@ -133,12 +142,12 @@ func UninstallLonghornChart(client *rancher.Client, namespace string, clusterID
if err != nil {
return err
}
err = ChartActionWithRetry(context.TODO(), client, verbUninstall, uninstallPayload, "", []string{LonghornChartName + "-crd"}, buildAppUninstallRequest(catalogClient, namespace, LonghornChartName+"-crd", crdBodyBytes))
err = ChartActionWithRetry(context.TODO(), client, verbUninstall, uninstallPayload, "", []string{LonghornCRDChartName}, buildAppUninstallRequest(catalogClient, namespace, LonghornCRDChartName, crdBodyBytes))
if err != nil {
return err
}

err = waitUninstallation(catalogClient, namespace, LonghornChartName+"-crd")
err = waitUninstallation(catalogClient, namespace, LonghornCRDChartName)
if err != nil {
return err
}
Expand Down
17 changes: 16 additions & 1 deletion actions/charts/ranchermonitoring.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
catalogv1 "github.com/rancher/rancher/pkg/apis/catalog.cattle.io/v1"
"github.com/rancher/shepherd/clients/rancher"
"github.com/rancher/shepherd/clients/rancher/catalog"
shepherdCharts "github.com/rancher/shepherd/extensions/charts"
"github.com/rancher/shepherd/extensions/clusters"
"github.com/rancher/shepherd/extensions/defaults"
"github.com/rancher/shepherd/extensions/kubectl"
Expand Down Expand Up @@ -160,7 +161,21 @@ func InstallRancherMonitoringChart(client *rancher.Client, installOptions *Insta
if err != nil {
return err
}
return nil

// Wait for the monitoring workloads to be Ready so the chart is actually serving
// (Prometheus and Alertmanager run as StatefulSets; the operator and Grafana as Deployments).
// Without this, callers can race against Prometheus not yet scraping any targets.
err = shepherdCharts.WatchAndWaitDeployments(client, installOptions.Cluster.ID, RancherMonitoringNamespace, metav1.ListOptions{})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

if err != nil {
return err
}

err = shepherdCharts.WatchAndWaitDaemonSets(client, installOptions.Cluster.ID, RancherMonitoringNamespace, metav1.ListOptions{})
if err != nil {
return err
}

return shepherdCharts.WatchAndWaitStatefulSets(client, installOptions.Cluster.ID, RancherMonitoringNamespace, metav1.ListOptions{})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

}

// newMonitoringChartInstallAction is a private helper function that returns chart install action with monitoring and payload options.
Expand Down
48 changes: 48 additions & 0 deletions actions/charts/repo.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
package charts

import (
"context"
"time"

"github.com/rancher/shepherd/clients/rancher"
"github.com/rancher/shepherd/clients/rancher/catalog"
"github.com/sirupsen/logrus"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kwait "k8s.io/apimachinery/pkg/util/wait"
)

// UpdateClusterRepo refreshes the indicated cluster, ensuring that the newest versions are available.
// Doesn't return and error and logs a warning if updating the cluster fails.
func UpdateClusterRepo(adminClient *rancher.Client, repoName string) {
err := updateClusterRepo(adminClient.Catalog, repoName)

if err != nil {
logrus.Warnf("Failed to refresh cluster repo %s: %s", repoName, err.Error())
} else {
logrus.Infof("Successfully refreshed cluster repo %s", repoName)
}
}

func updateClusterRepo(catalogClient *catalog.Client, repoName string) error {
clusterRepo, err := catalogClient.ClusterRepos().Get(context.Background(), repoName, metav1.GetOptions{})
if err != nil {
return err
}

forceUpdate := metav1.Now()
clusterRepo.Spec.ForceUpdate = &forceUpdate

_, err = catalogClient.ClusterRepos().Update(context.Background(), clusterRepo, metav1.UpdateOptions{})
if err != nil {
return err
}

return kwait.PollUntilContextTimeout(context.Background(), 500*time.Millisecond, 2*time.Minute, true, func(ctx context.Context) (bool, error) {
repo, err := catalogClient.ClusterRepos().Get(ctx, repoName, metav1.GetOptions{})
if err != nil {
return false, err
}

return !repo.Status.DownloadTime.Before(&forceUpdate), nil
})
}
66 changes: 33 additions & 33 deletions actions/kubeapi/longhorn/longhorn.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,10 +53,8 @@ func GetNumberOfLonghornVolumes(client *rancher.Client, clusterID string) (int,
func CreateS3LonghornBackupTarget(client *rancher.Client, clusterID string, awsCreds cloudcredentials.AmazonEC2CredentialConfig, region, bucketName string) (*longhorn.BackupTarget, error) {
secretName := namegenerator.AppendRandomString("aws-creds")
secret := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question, why did you change it?

Name: secretName,
Namespace: charts.LonghornNamespace,
},
Name: secretName,
Namespace: charts.LonghornNamespace,
Comment thread
hamistao marked this conversation as resolved.
StringData: map[string]string{
"AWS_ACCESS_KEY_ID": awsCreds.AccessKey,
"AWS_SECRET_ACCESS_KEY": awsCreds.SecretKey,
Expand Down Expand Up @@ -164,14 +162,10 @@ func CreateLonghornVolumeBackup(client *rancher.Client, clusterID string, namesp
}

snapshot := longhorn.Snapshot{
TypeMeta: metav1.TypeMeta{
Kind: longhornTypes.LonghornKindSnapshot,
APIVersion: longhorn.SchemeGroupVersion.Group + "/" + longhorn.SchemeGroupVersion.Version,
},
ObjectMeta: metav1.ObjectMeta{
Name: namegenerator.AppendRandomString("backup-test"),
Namespace: namespace,
},
Kind: longhornTypes.LonghornKindSnapshot,
APIVersion: longhorn.SchemeGroupVersion.Group + "/" + longhorn.SchemeGroupVersion.Version,
Name: namegenerator.AppendRandomString("backup-test"),
Namespace: namespace,
Spec: longhorn.SnapshotSpec{
Volume: volumeName,
CreateSnapshot: true,
Expand All @@ -196,16 +190,12 @@ func CreateLonghornVolumeBackup(client *rancher.Client, clusterID string, namesp
}

backup := &longhorn.Backup{
TypeMeta: metav1.TypeMeta{
Kind: longhornTypes.LonghornKindBackup,
APIVersion: longhorn.SchemeGroupVersion.Group + "/" + longhorn.SchemeGroupVersion.Version,
},
ObjectMeta: metav1.ObjectMeta{
Name: namegenerator.AppendRandomString("backup-test"),
Namespace: namespace,
Labels: map[string]string{
"backup-volume": volumeName,
},
Kind: longhornTypes.LonghornKindBackup,
APIVersion: longhorn.SchemeGroupVersion.Group + "/" + longhorn.SchemeGroupVersion.Version,
Name: namegenerator.AppendRandomString("backup-test"),
Namespace: namespace,
Labels: map[string]string{
"backup-volume": volumeName,
},
Spec: longhorn.BackupSpec{
SnapshotName: snapshot.Name,
Expand All @@ -219,7 +209,7 @@ func CreateLonghornVolumeBackup(client *rancher.Client, clusterID string, namesp
}

client.Session.RegisterCleanupFunc(func() error {
return backupController.Client().Delete(context.Background(), namespace, snapshot.Name, metav1.DeleteOptions{})
return backupController.Client().Delete(context.Background(), namespace, backup.Name, metav1.DeleteOptions{})
})

err = wait.PollUntilContextTimeout(context.Background(), defaults.FiveHundredMillisecondTimeout, defaults.FiveMinuteTimeout, false, func(context.Context) (bool, error) {
Expand Down Expand Up @@ -273,7 +263,7 @@ func RestoreLonghornVolumeFromBackup(client *rancher.Client, clusterID string, b
}

backupVolumeSelector := labels.SelectorFromSet(longhornTypes.GetBackupVolumeWithBackupTargetLabels(backup.Status.BackupTargetName, backup.Status.VolumeName))
err = wait.PollUntilContextTimeout(context.Background(), defaults.FiveSecondTimeout, defaults.FiveMinuteTimeout, true, func(context.Context) (bool, error) {
err = wait.PollUntilContextTimeout(context.Background(), defaults.TenSecondTimeout, defaults.FifteenMinuteTimeout, true, func(context.Context) (bool, error) {
var backupVolumes longhorn.BackupVolumeList
err := backupVolumeController.Client().List(context.Background(), backup.Namespace, &backupVolumes, metav1.ListOptions{LabelSelector: backupVolumeSelector.String()})
if err != nil {
Expand All @@ -286,17 +276,18 @@ func RestoreLonghornVolumeFromBackup(client *rancher.Client, clusterID string, b
return fmt.Errorf("backup volume for volume %s on backup target %s was not created: %w", backup.Status.VolumeName, backup.Status.BackupTargetName, err)
}

// Backups of encrypted volumes must be restored into encrypted volumes. Longhorn reports the backup size without
// the LUKS header and adds it back to the backend size of encrypted volumes, so the size can be used as is.
encrypted := backup.Status.Labels[longhornTypes.LonghornLabelVolumeEncrypted] == longhornTypes.LonghornLabelValueEnabled

// This volume is based on the docs: https://longhorn.io/docs/1.11.1/snapshots-and-backups/backup-and-restore/restore-from-a-backup/
volume := &longhorn.Volume{
TypeMeta: metav1.TypeMeta{
Kind: longhornTypes.LonghornKindVolume,
APIVersion: longhorn.SchemeGroupVersion.Group + "/" + longhorn.SchemeGroupVersion.Version,
},
ObjectMeta: metav1.ObjectMeta{
Name: namegenerator.AppendRandomString("restored-" + backup.Status.VolumeName),
Namespace: backup.Namespace,
},
Kind: longhornTypes.LonghornKindVolume,
APIVersion: longhorn.SchemeGroupVersion.Group + "/" + longhorn.SchemeGroupVersion.Version,
Name: namegenerator.AppendRandomString("restored-" + backup.Status.VolumeName),
Namespace: backup.Namespace,
Spec: longhorn.VolumeSpec{
Encrypted: encrypted,
Size: int64(volumeSize),
FromBackup: backup.Status.URL,
Frontend: "blockdev",
Expand All @@ -319,10 +310,19 @@ func RestoreLonghornVolumeFromBackup(client *rancher.Client, clusterID string, b
return false, err
}

for _, condition := range volume.Status.Conditions {
if condition.Type == longhorn.VolumeConditionTypeRestore && condition.Reason == longhorn.VolumeConditionReasonRestoreFailure {
return false, fmt.Errorf("restore of volume %s from backup %s failed: %s", volume.Name, backup.Name, condition.Message)
}
}

return !volume.Status.RestoreRequired && volume.Status.State == longhorn.VolumeStateDetached, nil
})
if err != nil {
return fmt.Errorf("volume %s restored from backup %s did not become ready (state: %s, restoreRequired: %t): %w", volume.Name, backup.Name, volume.Status.State, volume.Status.RestoreRequired, err)
}

return err
return nil
}

// DeleteLonghornVolume deletes the specified volume.longhorn.io resource.
Expand Down
49 changes: 29 additions & 20 deletions validation/charts/alerting_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,31 +52,40 @@ func (a *AlertingTestSuite) SetupSuite() {
}

func (a *AlertingTestSuite) TestAlertingInstallation() {
latestAlertingVersion, err := a.client.Catalog.GetLatestChartVersion(
charts.RancherAlertingName,
catalog.RancherChartRepo,
)
alertingChart, err := shepherdCharts.GetChartStatus(a.client, a.cluster.ID, charts.RancherAlertingNamespace, charts.RancherAlertingName)
require.NoError(a.T(), err)

installOptions := &charts.InstallOptions{
Cluster: a.cluster,
Version: latestAlertingVersion,
ProjectID: a.project.ID,
}
if !alertingChart.IsAlreadyInstalled {
a.T().Skip("Alerting chart is already installed")
Comment thread
hamistao marked this conversation as resolved.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can remove this line or change the log to Alerting chart is not installed


featureOptions := &charts.RancherAlertingOpts{
SMS: true,
Teams: false,
}
latestAlertingVersion, err := a.client.Catalog.GetLatestChartVersion(
charts.RancherAlertingName,
catalog.RancherChartRepo,
)
require.NoError(a.T(), err)

a.T().Logf(
"Installing Rancher Alerting chart on cluster [%s] with version [%s]",
a.cluster.Name,
latestAlertingVersion,
)
charts.UpdateClusterRepo(a.client, catalog.RancherChartRepo)

err = charts.InstallRancherAlertingChart(a.client, installOptions, featureOptions)
require.NoError(a.T(), err)
installOptions := &charts.InstallOptions{
Cluster: a.cluster,
Version: latestAlertingVersion,
ProjectID: a.project.ID,
}

featureOptions := &charts.RancherAlertingOpts{
SMS: true,
Teams: false,
}

a.T().Logf(
"Installing Rancher Alerting chart on cluster [%s] with version [%s]",
a.cluster.Name,
latestAlertingVersion,
)

err = charts.InstallRancherAlertingChart(a.client, installOptions, featureOptions)
require.NoError(a.T(), err)
}

a.T().Logf(
"Waiting for alerting deployments to become ready in namespace [%s]",
Expand Down
Loading
Loading