From abac782174c50d81e3696515ea30a0de5ae4f935 Mon Sep 17 00:00:00 2001 From: Dhiraj Bokde Date: Mon, 4 Nov 2024 17:21:47 -0800 Subject: [PATCH 1/2] fix: add check to remove old kube-rbac-proxy container in modelregistry operator, fixes RHOAIENG-15328 Signed-off-by: Dhiraj Bokde --- components/modelregistry/modelregistry.go | 39 +++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/components/modelregistry/modelregistry.go b/components/modelregistry/modelregistry.go index 242bea853f1..b9380a8aa38 100644 --- a/components/modelregistry/modelregistry.go +++ b/components/modelregistry/modelregistry.go @@ -11,8 +11,10 @@ import ( "text/template" operatorv1 "github.com/openshift/api/operator/v1" + appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -22,6 +24,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/conversion" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" _ "embed" ) @@ -159,6 +162,10 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien l.Info("apply extra manifests done") if enabled { + // remove leftover kube-rbac-proxy container from older ODH installs + if err := removeUnusedKubeRbacProxy(ctx, cli, m.GetComponentName(), dscispec.ApplicationsNamespace); err != nil { + return fmt.Errorf("failed to remove kube-rbac-proxy from deployment for %s in namespace %s: %w", ComponentName, dscispec.ApplicationsNamespace, err) + } if err := cluster.WaitForDeploymentAvailable(ctx, cli, m.GetComponentName(), dscispec.ApplicationsNamespace, 10, 1); err != nil { return fmt.Errorf("deployment for %s is not ready to server: %w", ComponentName, err) } @@ -180,6 +187,38 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien return nil } +// removeUnusedKubeRbacProxy removes older kube-rbac-proxy container in mr operator deployment. +// This is required because patching a deployment using apply-patch doesn't remove older containers. +// See: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch +func removeUnusedKubeRbacProxy(ctx context.Context, cli client.Client, name string, namespace string) error { + // get MR operator deployment + componentDeploymentList := &appsv1.DeploymentList{} + err := cli.List(ctx, componentDeploymentList, client.InNamespace(namespace), client.HasLabels{labels.ODH.Component(name)}) + if err != nil { + return fmt.Errorf("error fetching list of deployments: %w", err) + } + nItems := len(componentDeploymentList.Items) + if nItems != 1 { + return fmt.Errorf("error fetching model registry operator deployment, found %d deployments", nItems) + } + + // check if deployment has a kube-rbac-proxy container + deployment := componentDeploymentList.Items[0] + containers := deployment.Spec.Template.Spec.Containers + // there should be a single container in latest deployment without kube-rbac-proxy + if len(containers) == 1 { + return nil + } + for i, container := range containers { + if container.Name == "kube-rbac-proxy" { + // remove if found + return cli.Patch(ctx, &deployment, client.RawPatch(types.JSONPatchType, + []byte(fmt.Sprintf("[{\"op\": \"remove\", \"path\": \"/spec/template/spec/containers/%d\"}]", i)))) + } + } + return nil +} + func (m *ModelRegistry) createDependencies(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { // create DefaultModelRegistryCert if err := cluster.PropagateDefaultIngressCertificate(ctx, cli, DefaultModelRegistryCert, dscispec.ServiceMesh.ControlPlane.Namespace); err != nil { From d47ecfa76b2b51929893894541703f96d2a9fa5c Mon Sep 17 00:00:00 2001 From: Wen Zhou Date: Wed, 6 Nov 2024 11:47:53 +0100 Subject: [PATCH 2/2] update: remove check in ModelReg code, add it to upgrade logic Signed-off-by: Wen Zhou --- components/modelregistry/modelregistry.go | 39 ----------------------- pkg/upgrade/upgrade.go | 39 +++++++++++++++++++++++ 2 files changed, 39 insertions(+), 39 deletions(-) diff --git a/components/modelregistry/modelregistry.go b/components/modelregistry/modelregistry.go index b9380a8aa38..242bea853f1 100644 --- a/components/modelregistry/modelregistry.go +++ b/components/modelregistry/modelregistry.go @@ -11,10 +11,8 @@ import ( "text/template" operatorv1 "github.com/openshift/api/operator/v1" - appsv1 "k8s.io/api/apps/v1" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/types" "sigs.k8s.io/controller-runtime/pkg/client" logf "sigs.k8s.io/controller-runtime/pkg/log" @@ -24,7 +22,6 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/conversion" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy" - "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" _ "embed" ) @@ -162,10 +159,6 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien l.Info("apply extra manifests done") if enabled { - // remove leftover kube-rbac-proxy container from older ODH installs - if err := removeUnusedKubeRbacProxy(ctx, cli, m.GetComponentName(), dscispec.ApplicationsNamespace); err != nil { - return fmt.Errorf("failed to remove kube-rbac-proxy from deployment for %s in namespace %s: %w", ComponentName, dscispec.ApplicationsNamespace, err) - } if err := cluster.WaitForDeploymentAvailable(ctx, cli, m.GetComponentName(), dscispec.ApplicationsNamespace, 10, 1); err != nil { return fmt.Errorf("deployment for %s is not ready to server: %w", ComponentName, err) } @@ -187,38 +180,6 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien return nil } -// removeUnusedKubeRbacProxy removes older kube-rbac-proxy container in mr operator deployment. -// This is required because patching a deployment using apply-patch doesn't remove older containers. -// See: https://kubernetes.io/docs/tasks/manage-kubernetes-objects/update-api-object-kubectl-patch/#notes-on-the-strategic-merge-patch -func removeUnusedKubeRbacProxy(ctx context.Context, cli client.Client, name string, namespace string) error { - // get MR operator deployment - componentDeploymentList := &appsv1.DeploymentList{} - err := cli.List(ctx, componentDeploymentList, client.InNamespace(namespace), client.HasLabels{labels.ODH.Component(name)}) - if err != nil { - return fmt.Errorf("error fetching list of deployments: %w", err) - } - nItems := len(componentDeploymentList.Items) - if nItems != 1 { - return fmt.Errorf("error fetching model registry operator deployment, found %d deployments", nItems) - } - - // check if deployment has a kube-rbac-proxy container - deployment := componentDeploymentList.Items[0] - containers := deployment.Spec.Template.Spec.Containers - // there should be a single container in latest deployment without kube-rbac-proxy - if len(containers) == 1 { - return nil - } - for i, container := range containers { - if container.Name == "kube-rbac-proxy" { - // remove if found - return cli.Patch(ctx, &deployment, client.RawPatch(types.JSONPatchType, - []byte(fmt.Sprintf("[{\"op\": \"remove\", \"path\": \"/spec/template/spec/containers/%d\"}]", i)))) - } - } - return nil -} - func (m *ModelRegistry) createDependencies(ctx context.Context, cli client.Client, dscispec *dsciv1.DSCInitializationSpec) error { // create DefaultModelRegistryCert if err := cluster.PropagateDefaultIngressCertificate(ctx, cli, DefaultModelRegistryCert, dscispec.ServiceMesh.ControlPlane.Namespace); err != nil { diff --git a/pkg/upgrade/upgrade.go b/pkg/upgrade/upgrade.go index d0e9d44468d..e1dabb407a4 100644 --- a/pkg/upgrade/upgrade.go +++ b/pkg/upgrade/upgrade.go @@ -43,6 +43,7 @@ import ( "github.com/opendatahub-io/opendatahub-operator/v2/components/workbenches" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster" "github.com/opendatahub-io/opendatahub-operator/v2/pkg/cluster/gvk" + "github.com/opendatahub-io/opendatahub-operator/v2/pkg/metadata/labels" ) type ResourceSpec struct { @@ -275,6 +276,12 @@ func CleanupExistingResource(ctx context.Context, return err } } + // remove modelreg proxy container from deployment in ODH + if platform == cluster.OpenDataHub { + if err := removeRBACProxyModelRegistry(ctx, cli, "model-registry-operator", "kube-rbac-proxy", dscApplicationsNamespace); err != nil { + return err + } + } // to take a reference toDelete := getDashboardWatsonResources(dscApplicationsNamespace) @@ -487,6 +494,38 @@ func updateODCModelRegistry(ctx context.Context, cli client.Client, instanceName return nil } +// workaround for RHOAIENG-15328 +// TODO: this can be removed from ODH 2.22. +func removeRBACProxyModelRegistry(ctx context.Context, cli client.Client, componentName string, containerName string, applicationNS string) error { + log := logf.FromContext(ctx) + deploymentList := &appsv1.DeploymentList{} + if err := cli.List(ctx, deploymentList, client.InNamespace(applicationNS), client.HasLabels{labels.ODH.Component(componentName)}); err != nil { + return fmt.Errorf("error fetching list of deployments: %w", err) + } + + if len(deploymentList.Items) != 1 { // ModelRegistry operator is not deployed + return nil + } + mrDeployment := deploymentList.Items[0] + mrContainerList := mrDeployment.Spec.Template.Spec.Containers + // if only one container in deployment, we are already on newer deployment, no need more action + if len(mrContainerList) == 1 { + return nil + } + + log.Info("Upgrade force ModelRegistry to remove container from deployment") + for i, container := range mrContainerList { + if container.Name == containerName { + removeUnusedKubeRbacProxy := []byte(fmt.Sprintf("[{\"op\": \"remove\", \"path\": \"/spec/template/spec/containers/%d\"}]", i)) + if err := cli.Patch(ctx, &mrDeployment, client.RawPatch(types.JSONPatchType, removeUnusedKubeRbacProxy)); err != nil { + return fmt.Errorf("error removing ModelRegistry %s container from deployment: %w", containerName, err) + } + break + } + } + return nil +} + func RemoveLabel(ctx context.Context, cli client.Client, objectName string, labelKey string) error { foundNamespace := &corev1.Namespace{} if err := cli.Get(ctx, client.ObjectKey{Name: objectName}, foundNamespace); err != nil {