feat: enable Trusty's Bias Feature Flag #1220

zdtsw · 2024-09-04T15:52:26Z

Description

https://issues.redhat.com/browse/RHOAIENG-11891

How Has This Been Tested?

local build: from odh quay.io/wenzhou/opendatahub-operator-catalog:v2.18.11891-0

for odh build: quay.io/wenzhou/opendatahub-operator-catalog:v2.18.11891-0

for rhoai: quay.io/wenzhou/rhoai-operator-catalog:v2.14.11891-0
tests on rhoai local build:
New install

clean installation of local build
get disableBiasMetrics: false

Upgrade

prepare:
install RHOAI 2.12 + enable dashbaord
check ODC CR, get disableBiasMetrics: true
uninstall operator but keep DSCI not deleted
check DSCI CR should have release.name: OpenShift AI Self-Managed and release.version: 2.12.0
test:
install rhoai local build , no need enable any component
after pod stable, check ODC CR, get disableBiasMetrics: false => expected, as force to false on from any old version

update check ODC CR, to disableBiasMetrics: true
restart pod, check ODC CR, keeps disableBiasMetrics: true => expected, as from a 2.14 to 2.14, should keep it as is
cleanup: uninstall everything
prepare:
install RHOAI 2.10 + enable dashbaord
ensure ODC CR has disableBiasMetrics: true
should be no release block in DSCI CR
uninstall operator but keep DSCI not deleted + ODC CR
test:
install rhoai local build , no need enable any component
after pod stable, check ODC CR, get disableBiasMetrics: false => expected, as force to false from any old version

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

etirelli · 2024-09-04T16:01:13Z

pkg/upgrade/upgrade.go

@@ -424,6 +428,11 @@ func unsetOwnerReference(ctx context.Context, cli client.Client, instanceName st
 			return fmt.Errorf("error unset ownerreference for CR %s : %w", instanceName, err)
 		}
 	}
+	// flip TrustyAI BiasMetrics to false (.spec.dashboardConfig.disableBiasMetrics)


I think you still need to gate this configuration change to only execute when upgrade from version < 2.14 to version >= 2.14. This will prevent the case where a user running 2.14 (or later version) manually disables this feature, to be overriden on their next upgrade.

i was thinking this will be a temp workaround which only lives in 2.14, then we can remove the call in 2.15.
from downstream skipRange: (different from ODH upgrade path which we allow on jump from 2.X to the latest release 2.17 for now)

2.8.x can one jump to 2.10

2.11 can only go from 2.10

2.12 can only go from 2.11

2.13 can only go from 2.12

In theory, 2.8.x will also be able to jump to ~2.17 when that is released (EUS channel).

In order to know which version the operator is upgrading from, can we use the DSCI status? https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/pkg/cluster/cluster_config.go#L133

- for upgrade case, if previously flag is true, after install new version should set to false Signed-off-by: Wen Zhou <wenzhou@redhat.com>

- introduce upgrade.GetReleaseFromCR() - create upgradeODCCR() for calling unsetOwnerReference and updateODCBiasMetrics - updateODSCBiasMetrics() only patch if previous Release is lower than 2.14, e.g 2.13.1, 2.8.3 Signed-off-by: Wen Zhou <wenzhou@redhat.com>

andrewballantyne

Data change looks good to me! Thanks for your help with this.

VaishnaviHire

/lgtm

openshift-ci · 2024-09-12T20:20:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, VaishnaviHire

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~~ [VaishnaviHire]

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

VaishnaviHire · 2024-09-12T20:21:44Z

/hold cancel

ykaliuta · 2024-09-12T23:46:13Z

Let's discuss naming a bit.
For my understanding that GetReleaseFrom* actually serve different purposes. Not just getting release (like the same) from different places how I read it from function names. What I see is

GetReleaseFromCR() is actually discovers release of already deployed cluster
GetReleaseFromCSV() look up currently running operator release (former GetRelease()). Getting it from CSV is just an implementation (I would prefer compile it in but we did not finish that discussion).

I would personally kept GetRelease() and introduced GetDeployedRelease() or something like that.

Cc: @lburgazzoli

zdtsw · 2024-09-13T05:08:49Z

Let's discuss naming a bit. For my understanding that GetReleaseFrom* actually serve different purposes. Not just getting release (like the same) from different places how I read it from function names. What I see is

GetReleaseFromCR() is actually discovers release of already deployed cluster GetReleaseFromCSV() look up currently running operator release (former GetRelease()). Getting it from CSV is just an implementation (I would prefer compile it in but we did not finish that discussion).

