Skip to content

Restoring zero initial scale behavior#646

Merged
openshift-merge-bot[bot] merged 1 commit into
opendatahub-io:release-v0.15from
brettmthompson:feature/restore-zero-initial-scale-behavior
Jun 3, 2025
Merged

Restoring zero initial scale behavior#646
openshift-merge-bot[bot] merged 1 commit into
opendatahub-io:release-v0.15from
brettmthompson:feature/restore-zero-initial-scale-behavior

Conversation

@brettmthompson
Copy link
Copy Markdown

@brettmthompson brettmthompson commented May 30, 2025

What this PR does / why we need it:
RHOAIENG-26265

Syncing upstream initial scale behavior and restoring ODH specific zero initial scale behavior enabled in 537

Type of changes
Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Feature/Issue validation/testing:

Please describe the tests that you ran to verify your changes and relevant result summary. Provide instructions so it can be reproduced.
Please also list any relevant details for your test configuration.

  • Refer to 537 for manual testing strategy
  1. Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Checklist:

  • Have you added unit/e2e tests that prove your fix is effective or that this feature works?
  • Has code been commented, particularly in hard-to-understand areas?
  • Have you made corresponding changes to the documentation?
  • Have you linked the JIRA issue(s) to this PR?

Release note:

NONE

Re-running failed tests

  • /rerun-all - rerun all failed workflows.
  • /rerun-workflow <workflow name> - rerun a specific failed workflow. Only one workflow name can be specified. Multiple /rerun-workflow commands are allowed per comment.

Signed-off-by: Brett Thompson <196701379+brettmthompson@users.noreply.github.com>
@brettmthompson
Copy link
Copy Markdown
Author

/test e2e-graph
/test e2e-path-based-routing

@brettmthompson
Copy link
Copy Markdown
Author

/test e2e-graph

@hdefazio hdefazio requested review from israel-hdez and spolti June 2, 2025 16:02
Copy link
Copy Markdown
Member

@spolti spolti left a comment

Choose a reason for hiding this comment

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

Why so many removals?
Most of it seems to be related with knative, @brettmthompson are these correct?

Also, shouldn't the sync made by Edgar include these changes?

@brettmthompson
Copy link
Copy Markdown
Author

brettmthompson commented Jun 3, 2025

@spolti all removals are for code that was added for reading the knative configuration from the knative serving custom resource. After syncing the initial scale changes from upstream this is no longer needed because we read this configuration from the config-autoscaler config map instead. It is no longer necessary to have a dependency on the knativeserving crd being installed.

This PR is to the release-v0.15 branch. I believe this is the branch @israel-hdez is using for the sync. After the upstream changes get synced to the master branch we can cherry pick this commit to the master branch.

@spolti
Copy link
Copy Markdown
Member

spolti commented Jun 3, 2025

After the upstream changes get synced to the master branch we can cherry pick this commit to the master branch.

I might be missing here, is this PR the same than we already have on upstream but it wasn't synced yet with our master?
If that's the case, why do we need to cherry-pick this commit to master?

@brettmthompson
Copy link
Copy Markdown
Author

brettmthompson commented Jun 3, 2025

@spolti this PR is not the same as what we have on upstream. We are introducing logic exclusive to midstream to throw set initial scale to zero if zero min replicas are requested.

Copy link
Copy Markdown

@israel-hdez israel-hdez left a comment

Choose a reason for hiding this comment

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

Basically, this removes remainders from the Knative operator approach, and re-aligns with upstream for using the ConfigMap. I think this is good.

I only had one quite minor comment. Let me know if it is intended so I can approve.

Comment thread pkg/controller/v1alpha1/utils/utils.go
Copy link
Copy Markdown

@israel-hdez israel-hdez left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Jun 3, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brettmthompson, israel-hdez

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [brettmthompson,israel-hdez]

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

@brettmthompson
Copy link
Copy Markdown
Author

/test e2e-predictor

@brettmthompson
Copy link
Copy Markdown
Author

/test e2e-predictor
/test e2e-graph

@openshift-merge-bot openshift-merge-bot Bot merged commit 3b6d478 into opendatahub-io:release-v0.15 Jun 3, 2025
32 of 33 checks passed
@github-project-automation github-project-automation Bot moved this from New/Backlog to Done in ODH Model Serving Planning Jun 3, 2025
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