Skip to content

Commit 8d8179f

Browse files
myasnikovdaniilmyasnikovdaniil
committed
fix(controller): carry healthCheckExprs and waitStrategy to existing releases
cozystack-api writes healthCheckExprs and the resolved wait strategy from the ApplicationDefinition only when it builds a HelmRelease, and the definition reconciler that keeps existing releases in step copied neither. A release created before its definition exempted an object kept waiting on that object at every upgrade. For vm-disk that is every disk created before the DataVolume expression was added: a disk whose import is still running after a platform upgrade fails the upgrade at the timeout and, under RetryOnFailure, retries forever while holding a helm-controller worker. The reconciler now derives both fields the way the API does, in both directions, so an existing release ends up with what a fresh write would produce. On the shipped definitions this only relaxes waits: the vm-disk DataVolume and the postgres KEDA ScaledObject. A release that another chart renders carries Flux's origin label and keeps the gate its chart sets. A failed update used to be logged and dropped, and nothing else re-enqueues the definition for hours, so that release stayed out of sync. The errors are now returned and the definition is retried. Fixes #4409 Assisted-by: LLM Signed-off-by: Myasnikov Daniil <[email protected]>
1 parent a26c441 commit 8d8179f

2 files changed

Lines changed: 332 additions & 3 deletions

File tree

‎internal/controller/applicationdefinition_helmreconciler.go‎

Lines changed: 34 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@ package controller
22

