Skip to content

Commit

Permalink
Available CRDs check feature
Browse files Browse the repository at this point in the history 
Reasons for this enhancement:
- A controller cannot set up a watch for a CRD that is not installed on
 the cluster, trying to set up a watch will panic the operator
- There is no known way, that we are aware of, to add a watch later
 without client cache issue

How does the enhancement work around the issue:
- A new controller to watch creation/deletion for the CRDs of interest
 to prevent unnecessary reconciles
- On start of the operator(main), detect which CRDs are avail (out of a
 fixed list)
- At the start each reconcile of new controller, we fetch the CRDs
 available again and compare it with CRDs fetched in previous step,
  If there is any change, we panic the op

Signed-off-by: raaizik <[email protected]>
Co-Authored-By: Rewant Soni <[email protected]>
  • Loading branch information
@raaizik @rewantsoni
raaizik and rewantsoni committed Sep 30, 2024
1 parent c7cd50e commit 6a000ae
Show file tree
Hide file tree
Showing 11 changed files with 200 additions and 100 deletions.
105 changes: 61 additions & 44 deletions controllers/ocsinitialization/ocsinitialization_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,22 @@ import (
"strconv"
"strings"

"github.com/go-logr/logr"
secv1client "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1"
opv1a1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
promv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
"github.com/red-hat-storage/ocs-operator/v4/controllers/defaults"
"github.com/red-hat-storage/ocs-operator/v4/controllers/platform"
"github.com/red-hat-storage/ocs-operator/v4/controllers/storagecluster"
"github.com/red-hat-storage/ocs-operator/v4/controllers/util"
"github.com/red-hat-storage/ocs-operator/v4/templates"

"github.com/go-logr/logr"
secv1client "github.com/openshift/client-go/security/clientset/versioned/typed/security/v1"
opv1a1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
promv1 "github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring/v1"
rookCephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1"
"gopkg.in/yaml.v2"
corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -43,8 +45,8 @@ import (
var operatorNamespace string

const (
wrongNamespacedName = "Ignoring this resource. Only one should exist, and this one has the wrong name and/or namespace."
random30CharacterString = "KP7TThmSTZegSGmHuPKLnSaaAHSG3RSgqw6akBj0oVk"
random30CharacterString = "KP7TThmSTZegSGmHuPKLnSaaAHSG3RSgqw6akBj0oVk"

PrometheusOperatorDeploymentName = "prometheus-operator"
PrometheusOperatorCSVNamePrefix = "odf-prometheus-operator"
ClusterClaimCrdName = "clusterclaims.cluster.open-cluster-management.io"
Expand All @@ -63,13 +65,14 @@ func InitNamespacedName() types.NamespacedName {
// nolint:revive
type OCSInitializationReconciler struct {
client.Client
ctx context.Context
ctx context.Context
clusters *util.Clusters

Log logr.Logger
Scheme *runtime.Scheme
SecurityClient secv1client.SecurityV1Interface
OperatorNamespace string
clusters *util.Clusters
availableCrds map[string]bool
AvailableCrds map[string]bool
}

// +kubebuilder:rbac:groups=ocs.openshift.io,resources=*,verbs=get;list;watch;create;update;patch;delete
Expand All @@ -93,11 +96,26 @@ func (r *OCSInitializationReconciler) Reconcile(ctx context.Context, request rec

r.Log.Info("Reconciling OCSInitialization.", "OCSInitialization", klog.KRef(request.Namespace, request.Name))

crd := &metav1.PartialObjectMetadata{}
crd.SetGroupVersionKind(extv1.SchemeGroupVersion.WithKind("CustomResourceDefinition"))
crd.Name = ClusterClaimCrdName
if err := r.Client.Get(ctx, client.ObjectKeyFromObject(crd), crd); client.IgnoreNotFound(err) != nil {
r.Log.Error(err, "Failed to get CRD", "CRD", ClusterClaimCrdName)
return reconcile.Result{}, err
}
util.AssertEqual(r.AvailableCrds[ClusterClaimCrdName], crd.UID != "", util.ExitCodeThatShouldRestartTheProcess)

initNamespacedName := InitNamespacedName()
instance := &ocsv1.OCSInitialization{}
if initNamespacedName.Name != request.Name || initNamespacedName.Namespace != request.Namespace {
// Ignoring this resource because it has the wrong name or namespace
r.Log.Info(wrongNamespacedName)
r.Log.Info(
"Ignoring this resource. Only one OCSInitialization should exist.",
"Expected",
initNamespacedName,
"Got",
request.NamespacedName,
)
err := r.Client.Get(ctx, request.NamespacedName, instance)
if err != nil {
// the resource probably got deleted
Expand Down Expand Up @@ -147,11 +165,6 @@ func (r *OCSInitializationReconciler) Reconcile(ctx context.Context, request rec
}
}

r.availableCrds, err = util.MapCRDAvailability(r.ctx, r.Client, r.Log, ClusterClaimCrdName)
if err != nil {
return reconcile.Result{}, err
}

r.clusters, err = util.GetClusters(ctx, r.Client)
if err != nil {
r.Log.Error(err, "Failed to get clusters")
Expand Down Expand Up @@ -179,7 +192,7 @@ func (r *OCSInitializationReconciler) Reconcile(ctx context.Context, request rec
return reconcile.Result{}, err
}

if r.availableCrds[ClusterClaimCrdName] {
if r.AvailableCrds[ClusterClaimCrdName] {
err = r.ensureClusterClaimExists()
if err != nil {
r.Log.Error(err, "Failed to ensure odf-info namespacedname ClusterClaim")
Expand Down Expand Up @@ -271,6 +284,14 @@ func (r *OCSInitializationReconciler) SetupWithManager(mgr ctrl.Manager) error {
},
)

enqueueOCSInit := handler.EnqueueRequestsFromMapFunc(
func(context context.Context, obj client.Object) []reconcile.Request {
return []reconcile.Request{{
NamespacedName: InitNamespacedName(),
}}
},
)

ocsInitializationController := ctrl.NewControllerManagedBy(mgr).
For(&ocsv1.OCSInitialization{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
Owns(&corev1.Service{}).
Expand All @@ -283,13 +304,7 @@ func (r *OCSInitializationReconciler) SetupWithManager(mgr ctrl.Manager) error {
// ocs-operator-config configmap if storagecluster spec changes
Watches(
&ocsv1.StorageCluster{},
handler.EnqueueRequestsFromMapFunc(
func(context context.Context, obj client.Object) []reconcile.Request {
return []reconcile.Request{{
NamespacedName: InitNamespacedName(),
}}
},
),
enqueueOCSInit,
builder.WithPredicates(predicate.GenerationChangedPredicate{}),
).
// Watcher for storageClass required to update values related to replica-1
Expand Down Expand Up @@ -317,13 +332,7 @@ func (r *OCSInitializationReconciler) SetupWithManager(mgr ctrl.Manager) error {
Namespace: r.OperatorNamespace,
},
},
handler.EnqueueRequestsFromMapFunc(
func(context context.Context, obj client.Object) []reconcile.Request {
return []reconcile.Request{{
NamespacedName: InitNamespacedName(),
}}
},
),
enqueueOCSInit,
).
// Watcher for ocs-operator-config cm
Watches(
Expand All @@ -333,26 +342,34 @@ func (r *OCSInitializationReconciler) SetupWithManager(mgr ctrl.Manager) error {
Namespace: r.OperatorNamespace,
},
},
handler.EnqueueRequestsFromMapFunc(
func(context context.Context, obj client.Object) []reconcile.Request {
return []reconcile.Request{{
NamespacedName: InitNamespacedName(),
}}
},
),
enqueueOCSInit,
).
// Watcher for prometheus operator csv
Watches(
&opv1a1.ClusterServiceVersion{},
handler.EnqueueRequestsFromMapFunc(
func(context context.Context, obj client.Object) []reconcile.Request {
return []reconcile.Request{{
NamespacedName: InitNamespacedName(),
}}
},
),
enqueueOCSInit,
builder.WithPredicates(prometheusPredicate),
).
Watches(
&extv1.CustomResourceDefinition{},
enqueueOCSInit,
builder.WithPredicates(
util.NamePredicate(ClusterClaimCrdName),
util.CrdCreateAndDeletePredicate(&r.Log, ClusterClaimCrdName, r.AvailableCrds[ClusterClaimCrdName]),
),
builder.OnlyMetadata,
)

if r.AvailableCrds[ClusterClaimCrdName] {
ocsInitializationController = ocsInitializationController.Watches(
&v1alpha1.ClusterClaim{},
enqueueOCSInit,
builder.WithPredicates(
util.NamePredicate(util.OdfInfoNamespacedNameClaimName),
predicate.GenerationChangedPredicate{},
),
)
}
return ocsInitializationController.Complete(r)
}

Expand Down
38 changes: 29 additions & 9 deletions controllers/storagecluster/initialization_reconciler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,18 +6,18 @@ import (
"os"
"testing"

"github.com/blang/semver/v4"
oprverion "github.com/operator-framework/api/pkg/lib/version"
opv1a1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
api "github.com/red-hat-storage/ocs-operator/api/v4/v1"
"github.com/red-hat-storage/ocs-operator/v4/controllers/platform"
"github.com/red-hat-storage/ocs-operator/v4/controllers/util"
ocsversion "github.com/red-hat-storage/ocs-operator/v4/version"

"github.com/blang/semver/v4"
"github.com/imdario/mergo"
nbv1 "github.com/noobaa/noobaa-operator/v5/pkg/apis/noobaa/v1alpha1"
configv1 "github.com/openshift/api/config/v1"
routev1 "github.com/openshift/api/route/v1"
api "github.com/red-hat-storage/ocs-operator/api/v4/v1"
"github.com/red-hat-storage/ocs-operator/v4/controllers/platform"
"github.com/red-hat-storage/ocs-operator/v4/controllers/util"
oprverion "github.com/operator-framework/api/pkg/lib/version"
opv1a1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1"
"github.com/stretchr/testify/assert"
appsv1 "k8s.io/api/apps/v1"
Expand Down Expand Up @@ -255,7 +255,6 @@ func initStorageClusterResourceCreateUpdateTest(t *testing.T, runtimeObjs []clie
}
rtObjsToCreateReconciler = append(rtObjsToCreateReconciler, tbd)
}

reconciler := createFakeInitializationStorageClusterReconciler(
t, rtObjsToCreateReconciler...)

Expand All @@ -271,7 +270,6 @@ func initStorageClusterResourceCreateUpdateTest(t *testing.T, runtimeObjs []clie
assert.Equal(t, reconcile.Result{}, result)
err = os.Setenv("WATCH_NAMESPACE", cr.Namespace)
assert.NoError(t, err)

return t, reconciler, cr, requestOCSInit
}

Expand Down Expand Up @@ -395,13 +393,35 @@ func createFakeInitializationStorageClusterReconciler(t *testing.T, obj ...runti
}
}

runtimeObjects = append(runtimeObjects, mockNodeList.DeepCopy(), cbp, cfs, cnfs, cnfsbp, cnfssvc, infrastructure, networkConfig, rookCephMonSecret, csv, workerNode, ocsProviderServiceSecret, ocsProviderServiceDeployment, ocsProviderService)
runtimeObjects = append(
runtimeObjects,
mockNodeList.DeepCopy(),
cbp,
cfs,
cnfs,
cnfsbp,
cnfssvc,
infrastructure,
networkConfig,
rookCephMonSecret,
csv,
workerNode,
ocsProviderServiceSecret,
ocsProviderServiceDeployment,
ocsProviderService,
createVirtualMachineCRD(),
)
client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(runtimeObjects...).WithStatusSubresource(statusSubresourceObjs...).Build()

availCrds := map[string]bool{
VirtualMachineCrdName: true,
}

return StorageClusterReconciler{
Client: client,
Scheme: scheme,
OperatorCondition: newStubOperatorCondition(),
Log: logf.Log.WithName("controller_storagecluster_test"),
AvailableCrds: availCrds,
}
}
21 changes: 17 additions & 4 deletions controllers/storagecluster/reconcile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,16 +7,18 @@ import (
"strings"
"time"

"github.com/blang/semver/v4"
"github.com/go-logr/logr"
conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
"github.com/operator-framework/operator-lib/conditions"
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
ocsv1alpha1 "github.com/red-hat-storage/ocs-operator/api/v4/v1alpha1"
"github.com/red-hat-storage/ocs-operator/v4/controllers/util"
statusutil "github.com/red-hat-storage/ocs-operator/v4/controllers/util"
"github.com/red-hat-storage/ocs-operator/v4/version"

"github.com/blang/semver/v4"
"github.com/go-logr/logr"
conditionsv1 "github.com/openshift/custom-resource-status/conditions/v1"
"github.com/operator-framework/operator-lib/conditions"
corev1 "k8s.io/api/core/v1"
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/klog/v2"
Expand Down Expand Up @@ -70,6 +72,8 @@ const (

// EBS represents AWS EBS provisioner for StorageClass
EBS StorageClassProvisionerType = "kubernetes.io/aws-ebs"

VirtualMachineCrdName = "virtualmachines.kubevirt.io"
)

var storageClusterFinalizer = "storagecluster.ocs.openshift.io"
Expand Down Expand Up @@ -143,6 +147,15 @@ func (r *StorageClusterReconciler) Reconcile(ctx context.Context, request reconc
r.Log = r.Log.WithValues("Request.Namespace", request.Namespace, "Request.Name", request.Name)
r.ctx = ctrllog.IntoContext(ctx, r.Log)

crd := &metav1.PartialObjectMetadata{}
crd.SetGroupVersionKind(extv1.SchemeGroupVersion.WithKind("CustomResourceDefinition"))
crd.Name = VirtualMachineCrdName
if err := r.Client.Get(ctx, client.ObjectKeyFromObject(crd), crd); client.IgnoreNotFound(err) != nil {
r.Log.Error(err, "Failed to get CRD", "CRD", VirtualMachineCrdName)
return reconcile.Result{}, err
}
util.AssertEqual(r.AvailableCrds[VirtualMachineCrdName], crd.UID != "", util.ExitCodeThatShouldRestartTheProcess)

// Fetch the StorageCluster instance
sc := &ocsv1.StorageCluster{}
if err := r.Client.Get(ctx, request.NamespacedName, sc); err != nil {
Expand Down
5 changes: 1 addition & 4 deletions controllers/storagecluster/storageclasses.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ import (
cephv1 "github.com/rook/rook/pkg/apis/ceph.rook.io/v1"
corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand Down Expand Up @@ -452,9 +451,7 @@ func (r *StorageClusterReconciler) newStorageClassConfigurations(initData *ocsv1
// when allowing consumers, creation of storage classes should only be done via storagerequests
if !initData.Spec.AllowRemoteStorageConsumers {
// If kubevirt crd is present, we create a specialized rbd storageclass for virtualization environment
kvcrd := &extv1.CustomResourceDefinition{}
err := r.Client.Get(context.TODO(), types.NamespacedName{Name: "virtualmachines.kubevirt.io", Namespace: ""}, kvcrd)
if err == nil {
if r.AvailableCrds[VirtualMachineCrdName] {
ret = append(ret, newCephBlockPoolVirtualizationStorageClassConfiguration(initData))
}
}
Expand Down
2 changes: 1 addition & 1 deletion controllers/storagecluster/storageclasses_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ var (
},
ObjectMeta: metav1.ObjectMeta{
Name: pluralName + "." + "kubevirt.io",
UID: "uid",
},
Spec: extv1.CustomResourceDefinitionSpec{
Group: "kubevirt.io",
Expand Down Expand Up @@ -99,7 +100,6 @@ func TestCustomEncryptedStorageClasses(t *testing.T) {

func testStorageClasses(t *testing.T, pvEncryption bool, customSpec *api.StorageClusterSpec) {
runtimeObjs := []client.Object{}
runtimeObjs = append(runtimeObjs, createVirtualMachineCRD())
if pvEncryption {
runtimeObjs = append(runtimeObjs, createDummyKMSConfigMap(dummyKmsProvider, dummyKmsAddress, ""))
}
Expand Down
Loading

0 comments on commit 6a000ae

Please sign in to comment.