Skip to content

Commit

Permalink
Reduce component reconciler's action verbosity
Browse files Browse the repository at this point in the history
  • Loading branch information
lburgazzoli committed Nov 26, 2024
1 parent 89bf20a commit a1560ca
Show file tree
Hide file tree
Showing 16 changed files with 251 additions and 85 deletions.
13 changes: 6 additions & 7 deletions controllers/components/dashboard/dashboard_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,11 @@ import (
func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.Manager) error {
componentName := computeComponentName()

_, err := reconciler.ComponentReconcilerFor(mgr, componentsv1.DashboardInstanceName, &componentsv1.Dashboard{}).
_, err := reconciler.ComponentReconcilerFor(
mgr,
componentsv1.DashboardInstanceName,
&componentsv1.Dashboard{},
).
// operands - owned
Owns(&corev1.ConfigMap{}).
Owns(&corev1.Secret{}).
Expand Down Expand Up @@ -105,16 +109,11 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
WithAction(customizeResources).
WithAction(deploy.NewAction(
deploy.WithCache(),
deploy.WithFieldOwner(componentsv1.DashboardInstanceName),
deploy.WithLabel(labels.ComponentPartOf, componentsv1.DashboardInstanceName),
)).
WithAction(updatestatus.NewAction(
updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.DashboardInstanceName),
)).
WithAction(updatestatus.NewAction()).
WithAction(updateStatus).
// must be the final action
WithAction(gc.NewAction(
gc.WithLabel(labels.ComponentPartOf, componentsv1.DashboardInstanceName),
gc.WithUnremovables(gvk.OdhDashboardConfig),
)).
Build(ctx)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,16 +77,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
)).
WithAction(deploy.NewAction(
deploy.WithCache(),
deploy.WithFieldOwner(componentsv1.DataSciencePipelinesInstanceName),
deploy.WithLabel(labels.ComponentPartOf, componentsv1.DataSciencePipelinesInstanceName),
)).
WithAction(updatestatus.NewAction(
updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.DataSciencePipelinesInstanceName),
)).
WithAction(updatestatus.NewAction()).
// must be the final action
WithAction(gc.NewAction(
gc.WithLabel(labels.ComponentPartOf, componentsv1.DataSciencePipelinesInstanceName),
)).
WithAction(gc.NewAction()).
Build(ctx)

if err != nil {
Expand Down
10 changes: 2 additions & 8 deletions controllers/components/kueue/kueue_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,16 +70,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
)).
WithAction(deploy.NewAction(
deploy.WithCache(),
deploy.WithFieldOwner(componentsv1.KueueInstanceName),
deploy.WithLabel(labels.ComponentPartOf, componentsv1.KueueInstanceName),
)).
WithAction(updatestatus.NewAction(
updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.KueueInstanceName),
)).
WithAction(updatestatus.NewAction()).
// must be the final action
WithAction(gc.NewAction(
gc.WithLabel(labels.ComponentPartOf, componentsv1.KueueInstanceName),
)).
WithAction(gc.NewAction()).
Build(ctx)

if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -91,16 +91,11 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
WithAction(customizeResources).
WithAction(deploy.NewAction(
deploy.WithCache(),
deploy.WithFieldOwner(componentsv1.ModelRegistryInstanceName),
deploy.WithLabel(labels.ComponentPartOf, componentsv1.ModelRegistryInstanceName),
)).
WithAction(updatestatus.NewAction(
updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.ModelRegistryInstanceName),
)).
WithAction(updatestatus.NewAction()).
WithAction(updateStatus).
// must be the final action
WithAction(gc.NewAction(
gc.WithLabel(labels.ComponentPartOf, componentsv1.ModelRegistryInstanceName),
gc.WithUnremovables(gvk.ServiceMeshMember),
)).
Build(ctx)
Expand Down
10 changes: 2 additions & 8 deletions controllers/components/ray/ray_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,16 +63,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
)).
WithAction(deploy.NewAction(
deploy.WithCache(),
deploy.WithFieldOwner(componentsv1.RayInstanceName),
deploy.WithLabel(labels.ComponentPartOf, componentsv1.RayInstanceName),
)).
WithAction(updatestatus.NewAction(
updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.RayInstanceName),
)).
WithAction(updatestatus.NewAction()).
// must be the final action
WithAction(gc.NewAction(
gc.WithLabel(labels.ComponentPartOf, componentsv1.RayInstanceName),
)).
WithAction(gc.NewAction()).
Build(ctx)

