Report error in status if more than one DataScienceCluster installed #687

ykaliuta · 2023-11-01T08:37:44Z

Report error in status if more than one DataScienceCluster installed

V2

update phase to phaseError as well
add reporting for DSCInitialization

Description

How Has This Been Tested?

Installed 2 DSCs in the same namespace with different name
Checked oc describe datasciencecluster.datasciencecluster.opendatahub.io/<NAME> for both clusters

Both clusters report like

Status:
  Conditions:
    Last Heartbeat Time:   2023-11-01T08:33:42Z
    Last Transition Time:  2023-11-01T08:32:20Z
    Message:               only one instance of DataScienceCluster object is allowed. Update existing instance on namespace  and name ykaliuta.dsc2
    Reason:                DuplicateDataScienceCluster
    Status:                False
    Type:                  ReconcileComplete
    Last Heartbeat Time:   2023-11-01T08:33:42Z
    Last Transition Time:  2023-11-01T08:32:20Z
    Message:               only one instance of DataScienceCluster object is allowed. Update existing instance on namespace  and name ykaliuta.dsc2
    Reason:                DuplicateDataScienceCluster
    Status:                False
    Type:                  Available
    Last Heartbeat Time:   2023-11-01T08:33:42Z
    Last Transition Time:  2023-11-01T08:31:39Z
    Message:               only one instance of DataScienceCluster object is allowed. Update existing instance on namespace  and name ykaliuta.dsc2
    Reason:                DuplicateDataScienceCluster
    Status:                False
    Type:                  Progressing
    Last Heartbeat Time:   2023-11-01T08:33:42Z
    Last Transition Time:  2023-11-01T08:32:20Z
    Message:               only one instance of DataScienceCluster object is allowed. Update existing instance on namespace  and name ykaliuta.dsc2
    Reason:                DuplicateDataScienceCluster
    Status:                True
    Type:                  Degraded
    Last Heartbeat Time:   2023-11-01T08:33:42Z
    Last Transition Time:  2023-11-01T08:32:20Z
    Message:               only one instance of DataScienceCluster object is allowed. Update existing instance on namespace  and name ykaliuta.dsc2
    Reason:                DuplicateDataScienceCluster
    Status:                Unknown
    Type:                  Upgradeable
    Last Heartbeat Time:   2023-11-01T08:32:12Z
    Last Transition Time:  2023-11-01T08:32:12Z
    Message:               Component reconciled successfully
    Reason:                ReconcileCompleted
    Status:                True
    Type:                  workbenchesReady

For DSCInitialization checked both DataScinceCluster:


  status:
    conditions:
    - lastHeartbeatTime: "2023-11-01T20:26:35Z"
      lastTransitionTime: "2023-11-01T20:26:14Z"
      message: only one instance of DSCInitialization object is allowed
      reason: DuplicateDSCInitialization
      status: "False"
      type: ReconcileComplete
    - lastHeartbeatTime: "2023-11-01T20:26:35Z"
      lastTransitionTime: "2023-11-01T20:26:14Z"
      message: only one instance of DSCInitialization object is allowed
      reason: DuplicateDSCInitialization
      status: "False"
      type: Available
    - lastHeartbeatTime: "2023-11-01T20:26:35Z"
      lastTransitionTime: "2023-11-01T20:26:14Z"
      message: only one instance of DSCInitialization object is allowed
      reason: DuplicateDSCInitialization
      status: "False"
      type: Progressing
    - lastHeartbeatTime: "2023-11-01T20:26:35Z"
      lastTransitionTime: "2023-11-01T20:26:14Z"
      message: only one instance of DSCInitialization object is allowed
      reason: DuplicateDSCInitialization
      status: "True"
      type: Degraded
    - lastHeartbeatTime: "2023-11-01T20:26:35Z"
      lastTransitionTime: "2023-11-01T20:26:14Z"
      message: only one instance of DSCInitialization object is allowed
      reason: DuplicateDSCInitialization
      status: Unknown
      type: Upgradeable
    phase: Error

and DSCIinitialization:

  conditions:
  - lastHeartbeatTime: "2023-11-01T20:25:08Z"
    lastTransitionTime: "2023-11-01T20:23:46Z"
    message: only one instance of DSCInitialization object is allowed. Update existing
      instance name new
    reason: DuplicateDSCInitialization
    status: "False"
    type: ReconcileComplete
  - lastHeartbeatTime: "2023-11-01T20:25:08Z"
    lastTransitionTime: "2023-11-01T20:23:46Z"
    message: only one instance of DSCInitialization object is allowed. Update existing
      instance name new
    reason: DuplicateDSCInitialization
    status: "False"
    type: Available
  - lastHeartbeatTime: "2023-11-01T20:25:08Z"
    lastTransitionTime: "2023-11-01T20:19:40Z"
    message: only one instance of DSCInitialization object is allowed. Update existing
      instance name new
    reason: DuplicateDSCInitialization
    status: "False"
    type: Progressing
  - lastHeartbeatTime: "2023-11-01T20:25:08Z"
    lastTransitionTime: "2023-11-01T20:23:46Z"
    message: only one instance of DSCInitialization object is allowed. Update existing
      instance name new
    reason: DuplicateDSCInitialization
    status: "True"
    type: Degraded
  - lastHeartbeatTime: "2023-11-01T20:25:08Z"
    lastTransitionTime: "2023-11-01T20:23:46Z"
    message: only one instance of DSCInitialization object is allowed. Update existing
      instance name new
    reason: DuplicateDSCInitialization
    status: Unknown
    type: Upgradeable
  phase: Error

