Skip to content

remove the bundled NSTemplateTiers that are no longer used #1180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

remove the bundled NSTemplateTiers that are no longer used #1180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
a2f74ef
remove the bundled NSTemplateTiers that are no longer used
metlos May 27, 2025
cebe77c
merge master
metlos Jun 4, 2025
a9d224d
Merge branch 'master' into remove-unused-bundled-tiers
alexeykazakov Jun 4, 2025
05bb8c9
use the latest api
metlos Jun 5, 2025
cd13c5d
bundled tiers are created and deleted in the generator, controller ha…
metlos Jun 6, 2025
4303502
use just the presence of the finalizer as the "witness" for a bundled…
metlos Jun 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -425,7 +425,7 @@ func main() { // nolint:gocyclo

// create or update all NSTemplateTiers on the cluster at startup
setupLog.Info("Creating/updating the NSTemplateTier resources")
if err := nstemplatetiers.CreateOrUpdateResources(ctx, mgr.GetScheme(), mgr.GetClient(), namespace); err != nil {
if err := nstemplatetiers.SyncResources(ctx, mgr.GetScheme(), mgr.GetClient(), namespace); err != nil {
setupLog.Error(err, "")
os.Exit(1)
}
Expand Down
47 changes: 45 additions & 2 deletions controllers/nstemplatetier/nstemplatetier_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,10 @@

toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"github.com/codeready-toolchain/host-operator/controllers/toolchainconfig"
"github.com/codeready-toolchain/host-operator/pkg/constants"
"github.com/codeready-toolchain/host-operator/pkg/templates/nstemplatetiers"
"github.com/codeready-toolchain/toolchain-common/pkg/condition"
"github.com/codeready-toolchain/toolchain-common/pkg/hash"
"github.com/redhat-cop/operator-utils/pkg/util"
corev1 "k8s.io/api/core/v1"

Expand All @@ -20,6 +22,7 @@
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/manager"
Expand Down Expand Up @@ -64,7 +67,7 @@
}
// if the NSTemplateTier is being deleted, then do nothing
if util.IsBeingDeleted(tier) {
return reconcile.Result{}, nil
return reconcile.Result{}, r.handleDeletion(ctx, tier)
}
Comment on lines 69 to 71
Copy link
Contributor

Choose a reason for hiding this comment

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

the code is still missing a logic that would add a finalizer


_, err := toolchainconfig.GetToolchainConfig(r.Client)
Expand All @@ -91,6 +94,47 @@
return reconcile.Result{}, err
}

func (r *Reconciler) handleDeletion(ctx context.Context, tier *toolchainv1alpha1.NSTemplateTier) error {
if !util.HasFinalizer(tier, constants.BundledNSTemplateTierFinalizerName) {
// no special handling required for "ordinary" nstemplatetiers
return nil
}

bundled, err := nstemplatetiers.IsBundled(runtimeclient.ObjectKeyFromObject(tier))
if err != nil {
return fmt.Errorf("failed to find if a tier is bundled or not (this shouldn't happen): %w", err)
}

Check warning on line 106 in controllers/nstemplatetier/nstemplatetier_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nstemplatetier/nstemplatetier_controller.go#L105-L106 

Added lines #L105 - L106 were not covered by tests

if bundled {
// let's just keep the bundled tier around even if someone wants to delete it. It'd be recreated at
// the next start of the operator anyway.
return nil
}
Comment on lines +103 to +112
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think that we should really check if it's bundled or not as part of the finalizer. If you would block the deletion, then the CR would end up in the terminating state forever.
If someone want's to delete bundled tier, then just delete it if it's not used by a space. As you mentioned, then next operator start will recreated it anyway.


// We have a tier with a "bundled" finalizer that is not bundled - i.e. a tier that used to be bundled
// but is not anymore and that is being deleted. We allow such deletion only when the tier is not used
// by any space.
used, err := isTierUsed(ctx, r.Client, tier)
if err != nil {
return err
}

Check warning on line 120 in controllers/nstemplatetier/nstemplatetier_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nstemplatetier/nstemplatetier_controller.go#L119-L120 

Added lines #L119 - L120 were not covered by tests

if !used {
controllerutil.RemoveFinalizer(tier, constants.BundledNSTemplateTierFinalizerName)
return r.Client.Update(ctx, tier)
}

return nil
}

