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.

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

update: add check before calling ApplyParams #1181

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
12 changes: 9 additions & 3 deletions components/codeflare/codeflare.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package codeflare
import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -68,7 +69,6 @@ func (c *CodeFlare) ReconcileComponent(ctx context.Context,
var imageParamMap = map[string]string{
"codeflare-operator-controller-image": "RELATED_IMAGE_ODH_CODEFLARE_OPERATOR_IMAGE", // no need mcad, embedded in cfo
}

enabled := c.GetManagementState() == operatorv1.Managed
monitoringEnabled := dscispec.Monitoring.ManagementState == operatorv1.Managed

Expand All @@ -92,8 +92,14 @@ func (c *CodeFlare) ReconcileComponent(ctx context.Context,

// Update image parameters only when we do not have customized manifests set
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (c.DevFlags == nil || len(c.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(ParamsPath, imageParamMap, map[string]string{"namespace": dscispec.ApplicationsNamespace}); err != nil {
return fmt.Errorf("failed update image from %s : %w", CodeflarePath+"/bases", err)
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
if err := deploy.CheckParams(ParamsPath, []string{
os.Getenv("RELATED_IMAGE_ODH_CODEFLARE_OPERATOR_IMAGE"),
dscispec.ApplicationsNamespace,
}); err != nil {
if err := deploy.ApplyParams(ParamsPath, imageParamMap, map[string]string{"namespace": dscispec.ApplicationsNamespace}); err != nil {
return fmt.Errorf("failed to update image from %s: %w", ParamsPath, err)
}
}
}
}
Expand Down
15 changes: 12 additions & 3 deletions components/dashboard/dashboard.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"context"
"errors"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -121,9 +122,17 @@ func (d *Dashboard) ReconcileComponent(ctx context.Context,
return errors.New("failed to set variable for extraParamsMap")
}

// 4. update params.env regardless devFlags is provided of not
if err := deploy.ApplyParams(entryPath, imageParamMap, extraParamsMap); err != nil {
return fmt.Errorf("failed to update params.env from %s : %w", entryPath, err)
// 4. update params.env regardless devFlags is provided or not
// check is to avoid unnecessary disk written
var paramsMapValues []string
for _, envVariable := range extraParamsMap {
paramsMapValues = append(paramsMapValues, envVariable)
}
paramsMapValues = append(paramsMapValues, os.Getenv("RELATED_IMAGE_ODH_DASHBOARD_IMAGE"))
if err := deploy.CheckParams(entryPath, paramsMapValues); err != nil {
if err := deploy.ApplyParams(entryPath, imageParamMap, extraParamsMap); err != nil {
return fmt.Errorf("failed to update image and env variabls from %s: %w", entryPath, err)
}
}
}

Expand Down
12 changes: 10 additions & 2 deletions components/datasciencepipelines/datasciencepipelines.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package datasciencepipelines
import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -104,8 +105,15 @@ func (d *DataSciencePipelines) ReconcileComponent(ctx context.Context,
// skip check if the dependent operator has beeninstalled, this is done in dashboard
// Update image parameters only when we do not have customized manifests set
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (d.DevFlags == nil || len(d.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s : %w", Path, err)
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
var paramsMapValues []string
for _, image := range imageParamMap {
paramsMapValues = append(paramsMapValues, os.Getenv(image))
}
if err := deploy.CheckParams(Path, paramsMapValues); err != nil {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update images from %s: %w", Path, err)
}
}
}
// Check for existing Argo Workflows
Expand Down
8 changes: 6 additions & 2 deletions components/kserve/kserve.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package kserve
import (
"context"
"fmt"
"os"
"path/filepath"
"strings"

Expand Down Expand Up @@ -144,8 +145,11 @@ func (k *Kserve) ReconcileComponent(ctx context.Context, cli client.Client,
}
// Update image parameters for odh-model-controller
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (k.DevFlags == nil || len(k.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(DependentPath, dependentParamMap); err != nil {
return fmt.Errorf("failed to update image %s: %w", DependentPath, err)
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
if err := deploy.CheckParams(Path, []string{os.Getenv("RELATED_IMAGE_ODH_MODEL_CONTROLLER_IMAGE")}); err != nil {
if err := deploy.ApplyParams(DependentPath, dependentParamMap); err != nil {
return fmt.Errorf("failed to update image from %s: %w", DependentPath, err)
}
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions components/kueue/kueue.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ package kueue
import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -70,8 +71,11 @@ func (k *Kueue) ReconcileComponent(ctx context.Context, cli client.Client, logge
}
}
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (k.DevFlags == nil || len(k.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s : %w", Path, err)
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
if err := deploy.CheckParams(Path, []string{os.Getenv("RELATED_IMAGE_ODH_KUEUE_CONTROLLER_IMAGE")}); err != nil {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s : %w", Path, err)
}
}
}
}
Expand Down
20 changes: 15 additions & 5 deletions components/modelmeshserving/modelmeshserving.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package modelmeshserving
import (
"context"
"fmt"
"os"
"path/filepath"
"strings"

Expand Down Expand Up @@ -85,7 +86,6 @@ func (m *ModelMeshServing) ReconcileComponent(ctx context.Context,
"odh-modelmesh-runtime-adapter": "RELATED_IMAGE_ODH_MODELMESH_RUNTIME_ADAPTER_IMAGE",
"odh-modelmesh": "RELATED_IMAGE_ODH_MODELMESH_IMAGE",
"odh-modelmesh-controller": "RELATED_IMAGE_ODH_MODELMESH_CONTROLLER_IMAGE",
"odh-model-controller": "RELATED_IMAGE_ODH_MODEL_CONTROLLER_IMAGE",
}

// odh-model-controller to use
Expand Down Expand Up @@ -114,8 +114,15 @@ func (m *ModelMeshServing) ReconcileComponent(ctx context.Context,
}
// Update image parameters
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (m.DevFlags == nil || len(m.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed update image from %s : %w", Path, err)
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
var paramsMapValues []string
for _, image := range imageParamMap {
paramsMapValues = append(paramsMapValues, os.Getenv(image))
}
if err := deploy.CheckParams(Path, paramsMapValues); err != nil {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s: %w", Path, err)
}
}
}
}
Expand All @@ -132,8 +139,11 @@ func (m *ModelMeshServing) ReconcileComponent(ctx context.Context,
}
// Update image parameters for odh-model-controller
if dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "" {
if err := deploy.ApplyParams(DependentPath, dependentImageParamMap); err != nil {
return err
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
if err := deploy.CheckParams(Path, []string{os.Getenv("RELATED_IMAGE_ODH_MODEL_CONTROLLER_IMAGE")}); err != nil {
if err := deploy.ApplyParams(DependentPath, dependentImageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s: %w", dependentImageParamMap, err)
}
}
}
}
Expand Down
12 changes: 10 additions & 2 deletions components/modelregistry/modelregistry.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package modelregistry
import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -79,8 +80,15 @@ func (m *ModelRegistry) ReconcileComponent(ctx context.Context, cli client.Clien

// Update image parameters only when we do not have customized manifests set
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (m.DevFlags == nil || len(m.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s : %w", Path, err)
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
var paramsMapValues []string
for _, image := range imageParamMap {
paramsMapValues = append(paramsMapValues, os.Getenv(image))
}
if err := deploy.CheckParams(Path, paramsMapValues); err != nil {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s: %w", Path, err)
}
}
}

Expand Down
11 changes: 9 additions & 2 deletions components/ray/ray.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package ray
import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -74,8 +75,14 @@ func (r *Ray) ReconcileComponent(ctx context.Context, cli client.Client, logger
}
}
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (r.DevFlags == nil || len(r.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(RayPath, imageParamMap, map[string]string{"namespace": dscispec.ApplicationsNamespace}); err != nil {
return fmt.Errorf("failed to update image from %s : %w", RayPath, err)
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
if err := deploy.CheckParams(RayPath, []string{
os.Getenv("RELATED_IMAGE_ODH_KUBERAY_OPERATOR_CONTROLLER_IMAGE"),
dscispec.ApplicationsNamespace,
}); err != nil {
if err := deploy.ApplyParams(RayPath, imageParamMap, map[string]string{"namespace": dscispec.ApplicationsNamespace}); err != nil {
return fmt.Errorf("failed to update image from %s: %w", RayPath, err)
}
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions components/trainingoperator/trainingoperator.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package trainingoperator
import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -74,8 +75,11 @@ func (r *TrainingOperator) ReconcileComponent(ctx context.Context, cli client.Cl
}
}
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (r.DevFlags == nil || len(r.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(TrainingOperatorPath, imageParamMap); err != nil {
return err
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
if err := deploy.CheckParams(TrainingOperatorPath, []string{os.Getenv("RELATED_IMAGE_ODH_TRAINING_OPERATOR_IMAGE")}); err != nil {
if err := deploy.ApplyParams(TrainingOperatorPath, imageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s: %w", TrainingOperatorPath, err)
}
}
}
}
Expand Down
11 changes: 9 additions & 2 deletions components/trustyai/trustyai.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package trustyai
import (
"context"
"fmt"
"os"
"path/filepath"

"github.com/go-logr/logr"
Expand Down Expand Up @@ -73,8 +74,14 @@ func (t *TrustyAI) ReconcileComponent(ctx context.Context, cli client.Client, lo
}
}
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (t.DevFlags == nil || len(t.DevFlags.Manifests) == 0) {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update image %s: %w", Path, err)
// only update if passing image does not exist in params.env, to avoid unnecessary disk written
if err := deploy.CheckParams(Path, []string{
os.Getenv("RELATED_IMAGE_ODH_TRUSTYAI_SERVICE_IMAGE"),
os.Getenv("RELATED_IMAGE_ODH_TRUSTYAI_SERVICE_OPERATOR_IMAGE"),
}); err != nil {
if err := deploy.ApplyParams(Path, imageParamMap); err != nil {
return fmt.Errorf("failed to update image from %s: %w", Path, err)
}
}
}
}
Expand Down
18 changes: 12 additions & 6 deletions components/workbenches/workbenches.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package workbenches
import (
"context"
"fmt"
"os"
"path/filepath"
"strings"

Expand Down Expand Up @@ -140,13 +141,18 @@ func (w *Workbenches) ReconcileComponent(ctx context.Context, cli client.Client,
if enabled {
if (dscispec.DevFlags == nil || dscispec.DevFlags.ManifestsUri == "") && (w.DevFlags == nil || len(w.DevFlags.Manifests) == 0) {
if platform == cluster.ManagedRhods || platform == cluster.SelfManagedRhods {
// for kf-notebook-controller image
if err := deploy.ApplyParams(notebookControllerPath, imageParamMap); err != nil {
return fmt.Errorf("failed to update image %s: %w", notebookControllerPath, err)
// for odh-notebook-controller image: adding post-check to ensure params.env is updated
if err := deploy.CheckParams(notebookControllerPath, []string{os.Getenv("RELATED_IMAGE_ODH_NOTEBOOK_CONTROLLER_IMAGE")}); err != nil {
if err := deploy.ApplyParams(notebookControllerPath, imageParamMap); err != nil {
return fmt.Errorf("failed to update image %s: %w", notebookControllerPath, err)
}
}
// for odh-notebook-controller image
if err := deploy.ApplyParams(kfnotebookControllerPath, imageParamMap); err != nil {
return fmt.Errorf("failed to update image %s: %w", kfnotebookControllerPath, err)

// for kf-notebook-controller image: only update if passing image does not exist, to avoid unnecessary disk written
if err := deploy.CheckParams(kfnotebookControllerPath, []string{os.Getenv("RELATED_IMAGE_ODH_KF_NOTEBOOK_CONTROLLER_IMAGE")}); err != nil {
if err := deploy.ApplyParams(kfnotebookControllerPath, imageParamMap); err != nil {
return fmt.Errorf("failed to update image %s: %w", kfnotebookControllerPath, err)
}
}
}
}
Expand Down
77 changes: 77 additions & 0 deletions pkg/deploy/deploy_suite_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package deploy_test

import (
"os"
"path/filepath"
"testing"

"github.com/opendatahub-io/opendatahub-operator/v2/pkg/deploy"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
)

func TestStringSearch(t *testing.T) {
RegisterFailHandler(Fail)
RunSpecs(t, "Check Params Suite")
}

var _ = Describe("Throw error if image still use old image registry", func() {

var (
tmpFile *os.File
filePath = "/tmp/manifests/components/one"
err error
paramsenv string
)

// create a temp params.env file
BeforeEach(func() {
err = os.MkdirAll(filePath, 0755)
tmpFile, err = os.Create(filepath.Join(filePath, "params.env"))
Expect(err).NotTo(HaveOccurred())

// fake some lines
paramsenv = `url=https://www.odh.rbac.com
myimagee=quay.io/opendatahub/repo:abc
namespace=opendatahub`
_, err = tmpFile.WriteString(paramsenv)
Expect(err).NotTo(HaveOccurred())

err = tmpFile.Close()
Expect(err).NotTo(HaveOccurred())
})

AfterEach(func() {
err = os.Remove(tmpFile.Name())
Expect(err).NotTo(HaveOccurred())
})

Context("when searching in params.env", func() {
It("Should not throw error when string found", func() {
err := deploy.CheckParams(filePath, []string{"abc"})
Expect(err).NotTo(HaveOccurred())
})

It("Should not throw error when all string are matched", func() {
err := deploy.CheckParams(filePath, []string{"rbac", "abc"})
Expect(err).NotTo(HaveOccurred())
})

It("Should not throw error when emptry string provided(as env not found)", func() {
err := deploy.CheckParams(filePath, []string{""})
Expect(err).NotTo(HaveOccurred())
})

It("Should throw error when string not found", func() {
err := deploy.CheckParams(filePath, []string{"vw"})
Expect(err).To(HaveOccurred())
})

It("Should throw error when multiple strings are not found", func() {
err := deploy.CheckParams(filePath, []string{"vw", "123"})
Expect(err).To(HaveOccurred())
})
})

})
Loading
Loading