Skip to content
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

Jump to bottom

Available CRDs check feature #2712

Closed
wants to merge 2 commits into from
Closed

Available CRDs check feature #2712

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e5d154b
Available CRDs check feature
raaizik Jul 24, 2024
3617868
Available CRDs check feature w script
raaizik Aug 8, 2024
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
1 change: 1 addition & 0 deletions controllers/storagecluster/initialization_reconciler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,5 +401,6 @@ func createFakeInitializationStorageClusterReconciler(t *testing.T, obj ...runti
Scheme: scheme,
OperatorCondition: newStubOperatorCondition(),
Log: logf.Log.WithName("controller_storagecluster_test"),
AvailableCrds: make(map[string]bool),
}
}
14 changes: 13 additions & 1 deletion controllers/storagecluster/reconcile.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@ import (
"context"
error1 "errors"
"fmt"
"os"
"reflect"
"strings"
"time"

Expand Down Expand Up @@ -159,14 +161,24 @@ func (r *StorageClusterReconciler) Reconcile(ctx context.Context, request reconc
return reconcile.Result{}, err
}

var err error
availableCrds, err := statusutil.MapCRDAvailability(ctx, r.Client, statusutil.GetCrds()...)
if err != nil {
r.Log.Error(err, "error fetching the CRDs")
return reconcile.Result{}, err
}
if !reflect.DeepEqual(availableCrds, r.AvailableCrds) {
r.Log.Info("CustomResourceDefinitions created/deleted. Restarting process.")
os.Exit(42)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we please have the exit code in a constant with a name that explains the usage?
In addition, a comment about this line will help

Copy link
Member

Choose a reason for hiding this comment

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

Added it with #2745

}
Comment on lines +170 to +173
Copy link
Contributor

Choose a reason for hiding this comment

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

Available CRD is a map. I am not sure if there is an order guarantee on keys which means that DeepEqual might fail even if all the keys and values are the same. I would suggest changing the check to maps.Equal

Copy link
Member

Choose a reason for hiding this comment

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

I tried to create a map, added keys in a different order, and compared them using DeepEqual and it worked. maps.Equal produces same results
https://go.dev/play/p/uFkmp57xEIp


if err := r.validateStorageClusterSpec(sc); err != nil {
return reconcile.Result{}, err
}

r.IsNoobaaStandalone = sc.Spec.MultiCloudGateway != nil &&
ReconcileStrategy(sc.Spec.MultiCloudGateway.ReconcileStrategy) == ReconcileStrategyStandalone

var err error
r.clusters, err = statusutil.GetClusters(ctx, r.Client)
if err != nil {
r.Log.Error(err, "Failed to get clusters")
Expand Down
27 changes: 27 additions & 0 deletions controllers/storagecluster/storagecluster_controller.go
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Where is the conditional watch that checks the CRD exists before adding the watch?

Copy link
Member

Choose a reason for hiding this comment

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

Added it in #2745

Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ type StorageClusterReconciler struct {
IsMultipleStorageClusters bool
clusters *util.Clusters
OperatorNamespace string
AvailableCrds map[string]bool
}

// SetupWithManager sets up a controller with manager
Expand Down Expand Up @@ -211,6 +212,31 @@ func (r *StorageClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
},
}

crdPredicate := predicate.Funcs{
CreateFunc: func(e event.TypedCreateEvent[client.Object]) bool {
crdAvailable, keyExist := r.AvailableCrds[e.Object.GetName()]
if keyExist && !crdAvailable {
r.Log.Info("CustomResourceDefinition %s was Created.", e.Object.GetName())
return true
}
return false
},
DeleteFunc: func(e event.TypedDeleteEvent[client.Object]) bool {
crdAvailable, keyExist := r.AvailableCrds[e.Object.GetName()]
if keyExist && crdAvailable {
r.Log.Info("CustomResourceDefinition %s was Deleted.", e.Object.GetName())
return true
}
return false
},
Comment on lines +224 to +231
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a possibility that CRD deletion is less than our operator lifetime?

Copy link
Member

Choose a reason for hiding this comment

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

Although it is very unlikely but it is possible, and if they happens it will lead to a pod restart

UpdateFunc: func(e event.TypedUpdateEvent[client.Object]) bool {
return false
},
GenericFunc: func(e event.TypedGenericEvent[client.Object]) bool {
return false
},
}

build := ctrl.NewControllerManagedBy(mgr).
For(&ocsv1.StorageCluster{}, builder.WithPredicates(scPredicate)).
Owns(&cephv1.CephCluster{}, builder.WithPredicates(cephClusterIgnoreTimeUpdatePredicate)).
Expand All @@ -228,6 +254,7 @@ func (r *StorageClusterReconciler) SetupWithManager(mgr ctrl.Manager) error {
Owns(&corev1.Secret{}, builder.WithPredicates(predicate.GenerationChangedPredicate{})).
Owns(&routev1.Route{}).
Owns(&templatev1.Template{}).
Watches(&extv1.CustomResourceDefinition{}, enqueueStorageClusterRequest, builder.WithPredicates(crdPredicate)).
Copy link
Contributor

Choose a reason for hiding this comment

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

we already made a mistake (with Virt) in watching all CRDs increasing the memory footprint, my mistake in not pushing hard on this fix #2539, seeing this PR we don't want to watch all CRDs until & unless necessary.

Clubbed w/ 2539 we could do something like below as we are only interested in the metadata of CRDs while mapping CRDs.

crd := &metav1.PartialObjectMetadata{}
crd.SetGroupVersionKind(extv1.SchemeGroupVersion.WithKind("CustomResourceDefinition"))
crd.Name = <CRDName>
client.Get(<ctx>, <ns-name>, crd)

Copy link
Member

Choose a reason for hiding this comment

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

Looked at this and I couldn't find a way to specify multiple names in the cache using field selector, But I have updated it to use PartialObjectMetadata here: #2745

Watches(&storagev1.StorageClass{}, enqueueStorageClusterRequest).
Watches(&volumesnapshotv1.VolumeSnapshotClass{}, enqueueStorageClusterRequest).
Watches(&ocsclientv1a1.StorageClient{}, enqueueStorageClusterRequest).
Expand Down
1 change: 1 addition & 0 deletions controllers/storagecluster/storagecluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1168,6 +1168,7 @@ func createFakeStorageClusterReconciler(t *testing.T, obj ...runtime.Object) Sto
Log: logf.Log.WithName("controller_storagecluster_test"),
clusters: clusters,
OperatorNamespace: operatorNamespace,
AvailableCrds: make(map[string]bool),
}
}