if err != nil {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,16 +60,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
)).
WithAction(deploy.NewAction(
deploy.WithCache(),
deploy.WithFieldOwner(componentsv1.TrainingOperatorInstanceName),
deploy.WithLabel(labels.ComponentPartOf, componentsv1.TrainingOperatorInstanceName),
)).
WithAction(updatestatus.NewAction(
updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.TrainingOperatorInstanceName),
)).
WithAction(updatestatus.NewAction()).
// must be the final action
WithAction(gc.NewAction(
gc.WithLabel(labels.ComponentPartOf, componentsv1.TrainingOperatorInstanceName),
)).
WithAction(gc.NewAction()).
Build(ctx)

if err != nil {
Expand Down
10 changes: 2 additions & 8 deletions controllers/components/trustyai/trustyai_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,16 +61,10 @@ func (s *componentHandler) NewComponentReconciler(ctx context.Context, mgr ctrl.
)).
WithAction(deploy.NewAction(
deploy.WithCache(),
deploy.WithFieldOwner(componentsv1.TrustyAIInstanceName),
deploy.WithLabel(labels.ComponentPartOf, componentsv1.TrustyAIInstanceName),
)).
WithAction(updatestatus.NewAction(
updatestatus.WithSelectorLabel(labels.ComponentPartOf, componentsv1.TrustyAIInstanceName),
)).
WithAction(updatestatus.NewAction()).
// must be the final action
WithAction(gc.NewAction(
gc.WithLabel(labels.ComponentPartOf, componentsv1.TrustyAIInstanceName),
)).
WithAction(gc.NewAction()).
Build(ctx)

if err != nil {
Expand Down
38 changes: 30 additions & 8 deletions pkg/controller/actions/deploy/action_deploy.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
odhClient "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client"
odhTypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources"
)

Expand Down Expand Up @@ -156,10 +157,19 @@ func (a *Action) deploy(
return false, nil
}

fo := a.fieldOwner
if fo == "" {
fo = rr.OwnerName
}

resources.SetLabels(&obj, a.labels)
resources.SetAnnotations(&obj, a.annotations)
resources.SetAnnotations(&obj, deployAnnotations)

if resources.GetLabel(&obj, labels.ComponentPartOf) == "" && fo != "" {
resources.SetLabel(&obj, labels.ComponentPartOf, fo)
}

// backup copy for caching
origObj := obj.DeepCopy()

