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

chore: remove generics from Reconcier and use PlatformObject as base object type #1514

Merged

Conversation

lburgazzoli
Copy link
Contributor

@lburgazzoli lburgazzoli commented Jan 17, 2025

Description

  • remove generics from Reconciler struct as it is not needed
  • set PlatformObject as the base obnject that the Reconciler handles to
    ensure the reconciler can access to common objects fields

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 openshift-ci bot requested review from Sara4994 and ykaliuta January 17, 2025 13:31
@lburgazzoli lburgazzoli requested review from VaishnaviHire and removed request for Sara4994 January 17, 2025 13:31
@lburgazzoli
Copy link
Contributor Author

/retest

2 similar comments
@lburgazzoli
Copy link
Contributor Author

/retest

@lburgazzoli
Copy link
Contributor Author

/retest

Copy link

codecov bot commented Jan 18, 2025

Codecov Report

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

Project coverage is 19.69%. Comparing base (3e897ce) to head (3a0dc16).
Report is 5 commits behind head on main.

Files with missing lines Patch % Lines
pkg/controller/reconciler/reconciler.go 0.00% 12 Missing ⚠️
pkg/controller/reconciler/reconciler_support.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1514   +/-   ##
=======================================
  Coverage   19.69%   19.69%           
=======================================
  Files         161      161           
  Lines       11102    11102           
=======================================
  Hits         2187     2187           
  Misses       8683     8683           
  Partials      232      232           

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

@lburgazzoli
Copy link
Contributor Author

Keeps failing because existing KNativeServing resource was found; integrating to an existing installation is not supported

@lburgazzoli
Copy link
Contributor Author

/test all

@openshift-ci openshift-ci bot removed the lgtm label Jan 18, 2025
@lburgazzoli lburgazzoli requested a review from ykaliuta January 18, 2025 19:46
@lburgazzoli
Copy link
Contributor Author

keep failing because of existing KNativeServing resource was found:

{"level":"info","ts":"2025-01-18T20:51:37Z","logger":"DataScienceCluster","msg":"DataScienceCluster Deployment Completed.","controller":"datasciencecluster","controllerGroup":"datasciencecluster.opendatahub.io","controllerKind":"DataScienceCluster","DataScienceCluster":{"name":"e2e-test-dsc"},"namespace":"","name":"e2e-test-dsc","reconcileID":"23eb585a-2fe2-400a-8684-4c180e9d1365"}
{"level":"info","ts":"2025-01-18T20:51:57Z","logger":"features","msg":"waiting for resource to be created","feature":"serverless-serving-gateways","namespace":"knative-serving","resource":"operator.knative.dev/v1beta1, Kind=KnativeServing"}
{"level":"info","ts":"2025-01-18T20:51:59Z","logger":"features","msg":"resource created","feature":"serverless-serving-gateways","namespace":"knative-serving","resource":"operator.knative.dev/v1beta1, Kind=KnativeServing"}
{"level":"error","ts":"2025-01-18T20:51:59Z","msg":"Failed to execute action","controller":"kserve","controllerGroup":"components.platform.opendatahub.io","controllerKind":"Kserve","Kserve":{"name":"default-kserve"},"namespace":"","name":"default-kserve","reconcileID":"8559c8fc-c24f-4ebe-bf82-0f9ffa039045","action":"github.com/opendatahub-io/opendatahub-operator/v2/controllers/components/kserve.configureServerless","error":"2 errors occurred:\n\t* failed applying FeatureHandler features. cause: 1 error occurred:\n\t* 1 error occurred:\n\t* existing KNativeServing resource was found; integrating to an existing installation is not supported\n\n\n\n\n\t* failed applying FeatureHandler features. cause: 1 error occurred:\n\t* 1 error occurred:\n\t* context deadline exceeded\n\n\n\n\n\n}
{"level":"info","ts":"2025-01-18T20:51:59Z","msg":"reconcile","controller":"kserve","controllerGroup":"components.platform.opendatahub.io","controllerKind":"Kserve","Kserve":{"name":"default-kserve"},"namespace":"","name":"default-kserve","reconcileID":"89fb270d-7ab2-49a2-92b8-b2dc2ac8f22d"}

Adding some log to figure out what's going on

@lburgazzoli
Copy link
Contributor Author

It may be caused by the order of testing, is modelcontroller tests are executed before, maybe the KNativeServing is left on the cluster. Adding a temporary cleanup logic also as part of the kserve test suite

@lburgazzoli lburgazzoli marked this pull request as draft January 20, 2025 06:43
@lburgazzoli lburgazzoli marked this pull request as ready for review January 20, 2025 06:59
@openshift-ci openshift-ci bot requested review from Sara4994 and zdtsw January 20, 2025 06:59
@lburgazzoli lburgazzoli marked this pull request as draft January 20, 2025 06:59
@lburgazzoli lburgazzoli marked this pull request as ready for review January 20, 2025 07:00
@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

@lburgazzoli
Copy link
Contributor Author

lburgazzoli commented Jan 20, 2025

It looks like that when the kserve tests are executed before modelcontroller, the e2e tests are running fine so i suspect there is something left by the modelcontroller tests. Unfortunately I've not been able to reproduce the issue locally.

That said, the work @grdryn is doing to get rid of FT may help to improve the e2e tests but in the meantime I'd propose to:

  1. revert e2e: add cleanup logic to kserve tests to ensure no KnativeServing is present in the cluster #1517
  2. move the same logic in the kserve e2e tests as done in 59227d1

@ykaliuta @zdtsw WDYT ?

@ykaliuta
Copy link
Contributor

ykaliuta commented Jan 20, 2025

@ykaliuta @zdtsw WDYT ?

Sounds reasonable for me

@lburgazzoli
Copy link
Contributor Author

@ykaliuta @zdtsw WDYT ?

Sounds reasonable for me

#1522

@lburgazzoli
Copy link
Contributor Author

Removed commits moved to #1522

@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

…object type

- remove generics from Reconciler struct as it is not needed
- set PlatformObject as the base obnject that the Reconciler handles to
  ensure the reconciler can access to common objects fields
@lburgazzoli lburgazzoli marked this pull request as ready for review January 20, 2025 11:31
@openshift-ci openshift-ci bot requested review from biswassri and lphiri January 20, 2025 11:32
@openshift-ci openshift-ci bot added the lgtm label Jan 20, 2025
Copy link

openshift-ci bot commented Jan 20, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ykaliuta

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:

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

@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

2 similar comments
@lburgazzoli
Copy link
Contributor Author

/test opendatahub-operator-e2e

@biswassri
Copy link
Contributor

/test opendatahub-operator-e2e

@lburgazzoli
Copy link
Contributor Author

/test all

@openshift-merge-bot openshift-merge-bot bot merged commit 94ac12c into opendatahub-io:main Jan 21, 2025
10 checks passed
@lburgazzoli lburgazzoli deleted the controller-generics branch January 21, 2025 08:39
zdtsw pushed a commit to zdtsw-forking/opendatahub-operator that referenced this pull request Feb 3, 2025
…object type (opendatahub-io#1514)

- remove generics from Reconciler struct as it is not needed
- set PlatformObject as the base obnject that the Reconciler handles to
  ensure the reconciler can access to common objects fields

(cherry picked from commit 94ac12c)
zdtsw added a commit that referenced this pull request Feb 4, 2025
* Fix typo in file name: pkg/utils/test/matchers/matechers.go -> pkg/utils/test/matchers/matchers.go (#1505)

(cherry picked from commit 68c6cae)

* e2e: improve testOwnedNamespacesAllExist by leveragin gomega's async assertions (#1509)

When the e2e test suite starts, it may take a while for some of the
owned namespace to become available and in such case the related test
would fail, since it does not retry. This commit replace the current
synchronous test with an asynchronous one.

(cherry picked from commit c35c6c1)

* Improve unit tests for jq's toType and byteToType (#1506)

- Refactor the toType test to handle more cases
- Add additional cases to toTypes test
- Add a specific test for byteToType

(cherry picked from commit e1457ab)

* e2e: add cleanup logic to kserve tests to ensure no KnativeServing is present in the cluster (#1517)

* e2e: add cleanup logic to kserve tests to ensure no KnativeServing is present in the cluster

* Fix findings

* lint

(cherry picked from commit 72443c5)

* Revert "e2e: add cleanup logic to kserve tests to ensure no KnativeServing is present in the cluster" (#1522)

* Revert "e2e: add cleanup logic to kserve tests to ensure no KnativeServing is…"

This reverts commit 72443c5.

* Fix after rebase

(cherry picked from commit 6a762ad)

* Add support for dynamic template data computation in template rendering action (#1527)

(cherry picked from commit bf3c108)

* chore: remove generics from Reconcier and use PlatformObject as base object type (#1514)

- remove generics from Reconciler struct as it is not needed
- set PlatformObject as the base obnject that the Reconciler handles to
  ensure the reconciler can access to common objects fields

(cherry picked from commit 94ac12c)

* chore: remove unused structs and functions from controlelrs/status package (#1535)

(cherry picked from commit 614fea1)

* chore: remove unused code (#1543)

* chore: remove unused TrimToRFC1123Name function

* chore: remove unused pkg/cluster/roles functions

* chore: remove unused pkg/cluster/resources functions

(cherry picked from commit 8c7fef2)

* chore: use constants from the status package where possible (#1541)

Co-authored-by: Wen Zhou <[email protected]>
(cherry picked from commit d538956)

* Improve Kserve's FeatureTraker handing (#1562)

* Improve Kserve's FeatureTraker handing

- Make it possible to set FT's OwnerReference as Controller reference so
  the kubernetes garbage collector can block owner deletion till the FT
  has been deleted
- Make it possible to set FT's generated resources OwnerReference as
  Controller reference so the kubernetes garbage collector can block
  FT deletion till the resources have been deleted
- Add an Kserver reconciler action to remove legacy ownership on
  DSCI/DSC is any and related e2e tests

* Update RBACs

* Update RBACs

* Fix findings

* Update pkg/cluster/meta.go

Co-authored-by: Gerard Ryan <[email protected]>

* Fix renaming

* Remove unrelated methods

---------

Co-authored-by: Gerard Ryan <[email protected]>
(cherry picked from commit bd00c9b)

---------

Co-authored-by: Luca Burgazzoli <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants