Fix: Ensure odh-model-controller Deployment Waits for ConfigMap #361

trujillm · 2025-02-03T16:08:43Z

Fix: Ensure `odh-model-controller` Deployment Waits for ConfigMap & Adjust NIM_STATE Handling

Issue

The Kustomization for odh-model-controller creates both the Deployment and the ConfigMap.
However, the Deployment is set with optional: true for the ConfigMap, meaning it can start before the ConfigMap is available.
If the Deployment starts first (as seen in QE), it runs without the NIM_STATE environment variable, causing the backend to ignore Accounts.

Fix 1: Ensure ConfigMap Availability

Set optional: false for the ConfigMap in the Deployment.
This ensures the Deployment waits for the ConfigMap before starting.
Since both resources are managed by the same Kustomization and the ConfigMap has a default value, this change is safe.

Fix 2: Adjust `NIM_STATE` Handling

Stop checking for an empty string when validating NIM_STATE.

Instead of:

if !slices.Contains([]string{"removed", ""}, nimState) {

Change to:

if nimState != "removed" {

This ensures that if NIM_STATE is not specified, it defaults to enabled instead of off.

Impact

Ensures consistent deployment behavior.
Prevents the backend from ignoring Accounts due to missing NIM_STATE.
Enables NIM by default when not explicitly set.
The commits are squashed in a cohesive manner and have meaningful messages.
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

Signed-off-by: mtrujillo <[email protected]>

openshift-ci · 2025-02-03T16:08:55Z

Hi @trujillm. Thanks for your PR.

I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Jooho · 2025-02-03T16:24:43Z

@trujillm I know this is draft but could you please add unit ?

trujillm · 2025-02-03T21:01:24Z

@trujillm I know this is draft but could you please add unit ?

Can you clarify what you mean @Jooho ?

spolti · 2025-02-04T15:16:32Z

config/manager/manager.yaml

@@ -105,7 +105,6 @@ spec:
              configMapKeyRef:
                name: odh-model-controller-parameters
                key: nim-state
-                optional: true


shouldn't we keep it as optional?
I mean, the os.GetEnv is ok if the env is not set( with the suggested change), however, if for some reason it is not set in during the startup, it would prevent odh to start.

@spolti I believe the intent is to block the deployment until the configmap is ready which I believe is the expected behavior for having the optional value defaulted to false

@spolti

shouldn't we keep it as optional?
I mean, the os.GetEnv is ok if the env is not set( with the suggested change)

We have two different default behaviours, for IBM the default is "removed", and for everyone else, it's "managed". So we need the ConfigMap to be configured properly by the operator-controller and we cannot decide a default value without the ConfigMap.

however, if for some reason it is not set in during the startup, it would prevent odh to start.

Is this feasible? Both the Deployment and the ConfigMap are created by the same kustomization file. The ConfigMap is used and probably required by other parts too.

for IBM the default is "removed",

How is this achieved?

@TomerFi I understand why you choose removed for the condition. I am ok with it.
However, I have a question about the IBM case. How can you set the default value removed?
The configmap manifests will be reside in RHOAI operator so it can not be editable after release.

I imagin the pipelins running for IBM set the DataScienceCluster.spec.kserve.nim.managedState to "removed", this gets translated into the params.env before executing the kustomization file that creates both the ConfigMap and the Deployment.

if Opendatahub operator have the logic, it makes sense then.

Reading back my comment, I think I was unclear. When I wrote: We have two different default behaviours, of course, we don't actually have two defaults, sorry about that. My point was, we can't decide what default value to use, hence we must rely on the ConfigMap that always has the correct value for us.

cmd/main.go

Signed-off-by: mtrujillo <[email protected]>

trujillm · 2025-02-04T17:52:26Z

@Jooho @spolti Updates have been made please review at your earliest convenience.

Thanks

trujillm · 2025-02-04T18:20:29Z

/retest

openshift-ci · 2025-02-04T18:20:46Z

@trujillm: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

/retest

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.

Jooho

not related to this PR but I have a question about IBM case

Jooho · 2025-02-04T18:58:44Z

config/manager/manager.yaml

@@ -105,7 +105,6 @@ spec:
              configMapKeyRef:
                name: odh-model-controller-parameters
                key: nim-state
-                optional: true


@TomerFi I understand why you choose removed for the condition. I am ok with it.
However, I have a question about the IBM case. How can you set the default value removed?
The configmap manifests will be reside in RHOAI operator so it can not be editable after release.

Jooho · 2025-02-04T19:18:57Z

/rerun-all

israel-hdez · 2025-02-04T19:19:27Z

/ok-to-test

israel-hdez · 2025-02-04T19:53:07Z

/retest

Jooho · 2025-02-04T20:00:59Z

/approve
/lgtm

openshift-ci · 2025-02-04T20:01:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jooho, trujillm

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Jooho]

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

…datahub-io#361) * initial commit to fix NIM and deployment issues Signed-off-by: mtrujillo <[email protected]> * update to set default on nimState Signed-off-by: mtrujillo <[email protected]> * updated optinal to false Signed-off-by: mtrujillo <[email protected]> --------- Signed-off-by: mtrujillo <[email protected]> (cherry picked from commit 7806732)

…datahub-io#361) * initial commit to fix NIM and deployment issues Signed-off-by: mtrujillo <[email protected]> * update to set default on nimState Signed-off-by: mtrujillo <[email protected]> * updated optinal to false Signed-off-by: mtrujillo <[email protected]> --------- Signed-off-by: mtrujillo <[email protected]>

…datahub-io#361) Signed-off-by: mtrujillo <[email protected]>

…datahub-io#361) * initial commit to fix NIM and deployment issues Signed-off-by: Marcus Trujillo <[email protected]> * update to set default on nimState Signed-off-by: Marcus Trujillo <[email protected]> * updated optinal to false Signed-off-by: Marcus Trujillo <[email protected]> --------- Signed-off-by: Marcus Trujillo <[email protected]> (cherry picked from commit 7806732)

* fix: added accept header to nim manifest pull requests accepting oci (#362) * fix: added accept header to nim manifest pull requests accepting oci Signed-off-by: Tomer Figenblat <[email protected]> * chore: removed left-over debugging message Signed-off-by: Tomer Figenblat <[email protected]> --------- Signed-off-by: Tomer Figenblat <[email protected]> (cherry picked from commit f7fdf0a) * Fix: Ensure odh-model-controller Deployment Waits for ConfigMap (#361) * initial commit to fix NIM and deployment issues Signed-off-by: Marcus Trujillo <[email protected]> * update to set default on nimState Signed-off-by: Marcus Trujillo <[email protected]> * updated optinal to false Signed-off-by: Marcus Trujillo <[email protected]> --------- Signed-off-by: Marcus Trujillo <[email protected]> (cherry picked from commit 7806732) * fix: nim containers terminated prematurely (#363) Signed-off-by: Tomer Figenblat <[email protected]> Co-authored-by: Mikhail Mikhailitchenko <[email protected]> (cherry picked from commit c92b19f) * chore: added gocyclo no linting comment on the main function Signed-off-by: Tomer Figenblat <[email protected]> --------- Signed-off-by: Tomer Figenblat <[email protected]> Co-authored-by: Marcus Trujillo <[email protected]>

initial commit to fix NIM and deployment issues

bc452e7

Signed-off-by: mtrujillo <[email protected]>

openshift-ci bot added the needs-ok-to-test label Feb 3, 2025

trujillm marked this pull request as draft February 3, 2025 16:08

openshift-ci bot added the do-not-merge/work-in-progress label Feb 3, 2025

trujillm changed the title ~~initial commit to fix NIM and deployment issues~~ Fix: Ensure odh-model-controller Deployment Waits for ConfigMap Feb 3, 2025

lazarotti requested review from Jooho and israel-hdez February 3, 2025 17:04

trujillm marked this pull request as ready for review February 4, 2025 14:30

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 4, 2025

spolti reviewed Feb 4, 2025

View reviewed changes

cmd/main.go Show resolved Hide resolved

trujillm added 2 commits February 4, 2025 11:44

update to set default on nimState

2d2fc18

Signed-off-by: mtrujillo <[email protected]>

updated optinal to false

def7e1a

Signed-off-by: mtrujillo <[email protected]>

Jooho reviewed Feb 4, 2025

View reviewed changes

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Feb 4, 2025

openshift-ci bot assigned Jooho Feb 4, 2025

openshift-ci bot added the lgtm label Feb 4, 2025

openshift-ci bot added the approved label Feb 4, 2025

openshift-merge-bot bot merged commit 7806732 into opendatahub-io:incubating Feb 4, 2025
7 checks passed

TomerFi mentioned this pull request Feb 6, 2025

fix: cherry picking appeng fixes for 2.18 #367

Merged

3 tasks

TomerFi pushed a commit to TomerFi/odh-model-controller that referenced this pull request Feb 6, 2025

fix: ensure odh-model-controller Deployment Waits for ConfigMap (open…

c0bf928

…datahub-io#361) Signed-off-by: mtrujillo <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Ensure odh-model-controller Deployment Waits for ConfigMap #361

Fix: Ensure odh-model-controller Deployment Waits for ConfigMap #361

trujillm commented Feb 3, 2025

openshift-ci bot commented Feb 3, 2025

Jooho commented Feb 3, 2025

trujillm commented Feb 3, 2025

spolti Feb 4, 2025

trujillm Feb 4, 2025

TomerFi Feb 4, 2025

spolti Feb 4, 2025

Jooho Feb 4, 2025

TomerFi Feb 4, 2025 •

edited

Loading

Jooho Feb 4, 2025 •

edited

Loading

TomerFi Feb 4, 2025

trujillm commented Feb 4, 2025

trujillm commented Feb 4, 2025

openshift-ci bot commented Feb 4, 2025

Jooho left a comment

Jooho Feb 4, 2025

Jooho commented Feb 4, 2025

israel-hdez commented Feb 4, 2025

israel-hdez commented Feb 4, 2025

Jooho commented Feb 4, 2025

openshift-ci bot commented Feb 4, 2025

Fix: Ensure odh-model-controller Deployment Waits for ConfigMap #361

Fix: Ensure odh-model-controller Deployment Waits for ConfigMap #361

Conversation

trujillm commented Feb 3, 2025

Fix: Ensure odh-model-controller Deployment Waits for ConfigMap & Adjust NIM_STATE Handling

Issue

Fix 1: Ensure ConfigMap Availability

Fix 2: Adjust NIM_STATE Handling

Impact

openshift-ci bot commented Feb 3, 2025

Jooho commented Feb 3, 2025

trujillm commented Feb 3, 2025

spolti Feb 4, 2025

Choose a reason for hiding this comment

trujillm Feb 4, 2025

Choose a reason for hiding this comment

TomerFi Feb 4, 2025

Choose a reason for hiding this comment

spolti Feb 4, 2025

Choose a reason for hiding this comment

Jooho Feb 4, 2025

Choose a reason for hiding this comment

TomerFi Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Jooho Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

TomerFi Feb 4, 2025

Choose a reason for hiding this comment

trujillm commented Feb 4, 2025

trujillm commented Feb 4, 2025

openshift-ci bot commented Feb 4, 2025

Jooho left a comment

Choose a reason for hiding this comment

Jooho Feb 4, 2025

Choose a reason for hiding this comment

Jooho commented Feb 4, 2025

israel-hdez commented Feb 4, 2025

israel-hdez commented Feb 4, 2025

Jooho commented Feb 4, 2025

openshift-ci bot commented Feb 4, 2025

Fix: Ensure `odh-model-controller` Deployment Waits for ConfigMap & Adjust NIM_STATE Handling

Fix 2: Adjust `NIM_STATE` Handling

TomerFi Feb 4, 2025 •

edited

Loading

Jooho Feb 4, 2025 •

edited

Loading