I would personally kept GetRelease() and introduced GetDeployedRelease() or something like that.

Cc: @lburgazzoli

i am ok if we want to discuss naming and refine code.
to get the main feature out, so we can start basic verification process is the main point here.

ykaliuta · 2024-09-13T08:46:34Z

No, I'm not about blocking or something. Renaming follow-up if we agree is a tiny thing :) I raised it here since this is the origin.

zdtsw · 2024-09-13T13:58:43Z

No, I'm not about blocking or something. Renaming follow-up if we agree is a tiny thing :) I raised it here since this is the origin.

FYI: #1232

* feat: enable Trusty's Bias Feature Flag - for upgrade case, if previously flag is true, after install new version should set to false Signed-off-by: Wen Zhou <wenzhou@redhat.com> * - rename cluster.GetRelease to cluster.GetReleaseFromCSV() - introduce upgrade.GetReleaseFromCR() - create upgradeODCCR() for calling unsetOwnerReference and updateODCBiasMetrics - updateODSCBiasMetrics() only patch if previous Release is lower than 2.14, e.g 2.13.1, 2.8.3 Signed-off-by: Wen Zhou <wenzhou@redhat.com> --------- Signed-off-by: Wen Zhou <wenzhou@redhat.com> (cherry picked from commit 562efd1)

openshift-ci bot requested review from ajaypratap003 and biswassri September 4, 2024 15:52

zdtsw force-pushed the jira/11891 branch from 0971b7c to 7fe4b62 Compare September 4, 2024 15:54

zdtsw removed request for ajaypratap003 and biswassri September 4, 2024 15:55

zdtsw added the do-not-merge/hold label Sep 4, 2024

etirelli reviewed Sep 4, 2024

View reviewed changes

zdtsw added 2 commits September 6, 2024 10:30

feat: enable Trusty's Bias Feature Flag

c463c10

- for upgrade case, if previously flag is true, after install new version should set to false Signed-off-by: Wen Zhou <wenzhou@redhat.com>

zdtsw force-pushed the jira/11891 branch from 0fb004a to 1eb8044 Compare September 6, 2024 16:19

zdtsw requested review from andrewballantyne and VaishnaviHire September 6, 2024 16:19

andrewballantyne approved these changes Sep 6, 2024

View reviewed changes

openshift-ci bot assigned andrewballantyne Sep 6, 2024

openshift-ci bot added the lgtm label Sep 6, 2024

zdtsw requested a review from etirelli September 12, 2024 10:59

VaishnaviHire approved these changes Sep 12, 2024

View reviewed changes

openshift-ci bot assigned VaishnaviHire Sep 12, 2024

openshift-ci bot added the approved label Sep 12, 2024

openshift-ci bot removed the do-not-merge/hold label Sep 12, 2024

openshift-merge-bot bot merged commit 562efd1 into opendatahub-io:incubation Sep 12, 2024
8 checks passed

zdtsw added the rhoai-2.14 label Sep 13, 2024

zdtsw mentioned this pull request Sep 16, 2024

[sync] feat: enable Trusty's Bias Feature Flag red-hat-data-services/rhods-operator#353

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: enable Trusty's Bias Feature Flag #1220

feat: enable Trusty's Bias Feature Flag #1220

zdtsw commented Sep 4, 2024 •

edited

Loading

etirelli Sep 4, 2024

zdtsw Sep 4, 2024

etirelli Sep 4, 2024

andrewballantyne left a comment

VaishnaviHire left a comment

openshift-ci bot commented Sep 12, 2024

VaishnaviHire commented Sep 12, 2024

ykaliuta commented Sep 12, 2024

zdtsw commented Sep 13, 2024

ykaliuta commented Sep 13, 2024

zdtsw commented Sep 13, 2024

feat: enable Trusty's Bias Feature Flag #1220

feat: enable Trusty's Bias Feature Flag #1220

Conversation

zdtsw commented Sep 4, 2024 • edited Loading

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

etirelli Sep 4, 2024

Choose a reason for hiding this comment

zdtsw Sep 4, 2024

Choose a reason for hiding this comment

etirelli Sep 4, 2024

Choose a reason for hiding this comment

andrewballantyne left a comment

Choose a reason for hiding this comment

VaishnaviHire left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Sep 12, 2024

VaishnaviHire commented Sep 12, 2024

ykaliuta commented Sep 12, 2024

zdtsw commented Sep 13, 2024

ykaliuta commented Sep 13, 2024

zdtsw commented Sep 13, 2024

zdtsw commented Sep 4, 2024 •

edited

Loading