Expand Down Expand Up @@ -205,9 +215,21 @@ func (a *Action) deploy(

switch a.deployMode {
case ModePatch:
deployedObj, err = a.patch(ctx, rr.Client, &obj, current)
deployedObj, err = a.patch(
ctx,
rr.Client,
&obj,
current,
client.ForceOwnership,
client.FieldOwner(fo))
case ModeSSA:
deployedObj, err = a.apply(ctx, rr.Client, &obj, current)
deployedObj, err = a.apply(
ctx,
rr.Client,
&obj,
current,
client.ForceOwnership,
client.FieldOwner(fo))
default:
err = fmt.Errorf("unsupported deploy mode %s", a.deployMode)
}
Expand Down Expand Up @@ -278,6 +300,7 @@ func (a *Action) patch(
c *odhClient.Client,
obj *unstructured.Unstructured,
old *unstructured.Unstructured,
opts ...client.PatchOption,
) (*unstructured.Unstructured, error) {
logf.FromContext(ctx).V(3).Info("patch",
"gvk", obj.GroupVersionKind(),
Expand Down Expand Up @@ -305,7 +328,7 @@ func (a *Action) patch(
}

default:
// do noting
// do nothing
break
}

Expand All @@ -324,8 +347,7 @@ func (a *Action) patch(
ctx,
old,
client.RawPatch(types.ApplyPatchType, data),
client.ForceOwnership,
client.FieldOwner(a.fieldOwner),
opts...,
)

if err != nil {
Expand All @@ -341,6 +363,7 @@ func (a *Action) apply(
c *odhClient.Client,
obj *unstructured.Unstructured,
old *unstructured.Unstructured,
opts ...client.PatchOption,
) (*unstructured.Unstructured, error) {
logf.FromContext(ctx).V(3).Info("apply",
"gvk", obj.GroupVersionKind(),
Expand Down Expand Up @@ -369,15 +392,14 @@ func (a *Action) apply(
return nil, fmt.Errorf("failed to merge Deployment %s/%s: %w", obj.GetNamespace(), obj.GetName(), err)
}
default:
// do noting
// do nothing
break
}

err := c.Apply(
ctx,
obj,
client.ForceOwnership,
client.FieldOwner(a.fieldOwner),
opts...,
)

if err != nil {
Expand Down
9 changes: 6 additions & 3 deletions pkg/controller/actions/deploy/action_deploy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions/deploy"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/resources"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/fakeclient"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/utils/test/matchers/jq"
Expand Down Expand Up @@ -56,9 +57,10 @@ func TestDeployAction(t *testing.T) {
g.Expect(err).ShouldNot(HaveOccurred())

rr := types.ReconciliationRequest{
Client: cl,
DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}},
DSC: &dscv1.DataScienceCluster{},
OwnerName: xid.New().String(),
Client: cl,
DSCI: &dsciv1.DSCInitialization{Spec: dsciv1.DSCInitializationSpec{ApplicationsNamespace: ns}},
DSC: &dscv1.DataScienceCluster{},
Instance: &componentsv1.Dashboard{
ObjectMeta: metav1.ObjectMeta{
Generation: 1,
Expand All @@ -82,6 +84,7 @@ func TestDeployAction(t *testing.T) {
g.Expect(err).ShouldNot(HaveOccurred())

g.Expect(obj1).Should(And(
jq.Match(`.metadata.labels."%s" == "%s"`, labels.ComponentPartOf, rr.OwnerName),
jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.ComponentGeneration, strconv.FormatInt(rr.Instance.GetGeneration(), 10)),
jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.ComponentHash, dh),
jq.Match(`.metadata.annotations."%s" == "%s"`, annotations.PlatformVersion, "1.2.3"),
Expand Down
14 changes: 12 additions & 2 deletions pkg/controller/actions/gc/action_gc.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions"
odhTypes "github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
odhLabels "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/services/gc"
)

Expand Down Expand Up @@ -91,9 +92,16 @@ func (a *Action) run(ctx context.Context, rr *odhTypes.ReconciliationRequest) er
return err
}

selector := a.selector
if selector == nil {
selector = labels.SelectorFromSet(map[string]string{
odhLabels.ComponentPartOf: rr.OwnerName,
})
}

deleted, err := a.gc.Run(
ctx,
a.selector,
selector,
func(ctx context.Context, obj unstructured.Unstructured) (bool, error) {
if slices.Contains(a.unremovables, obj.GroupVersionKind()) {
return false, nil
Expand Down Expand Up @@ -128,7 +136,9 @@ func NewAction(opts ...ActionOpts) actions.Fn {
opt(&action)
}

action.selector = labels.SelectorFromSet(action.labels)
if len(action.labels) > 0 {
action.selector = labels.SelectorFromSet(action.labels)
}

// TODO: refactor
if action.gc == nil {
Expand Down
11 changes: 10 additions & 1 deletion pkg/controller/actions/gc/action_gc_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/client"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/annotations"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
gcSvc "github.com/opendatahub-io/opendatahub-operator/v2/pkg/services/gc"

. "github.com/onsi/gomega"
Expand Down Expand Up @@ -175,6 +176,7 @@ func TestGcAction(t *testing.T) {
NotTo(HaveOccurred())

rr := types.ReconciliationRequest{
OwnerName: componentsv1.DashboardInstanceName,
DSCI: &dsciv1.DSCInitialization{
ObjectMeta: metav1.ObjectMeta{
Generation: 1,
Expand Down Expand Up @@ -204,6 +206,13 @@ func TestGcAction(t *testing.T) {
g.Expect(err).NotTo(HaveOccurred())
}

l := make(map[string]string)
for k, v := range tt.labels {
l[k] = v
}

l[labels.ComponentPartOf] = rr.OwnerName

cm := corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "gc-cm",
Expand All @@ -214,7 +223,7 @@ func TestGcAction(t *testing.T) {
annotations.PlatformVersion: "0.1.0",
annotations.PlatformType: string(cluster.OpenDataHub),
},
Labels: tt.labels,
Labels: l,
},
}

Expand Down
12 changes: 9 additions & 3 deletions pkg/controller/actions/updatestatus/action_update_status.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import (
"github.com/opendatahub-io/opendatahub-operator/v2/controllers/status"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/actions"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/controller/types"
"github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels"
)

const (
Expand Down Expand Up @@ -40,8 +41,13 @@ func WithSelectorLabels(values map[string]string) ActionOpts {
}

func (a *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error {
if len(a.labels) == 0 {
return nil
l := make(map[string]string, len(a.labels))
for k, v := range a.labels {
l[k] = v
}

if l[labels.ComponentPartOf] == "" {
l[labels.ComponentPartOf] = rr.OwnerName
}

obj, ok := rr.Instance.(types.ResourceObject)
Expand All @@ -55,7 +61,7 @@ func (a *Action) run(ctx context.Context, rr *types.ReconciliationRequest) error
ctx,
deployments,
client.InNamespace(rr.DSCI.Spec.ApplicationsNamespace),
client.MatchingLabels(a.labels),
client.MatchingLabels(l),
)

if err != nil {
Expand Down
Loading

0 comments on commit a1560ca

Please sign in to comment.