func isTierUsed(ctx context.Context, client runtimeclient.Client, tier *toolchainv1alpha1.NSTemplateTier) (bool, error) {
list := &toolchainv1alpha1.SpaceList{}
if err := client.List(ctx, list, runtimeclient.InNamespace(tier.Namespace), runtimeclient.HasLabels{hash.TemplateTierHashLabelKey(tier.Name)}); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

only an optimization tip (in this particular case it doesn't matter much) - you can use runtimeclient.Limit(1) as a list option to get only one result from the list. The idea is that you don't need to get all Spaces with the label, but only one

return false, err
}

Check warning on line 134 in controllers/nstemplatetier/nstemplatetier_controller.go

View check run for this annotation

Codecov / codecov/patch

controllers/nstemplatetier/nstemplatetier_controller.go#L133-L134 

Added lines #L133 - L134 were not covered by tests
return len(list.Items) > 0, nil
}

// ensureRevision ensures that there is a TierTemplateRevision CR for each of the TierTemplate.
// returns `true` if a new TierTemplateRevision CR was created, `err` if something wrong happened
func (r *Reconciler) ensureRevision(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier) (bool, error) {
Expand Down Expand Up @@ -183,7 +227,6 @@
return false, "", err
}
return true, ttrName, nil

}

func (r *Reconciler) createNewTierTemplateRevision(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, tierTemplate *toolchainv1alpha1.TierTemplate) (string, error) {
Expand Down
156 changes: 122 additions & 34 deletions controllers/nstemplatetier/nstemplatetier_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,14 @@ import (
toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"
"github.com/codeready-toolchain/host-operator/controllers/nstemplatetier"
"github.com/codeready-toolchain/host-operator/pkg/apis"
"github.com/codeready-toolchain/host-operator/pkg/constants"
"github.com/codeready-toolchain/host-operator/pkg/templates/nstemplatetiers"
tiertest "github.com/codeready-toolchain/host-operator/test/nstemplatetier"
"github.com/codeready-toolchain/host-operator/test/tiertemplaterevision"
"github.com/codeready-toolchain/toolchain-common/pkg/hash"
"github.com/codeready-toolchain/toolchain-common/pkg/test"
templatev1 "github.com/openshift/api/template/v1"
"github.com/redhat-cop/operator-utils/pkg/util"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"k8s.io/apimachinery/pkg/api/errors"
Expand All @@ -27,6 +31,7 @@ import (
"k8s.io/apimachinery/pkg/util/validation"
"k8s.io/client-go/kubernetes/scheme"
runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

Expand All @@ -37,9 +42,7 @@ const (
func TestReconcile(t *testing.T) {
// given
t.Run("failures", func(t *testing.T) {

t.Run("unable to get NSTemplateTier", func(t *testing.T) {

t.Run("tier not found", func(t *testing.T) {
// given
base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates)
Expand Down Expand Up @@ -75,7 +78,6 @@ func TestReconcile(t *testing.T) {
assert.Equal(t, reconcile.Result{}, res) // no explicit requeue
})
})

})

t.Run("revisions management", func(t *testing.T) {
Expand Down Expand Up @@ -194,7 +196,6 @@ func TestReconcile(t *testing.T) {
require.Len(t, ttrs.Items, len(tierTemplatesRefs)) // it's one TTR per each tiertemplate in the NSTemplateTier
})
})

})

t.Run("revision field is set but TierTemplateRevision CRs are missing, they should be created", func(t *testing.T) {
Expand Down Expand Up @@ -264,7 +265,6 @@ func TestReconcile(t *testing.T) {
})

t.Run("errors", func(t *testing.T) {

t.Run("error when TierTemplate is missing ", func(t *testing.T) {
// given
// make sure revisions field is nill before starting the test
Expand All @@ -285,41 +285,130 @@ func TestReconcile(t *testing.T) {
// and the TierTemplateRevision CRs are not created
tiertemplaterevision.AssertThatTTRs(t, cl, base1nsTier.GetNamespace()).DoNotExist()
})
})
})

t.Run("if being deleted", func(t *testing.T) {
t.Run("do nothing on non-bundled tiers", func(t *testing.T) {
// given
tierBeingDeleted := base1nsTier.DeepCopy()
tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()}
controllerutil.AddFinalizer(tierBeingDeleted, "dummy") // so that the fake client allows us to create an object with non-nil deletion timestamp
r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted)

// when
res, err := r.Reconcile(context.TODO(), req)
inCluster := &toolchainv1alpha1.NSTemplateTier{}
require.NoError(t, cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster))

// then
require.NoError(t, err)
assert.Empty(t, res.Requeue)
assert.Len(t, inCluster.Finalizers, 1)
assert.Contains(t, inCluster.Finalizers, "dummy")
})
t.Run("keep the finalizer if no-longer-bundled tier is still used", func(t *testing.T) {
// given
tierBeingDeleted := base1nsTier.DeepCopy()
tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()}
tierBeingDeleted.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue}
util.AddFinalizer(tierBeingDeleted, constants.BundledNSTemplateTierFinalizerName)
nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{}
space := &toolchainv1alpha1.Space{
ObjectMeta: metav1.ObjectMeta{
Name: "test-space",
Namespace: tierBeingDeleted.Namespace,
Labels: map[string]string{hash.TemplateTierHashLabelKey(tierBeingDeleted.Name): "1234"},
},
}

})
r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted, space)

