components: move params.env image updating to Init stage #1191

ykaliuta · 2024-08-19T11:59:08Z

Jira: https://issues.redhat.com/browse/RHOAIENG-11592

Image names in environment are not supposed to be changed during
runtime of the operator, so it makes sense to update them only on
startup.

If manifests are overriden by DevFlags, the DevFlags' version will
be used.

The change is straight forward for most of the components where only
images are updated and params.env is located in the kustomize root
directory, but some components (dashboard, ray, codeflare,
modelregistry) also update some extra parameters. For them image
part only is moved to Init since other updates require runtime DSCI
information.

The patch also changes logic for ray, codeflare, and modelregistry
in this regard to update non-image parameters regardless of DevFlags
like it was changed in dashboard recently.

The DevFlags functionality brings some concerns:

For most components the code is written such a way that as soon as
DevFlags supplied the global path variables are changed and never
reverted back to the defaults. For some (dashboard, trustyai) there
is (still global) OverridePath/entryPath pair and manifests reverted
to the default, BUT there is no logic of transition.
codeflare: when manifests are overridden namespace param is
updated in the hardcoded (stock) path;

This logic is preserved.

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

You have read the contributors guide.
Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has manually tested the changes and verified that the changes work

openshift-ci · 2024-08-19T11:59:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from ykaliuta. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

zdtsw · 2024-08-27T15:46:41Z

i am fine with this change, but would like to confirm one requirement first.

ykaliuta · 2024-08-29T09:10:17Z

@bartoszmajsak may be you want to be aware of the proposal

zdtsw · 2024-09-09T09:58:26Z

main.go

@@ -311,6 +328,10 @@ func main() { //nolint:funlen,maintidx
 		setupLog.Error(err, "unable to set up ready check")
 		os.Exit(1)
 	}
+	if err := initComponents(platform); err != nil {
+		setupLog.Error(err, "unable to init components")
+		os.Exit(1)


should here be os.Exit(1)?
say any of the downstream component fails to set image, will cause operator crash?

Good question. One one hand it makes problems obvious on startup. On it would make some of the system working even if dashboard tries to use wrong repo.

what i am afarid is
if compoent X fail to update, but user does not even want to enable component X but only Y and Z, should that make pod crash ?
in thoery , it should not even fail,
and in another theory if it was a glitch: the restart of pod , due to crash, can fix X in the next run.

If fatal init is not needed, implicit init() will work. UPD: missed platform parameter

For non-fatal case I would like at least to log the error. Would it be ok to pass Context and use Bartosz' idea to wrap logger in it?

that sounds better, we will need to show the error, only part is i dont know if we should make it fatal.
@lburgazzoli / @VaishnaviHire how you feel about this?

I made it non-fatal, but on the per-Init level. Leaving possibility :)

zdtsw · 2024-09-09T09:59:10Z

components/component.go

@@ -39,6 +39,10 @@ type Component struct {
 	DevFlags *DevFlags `json:"devFlags,omitempty"`
 }

+func (c *Component) Init(_ cluster.Platform) error {


maybe rename to InitImage() if it is only for image not other extra params.

I did hope it can be useful for other initializations as well. (but actually could no find any independent of reconcile loop)

zdtsw · 2024-09-09T10:07:39Z

components/codeflare/codeflare.go

+		"codeflare-operator-controller-image": "RELATED_IMAGE_ODH_CODEFLARE_OPERATOR_IMAGE", // no need mcad, embedded in cfo
+	}
+
+	if err := deploy.ApplyParams(ParamsPath, imageParamMap); err != nil {


can we also update docs on https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/pkg/deploy/envParams.go#L67
just to be clear that it only update if it does not match otherwise it wont do anything

You mean 450e19f ("deploy: ApplyParams: do not write params.env if there are no updates (#1186)") functionality?

yes, by reading the comments, i do not feel it reflects what the fuction is doing with recent chagnes

It's anyway unrelated to this patchset, can come as fixup for the aforementioned patch.

zdtsw · 2024-09-09T10:19:55Z

components/codeflare/codeflare.go

-			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)
-			}
+		if err := deploy.ApplyParams(ParamsPath, map[string]string{}, map[string]string{"namespace": dscispec.ApplicationsNamespace}); err != nil {


Suggested change

if err := deploy.ApplyParams(ParamsPath, map[string]string{}, map[string]string{"namespace": dscispec.ApplicationsNamespace}); err != nil {

if err := deploy.ApplyParams(ParamsPath, nil, map[string]string{"namespace": dscispec.ApplicationsNamespace}); err != nil {

do not even need an empty map ? same for the ray one?

yeah, leftover. (It's a bit counter intuitive that access nil map does not crash)

zdtsw · 2024-09-11T10:27:39Z

FYI @csams

ykaliuta · 2024-09-12T13:28:19Z

/retest-required

ykaliuta · 2024-09-12T14:53:45Z

/retest required

openshift-ci · 2024-09-12T14:53:50Z

@ykaliuta: The /retest command does not accept any targets.
The following commands are available to trigger required jobs:

/test ci-index
/test images
/test opendatahub-operator-e2e
/test opendatahub-operator-pr-image-mirror

Use /test all to run all jobs.

In response to this:

/retest required

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

ykaliuta · 2024-09-12T14:55:33Z

/retest-required

ykaliuta · 2024-09-12T16:27:39Z

/retest-required

ykaliuta · 2024-09-13T03:53:48Z

/retest-required

ykaliuta · 2024-09-13T08:50:13Z

/retest-required

ykaliuta · 2024-09-13T12:08:46Z