33
import (
44
"context"
5+
"errors"
56
"fmt"
67

78
cozyv1alpha1 "github.com/cozystack/cozystack/api/v1alpha1"
@@ -10,6 +11,7 @@ import (
1011
helmv2 "github.com/fluxcd/helm-controller/api/v2"
1112

1213
corev1 "k8s.io/api/core/v1"
14+
"k8s.io/apimachinery/pkg/api/equality"
1315
"k8s.io/apimachinery/pkg/runtime"
1416
"k8s.io/apimachinery/pkg/types"
1517
"k8s.io/client-go/tools/record"
@@ -133,16 +135,20 @@ func (r *ApplicationDefinitionHelmReconciler) updateHelmReleasesForAppDef(ctx co
133135

134136
logger.V(4).Info("Found HelmReleases to update", "appDef", appDef.Name, "kind", applicationKind, "count", len(hrList.Items))
135137

136-
// Update each HelmRelease
138+
// Update each HelmRelease. Only definition events and the manager's
139+
// hours-long resync enqueue this reconciler, so a release whose update
140+
// failed (a conflict with helm-controller writing it, say) would stay out
141+
// of sync until then unless the failure is returned and retried.
142+
var errs []error
137143
for i := range hrList.Items {
138144
hr := &hrList.Items[i]
139145
if err := r.updateHelmReleaseChart(ctx, hr, appDef); err != nil {
140146
logger.Error(err, "failed to update HelmRelease", "name", hr.Name, "namespace", hr.Namespace)
141-
continue
147+
errs = append(errs, err)
142148
}
143149
}
144150

145-
return nil
151+
return errors.Join(errs...)
146152
}
147153

148154
// expectedValuesFrom returns the expected valuesFrom configuration for HelmReleases
@@ -250,6 +256,31 @@ func (r *ApplicationDefinitionHelmReconciler) updateHelmReleaseChart(ctx context
250256
}
251257
}
252258

259+
// Check and update the health checks the wait applies. cozystack-api writes
260+
// healthCheckExprs and the wait strategy only when it builds a release, so
261+
// a release created before the definition exempted an object would keep
262+
// waiting on it at every upgrade, and fail for good on one that never
263+
// settles. Both are derived the way the API derives them, in both
264+
// directions, so the release matches what a fresh write would produce. A
265+
// release another chart renders carries Flux's origin label and takes its
266+
// gate from that chart.
267+
if _, rendered := hr.Labels["helm.toolkit.fluxcd.io/name"]; !rendered {
268+
exprs := appDef.Spec.Release.HealthCheckExprs
269+
// Semantic equality treats nil and empty as equal, so a release
270+
// already in sync is not rewritten.
271+
if !equality.Semantic.DeepEqual(hrCopy.Spec.HealthCheckExprs, exprs) {
272+
logger.V(4).Info("Updating HelmRelease healthCheckExprs", "name", hr.Name, "namespace", hr.Namespace)
273+
hrCopy.Spec.HealthCheckExprs = exprs
274+
updated = true
275+
}
276+
waitStrategy := config.ResolveWaitStrategy(appDef.Spec.Release.WaitStrategy, len(exprs) > 0)
277+
if !equality.Semantic.DeepEqual(hrCopy.Spec.WaitStrategy, waitStrategy) {
278+
logger.V(4).Info("Updating HelmRelease waitStrategy", "name", hr.Name, "namespace", hr.Namespace, "waitStrategy", waitStrategy)
279+
hrCopy.Spec.WaitStrategy = waitStrategy
280+
updated = true
281+
}
282+
}
283+
253284
// Check and update labels from ApplicationDefinition
254285
if len(appDef.Spec.Release.Labels) > 0 {
255286
if hrCopy.Labels == nil {

‎internal/controller/applicationdefinition_helmreconciler_test.go‎

Lines changed: 298 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,24 @@ package controller
22

33
import (
44
"context"
5+
"errors"
6+
"reflect"
57
"strings"
68
"testing"
79
"time"
810

911
cozyv1alpha1 "github.com/cozystack/cozystack/api/v1alpha1"
1012
"github.com/cozystack/cozystack/pkg/config"
1113
helmv2 "github.com/fluxcd/helm-controller/api/v2"
14+
"github.com/fluxcd/pkg/apis/kustomize"
15+
apierrors "k8s.io/apimachinery/pkg/api/errors"
1216
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
1317
"k8s.io/apimachinery/pkg/runtime"
1418
"k8s.io/apimachinery/pkg/types"
1519
"k8s.io/client-go/tools/record"
20+
"sigs.k8s.io/controller-runtime/pkg/client"
1621
"sigs.k8s.io/controller-runtime/pkg/client/fake"
22+
"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
1723
"sigs.k8s.io/controller-runtime/pkg/reconcile"
1824
)
1925

@@ -661,3 +667,295 @@ func TestAppDefHelm_SameKindRequests(t *testing.T) {
661667
t.Fatalf("requests = %v (%d), want harbor and tap-harbor once each", names, len(reqs))
662668
}
663669
}
670+
671+
// dataVolumeCurrent is the expression the vm-disk definition carries, so that a
672+
// disk CDI is still importing does not hold its release's wait.
673+
var dataVolumeCurrent = kustomize.CustomHealthCheck{
674+
APIVersion: "cdi.kubevirt.io/v1beta1",
675+
Kind: "DataVolume",
676+
HealthCheckExpressions: kustomize.HealthCheckExpressions{Current: "true"},
677+
}
678+
679+
// vmDiskRelease builds a VMDisk HelmRelease the way cozystack-api wrote it
680+
// before the definition declared any expression: chartRef and valuesFrom in
681+
// sync, no healthCheckExprs, no waitStrategy.
682+
func vmDiskRelease() *helmv2.HelmRelease {
683+
return &helmv2.HelmRelease{
684+
ObjectMeta: metav1.ObjectMeta{
685+
Name: "vm-disk-data",
686+
Namespace: "tenant-foo",
687+
Labels: map[string]string{
688+
"apps.cozystack.io/application.kind": "VMDisk",
689+
"apps.cozystack.io/application.group": "apps.cozystack.io",
690+
},
691+
},
692+
Spec: helmv2.HelmReleaseSpec{
693+
ChartRef: &helmv2.CrossNamespaceSourceReference{
694+
Kind: "ExternalArtifact",
695+
Name: "cozystack-vm-disk-application-kubevirt-vm-disk",
696+
Namespace: "cozy-system",
697+
},
698+
ValuesFrom: expectedValuesFrom(),
699+
},
700+
}
701+
}
702+
703+
// vmDiskAppDef builds the VMDisk ApplicationDefinition with the given wait
704+
// strategy and expressions.
705+
func vmDiskAppDef(waitStrategy string, exprs ...kustomize.CustomHealthCheck) *cozyv1alpha1.ApplicationDefinition {
706+
return &cozyv1alpha1.ApplicationDefinition{
707+
ObjectMeta: metav1.ObjectMeta{Name: "vm-disk"},
708+
Spec: cozyv1alpha1.ApplicationDefinitionSpec{
709+
Application: cozyv1alpha1.ApplicationDefinitionApplication{Kind: "VMDisk"},
710+
Release: cozyv1alpha1.ApplicationDefinitionRelease{
711+
ChartRef: &helmv2.CrossNamespaceSourceReference{
712+
Kind: "ExternalArtifact",
713+
Name: "cozystack-vm-disk-application-kubevirt-vm-disk",
714+
Namespace: "cozy-system",
715+
},
716+
WaitStrategy: waitStrategy,
717+
HealthCheckExprs: exprs,
718+
},
719+
},
720+
}
721+
}
722+
723+
// TestAppDefHelm_HealthCheckExprsApplied pins the drift behind #4409:
724+
// cozystack-api writes healthCheckExprs only when it builds a release, so a
725+
// disk created before the definition exempted its DataVolume kept waiting on
726+
// it at every upgrade, and one CDI never finished importing failed its upgrade
727+
// for good. The expressions must land, with the poller strategy that is the
728+
// only one evaluating them.
729+
func TestAppDefHelm_HealthCheckExprsApplied(t *testing.T) {
730+
got := reconcileVMInstance(t, vmDiskAppDef("", dataVolumeCurrent), vmDiskRelease())
731+
732+
if want := []kustomize.CustomHealthCheck{dataVolumeCurrent}; !reflect.DeepEqual(got.Spec.HealthCheckExprs, want) {
733+
t.Fatalf("expected HealthCheckExprs %+v, got %+v", want, got.Spec.HealthCheckExprs)
734+
}
735+
if got.Spec.WaitStrategy == nil || got.Spec.WaitStrategy.Name != helmv2.WaitStrategyPoller {
736+
t.Fatalf("expected WaitStrategy poller, got %+v", got.Spec.WaitStrategy)
737+
}
738+
}
739+
740+
// TestAppDefHelm_HealthCheckExprsRemoved pins the other direction: the API
741+
// rebuilds the whole spec on every write, so a release left carrying what the
742+
// definition dropped would differ from one written a moment later.
743+
func TestAppDefHelm_HealthCheckExprsRemoved(t *testing.T) {
744+
hr := vmDiskRelease()
745+
hr.Spec.HealthCheckExprs = []kustomize.CustomHealthCheck{dataVolumeCurrent}
746+
hr.Spec.WaitStrategy = &helmv2.WaitStrategy{Name: helmv2.WaitStrategyPoller}
747+
748+
got := reconcileVMInstance(t, vmDiskAppDef(""), hr)
749+
750+
if len(got.Spec.HealthCheckExprs) != 0 {
751+
t.Fatalf("expected HealthCheckExprs removed, got %+v", got.Spec.HealthCheckExprs)
752+
}
753+
if got.Spec.WaitStrategy != nil {
754+
t.Fatalf("expected WaitStrategy removed, got %+v", got.Spec.WaitStrategy)
755+
}
756+
}
757+
758+
// TestAppDefHelm_WaitStrategyWithoutExprsApplied pins that an explicit strategy
759+
// is carried on its own, not only as the default the expressions imply.
760+
func TestAppDefHelm_WaitStrategyWithoutExprsApplied(t *testing.T) {
761+
got := reconcileVMInstance(t, vmDiskAppDef("legacy"), vmDiskRelease())
762+
763+
if got.Spec.WaitStrategy == nil || got.Spec.WaitStrategy.Name != helmv2.WaitStrategyLegacy {
764+
t.Fatalf("expected WaitStrategy legacy, got %+v", got.Spec.WaitStrategy)
765+
}
766+
if len(got.Spec.HealthCheckExprs) != 0 {
767+
t.Fatalf("expected no HealthCheckExprs, got %+v", got.Spec.HealthCheckExprs)
768+
}
769+
}
770+
771+
// TestAppDefHelm_HealthCheckExprsInSyncSkipsUpdate pins idempotence, like
772+
// TestAppDefHelm_DisableWaitInSyncSkipsUpdate. A definition read with
773+
// healthCheckExprs: [] decodes to an empty slice, while a release built without
774+
// expressions carries none at all; comparing the two as different would
775+
// rewrite every release of the kind on every reconcile. The fake client drops
776+
// an empty slice on its JSON round trip, so the objects are handed to
777+
// updateHelmReleaseChart directly.
778+
func TestAppDefHelm_HealthCheckExprsInSyncSkipsUpdate(t *testing.T) {
779+
cases := []struct {
780+
name string
781+
defExprs []kustomize.CustomHealthCheck
782+
hrExprs []kustomize.CustomHealthCheck
783+
hrStrategy *helmv2.WaitStrategy
784+
}{
785+
{
786+
name: "expressions and poller already set",
787+
defExprs: []kustomize.CustomHealthCheck{dataVolumeCurrent},
788+
hrExprs: []kustomize.CustomHealthCheck{dataVolumeCurrent},
789+
hrStrategy: &helmv2.WaitStrategy{Name: helmv2.WaitStrategyPoller},
790+
},
791+
{
792+
name: "empty on the definition, absent on the release",
793+
defExprs: []kustomize.CustomHealthCheck{},
794+
},
795+
{
796+
name: "absent on the definition, empty on the release",
797+
hrExprs: []kustomize.CustomHealthCheck{},
798+
},
799+
}
800+
for _, tc := range cases {
801+
t.Run(tc.name, func(t *testing.T) {
802+
stored := vmDiskRelease()
803+
stored.Spec.HealthCheckExprs = tc.hrExprs
804+
stored.Spec.WaitStrategy = tc.hrStrategy
805+
806+
scheme := newAppDefHelmScheme(t)
807+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(stored).Build()
808+
809+
before := &helmv2.HelmRelease{}
810+
if err := fakeClient.Get(context.TODO(), types.NamespacedName{Name: stored.Name, Namespace: stored.Namespace}, before); err != nil {
811+
t.Fatalf("get HR: %v", err)
812+
}
813+
hr := before.DeepCopy()
814+
hr.Spec.HealthCheckExprs = tc.hrExprs
815+
816+
r := &ApplicationDefinitionHelmReconciler{Client: fakeClient, Scheme: scheme}
817+
if err := r.updateHelmReleaseChart(context.TODO(), hr, vmDiskAppDef("", tc.defExprs...)); err != nil {
818+
t.Fatalf("unexpected error: %v", err)
819+
}
820+
821+
after := &helmv2.HelmRelease{}
822+
if err := fakeClient.Get(context.TODO(), types.NamespacedName{Name: stored.Name, Namespace: stored.Namespace}, after); err != nil {
823+
t.Fatalf("get HR after update: %v", err)
824+
}
825+
if after.ResourceVersion != before.ResourceVersion {
826+
t.Fatalf("release rewritten with nothing to change: %q -> %q", before.ResourceVersion, after.ResourceVersion)
827+
}
828+
})
829+
}
830+
}
831+
832+
// TestAppDefHelm_HealthCheckExprsLeaveRenderedReleaseAlone pins the scope of
833+
// the sync. The tenant chart renders an Etcd HelmRelease carrying the same kind
834+
// labels as one the API builds, and it sets that release's wait gate itself;
835+
// rewriting it from the definition would strip the chart's gate until the
836+
// parent's next upgrade put it back. Its chartRef still follows the definition.
837+
func TestAppDefHelm_HealthCheckExprsLeaveRenderedReleaseAlone(t *testing.T) {
838+
chartGate := kustomize.CustomHealthCheck{
839+
APIVersion: "etcd-operator.cozystack.io/v1alpha2",
840+
Kind: "EtcdCluster",
841+
HealthCheckExpressions: kustomize.HealthCheckExpressions{Current: "status.readyMembers == spec.replicas"},
842+
}
843+
hr := &helmv2.HelmRelease{
844+
ObjectMeta: metav1.ObjectMeta{Name: "etcd", Namespace: "tenant-foo", Labels: map[string]string{
845+
"apps.cozystack.io/application.kind": "Etcd",
846+
"apps.cozystack.io/application.group": "apps.cozystack.io",
847+
"helm.toolkit.fluxcd.io/name": "tenant-foo",
848+
}},
849+
Spec: helmv2.HelmReleaseSpec{
850+
ChartRef: &helmv2.CrossNamespaceSourceReference{Kind: "ExternalArtifact", Name: "stale", Namespace: "cozy-system"},
851+
ValuesFrom: expectedValuesFrom(),
852+
HealthCheckExprs: []kustomize.CustomHealthCheck{chartGate},
853+
WaitStrategy: &helmv2.WaitStrategy{Name: helmv2.WaitStrategyPoller},
854+
},
855+
}
856+
appDef := &cozyv1alpha1.ApplicationDefinition{
857+
ObjectMeta: metav1.ObjectMeta{Name: "etcd"},
858+
Spec: cozyv1alpha1.ApplicationDefinitionSpec{
859+
Application: cozyv1alpha1.ApplicationDefinitionApplication{Kind: "Etcd"},
860+
Release: cozyv1alpha1.ApplicationDefinitionRelease{
861+
ChartRef: &helmv2.CrossNamespaceSourceReference{Kind: "ExternalArtifact", Name: "cozystack-etcd-application-default-etcd", Namespace: "cozy-system"},
862+
},
863+
},
864+
}
865+
866+
got := reconcileVMInstance(t, appDef, hr)
867+
868+
if got.Spec.ChartRef == nil || got.Spec.ChartRef.Name != "cozystack-etcd-application-default-etcd" {
869+
t.Fatalf("expected ChartRef synced from the definition, got %+v", got.Spec.ChartRef)
870+
}
871+
if want := []kustomize.CustomHealthCheck{chartGate}; !reflect.DeepEqual(got.Spec.HealthCheckExprs, want) {
872+
t.Fatalf("expected the chart's HealthCheckExprs %+v kept, got %+v", want, got.Spec.HealthCheckExprs)
873+
}
874+
if got.Spec.WaitStrategy == nil || got.Spec.WaitStrategy.Name != helmv2.WaitStrategyPoller {
875+
t.Fatalf("expected the chart's WaitStrategy poller kept, got %+v", got.Spec.WaitStrategy)
876+
}
877+
}
878+
879+
// TestAppDefHelm_HealthCheckExprsScopedToKind pins the kind label in the
880+
// selector: a release of another kind must not pick up the VMDisk expressions.
881+
func TestAppDefHelm_HealthCheckExprsScopedToKind(t *testing.T) {
882+
disk := vmDiskRelease()
883+
other := vmDiskRelease()
884+
other.Name = "harbor-registry"
885+
other.Labels["apps.cozystack.io/application.kind"] = "Harbor"
886+
887+
scheme := newAppDefHelmScheme(t)
888+
appDef := vmDiskAppDef("", dataVolumeCurrent)
889+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(appDef, disk, other).Build()
890+
891+
r := &ApplicationDefinitionHelmReconciler{Client: fakeClient, Scheme: scheme}
892+
if _, err := r.Reconcile(context.TODO(), reconcile.Request{
893+
NamespacedName: types.NamespacedName{Name: appDef.Name},
894+
}); err != nil {
895+
t.Fatalf("unexpected error: %v", err)
896+
}
897+
898+
gotDisk := &helmv2.HelmRelease{}
899+
if err := fakeClient.Get(context.TODO(), types.NamespacedName{Name: disk.Name, Namespace: disk.Namespace}, gotDisk); err != nil {
900+
t.Fatalf("get VMDisk HR: %v", err)
901+
}
902+
if len(gotDisk.Spec.HealthCheckExprs) != 1 {
903+
t.Fatalf("expected VMDisk HealthCheckExprs synced, got %+v", gotDisk.Spec.HealthCheckExprs)
904+
}
905+
906+
gotOther := &helmv2.HelmRelease{}
907+
if err := fakeClient.Get(context.TODO(), types.NamespacedName{Name: other.Name, Namespace: other.Namespace}, gotOther); err != nil {
908+
t.Fatalf("get Harbor HR: %v", err)
909+
}
910+
if len(gotOther.Spec.HealthCheckExprs) != 0 {
911+
t.Fatalf("expected Harbor HealthCheckExprs untouched, got %+v", gotOther.Spec.HealthCheckExprs)
912+
}
913+
if gotOther.Spec.WaitStrategy != nil {
914+
t.Fatalf("expected Harbor WaitStrategy untouched, got %+v", gotOther.Spec.WaitStrategy)
915+
}
916+
}
917+
918+
// TestAppDefHelm_UpdateFailureRequeues pins that a failed write is returned
919+
// rather than logged and dropped: nothing else re-enqueues the definition for
920+
// hours, so a release that lost a conflict to helm-controller would keep its
921+
// old wait until then. The releases after it are still written.
922+
func TestAppDefHelm_UpdateFailureRequeues(t *testing.T) {
923+
first := vmDiskRelease()
924+
second := vmDiskRelease()
925+
second.Name = "vm-disk-logs"
926+
927+
failed := ""
928+
scheme := newAppDefHelmScheme(t)
929+
appDef := vmDiskAppDef("", dataVolumeCurrent)
930+
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(appDef, first, second).
931+
WithInterceptorFuncs(interceptor.Funcs{
932+
Update: func(ctx context.Context, cl client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
933+
// Fail whichever release is written first, so the assertion
934+
// below does not depend on the order List returns them in.
935+
if failed == "" {
936+
failed = obj.GetName()
937+
return apierrors.NewConflict(helmv2.GroupVersion.WithResource("helmreleases").GroupResource(), obj.GetName(), errors.New("the object has been modified"))
938+
}
939+
return cl.Update(ctx, obj, opts...)
940+
},
941+
}).Build()
942+
943+
r := &ApplicationDefinitionHelmReconciler{Client: fakeClient, Scheme: scheme}
944+
if _, err := r.Reconcile(context.TODO(), reconcile.Request{
945+
NamespacedName: types.NamespacedName{Name: appDef.Name},
946+
}); !apierrors.IsConflict(err) {
947+
t.Fatalf("expected the conflict returned so the definition is requeued, got %v", err)
948+
}
949+
950+
written := first.Name
951+
if failed == first.Name {
952+
written = second.Name
953+
}
954+
got := &helmv2.HelmRelease{}
955+
if err := fakeClient.Get(context.TODO(), types.NamespacedName{Name: written, Namespace: "tenant-foo"}, got); err != nil {
956+
t.Fatalf("get HR: %v", err)
957+
}
958+
if len(got.Spec.HealthCheckExprs) != 1 {
959+
t.Fatalf("expected %s synced past the failed %s, got %+v", written, failed, got.Spec.HealthCheckExprs)
960+
}
961+
}

0 commit comments

Comments
 (0)