feat: add check on 2 MultiKueue CRD if v1alpha1 version in the cluster and not in termination, error out #1616

zdtsw · 2025-02-05T11:22:36Z

DSC condition on kueueReady:
reason: MultiKueueCRDV1Alpah1Exist
message: MultiKueue CRDs MultiKueueConfig v1alpha1 and MultiKueueCluster v1alpah1 exist, please remove them before proceed
reconcile gets triggered by user's deletion on these 2 CRD

Description

ref: https://issues.redhat.com/browse/RHOAIENG-18251

How Has This Been Tested?

local build: quay.io/wenzhou/opendatahub-operator-catalog:v2.23.205

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

bundle/manifests/opendatahub-operator.clusterserviceversion.yaml

apis/components/v1alpha1/kserve_types.go

lburgazzoli · 2025-02-05T11:33:52Z

controllers/components/kueue/kueue_controller.go

+				// For the case when user manually delete old MultiKueueConfig/MultiKueueCluster CRDs v1alpha1
+				predicate.Funcs{
+					DeleteFunc: func(e event.DeleteEvent) bool {
+						return e.Object.GetName() == gvk.MultiKueueConfigV1Alpha1.Kind || e.Object.GetName() == gvk.MultikueueClusterV1Alpha1.Kind


this should probably already covered by the component.ForLabel predicate (https://github.com/opendatahub-io/opendatahub-operator/blob/main/pkg/controller/predicates/component/component.go#L15-L17), unless the crd were not deployed by the operator.

right, if the CRD has app.opendatahub.io/kueue: 'true'

i dont know if we should cover it or not, for the case, kueue team can run "make deploy" to get CRD into cluster but these wont have labels/annotations.
maybe we can remove DeleteFuncs here

I'd be strict here and only support what the operator expects, if you do something outside of the operator, I guess there's not so much we can do/support

pkg/cluster/operator.go

…r, error out - DSC condition on kueueReady: reason: MultiKueueCRDV1Alpah1Exist message: MultiKueue CRDs MultiKueueConfig v1alpha1 and MultiKueueCluster v1alpah1 exist, please remove them before proceed - reconcile gets triggered by user's deletion on these 2 CRD Signed-off-by: Wen Zhou <[email protected]>

ChristianZaccaria

Thanks @zdtsw , I've tested this and works as expected:

Old version of Kueue deployed with v1alpha1 CRDs.
Deleted deployment, and checked that the v1alpha1 CRDs remained.
Installed this version of opendatahub-operator.
Kueue pods were blocked from creation, and found the message in the DSC resource.
After deletion of both v1alpha1 CRDs, the Kueue pod starts and installs newer v1beta1 CRDs.

controllers/status/status.go

- rename function to postive - remove prediate on non-operator created CRD Signed-off-by: Wen Zhou <[email protected]>

Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli · 2025-02-05T13:14:09Z

@zdtsw it looks good to me, we'd need an e2e test for this I guess

ChristianZaccaria

Tested a new clean installation of opendatahub-operator when cluster has none of the 2 CRDs. Kueue pods came up as expected and installed the new CRDs.

zdtsw · 2025-02-05T13:21:49Z

@zdtsw it looks good to me, we'd need an e2e test for this I guess

i will add one on that

codecov · 2025-02-05T14:24:49Z

Codecov Report

Attention: Patch coverage is 0% with 41 lines in your changes missing coverage. Please review.

Project coverage is 20.28%. Comparing base (cc7bf04) to head (c9054ac).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...llers/components/kueue/kueue_controller_actions.go	0.00%	23 Missing ⚠️
pkg/cluster/operator.go	0.00%	17 Missing ⚠️
controllers/components/kueue/kueue_controller.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1616      +/-   ##
==========================================
- Coverage   20.35%   20.28%   -0.08%     
==========================================
  Files         163      163              
  Lines       11096    11137      +41     
==========================================
  Hits         2259     2259              
- Misses       8597     8638      +41     
  Partials      240      240

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Wen Zhou <[email protected]>

tests/e2e/components_test.go

zdtsw · 2025-02-05T18:51:13Z

/test opendatahub-operator-e2e

lburgazzoli · 2025-02-06T07:52:23Z

I think we probably need a test to validate that the component goes into a non ready state if v1alpha1 is installed, and then gets back to ready when the CRD is deleted & reinstalled

Signed-off-by: Wen Zhou <[email protected]>

openshift-ci · 2025-02-06T11:05:31Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lburgazzoli

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

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

zdtsw · 2025-02-06T12:25:42Z

/test opendatahub-operator-e2e

zdtsw · 2025-02-06T17:37:00Z

/test image-build

openshift-ci · 2025-02-06T17:37:03Z

@zdtsw: The specified target(s) for /test were not found.
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:

/test image-build

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.

zdtsw · 2025-02-06T17:37:13Z

/test all

zdtsw · 2025-02-06T19:39:33Z

/test opendatahub-operator-e2e

…r and not in termination, error out (opendatahub-io#1616) * feat: add check on 2 MultiKueue CRD if v1alpha1 version in the cluster, error out - DSC condition on kueueReady: reason: MultiKueueCRDV1Alpah1Exist message: MultiKueue CRDs MultiKueueConfig v1alpha1 and MultiKueueCluster v1alpah1 exist, please remove them before proceed - reconcile gets triggered by user's deletion on these 2 CRD Signed-off-by: Wen Zhou <[email protected]> * update: - rename function to postive - remove prediate on non-operator created CRD Signed-off-by: Wen Zhou <[email protected]> * update: message Signed-off-by: Wen Zhou <[email protected]> * update: add test to check muttikueue api version Signed-off-by: Wen Zhou <[email protected]> * update: add e2e test to mimic CRD API version Signed-off-by: Wen Zhou <[email protected]> --------- Signed-off-by: Wen Zhou <[email protected]> (cherry picked from commit b1c2524)

openshift-ci bot requested review from AjayJagan and jackdelahunt February 5, 2025 11:22

zdtsw force-pushed the jira_18251 branch from 466f482 to 27b1834 Compare February 5, 2025 11:23

zdtsw requested review from lburgazzoli, VaishnaviHire and ChristianZaccaria and removed request for AjayJagan and jackdelahunt February 5, 2025 11:23

lburgazzoli reviewed Feb 5, 2025

View reviewed changes

bundle/manifests/opendatahub-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

lburgazzoli reviewed Feb 5, 2025

View reviewed changes

bundle/manifests/opendatahub-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

lburgazzoli reviewed Feb 5, 2025

View reviewed changes

apis/components/v1alpha1/kserve_types.go Outdated Show resolved Hide resolved

lburgazzoli reviewed Feb 5, 2025

View reviewed changes

pkg/cluster/operator.go Outdated Show resolved Hide resolved

ChristianZaccaria reviewed Feb 5, 2025

View reviewed changes

controllers/status/status.go Outdated Show resolved Hide resolved

update:

1f419bf

- rename function to postive - remove prediate on non-operator created CRD Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the jira_18251 branch from 27b1834 to 1f419bf Compare February 5, 2025 12:43

update: message

908742a

Signed-off-by: Wen Zhou <[email protected]>

ChristianZaccaria reviewed Feb 5, 2025

View reviewed changes

update: add test to check muttikueue api version

8f3bfe1

Signed-off-by: Wen Zhou <[email protected]>

lburgazzoli reviewed Feb 5, 2025

View reviewed changes

tests/e2e/components_test.go Show resolved Hide resolved

update: add e2e test to mimic CRD API version

c9054ac

Signed-off-by: Wen Zhou <[email protected]>

zdtsw force-pushed the jira_18251 branch from 36766db to c9054ac Compare February 6, 2025 10:34

lburgazzoli approved these changes Feb 6, 2025

View reviewed changes

openshift-ci bot assigned lburgazzoli Feb 6, 2025

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

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

openshift-merge-bot bot merged commit b1c2524 into opendatahub-io:main Feb 6, 2025
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add check on 2 MultiKueue CRD if v1alpha1 version in the cluster and not in termination, error out #1616

feat: add check on 2 MultiKueue CRD if v1alpha1 version in the cluster and not in termination, error out #1616

zdtsw commented Feb 5, 2025

lburgazzoli Feb 5, 2025

zdtsw Feb 5, 2025

lburgazzoli Feb 5, 2025

ChristianZaccaria left a comment

lburgazzoli commented Feb 5, 2025

ChristianZaccaria left a comment

zdtsw commented Feb 5, 2025

codecov bot commented Feb 5, 2025 •

edited

Loading

zdtsw commented Feb 5, 2025

lburgazzoli commented Feb 6, 2025

openshift-ci bot commented Feb 6, 2025

zdtsw commented Feb 6, 2025

zdtsw commented Feb 6, 2025

openshift-ci bot commented Feb 6, 2025

zdtsw commented Feb 6, 2025

zdtsw commented Feb 6, 2025

feat: add check on 2 MultiKueue CRD if v1alpha1 version in the cluster and not in termination, error out #1616

feat: add check on 2 MultiKueue CRD if v1alpha1 version in the cluster and not in termination, error out #1616

Conversation

zdtsw commented Feb 5, 2025

Description

How Has This Been Tested?

Screenshot or short clip

Merge criteria

lburgazzoli Feb 5, 2025

Choose a reason for hiding this comment

zdtsw Feb 5, 2025

Choose a reason for hiding this comment

lburgazzoli Feb 5, 2025

Choose a reason for hiding this comment

ChristianZaccaria left a comment

Choose a reason for hiding this comment

lburgazzoli commented Feb 5, 2025

ChristianZaccaria left a comment

Choose a reason for hiding this comment

zdtsw commented Feb 5, 2025

codecov bot commented Feb 5, 2025 • edited Loading

Codecov Report

zdtsw commented Feb 5, 2025

lburgazzoli commented Feb 6, 2025

openshift-ci bot commented Feb 6, 2025

zdtsw commented Feb 6, 2025

zdtsw commented Feb 6, 2025

openshift-ci bot commented Feb 6, 2025

zdtsw commented Feb 6, 2025

zdtsw commented Feb 6, 2025

codecov bot commented Feb 5, 2025 •

edited

Loading