diff --git a/pkg/controllers/dashboard/helm/repo.go b/pkg/controllers/dashboard/helm/repo.go index d04c68e1523..a32e126e113 100644 --- a/pkg/controllers/dashboard/helm/repo.go +++ b/pkg/controllers/dashboard/helm/repo.go @@ -6,9 +6,10 @@ import ( "context" "encoding/json" "fmt" - "k8s.io/apimachinery/pkg/api/equality" "time" + "k8s.io/apimachinery/pkg/api/equality" + catalog "github.com/rancher/rancher/pkg/apis/catalog.cattle.io/v1" "github.com/rancher/rancher/pkg/catalogv2" "github.com/rancher/rancher/pkg/catalogv2/git" @@ -31,10 +32,11 @@ import ( const ( maxSize = 100_000 - defaultInterval = 6 * time.Hour + defaultInterval = 1 * time.Hour + repoCondition = catalog.RepoDownloaded ) -var defaultRetryPolicy = retryPolicy{ +var defaultHandlerErrRetryPolicy = retryPolicy{ MinWait: 5 * time.Minute, MaxWait: 20 * time.Minute, MaxRetry: 3, @@ -62,7 +64,7 @@ func RegisterRepos(ctx context.Context, apply: apply.WithCacheTypes(configMap).WithStrictCaching().WithSetOwnerReference(false, false), } - clusterRepos.OnChange(ctx, "helm-clusterrepo-download-on-change", h.ClusterRepoDownloadStatusHandler2) + clusterRepos.OnChange(ctx, "helm-clusterrepo-download-on-change", h.ClusterRepoOnChange) } @@ -93,7 +95,10 @@ func (r *repoHandler) ClusterRepoDownloadEnsureStatusHandler(repo *catalog.Clust return r.ensure(&repo.Spec, status, &repo.ObjectMeta) } -func (r *repoHandler) ClusterRepoDownloadStatusHandler2(key string, repo *catalog.ClusterRepo) (*catalog.ClusterRepo, error) { +func (r *repoHandler) ClusterRepoOnChange(key string, repo *catalog.ClusterRepo) (*catalog.ClusterRepo, error) { + if repo == nil { + return nil, nil + } // Ignore OCI Based Helm Repositories if registry.IsOCI(repo.Spec.URL) { return repo, nil @@ -108,25 +113,21 @@ func (r *repoHandler) ClusterRepoDownloadStatusHandler2(key string, repo *catalo retryPolicy, err := getRetryPolicy(repo) if err != nil { err = fmt.Errorf("failed to get retry policy: %w", err) - return r.setErrorCondition(repo, err, newStatus, interval) - } - if r.shouldSkip(repo, retryPolicy, repo.Name, interval) { - return repo, nil + return setErrorCondition(repo, err, newStatus, interval, repoCondition, r.clusterRepos) } - newStatus.ShouldNotSkip = false - err = ensureIndexConfigMap(repo, &repo.Status, r.configMaps) + err = ensureIndexConfigMap(repo, newStatus, r.configMaps) if err != nil { err = fmt.Errorf("failed to ensure index config map: %w", err) - return r.setErrorCondition(repo, err, newStatus, interval) + return setErrorCondition(repo, err, newStatus, interval, repoCondition, r.clusterRepos) } - if !shouldRefresh(&repo.Spec, &repo.Status, interval) { //reset retries too - r.clusterRepos.EnqueueAfter(repo.Name, interval) + if shouldSkip(repo, retryPolicy, repoCondition, interval, r.clusterRepos, newStatus) { return repo, nil } + newStatus.ShouldNotSkip = false - return r.download(repo, *newStatus, metav1.OwnerReference{ + return r.download(repo, newStatus, metav1.OwnerReference{ APIVersion: catalog.SchemeGroupVersion.Group + "/" + catalog.SchemeGroupVersion.Version, Kind: "ClusterRepo", Name: repo.Name, @@ -221,7 +222,6 @@ func (r *repoHandler) ensure(repoSpec *catalog.RepoSpec, status catalog.RepoStat return status, nil } - status.ObservedGeneration = metadata.Generation secret, err := catalogv2.GetSecret(r.secrets, repoSpec, metadata.Namespace) if err != nil { return status, err @@ -230,7 +230,7 @@ func (r *repoHandler) ensure(repoSpec *catalog.RepoSpec, status catalog.RepoStat return status, git.Ensure(secret, metadata.Namespace, metadata.Name, status.URL, status.Commit, repoSpec.InsecureSkipTLSverify, repoSpec.CABundle) } -func (r *repoHandler) download(repository *catalog.ClusterRepo, newStatus catalog.RepoStatus, owner metav1.OwnerReference, interval time.Duration, retryPolicy retryPolicy) (*catalog.ClusterRepo, error) { +func (r *repoHandler) download(repository *catalog.ClusterRepo, newStatus *catalog.RepoStatus, owner metav1.OwnerReference, interval time.Duration, retryPolicy retryPolicy) (*catalog.ClusterRepo, error) { var ( index *repo.IndexFile commit string @@ -239,77 +239,63 @@ func (r *repoHandler) download(repository *catalog.ClusterRepo, newStatus catalo metadata := repository.ObjectMeta repoSpec := repository.Spec - newStatus.ObservedGeneration = metadata.Generation secret, err := catalogv2.GetSecret(r.secrets, &repoSpec, metadata.Namespace) if err != nil { - return r.setErrorCondition(repository, err, &newStatus, interval) + return setErrorCondition(repository, err, newStatus, interval, repoCondition, r.clusterRepos) } downloadTime := metav1.Now() backoff := calculateBackoff(repository, retryPolicy) retriable := false - // git repo and no configmap if repoSpec.GitRepo != "" && newStatus.IndexConfigMapName == "" { - //retry here commit, err = git.Head(secret, metadata.Namespace, metadata.Name, repoSpec.GitRepo, repoSpec.GitBranch, repoSpec.InsecureSkipTLSverify, repoSpec.CABundle) if err != nil { retriable = true } else { newStatus.URL = repoSpec.GitRepo newStatus.Branch = repoSpec.GitBranch - //no retry here, no external calls index, err = git.BuildOrGetIndex(metadata.Namespace, metadata.Name, repoSpec.GitRepo) } - //git repo } else if repoSpec.GitRepo != "" { - //retry here commit, err = git.Update(secret, metadata.Namespace, metadata.Name, repoSpec.GitRepo, repoSpec.GitBranch, repoSpec.InsecureSkipTLSverify, repoSpec.CABundle) if err != nil { retriable = true - //return status, err } else { newStatus.URL = repoSpec.GitRepo newStatus.Branch = repoSpec.GitBranch if newStatus.Commit == commit { newStatus.DownloadTime = downloadTime - return repository, nil + return setErrorCondition(repository, err, newStatus, interval, repoCondition, r.clusterRepos) } - //no retry here, no external calls index, err = git.BuildOrGetIndex(metadata.Namespace, metadata.Name, repoSpec.GitRepo) } - // http repo } else if repoSpec.URL != "" { - //retry here index, err = helmhttp.DownloadIndex(secret, repoSpec.URL, repoSpec.CABundle, repoSpec.InsecureSkipTLSverify, repoSpec.DisableSameOriginCheck) retriable = true newStatus.URL = repoSpec.URL newStatus.Branch = "" - // something weird } else { - return repository, nil + return setErrorCondition(repository, err, newStatus, interval, repoCondition, r.clusterRepos) } if retriable && err != nil { newStatus.NumberOfRetries++ if newStatus.NumberOfRetries > retryPolicy.MaxRetry { newStatus.NumberOfRetries = 0 newStatus.NextRetryAt = metav1.Time{} - return r.setConditionWithInterval(repository, err, &newStatus, nil, interval) + return r.setConditionWithInterval(repository, err, newStatus, nil, interval) } newStatus.NextRetryAt = metav1.Time{Time: timeNow().UTC().Add(backoff)} - return r.setConditionWithInterval(repository, err, &newStatus, &backoff, interval) - } - if err != nil { - return repository, err + return r.setConditionWithInterval(repository, err, newStatus, &backoff, interval) } - if index == nil { - return repository, nil + if err != nil || index == nil { + return setErrorCondition(repository, err, newStatus, interval, repoCondition, r.clusterRepos) } index.SortEntries() cm, err := createOrUpdateMap(metadata.Namespace, index, owner, r.apply) if err != nil { - return repository, err + return setErrorCondition(repository, err, newStatus, interval, repoCondition, r.clusterRepos) } newStatus.IndexConfigMapName = cm.Name @@ -317,15 +303,14 @@ func (r *repoHandler) download(repository *catalog.ClusterRepo, newStatus catalo newStatus.IndexConfigMapResourceVersion = cm.ResourceVersion newStatus.DownloadTime = downloadTime newStatus.Commit = commit - repository.Status = newStatus - return r.clusterRepos.UpdateStatus(repository) + return setErrorCondition(repository, nil, newStatus, interval, repoCondition, r.clusterRepos) } func ensureIndexConfigMap(repo *catalog.ClusterRepo, status *catalog.RepoStatus, configMap corev1controllers.ConfigMapClient) error { // Charts from the clusterRepo will be unavailable if the IndexConfigMap recorded in the status does not exist. // By resetting the value of IndexConfigMapName, IndexConfigMapNamespace, IndexConfigMapResourceVersion to "", // the method shouldRefresh will return true and trigger the rebuild of the IndexConfigMap and accordingly update the status. - if repo.Spec.GitRepo != "" && status.IndexConfigMapName != "" { + if status.IndexConfigMapName != "" { _, err := configMap.Get(status.IndexConfigMapNamespace, status.IndexConfigMapName, metav1.GetOptions{}) if err != nil { if apierrors.IsNotFound(err) { @@ -345,32 +330,6 @@ func ensureIndexConfigMap(repo *catalog.ClusterRepo, status *catalog.RepoStatus, return nil } -func shouldRefresh(spec *catalog.RepoSpec, status *catalog.RepoStatus, interval time.Duration) bool { - // check if branch changed - if spec.GitRepo != "" && status.Branch != spec.GitBranch { - return true - } - // check if url changed - if spec.URL != "" && spec.URL != status.URL { - return true - } - // check if git repo changed - if spec.GitRepo != "" && spec.GitRepo != status.URL { - return true - } - // check if there's no index config map - if status.IndexConfigMapName == "" { - return true - } - // check if it's a force update - if spec.ForceUpdate != nil && spec.ForceUpdate.After(status.DownloadTime.Time) && spec.ForceUpdate.Time.Before(time.Now()) { - return true - } - refreshTime := time.Now().Add(-interval) - // check if interval has passed - return refreshTime.After(status.DownloadTime.Time) -} - func GenerateConfigMapName(ownerName string, index int, UID types.UID) string { return name2.SafeConcatName(ownerName, fmt.Sprint(index), string(UID)) } @@ -385,75 +344,89 @@ func GetConfigMapNamespace(namespace string) string { // setErrorCondition is only called when error happens in the handler, and // we need to depend on wrangler to requeue the handler -func (r *repoHandler) setErrorCondition(clusterRepo *catalog.ClusterRepo, err error, newStatus *catalog.RepoStatus, interval time.Duration) (*catalog.ClusterRepo, error) { +func setErrorCondition(clusterRepo *catalog.ClusterRepo, + err error, + newStatus *catalog.RepoStatus, + interval time.Duration, + cond catalog.RepoCondition, + controller catalogcontrollers.ClusterRepoController) (*catalog.ClusterRepo, error) { var statusErr error newStatus.NumberOfRetries = 0 newStatus.NextRetryAt = metav1.Time{} if err != nil { newStatus.ShouldNotSkip = true } - downloaded := condition.Cond(catalog.RepoDownloaded) + condDownloaded := condition.Cond(cond) + if apierrors.IsConflict(err) { - downloaded.SetError(newStatus, "", nil) - } else { - downloaded.SetError(newStatus, "", err) + err = nil } + condDownloaded.SetError(newStatus, "", err) + newStatus.ObservedGeneration = clusterRepo.Generation if !equality.Semantic.DeepEqual(newStatus, &clusterRepo.Status) { - downloaded.LastUpdated(newStatus, timeNow().UTC().Format(time.RFC3339)) + condDownloaded.LastUpdated(newStatus, timeNow().UTC().Format(time.RFC3339)) clusterRepo.Status = *newStatus - //status handler will run again without waiting and enqueue after won't work - clusterRepo, statusErr = r.clusterRepos.UpdateStatus(clusterRepo) + clusterRepo, statusErr = controller.UpdateStatus(clusterRepo) if statusErr != nil { err = statusErr } if err == nil { - r.clusterRepos.EnqueueAfter(clusterRepo.Name, interval) + controller.EnqueueAfter(clusterRepo.Name, interval) } return clusterRepo, err } if err == nil { - r.clusterRepos.EnqueueAfter(clusterRepo.Name, interval) + controller.EnqueueAfter(clusterRepo.Name, interval) } return clusterRepo, err } // shouldSkip checks certain conditions to see if the handler should be skipped. // For information regarding the conditions, check the comments in the implementation. -func (r *repoHandler) shouldSkip(clusterRepo *catalog.ClusterRepo, policy retryPolicy, key string, ociInterval time.Duration) bool { +func shouldSkip(clusterRepo *catalog.ClusterRepo, + policy retryPolicy, + cond catalog.RepoCondition, + interval time.Duration, + controller catalogcontrollers.ClusterRepoController, + newStatus *catalog.RepoStatus) bool { // this is to prevent the handler from making calls when the crd is outdated. - updatedRepo, err := r.clusterRepos.Get(key, metav1.GetOptions{}) + updatedRepo, err := controller.Get(clusterRepo.Name, metav1.GetOptions{}) if err == nil && updatedRepo.ResourceVersion != clusterRepo.ResourceVersion { return true } - if clusterRepo.Status.ObservedGeneration < clusterRepo.Generation { - clusterRepo.Status.NumberOfRetries = 0 - clusterRepo.Status.NextRetryAt = metav1.Time{} + if newStatus.IndexConfigMapName == "" { + return false + } + + if newStatus.ObservedGeneration < clusterRepo.Generation { + newStatus.NumberOfRetries = 0 + newStatus.NextRetryAt = metav1.Time{} return false } // The handler is triggered immediately after any changes, including when updating the number of retries done. // This check is to prevent the handler from executing before the backoff time has passed - if !clusterRepo.Status.NextRetryAt.IsZero() && clusterRepo.Status.NextRetryAt.Time.After(timeNow().UTC()) { + if !newStatus.NextRetryAt.IsZero() && newStatus.NextRetryAt.Time.After(timeNow().UTC()) { return true } - if clusterRepo.Status.ShouldNotSkip { //checks if we should skip running the handler or not + if newStatus.ShouldNotSkip { //checks if we should skip running the handler or not return false } - downloaded := condition.Cond(catalog.RepoDownloaded) - downloadedUpdateTime, _ := time.Parse(time.RFC3339, downloaded.GetLastUpdated(clusterRepo)) + condDownloaded := condition.Cond(cond) + downloadedUpdateTime, _ := time.Parse(time.RFC3339, condDownloaded.GetLastUpdated(clusterRepo)) - if (clusterRepo.Status.NumberOfRetries > policy.MaxRetry || clusterRepo.Status.NumberOfRetries == 0) && // checks if it's not retrying - clusterRepo.Generation == clusterRepo.Status.ObservedGeneration && // checks if the generation has not changed - downloadedUpdateTime.Add(ociInterval).After(timeNow().UTC()) { // checks if the interval has not passed + if (newStatus.NumberOfRetries > policy.MaxRetry || newStatus.NumberOfRetries == 0) && // checks if it's not retrying + clusterRepo.Generation == newStatus.ObservedGeneration && // checks if the generation has not changed + downloadedUpdateTime.Add(interval).After(timeNow().UTC()) { // checks if the interval has not passed - r.clusterRepos.EnqueueAfter(clusterRepo.Name, ociInterval) + controller.EnqueueAfter(clusterRepo.Name, interval) return true } return false @@ -462,34 +435,44 @@ func (r *repoHandler) shouldSkip(clusterRepo *catalog.ClusterRepo, policy retryP // setConditionWithInterval is called to reenqueue the object // after the interval of 6 hours. func (r *repoHandler) setConditionWithInterval(clusterRepo *catalog.ClusterRepo, err error, newStatus *catalog.RepoStatus, backoff *time.Duration, interval time.Duration) (*catalog.ClusterRepo, error) { - var newErr error + var msg string + backoffInterval := interval.Round(time.Second) if backoff != nil { - newErr = fmt.Errorf("%s. %s", err.Error(), fmt.Sprintf("Will retry after %s", backoff.Round(time.Second))) - } else { - newErr = fmt.Errorf("%s. %s", err.Error(), fmt.Sprintf("Will retry after %s", interval.Round(time.Second))) + backoffInterval = backoff.Round(time.Second) } + msg = fmt.Sprintf("%s. Will retry after %s", err.Error(), backoffInterval) + + return setConditionWithInterval(clusterRepo, err, newStatus, backoffInterval, repoCondition, msg, r.clusterRepos) +} + +func setConditionWithInterval(clusterRepo *catalog.ClusterRepo, + err error, + newStatus *catalog.RepoStatus, + backoff time.Duration, + cond catalog.RepoCondition, + condMessage string, + controller catalogcontrollers.ClusterRepoController, +) (*catalog.ClusterRepo, error) { + newErr := fmt.Errorf(condMessage) + downloaded := condition.Cond(cond) - downloaded := condition.Cond(catalog.RepoDownloaded) if apierrors.IsConflict(err) { - downloaded.SetError(newStatus, "", nil) - } else { - downloaded.SetError(newStatus, "", newErr) + err = nil } + downloaded.SetError(newStatus, "", newErr) + newStatus.ObservedGeneration = clusterRepo.Generation if !equality.Semantic.DeepEqual(newStatus, &clusterRepo.Status) { //Since status has changed, update the lastUpdatedTime downloaded.LastUpdated(newStatus, timeNow().UTC().Format(time.RFC3339)) clusterRepo.Status = *newStatus - _, statusErr := r.clusterRepos.UpdateStatus(clusterRepo) + _, statusErr := controller.UpdateStatus(clusterRepo) if statusErr != nil { return clusterRepo, statusErr } } - if backoff != nil { - r.clusterRepos.EnqueueAfter(clusterRepo.Name, *backoff) - } else { - r.clusterRepos.EnqueueAfter(clusterRepo.Name, interval) - } + controller.EnqueueAfter(clusterRepo.Name, backoff) + return clusterRepo, nil } diff --git a/pkg/controllers/dashboard/helm/repo_oci.go b/pkg/controllers/dashboard/helm/repo_oci.go index f448dc400aa..912e5b2c5c5 100644 --- a/pkg/controllers/dashboard/helm/repo_oci.go +++ b/pkg/controllers/dashboard/helm/repo_oci.go @@ -21,12 +21,10 @@ import ( catalogcontrollers "github.com/rancher/rancher/pkg/generated/controllers/catalog.cattle.io/v1" corev1 "github.com/rancher/rancher/pkg/generated/norman/core/v1" "github.com/rancher/wrangler/v3/pkg/apply" - "github.com/rancher/wrangler/v3/pkg/condition" corev1controllers "github.com/rancher/wrangler/v3/pkg/generated/controllers/core/v1" "github.com/sirupsen/logrus" "helm.sh/helm/v3/pkg/registry" "helm.sh/helm/v3/pkg/repo" - "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -42,7 +40,10 @@ var ( } ) -const defaultOCIInterval = 24 * time.Hour +const ( + defaultOCIInterval = 24 * time.Hour + ociCondition = catalog.OCIDownloaded +) type OCIRepohandler struct { clusterRepoController catalogcontrollers.ClusterRepoController @@ -101,9 +102,16 @@ func (o *OCIRepohandler) onClusterRepoChange(key string, clusterRepo *catalog.Cl retryPolicy, err := getRetryPolicy(clusterRepo) if err != nil { err = fmt.Errorf("failed to get retry policy: %w", err) - return o.setErrorCondition(clusterRepo, err, newStatus, ociInterval) + return setErrorCondition(clusterRepo, err, newStatus, ociInterval, ociCondition, o.clusterRepoController) + } + + err = ensureIndexConfigMap(clusterRepo, newStatus, o.configMapController) + if err != nil { + err = fmt.Errorf("failed to ensure index configmap: %w", err) + return setErrorCondition(clusterRepo, err, newStatus, ociInterval, ociCondition, o.clusterRepoController) } - if o.shouldSkip(clusterRepo, retryPolicy, key, ociInterval) { + + if shouldSkip(clusterRepo, retryPolicy, ociCondition, ociInterval, o.clusterRepoController, newStatus) { return clusterRepo, nil } newStatus.ShouldNotSkip = false @@ -111,12 +119,6 @@ func (o *OCIRepohandler) onClusterRepoChange(key string, clusterRepo *catalog.Cl logrus.Debugf("OCIRepoHandler triggered for clusterrepo %s", clusterRepo.Name) var index *repo.IndexFile - err = ensureIndexConfigMap(clusterRepo, newStatus, o.configMapController) - if err != nil { - err = fmt.Errorf("failed to ensure index configmap: %w", err) - return o.setErrorCondition(clusterRepo, err, newStatus, ociInterval) - } - secret, err := catalogv2.GetSecret(o.secretCacheController, &clusterRepo.Spec, clusterRepo.Namespace) if err != nil { logrus.Errorf("Error while fetching secret for cluster repo %s: %v", clusterRepo.Name, err) @@ -124,7 +126,7 @@ func (o *OCIRepohandler) onClusterRepoChange(key string, clusterRepo *catalog.Cl if reason != metav1.StatusReasonUnknown { err = fmt.Errorf("failed to fetch secret: %s", reason) } - return o.setErrorCondition(clusterRepo, err, newStatus, ociInterval) + return setErrorCondition(clusterRepo, err, newStatus, ociInterval, ociCondition, o.clusterRepoController) } owner := metav1.OwnerReference{ @@ -137,12 +139,12 @@ func (o *OCIRepohandler) onClusterRepoChange(key string, clusterRepo *catalog.Cl downloadTime := metav1.Now() index, err = getIndexfile(clusterRepo.Status, clusterRepo.Spec, o.configMapController, owner, clusterRepo.Namespace) if err != nil { - return o.setErrorCondition(clusterRepo, fmt.Errorf("error while getting indexfile: %w", err), newStatus, ociInterval) + return setErrorCondition(clusterRepo, fmt.Errorf("error while getting indexfile: %w", err), newStatus, ociInterval, ociCondition, o.clusterRepoController) } originalIndexBytes, err := json.Marshal(index) if err != nil { logrus.Errorf("Error while marshalling indexfile for cluster repo %s: %v", clusterRepo.Name, err) - return o.setErrorCondition(clusterRepo, fmt.Errorf("error while reading indexfile"), newStatus, ociInterval) + return setErrorCondition(clusterRepo, fmt.Errorf("error while reading indexfile"), newStatus, ociInterval, ociCondition, o.clusterRepoController) } // Create a new oci client ociClient, err := oci.NewClient(clusterRepo.Spec.URL, clusterRepo.Spec, secret) @@ -195,20 +197,20 @@ func (o *OCIRepohandler) onClusterRepoChange(key string, clusterRepo *catalog.Cl } if index == nil || len(index.Entries) <= 0 { err = errors.New("there are no helm charts in the repository specified") - return o.setErrorCondition(clusterRepo, err, newStatus, ociInterval) + return setErrorCondition(clusterRepo, err, newStatus, ociInterval, ociCondition, o.clusterRepoController) } newIndexBytes, err := json.Marshal(index) if err != nil { logrus.Errorf("Error while marshalling indexfile for cluster repo %s: %v", clusterRepo.Name, err) - return o.setErrorCondition(clusterRepo, fmt.Errorf("error while reading indexfile"), newStatus, ociInterval) + return setErrorCondition(clusterRepo, fmt.Errorf("error while reading indexfile"), newStatus, ociInterval, ociCondition, o.clusterRepoController) } // Only update, if the index got updated if !bytes.Equal(originalIndexBytes, newIndexBytes) { index.SortEntries() cm, err := createOrUpdateMap(clusterRepo.Namespace, index, owner, o.apply) if err != nil { - return o.setErrorCondition(clusterRepo, fmt.Errorf("error while creating or updating confimap"), newStatus, ociInterval) + return setErrorCondition(clusterRepo, fmt.Errorf("error while creating or updating confimap"), newStatus, ociInterval, ociCondition, o.clusterRepoController) } newStatus.URL = clusterRepo.Spec.URL @@ -218,88 +220,29 @@ func (o *OCIRepohandler) onClusterRepoChange(key string, clusterRepo *catalog.Cl newStatus.DownloadTime = downloadTime } - return o.setErrorCondition(clusterRepo, nil, newStatus, ociInterval) -} - -// setErrorCondition is only called when error happens in the handler, and -// we need to depend on wrangler to requeue the handler -func (o *OCIRepohandler) setErrorCondition(clusterRepo *catalog.ClusterRepo, err error, newStatus *catalog.RepoStatus, ociInterval time.Duration) (*catalog.ClusterRepo, error) { - var statusErr error - newStatus.NumberOfRetries = 0 - newStatus.NextRetryAt = metav1.Time{} - if err != nil { - newStatus.ShouldNotSkip = true - } - ociDownloaded := condition.Cond(catalog.OCIDownloaded) - if apierrors.IsConflict(err) { - ociDownloaded.SetError(newStatus, "", nil) - } else { - ociDownloaded.SetError(newStatus, "", err) - } - newStatus.ObservedGeneration = clusterRepo.Generation - - if !equality.Semantic.DeepEqual(newStatus, &clusterRepo.Status) { - ociDownloaded.LastUpdated(newStatus, timeNow().UTC().Format(time.RFC3339)) - - clusterRepo.Status = *newStatus - clusterRepo, statusErr = o.clusterRepoController.UpdateStatus(clusterRepo) - if statusErr != nil { - err = statusErr - } - if err == nil { - o.clusterRepoController.EnqueueAfter(clusterRepo.Name, ociInterval) - } - return clusterRepo, err - } - - if err == nil { - o.clusterRepoController.EnqueueAfter(clusterRepo.Name, ociInterval) - } - return clusterRepo, err + return setErrorCondition(clusterRepo, nil, newStatus, ociInterval, ociCondition, o.clusterRepoController) } // setConditionWithInterval is called to reenqueue the object // after the interval of 24 hours. func (o *OCIRepohandler) setConditionWithInterval(clusterRepo *catalog.ClusterRepo, err error, newStatus *catalog.RepoStatus, backoff *time.Duration, ociInterval time.Duration) (*catalog.ClusterRepo, error) { var errResp *errcode.ErrorResponse - var newErr error + var errorMsg string if errors.As(err, &errResp) { - errorMsg := fmt.Sprintf("error %d: %s", errResp.StatusCode, http.StatusText(errResp.StatusCode)) + errorMsg = fmt.Sprintf("error %d: %s", errResp.StatusCode, http.StatusText(errResp.StatusCode)) if backoff != nil { errorMsg = fmt.Sprintf("%s. %s", errorMsg, fmt.Sprintf("Will retry after %s", backoff.Round(time.Second))) } else { errorMsg = fmt.Sprintf("%s. %s", errorMsg, fmt.Sprintf("Will retry after %s", ociInterval.Round(time.Second))) } - newErr = fmt.Errorf(errorMsg) - } else { - newErr = err - } - ociDownloaded := condition.Cond(catalog.OCIDownloaded) - if apierrors.IsConflict(err) { - ociDownloaded.SetError(newStatus, "", nil) } else { - ociDownloaded.SetError(newStatus, "", newErr) - } - newStatus.ObservedGeneration = clusterRepo.Generation - - if !equality.Semantic.DeepEqual(newStatus, &clusterRepo.Status) { - // Since status has changed, update the lastUpdatedTime - ociDownloaded.LastUpdated(newStatus, timeNow().UTC().Format(time.RFC3339)) - - clusterRepo.Status = *newStatus - - _, statusErr := o.clusterRepoController.UpdateStatus(clusterRepo) - if statusErr != nil { - return clusterRepo, statusErr - } + errorMsg = err.Error() } - + backoffInterval := ociInterval.Round(time.Second) if backoff != nil { - o.clusterRepoController.EnqueueAfter(clusterRepo.Name, *backoff) - } else { - o.clusterRepoController.EnqueueAfter(clusterRepo.Name, ociInterval) + backoffInterval = backoff.Round(time.Second) } - return clusterRepo, nil + return setConditionWithInterval(clusterRepo, err, newStatus, backoffInterval, ociCondition, errorMsg, o.clusterRepoController) } // getIndexfile fetches the indexfile if it already exits for the clusterRepo @@ -407,7 +350,7 @@ func getRetryPolicy(clusterRepo *catalog.ClusterRepo) (retryPolicy, error) { policy := defaultOCIRetryPolicy if !registry.IsOCI(clusterRepo.Spec.URL) { - policy = defaultRetryPolicy + policy = defaultHandlerErrRetryPolicy } if clusterRepo.Spec.ExponentialBackOffValues != nil { @@ -436,41 +379,3 @@ func getRetryPolicy(clusterRepo *catalog.ClusterRepo) (retryPolicy, error) { return policy, nil } - -// shouldSkip checks certain conditions to see if the handler should be skipped. -// For information regarding the conditions, check the comments in the implementation. -func (o *OCIRepohandler) shouldSkip(clusterRepo *catalog.ClusterRepo, policy retryPolicy, key string, ociInterval time.Duration) bool { - // this is to prevent the handler from making calls when the crd is outdated. - updatedRepo, err := o.clusterRepoController.Get(key, metav1.GetOptions{}) - if err == nil && updatedRepo.ResourceVersion != clusterRepo.ResourceVersion { - return true - } - - if clusterRepo.Status.ObservedGeneration < clusterRepo.Generation { - clusterRepo.Status.NumberOfRetries = 0 - clusterRepo.Status.NextRetryAt = metav1.Time{} - return false - } - - // The handler is triggered immediately after any changes, including when updating the number of retries done. - // This check is to prevent the handler from executing before the backoff time has passed - if !clusterRepo.Status.NextRetryAt.IsZero() && clusterRepo.Status.NextRetryAt.Time.After(timeNow().UTC()) { - return true - } - - if clusterRepo.Status.ShouldNotSkip { //checks if we should skip running the handler or not - return false - } - - ociDownloaded := condition.Cond(catalog.OCIDownloaded) - ociDownloadedUpdateTime, _ := time.Parse(time.RFC3339, ociDownloaded.GetLastUpdated(clusterRepo)) - - if (clusterRepo.Status.NumberOfRetries > policy.MaxRetry || clusterRepo.Status.NumberOfRetries == 0) && // checks if it's not retrying - clusterRepo.Generation == clusterRepo.Status.ObservedGeneration && // checks if the generation has not changed - ociDownloadedUpdateTime.Add(ociInterval).After(timeNow().UTC()) { // checks if the interval has not passed - - o.clusterRepoController.EnqueueAfter(clusterRepo.Name, ociInterval) - return true - } - return false -} diff --git a/pkg/controllers/dashboard/helm/repo_oci_test.go b/pkg/controllers/dashboard/helm/repo_oci_test.go index e7bcffa4c52..3eb91d3ac84 100644 --- a/pkg/controllers/dashboard/helm/repo_oci_test.go +++ b/pkg/controllers/dashboard/helm/repo_oci_test.go @@ -348,6 +348,7 @@ func TestGetRetryPolicy(t *testing.T) { t.Run(testCase.name, func(t *testing.T) { clusterRepo := &catalog.ClusterRepo{ Spec: catalog.RepoSpec{ + URL: "oci://dp.apps.rancher.io", ExponentialBackOffValues: testCase.backOffValues, }, } @@ -468,7 +469,7 @@ func TestShouldSkip(t *testing.T) { newClusterRepoController: func(ctrl *gomock.Controller) *fake.MockNonNamespacedControllerInterface[*catalog.ClusterRepo, *catalog.ClusterRepoList] { mockController := fake.NewMockNonNamespacedControllerInterface[*catalog.ClusterRepo, *catalog.ClusterRepoList](ctrl) mockController.EXPECT().Get("clusterRepo", metav1.GetOptions{}).Return(&catalog.ClusterRepo{ObjectMeta: metav1.ObjectMeta{ResourceVersion: "1"}}, nil) - mockController.EXPECT().EnqueueAfter("", ociInterval).Return() + mockController.EXPECT().EnqueueAfter("clusterRepo", ociInterval).Return() return mockController }, generation: 1, @@ -507,12 +508,14 @@ func TestShouldSkip(t *testing.T) { ObjectMeta: metav1.ObjectMeta{ Generation: testCase.generation, ResourceVersion: "1", + Name: "clusterRepo", }, Status: catalog.RepoStatus{ ObservedGeneration: testCase.observedGeneration, NumberOfRetries: testCase.numberOfRetries, NextRetryAt: testCase.nextRetryAt, ShouldNotSkip: testCase.shouldNotSkip, + IndexConfigMapName: "indexConfigMap", Conditions: []genericcondition.GenericCondition{ { Type: string(catalog.OCIDownloaded), @@ -520,7 +523,7 @@ func TestShouldSkip(t *testing.T) { }, }}, } - assert.Equal(t, testCase.expected, handler.shouldSkip(clusterRepo, policy, "clusterRepo", ociInterval)) + assert.Equal(t, testCase.expected, shouldSkip(clusterRepo, policy, catalog.OCIDownloaded, ociInterval, handler.clusterRepoController, &clusterRepo.Status)) }) } } diff --git a/pkg/controllers/dashboard/helm/repo_test.go b/pkg/controllers/dashboard/helm/repo_test.go deleted file mode 100644 index db38d5874ec..00000000000 --- a/pkg/controllers/dashboard/helm/repo_test.go +++ /dev/null @@ -1,211 +0,0 @@ -package helm - -import ( - "testing" - "time" - - catalog "github.com/rancher/rancher/pkg/apis/catalog.cattle.io/v1" - "github.com/stretchr/testify/assert" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" -) - -func TestShouldRefresh(t *testing.T) { - tests := []struct { - name string - spec *catalog.RepoSpec - status *catalog.RepoStatus - expectedResult bool - }{ - { - "http repo - spec equals status", - &catalog.RepoSpec{ - URL: "https://example.com", - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now(), - }, - }, - false, - }, - { - "http repo - url changed", - &catalog.RepoSpec{ - URL: "https://changed-url.com", - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now(), - }, - }, - true, - }, - { - "http repo - missing indexConfigMap", - &catalog.RepoSpec{ - URL: "https://example.com", - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "", - DownloadTime: metav1.Time{ - Time: time.Now(), - }, - }, - true, - }, - { - "http repo - download not so long ago", - &catalog.RepoSpec{ - URL: "https://example.com", - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now().Add(-1 * time.Minute), - }, - }, - false, - }, - { - "http repo - download to long ago", - &catalog.RepoSpec{ - URL: "https://example.com", - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now().Add(-7 * time.Hour), - }, - }, - true, - }, - { - "http repo - force update", - &catalog.RepoSpec{ - URL: "https://example.com", - ForceUpdate: &metav1.Time{ - Time: time.Now(), - }, - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now().Add(-1 * time.Minute), - }, - }, - true, - }, - { - "http repo - force update older than download time", - &catalog.RepoSpec{ - URL: "https://example.com", - ForceUpdate: &metav1.Time{ - Time: time.Now().Add(-2 * time.Minute), - }, - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now().Add(-1 * time.Minute), - }, - }, - false, - }, - { - "http repo - force update in the future", - &catalog.RepoSpec{ - URL: "https://example.com", - ForceUpdate: &metav1.Time{ - Time: time.Now().Add(2 * time.Minute), - }, - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now(), - }, - }, - false, - }, - { - "git repo - spec equals status", - &catalog.RepoSpec{ - GitBranch: "master", - GitRepo: "git.example.com", - }, - &catalog.RepoStatus{ - Branch: "master", - URL: "git.example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now(), - }, - }, - false, - }, - { - "git repo - branch changed", - &catalog.RepoSpec{ - GitBranch: "main", - GitRepo: "git.example.com", - }, - &catalog.RepoStatus{ - Branch: "master", - URL: "git.example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now(), - }, - }, - true, - }, - { - "git repo - repo changed", - &catalog.RepoSpec{ - GitBranch: "master", - GitRepo: "newgit.example.com", - }, - &catalog.RepoStatus{ - Branch: "master", - URL: "git.example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now(), - }, - }, - true, - }, - { - "http repo - spec equals status, but unnecessary git branch in spec", - &catalog.RepoSpec{ - URL: "https://example.com", - GitBranch: "master", - }, - &catalog.RepoStatus{ - URL: "https://example.com", - IndexConfigMapName: "configmap", - DownloadTime: metav1.Time{ - Time: time.Now(), - }, - }, - false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := shouldRefresh(tt.spec, tt.status, 6*time.Hour) - assert.Equal(t, tt.expectedResult, result) - }) - } -} diff --git a/pkg/generated/controllers/catalog.cattle.io/v1/clusterrepo.go b/pkg/generated/controllers/catalog.cattle.io/v1/clusterrepo.go index 139b9923006..a4179b3ff2b 100644 --- a/pkg/generated/controllers/catalog.cattle.io/v1/clusterrepo.go +++ b/pkg/generated/controllers/catalog.cattle.io/v1/clusterrepo.go @@ -106,7 +106,7 @@ func (a *clusterRepoStatusHandler) sync(key string, obj *v1.ClusterRepo) (*v1.Cl if a.condition != "" { if errors.IsConflict(err) { a.condition.SetError(&newStatus, "", nil) - } else { //this will reset any reason + } else { a.condition.SetError(&newStatus, "", err) } }