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.

Sign up for GitHub

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

Jump to bottom

Chore: Remove v1 fields and reconcile logic from dspo #712

Merged
merged 2 commits into from
Oct 24, 2024
Merged

Chore: Remove v1 fields and reconcile logic from dspo #712

merged 2 commits into from
Oct 24, 2024

Conversation

HumairAK
Copy link
Contributor

@HumairAK HumairAK commented Oct 1, 2024 •
edited
Loading

The issue resolved by this Pull Request:

Resolves #https://issues.redhat.com/browse/RHOAIENG-13572

Description of your changes:

This PR removes V1 from DSPO, more specifically it:

  • Removes all v1 only fields from DSPA CR
  • DSPA version field here, is assumed as "v2" by default when not provided (previously it was v1)
    • If user specifies v1, DSPO throws a reconcile log message and does not proceed with the reconcilliation for that
    • If user specifies v1, DSPA status reports a meaningful error with suggested action
  • DSPO continues to reconcile on the v1alpha1 resources (for both v1/v2), but when it encounters v1 version set in .spec.dspversion field in DSPA, dspo logs an error and dspa status error condition is thrown
  • DSPA CRD continues to serve at v1apha1, but has a deprecation set with a warning
  • DSPA uses a conversion strategy of None (default) 1. In this scenario when fetching the v1alpha1 resources, they are served with the unknown v1 fields as pruned.
  • Removes tekton related RBAC grants to pipeline runner SA, as well as the same rbac requirements on the DSPO
  • Removes any logic that will reconcile on v1 related fields
  • Updates all unit tests and functional tests to only test for v2, and remove any v1 related tests
  • DSPO should not trigger reconcilliations on custom watches on resources based on labels that belong to v1, example this

Testing instructions

DSPA v2 before after upgrade diff, only showing the relevant bits, the rest is the same
image

When v1 is detected, DSPO will log:

2024-10-01T20:06:08Z INFO unsupported DSP version v1 detected. Please manually remove this DSP resource and re-apply with a supported version field set {"namespace": "dspa1", "dspa_name": "sample"}

and show the following dspa status:

image

otherwise the DSPA v1 cr is left untouched, only the removed deprecated fields are synch'd by the cluster.

DSPO logs should not be showing any stacktrace errors.

@HumairAK
Copy link
Contributor Author

HumairAK commented Oct 1, 2024

/hold

@dsp-developers
Copy link
Contributor

A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712
An OCP cluster where you are logged in as cluster admin is required.

To use this image run the following:

cd $(mktemp -d)
git clone [email protected]:opendatahub-io/data-science-pipelines-operator.git
cd data-science-pipelines-operator/
git fetch origin pull/712/head
git checkout -b pullrequest 064a1e6d7334dab3ad37937c29885cdd42de39d8
oc new-project opendatahub
make deploy IMG="quay.io/opendatahub/data-science-pipelines-operator:pr-712"

More instructions here on how to deploy and test a Data Science Pipelines Application.

@gregsheremeta
Copy link
Contributor

I think a fine way to test this is to just make sure v2 still works 🤷

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@HumairAK HumairAK changed the title Chore: Remove v1 fields and reconcile logic from dspo WIP: Chore: Remove v1 fields and reconcile logic from dspo Oct 1, 2024
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

jgarciao
jgarciao reviewed Oct 2, 2024
View reviewed changes
api/v1/groupversion_info.go Outdated Show resolved Hide resolved
api/v1/groupversion_info.go Show resolved Hide resolved
@HumairAK
Copy link
Contributor Author

HumairAK commented Oct 2, 2024 •
edited
Loading

FYI this is likely going to go through many significant changes, I would hold off on reviews for now

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@HumairAK HumairAK changed the title WIP: Chore: Remove v1 fields and reconcile logic from dspo Chore: Remove v1 fields and reconcile logic from dspo Oct 3, 2024
@HumairAK
Copy link
Contributor Author

HumairAK commented Oct 3, 2024

/retest

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

1 similar comment
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

@HumairAK
remove dsp v1
cd6d5a6 
This change adds a new apiversion "v1", which will remove all the dsp "v1" related fields, and also defaults to dsp "v2". The new apiversion is set as the stored version, however k8s will continue to serve v1alpha1, however it is deprecated.

All tests are updated acordingly.

Signed-off-by: Humair Khan <[email protected]>
@HumairAK
Set dsp-version label on managed resources
aaf3510 
This change makes it so that new reconciles on dspa owned resources are
labelled with a dsp-version=<dspa-version> label. This is to allow for
the dspa selectively watch (for manual WatchesRawSources) on resources
that are descendents of DSPAs with .spec.dspVersion set to supported
versions.

Signed-off-by: Humair Khan <[email protected]>
@dsp-developers
Copy link
Contributor

Change to PR detected. A new PR build was completed.
A new image has been built to help with testing out this PR: quay.io/opendatahub/data-science-pipelines-operator:pr-712

gregsheremeta
gregsheremeta reviewed Oct 24, 2024
View reviewed changes
Copy link
Contributor

@gregsheremeta gregsheremeta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Oct 24, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 24, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gregsheremeta

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

@HumairAK HumairAK merged commit 134a96d into opendatahub-io:main Oct 24, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gregsheremeta gregsheremeta gregsheremeta left review comments

@jgarciao jgarciao jgarciao left review comments

@rimolive rimolive Awaiting requested review from rimolive

@VaniHaripriya VaniHaripriya Awaiting requested review from VaniHaripriya

Assignees

@gregsheremeta gregsheremeta

Labels
approved do-not-merge/hold lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HumairAK @dsp-developers @gregsheremeta @jgarciao @openshift-merge-robot