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

Sync changes from the rhds/old-main branch that Rishab created, so we can delete that branch #485

Closed
wants to merge 7 commits into from

Conversation

jiridanek
Copy link
Member

@jiridanek jiridanek commented Dec 4, 2024

I don't understand what it does. Do we want it? Maybe we should just delete old-main without merging it.

The description on the original commit was

Perform update of the notebook controller without surge

This allows updating the notebook controllers without increasing the
resources requirements of ODH during the update, which means it can
better tolerates situation such as the cluster being full.

This means the notebook controller will be unavailable during the
update. However, this has not visible consequences except for a slight
delay in the notebooks CR reconciliation, until the new controller pod
start reconciling.

I guess it's quite plausible it has that effect. Do we still want it?

VaishnaviHire and others added 7 commits April 14, 2023 14:05
This allows updating the notebook controllers without increasing the
resources requirements of ODH during the update, which means it can
better tolerates situation such as the cluster being full.

This means the notebook controller will be unavailable during the
update. However, this has not visible consequences except for a slight
delay in the notebooks CR reconciliation, until the new controller pod
start reconciling.
Sync main branch with odh v1.6-branch
@openshift-ci openshift-ci bot requested review from atheo89 and jstourac December 4, 2024 09:44
Copy link

openshift-ci bot commented Dec 4, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from jiridanek. 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:

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

@jiridanek jiridanek changed the title Sync changes from the old-main branch that Rishab created, so we can delete that branch Sync changes from the rhds/old-main branch that Rishab created, so we can delete that branch Dec 4, 2024
@atheo89
Copy link
Member

atheo89 commented Dec 4, 2024

This main branch looks old, and we didn't use it for long time (I guess this one was in sync with the v1.7 or older kbf release). I don't think that is a good idea to sync our main with this one.. Let's ping @harshad16 to add also his opinion on this.

The current v1.9-branch checked out from https://github.com/opendatahub-io/kubeflow/tree/v1.7-branch at the point

@jiridanek
Copy link
Member Author

/cc @harshad16

@openshift-ci openshift-ci bot requested a review from harshad16 December 4, 2024 13:33
Copy link
Member

@atheo89 atheo89 left a comment

Choose a reason for hiding this comment

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

Hey Jiri, it looks like these changes already exist on both main and the v1.9-branch. I think there might have been a mix-up with the branch references.
image

@@ -10,6 +10,10 @@ kind: Deployment
metadata:
name: deployment
spec:
strategy:
Copy link
Member

Choose a reason for hiding this comment

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

That spec.strategy option is already included on the managers' deployment files in both branches main and v1.9-branch

https://github.com/opendatahub-io/kubeflow/blob/main/components/notebook-controller/config/manager/manager.yaml#L13-L16

@@ -6,6 +6,10 @@ metadata:
namespace: system
spec:
replicas: 1
strategy:
Copy link
Member

Choose a reason for hiding this comment

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

@jiridanek
Copy link
Member Author

@atheo89 thanks! I trusted the gh diff too much, thought that when it shows these lines as addition, they have to be new.

@jiridanek jiridanek closed this Dec 12, 2024
@jiridanek jiridanek deleted the old-main branch December 12, 2024 09:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants