From c9ccfd96289c455f75f3ef552a43a1ab9303e77a Mon Sep 17 00:00:00 2001 From: Vu Dinh Date: Thu, 29 Jul 2021 20:29:50 -0400 Subject: [PATCH] Add PriorityClass setting to registry pods for default CatalogSource The registry pods may need to have necessary priorityclass settings. OLM will set the priorityclass setting for registry pods by using the priorityclass annotations in the default catalogsources. Signed-off-by: Vu Dinh --- .../registry/reconciler/configmap_test.go | 3 +- pkg/controller/registry/reconciler/grpc.go | 40 +++++++++-- .../registry/reconciler/grpc_test.go | 68 +++++++++++++++++++ .../registry/reconciler/reconciler.go | 28 ++++++++ 4 files changed, 132 insertions(+), 7 deletions(-) diff --git a/pkg/controller/registry/reconciler/configmap_test.go b/pkg/controller/registry/reconciler/configmap_test.go index d3f10f1c3b..ced4b9115c 100644 --- a/pkg/controller/registry/reconciler/configmap_test.go +++ b/pkg/controller/registry/reconciler/configmap_test.go @@ -204,8 +204,9 @@ func objectsForCatalogSource(catsrc *v1alpha1.CatalogSource) []runtime.Object { if catsrc.Spec.Image != "" { decorated := grpcCatalogSourceDecorator{catsrc} objs = clientfake.AddSimpleGeneratedNames( - decorated.Pod(""), + decorated.Pod(catsrc.GetName()), decorated.Service(), + decorated.ServiceAccount(), ) } } diff --git a/pkg/controller/registry/reconciler/grpc.go b/pkg/controller/registry/reconciler/grpc.go index 8743ef5531..ce47963df1 100644 --- a/pkg/controller/registry/reconciler/grpc.go +++ b/pkg/controller/registry/reconciler/grpc.go @@ -171,15 +171,16 @@ func (c *GrpcRegistryReconciler) currentUpdatePods(source grpcCatalogSourceDecor return pods } -func (c *GrpcRegistryReconciler) currentPodsWithCorrectImage(source grpcCatalogSourceDecorator) []*corev1.Pod { +func (c *GrpcRegistryReconciler) currentPodsWithCorrectImageAndSpec(source grpcCatalogSourceDecorator, saName string) []*corev1.Pod { pods, err := c.Lister.CoreV1().PodLister().Pods(source.GetNamespace()).List(labels.SelectorFromValidatedSet(source.Labels())) if err != nil { logrus.WithError(err).Warn("couldn't find pod in cache") return nil } found := []*corev1.Pod{} + newPod := source.Pod(saName) for _, p := range pods { - if p.Spec.Containers[0].Image == source.Spec.Image { + if p.Spec.Containers[0].Image == source.Spec.Image && podHashMatch(p, newPod) { found = append(found, p) } } @@ -192,11 +193,12 @@ func (c *GrpcRegistryReconciler) EnsureRegistryServer(catalogSource *v1alpha1.Ca // if service status is nil, we force create every object to ensure they're created the first time overwrite := source.Status.RegistryServiceStatus == nil - // recreate the pod if no existing pod is serving the latest image - overwritePod := overwrite || len(c.currentPodsWithCorrectImage(source)) == 0 //TODO: if any of these error out, we should write a status back (possibly set RegistryServiceStatus to nil so they get recreated) sa, err := c.ensureSA(source) + // recreate the pod if no existing pod is serving the latest image or correct spec + overwritePod := overwrite || len(c.currentPodsWithCorrectImageAndSpec(source, sa.GetName())) == 0 + if err != nil && !k8serror.IsAlreadyExists(err) { return errors.Wrapf(err, "error ensuring service account: %s", source.GetName()) } @@ -421,10 +423,9 @@ func (c *GrpcRegistryReconciler) removePods(pods []*corev1.Pod, namespace string // CheckRegistryServer returns true if the given CatalogSource is considered healthy; false otherwise. func (c *GrpcRegistryReconciler) CheckRegistryServer(catalogSource *v1alpha1.CatalogSource) (healthy bool, err error) { source := grpcCatalogSourceDecorator{catalogSource} - // Check on registry resources // TODO: add gRPC health check - if len(c.currentPodsWithCorrectImage(source)) < 1 || + if len(c.currentPodsWithCorrectImageAndSpec(source, source.ServiceAccount().GetName())) < 1 || c.currentService(source) == nil { healthy = false return @@ -478,3 +479,30 @@ func (c *GrpcRegistryReconciler) podFailed(pod *corev1.Pod) (bool, error) { } return false, nil } + +// podHashMatch will check the hash info in existing pod to ensure its +// hash info matches the desired Service's hash. +func podHashMatch(existing, new *corev1.Pod) bool { + labels := existing.GetLabels() + newLabels := new.GetLabels() + // If both new & existing pods don't have labels, consider it not matched + if len(labels) == 0 || len(newLabels) == 0 { + return false + } + + existingPodSpecHash, ok := labels[PodHashLabelKey] + if !ok { + return false + } + + newPodSpecHash, ok := newLabels[PodHashLabelKey] + if !ok { + return false + } + + if existingPodSpecHash != newPodSpecHash { + return false + } + + return true +} diff --git a/pkg/controller/registry/reconciler/grpc_test.go b/pkg/controller/registry/reconciler/grpc_test.go index 5418832219..8827286211 100644 --- a/pkg/controller/registry/reconciler/grpc_test.go +++ b/pkg/controller/registry/reconciler/grpc_test.go @@ -339,6 +339,74 @@ func TestGrpcRegistryReconciler(t *testing.T) { } } +func TestRegistryPodPriorityClass(t *testing.T) { + now := func() metav1.Time { return metav1.Date(2018, time.January, 26, 20, 40, 0, 0, time.UTC) } + + type cluster struct { + k8sObjs []runtime.Object + } + type in struct { + cluster cluster + catsrc *v1alpha1.CatalogSource + } + tests := []struct { + testName string + in in + priorityclass string + }{ + { + testName: "Grpc/WithValidPriorityClassAnnotation", + in: in{ + catsrc: grpcCatalogSourceWithAnnotations(map[string]string{ + "operatorframework.io/priorityclass": "system-cluster-critical", + }), + }, + priorityclass: "system-cluster-critical", + }, + { + testName: "Grpc/WithInvalidPriorityClassAnnotation", + in: in{ + catsrc: grpcCatalogSourceWithAnnotations(map[string]string{ + "operatorframework.io/priorityclass": "", + }), + }, + priorityclass: "", + }, + { + testName: "Grpc/WithNoPriorityClassAnnotation", + in: in{ + catsrc: grpcCatalogSourceWithAnnotations(map[string]string{ + "annotationkey": "annotationvalue", + }), + }, + priorityclass: "", + }, + } + for _, tt := range tests { + t.Run(tt.testName, func(t *testing.T) { + stopc := make(chan struct{}) + defer close(stopc) + + factory, client := fakeReconcilerFactory(t, stopc, withNow(now), withK8sObjs(tt.in.cluster.k8sObjs...), withK8sClientOptions(clientfake.WithNameGeneration(t))) + rec := factory.ReconcilerForSource(tt.in.catsrc) + + err := rec.EnsureRegistryServer(tt.in.catsrc) + require.NoError(t, err) + + // Check for resource existence + decorated := grpcCatalogSourceDecorator{tt.in.catsrc} + pod := decorated.Pod(tt.in.catsrc.GetName()) + listOptions := metav1.ListOptions{LabelSelector: labels.SelectorFromSet(labels.Set{CatalogSourceLabelKey: tt.in.catsrc.GetName()}).String()} + outPods, podErr := client.KubernetesInterface().CoreV1().Pods(pod.GetNamespace()).List(context.TODO(), listOptions) + require.NoError(t, podErr) + require.Len(t, outPods.Items, 1) + outPod := outPods.Items[0] + require.Equal(t, tt.priorityclass, outPod.Spec.PriorityClassName) + require.Equal(t, pod.GetLabels()[PodHashLabelKey], outPod.GetLabels()[PodHashLabelKey]) + }) + } +} + func TestGrpcRegistryChecker(t *testing.T) { type cluster struct { k8sObjs []runtime.Object diff --git a/pkg/controller/registry/reconciler/reconciler.go b/pkg/controller/registry/reconciler/reconciler.go index ddfd7fb921..88315b5688 100644 --- a/pkg/controller/registry/reconciler/reconciler.go +++ b/pkg/controller/registry/reconciler/reconciler.go @@ -2,14 +2,18 @@ package reconciler import ( + "fmt" + "hash/fnv" "strings" v1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/rand" "github.com/operator-framework/api/pkg/operators/v1alpha1" controllerclient "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/controller-runtime/client" + hashutil "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/kubernetes/pkg/util/hash" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorclient" "github.com/operator-framework/operator-lifecycle-manager/pkg/lib/operatorlister" ) @@ -19,6 +23,10 @@ type nowFunc func() metav1.Time const ( // CatalogSourceLabelKey is the key for a label containing a CatalogSource name. CatalogSourceLabelKey string = "olm.catalogSource" + // CatalogPriorityClassKey is the key of an annotation in default catalogsources + CatalogPriorityClassKey string = "operatorframework.io/priorityclass" + // PodHashLabelKey is the key of a label for podspec hash information + PodHashLabelKey = "olm.pod-spec-hash" ) // RegistryEnsurer describes methods for ensuring a registry exists. @@ -160,5 +168,25 @@ func Pod(source *v1alpha1.CatalogSource, name string, image string, saName strin ServiceAccountName: saName, }, } + + // Set priorityclass if its annotation exists + if prio, ok := annotations[CatalogPriorityClassKey]; ok && prio != "" { + pod.Spec.PriorityClassName = prio + } + + // Add PodSpec hash + // This hash info will be used to detect PodSpec changes + if labels == nil { + labels = make(map[string]string) + } + labels[PodHashLabelKey] = hashPodSpec(pod.Spec) + pod.SetLabels(labels) return pod } + +// hashPodSpec calculates a hash given a copy of the pod spec +func hashPodSpec(spec v1.PodSpec) string { + hasher := fnv.New32a() + hashutil.DeepHashObject(hasher, &spec) + return rand.SafeEncodeString(fmt.Sprint(hasher.Sum32())) +}