Merge criteria:

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

ykaliuta · 2023-11-01T08:48:59Z

Actually, should be revisited after issue #645 integration.

zdtsw

/lgtm

zdtsw · 2023-11-01T15:40:19Z

ref: #644

zdtsw · 2023-11-01T15:40:35Z

do we want to add the same status update for DSCI as well?

ykaliuta · 2023-11-01T19:14:15Z

do we want to add the same status update for DSCI as well?

Good question. Probably it will not do any harm. Just IIUC it is more complicated for the user to encounter. Like install the operator twice. When deploying twice just the DataScienceCluster is much easier.

Well, if there are no objections I'll add it as well. To both DSCI and DSC Reconcile.

zdtsw · 2023-11-01T19:17:18Z

do we want to add the same status update for DSCI as well?

Good question. Probably it will not do any harm. Just IIUC it is more complicated for the user to encounter. Like install the operator twice. When deploying twice just the DataScienceCluster is much easier.

Well, if there are no objections I'll add it as well. To both DSCI and DSC Reconcile.

ok, lets do like this way.
we can get this out first and create a new issue for the DSCI, then have another PR for that issue, how you feel?

ykaliuta · 2023-11-01T19:37:53Z

do we want to add the same status update for DSCI as well?

Good question. Probably it will not do any harm. Just IIUC it is more complicated for the user to encounter. Like install the operator twice. When deploying twice just the DataScienceCluster is much easier.
Well, if there are no objections I'll add it as well. To both DSCI and DSC Reconcile.

ok, lets do like this way. we can get this out first and create a new issue for the DSCI, then have another PR for that issue, how you feel?

I'm ok with several patches in one PR.

Just thought a bit about that. May be there should exist some sort of Webhook to prevent such configurations at the first place?

The instance will be used to report failed status for > 1 case. Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

In case of > 1 dataclusters installed report error condition in the kubernetes status which previously was only reported in the operator logs. Add DuplicateDataScienceCluster string constant for that. Reuse `instance` variable for reportError. Fixes: opendatahub-io#644 Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

Repeat for DSCInitialization the same logic as for DataScienceCluster. The check is performed in both DSCInitialization and DataScienceCluster controllers. Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

zdtsw · 2023-11-02T07:19:22Z

do we want to add the same status update for DSCI as well?

Good question. Probably it will not do any harm. Just IIUC it is more complicated for the user to encounter. Like install the operator twice. When deploying twice just the DataScienceCluster is much easier.
Well, if there are no objections I'll add it as well. To both DSCI and DSC Reconcile.

ok, lets do like this way. we can get this out first and create a new issue for the DSCI, then have another PR for that issue, how you feel?

I'm ok with several patches in one PR.

Just thought a bit about that. May be there should exist some sort of Webhook to prevent such configurations at the first place?

i do not want to enforce git webhook (if this is the webhook you meant) or it will be a new commit everytime and difficult for reviewer, we do have squash-merge for PR so the committer (or reviewer) is responsibile to get a clean history for the PR before merge

ykaliuta · 2023-11-02T07:47:49Z

do we want to add the same status update for DSCI as well?

Good question. Probably it will not do any harm. Just IIUC it is more complicated for the user to encounter. Like install the operator twice. When deploying twice just the DataScienceCluster is much easier.
Well, if there are no objections I'll add it as well. To both DSCI and DSC Reconcile.

ok, lets do like this way. we can get this out first and create a new issue for the DSCI, then have another PR for that issue, how you feel?

I'm ok with several patches in one PR.
Just thought a bit about that. May be there should exist some sort of Webhook to prevent such configurations at the first place?

i do not want to enforce git webhook (if this is the webhook you meant) or it will be a new commit everytime and difficult for reviewer, we do have squash-merge for PR so the committer (or reviewer) is responsibile to get a clean history for the PR before merge

No, I meant kubernetes validating webhook, like https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation

zdtsw · 2023-11-02T08:21:25Z

do we want to add the same status update for DSCI as well?

Good question. Probably it will not do any harm. Just IIUC it is more complicated for the user to encounter. Like install the operator twice. When deploying twice just the DataScienceCluster is much easier.
Well, if there are no objections I'll add it as well. To both DSCI and DSC Reconcile.

ok, lets do like this way. we can get this out first and create a new issue for the DSCI, then have another PR for that issue, how you feel?

I'm ok with several patches in one PR.
Just thought a bit about that. May be there should exist some sort of Webhook to prevent such configurations at the first place?

i do not want to enforce git webhook (if this is the webhook you meant) or it will be a new commit everytime and difficult for reviewer, we do have squash-merge for PR so the committer (or reviewer) is responsibile to get a clean history for the PR before merge

No, I meant kubernetes validating webhook, like https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation

ok, but then it is a totally different story which will add new webhook in api to validate fields etc.
I started this way a couple of months ago but did not continue with it due to other high priority tasks, but if you would like you can create a ticket and work on this.

ykaliuta · 2023-11-02T09:52:39Z

Just thought a bit about that. May be there should exist some sort of Webhook to prevent such configurations at the first place?

i do not want to enforce git webhook (if this is the webhook you meant) or it will be a new commit everytime and difficult for reviewer, we do have squash-merge for PR so the committer (or reviewer) is responsibile to get a clean history for the PR before merge

No, I meant kubernetes validating webhook, like https://book.kubebuilder.io/cronjob-tutorial/webhook-implementation

ok, but then it is a totally different story which will add new webhook in api to validate fields etc.

Sure, it's a further enhancement and it's good to get proper reporting first (I've updated the PR BTW and added phaseError set like in some other places)

I started this way a couple of months ago but did not continue with it due to other high priority tasks, but if you would like you can create a ticket and work on this.

Sounds great actually!

zdtsw · 2023-11-02T10:01:32Z

create issue : #693

zdtsw

/lgtm

openshift-ci · 2023-11-02T13:28:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zdtsw
Once this PR has been reviewed and has the lgtm label, please assign lavlas for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

OWNERS

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

controllers/datasciencecluster/datasciencecluster_controller.go

…pendatahub-io#687) * DSCc, DSCIc: move len > 1 check after getting first instance. In case of > 1 dataclusters installed report error condition in the kubernetes status which previously was only reported in the operator logs. Add DuplicateDataScienceCluster string constant for that. * DSCc, DSCIc: set status for > 1 DSCInitialization Repeat for DSCInitialization the same logic as for DataScienceCluster. The check is performed in both DSCInitialization and DataScienceCluster controllers. Fixes: opendatahub-io#644 Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> --------- Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com> (cherry picked from commit 4e95d43)

openshift-ci bot requested review from etirelli and LaVLaS November 1, 2023 08:37

zdtsw approved these changes Nov 1, 2023

View reviewed changes

openshift-ci bot assigned zdtsw Nov 1, 2023

openshift-ci bot added the lgtm label Nov 1, 2023

ykaliuta added 2 commits November 1, 2023 22:14

DSCc, DSCIc: move len > 1 check after getting first instance.

862bf17

The instance will be used to report failed status for > 1 case. Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

ykaliuta force-pushed the update-status branch from 849bdde to 1e4b6f3 Compare November 1, 2023 20:24

openshift-ci bot removed the lgtm label Nov 1, 2023

DSCc, DSCIc: set status for > 1 DSCInitialization

d048f2a

Repeat for DSCInitialization the same logic as for DataScienceCluster. The check is performed in both DSCInitialization and DataScienceCluster controllers. Signed-off-by: Yauheni Kaliuta <ykaliuta@redhat.com>

ykaliuta force-pushed the update-status branch from 1e4b6f3 to d048f2a Compare November 1, 2023 20:30

zdtsw added the odh-2.4 label Nov 2, 2023

zdtsw approved these changes Nov 2, 2023

View reviewed changes

openshift-ci bot added the lgtm label Nov 2, 2023

zdtsw reviewed Nov 2, 2023

View reviewed changes

controllers/datasciencecluster/datasciencecluster_controller.go Show resolved Hide resolved

zdtsw merged commit 4e95d43 into opendatahub-io:incubation Nov 2, 2023
6 of 7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Report error in status if more than one DataScienceCluster installed #687

Report error in status if more than one DataScienceCluster installed #687

ykaliuta commented Nov 1, 2023 •

edited

Loading

ykaliuta commented Nov 1, 2023

zdtsw left a comment

zdtsw commented Nov 1, 2023

zdtsw commented Nov 1, 2023

ykaliuta commented Nov 1, 2023

zdtsw commented Nov 1, 2023

ykaliuta commented Nov 1, 2023

zdtsw commented Nov 2, 2023

ykaliuta commented Nov 2, 2023

zdtsw commented Nov 2, 2023

ykaliuta commented Nov 2, 2023

zdtsw commented Nov 2, 2023

zdtsw left a comment

openshift-ci bot commented Nov 2, 2023

Report error in status if more than one DataScienceCluster installed #687

Report error in status if more than one DataScienceCluster installed #687

Conversation

ykaliuta commented Nov 1, 2023 • edited Loading

Description

How Has This Been Tested?

Merge criteria:

ykaliuta commented Nov 1, 2023

zdtsw left a comment

Choose a reason for hiding this comment

zdtsw commented Nov 1, 2023

zdtsw commented Nov 1, 2023

ykaliuta commented Nov 1, 2023

zdtsw commented Nov 1, 2023

ykaliuta commented Nov 1, 2023

zdtsw commented Nov 2, 2023

ykaliuta commented Nov 2, 2023

zdtsw commented Nov 2, 2023

ykaliuta commented Nov 2, 2023

zdtsw commented Nov 2, 2023

zdtsw left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Nov 2, 2023

ykaliuta commented Nov 1, 2023 •

edited

Loading