t.Run("if being deleted, then do nothing", func(t *testing.T) {
// given
tierBeingDeleted := base1nsTier.DeepCopy()
tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()}
tierBeingDeleted.Finalizers = []string{"dummy"}
r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted)
called := false
cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error {
if called {
return fmt.Errorf("should not call Get more than once")
// when
res, err := r.Reconcile(context.TODO(), req)
inCluster := &toolchainv1alpha1.NSTemplateTier{}
gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster)

// then
require.NoError(t, err)
require.Empty(t, res)
require.NoError(t, gerr)
require.Contains(t, inCluster.Finalizers, constants.BundledNSTemplateTierFinalizerName)
})
t.Run("remove the finalizer if no-longer-bundled tier is not used anymore", func(t *testing.T) {
// given
tierBeingDeleted := base1nsTier.DeepCopy()
tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()}
tierBeingDeleted.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue}
util.AddFinalizer(tierBeingDeleted, constants.BundledNSTemplateTierFinalizerName)
nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{}

r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted)

// when
res, err := r.Reconcile(context.TODO(), req)
inCluster := &toolchainv1alpha1.NSTemplateTier{}
gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster)

// then
require.NoError(t, err)
require.Empty(t, res)
require.True(t, errors.IsNotFound(gerr))
require.Empty(t, inCluster.Finalizers)
})
t.Run("keep the finalizer if a bundled finalizer is still used", func(t *testing.T) {
// given
tierBeingDeleted := base1nsTier.DeepCopy()
tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()}
tierBeingDeleted.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue}
util.AddFinalizer(tierBeingDeleted, constants.BundledNSTemplateTierFinalizerName)
nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{runtimeclient.ObjectKeyFromObject(tierBeingDeleted)}
space := &toolchainv1alpha1.Space{
ObjectMeta: metav1.ObjectMeta{
Name: "test-space",
Namespace: tierBeingDeleted.Namespace,
Labels: map[string]string{hash.TemplateTierHashLabelKey(tierBeingDeleted.Name): "1234"},
},
}
called = true
return cl.Client.Get(ctx, key, obj, opts...)
}
cl.MockCreate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.CreateOption) error {
return fmt.Errorf("should not call Create")
}
cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.SubResourceUpdateOption) error {
return fmt.Errorf("should not call StatusUpdate")
}

// when
res, err := r.Reconcile(context.TODO(), req)
r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted, space)

// then
require.NoError(t, err)
assert.Empty(t, res.Requeue)
// when
res, err := r.Reconcile(context.TODO(), req)
inCluster := &toolchainv1alpha1.NSTemplateTier{}
gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster)

// then
require.NoError(t, err)
require.Empty(t, res)
require.NoError(t, gerr)
require.Len(t, inCluster.Finalizers, 1)
require.Contains(t, inCluster.Finalizers, constants.BundledNSTemplateTierFinalizerName)
})
t.Run("keep the finalizer if a bundled tier is not used", func(t *testing.T) {
// given
tierBeingDeleted := base1nsTier.DeepCopy()
tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()}
tierBeingDeleted.Annotations = map[string]string{toolchainv1alpha1.BundledAnnotationKey: constants.BundledWithHostOperatorAnnotationValue}
util.AddFinalizer(tierBeingDeleted, constants.BundledNSTemplateTierFinalizerName)
nstemplatetiers.BundledTierKeys = []runtimeclient.ObjectKey{runtimeclient.ObjectKeyFromObject(tierBeingDeleted)}

r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted)

// when
res, err := r.Reconcile(context.TODO(), req)
inCluster := &toolchainv1alpha1.NSTemplateTier{}
gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster)

// then
require.NoError(t, err)
require.Empty(t, res)
require.NoError(t, gerr)
require.Len(t, inCluster.Finalizers, 1)
require.Contains(t, inCluster.Finalizers, constants.BundledNSTemplateTierFinalizerName)
})
})
})

}

func TestUpdateNSTemplateTier(t *testing.T) {
Expand Down Expand Up @@ -454,13 +543,11 @@ func TestUpdateNSTemplateTier(t *testing.T) {
HasStatusTierTemplateRevisions(tierTemplatesRefs).Tier()
require.NotEqual(t, tierBeingUpdated.Status.Revisions, newNSTmplTier.Status.Revisions)
})

})

}

func newTestCRQ(podsCount string) unstructured.Unstructured {
var crq = unstructured.Unstructured{Object: map[string]interface{}{
crq := unstructured.Unstructured{Object: map[string]interface{}{
"kind": "ClusterResourceQuota",
"metadata": map[string]interface{}{
"name": "for-{{.SPACE_NAME}}-deployments",
Expand Down Expand Up @@ -512,6 +599,7 @@ func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Objec
Client: cl,
Scheme: s,
}

return r, reconcile.Request{
NamespacedName: types.NamespacedName{
Name: name,
Expand Down
2 changes: 1 addition & 1 deletion go.mod
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ module github.com/codeready-toolchain/host-operator

require (
cloud.google.com/go/recaptchaenterprise/v2 v2.13.0
github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271
github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27
github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12
github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
github.com/go-logr/logr v1.4.2
Expand Down
4 changes: 2 additions & 2 deletions go.sum
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,8 @@ github.com/cloudflare/circl v1.1.0/go.mod h1:prBCrKB9DV4poKZY1l9zBXg2QJY7mvgRvtM
github.com/cloudflare/circl v1.3.7 h1:qlCDlTPz2n9fu58M0Nh1J/JzcFpfgkFHHX3O35r5vcU=
github.com/cloudflare/circl v1.3.7/go.mod h1:sRTcRWXGLrKw6yIGJ+l7amYJFfAXbZG0kBSc8r4zxgA=
github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc=
github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271 h1:AHrFr/aPuJ4+0zHw4sFXcfMA92kChy12JAPS5bLODlw=
github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271/go.mod h1:20258od6i5+jP406Z76YaI2ow/vc7URwsDU2bokpkRE=
github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27 h1:g1ivSPPHTC96RHp8S/gRmqODWgoHyivq+/d5kSI0pEs=
github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27/go.mod h1:20258od6i5+jP406Z76YaI2ow/vc7URwsDU2bokpkRE=
github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12 h1:w54sojJJ8PsHZzK1mC+/EUBrQ9F2sC/k7JUVc8LSqK4=
github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12/go.mod h1:TrMvD0sP69wI6Rouzfs7OsOUSj4CGn/ZiIdiDBAFQjk=
github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=
Expand Down
13 changes: 13 additions & 0 deletions pkg/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package constants

import toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1"

// HostOperatorFieldManager is the field manager we want to use in the managed fields
// of objects deployed by the host operator.
const HostOperatorFieldManager = "kubesaw-host-operator"

// BundledWithHostOperatorAnnotationValue is meant to be the value of the toolchainv1alpha1.BundledLabelKey that marks
// the objects as bundled with the host operator and therefore managed by it.
const BundledWithHostOperatorAnnotationValue = "host-operator"

const BundledNSTemplateTierFinalizerName = toolchainv1alpha1.LabelKeyPrefix + "bundled-tier"
5 changes: 0 additions & 5 deletions pkg/constants/field_manager.go
View file
Open in desktop

This file was deleted.

Loading
Loading