Expand Down
18 changes: 18 additions & 0 deletions controllers/util/k8sutil.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -44,6 +45,10 @@ const (
OwnerUIDIndexName = "ownerUID"
)

func GetCrds() []string {
return []string{}
Copy link
Contributor

Choose a reason for hiding this comment

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

is my understanding correct that in upcoming PRs we register the CRDNames in this and conditionally watch on them in the setupwithmanager?

Copy link
Member

Choose a reason for hiding this comment

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

Added it with #2745

}

// GetWatchNamespace returns the namespace the operator should be watching for changes
func GetWatchNamespace() (string, error) {
ns, found := os.LookupEnv(WatchNamespaceEnvVar)
Expand Down Expand Up @@ -149,3 +154,16 @@ func GenerateNameForNonResilientCephBlockPoolSC(initData *ocsv1.StorageCluster)
}
return fmt.Sprintf("%s-ceph-non-resilient-rbd", initData.Name)
}

func MapCRDAvailability(ctx context.Context, clnt client.Client, crdNames ...string) (map[string]bool, error) {
crdExist := map[string]bool{}
for _, crdName := range crdNames {
crd := &apiextensionsv1.CustomResourceDefinition{}
crd.Name = crdName
if err := clnt.Get(ctx, client.ObjectKeyFromObject(crd), crd); client.IgnoreNotFound(err) != nil {
return nil, fmt.Errorf("error getting CRD, %v", err)
}
crdExist[crdName] = crd.UID != ""
}
return crdExist, nil
}
28 changes: 17 additions & 11 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,22 @@ func main() {
setupLog.Error(err, "Unable to get OperatorCondition")
os.Exit(1)
}
// apiclient.New() returns a client without cache.
// cache is not initialized before mgr.Start()
// we need this because we need to interact with OperatorCondition
apiClient, err := apiclient.New(mgr.GetConfig(), apiclient.Options{
Scheme: mgr.GetScheme(),
})
if err != nil {
setupLog.Error(err, "Unable to get Client")
os.Exit(1)
}

availCrds, err := util.MapCRDAvailability(context.Background(), apiClient, util.GetCrds()...)
if err != nil {
setupLog.Error(err, "Unable to get CRD")
os.Exit(1)
}

if err = (&ocsinitialization.OCSInitializationReconciler{
Client: mgr.GetClient(),
Expand All @@ -184,6 +200,7 @@ func main() {
Scheme: mgr.GetScheme(),
OperatorNamespace: operatorNamespace,
OperatorCondition: condition,
AvailableCrds: availCrds,
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "StorageCluster")
os.Exit(1)
Expand Down Expand Up @@ -241,17 +258,6 @@ func main() {
os.Exit(1)
}

// apiclient.New() returns a client without cache.
// cache is not initialized before mgr.Start()
// we need this because we need to interact with OperatorCondition
apiClient, err := apiclient.New(mgr.GetConfig(), apiclient.Options{
Scheme: mgr.GetScheme(),
})
if err != nil {
setupLog.Error(err, "Unable to get Client")
os.Exit(1)
}

// Set OperatorCondition Upgradeable to True
// We have to at least default the condition to True or
// OLM will use the Readiness condition via our readiness probe instead:
Expand Down