Skip to content

Conversation

@y-rabie
Copy link
Contributor

@y-rabie y-rabie commented Jun 17, 2025

The PR's intent is to fix a behavior whereby the SO creation fails when the targetRef object does not exist, which seems to be a byproduct of verifying that container resources are set, not an intentional check. The following are my reasons:

  • To be similar to HPA's behavior, which doesn't fail when the targetRef object doesn't exist.
  • This also makes sense through an eventual consistency lense: we shouldn't expect the SO to be always created before the Deployment/StatefulSet/etc.
  • Also, currently if the target is not Deployment or StatefulSet, the container resouce check doesn't handle it, it just returns and doesn't error out even if the target doesn't exist, which corroborates the view that the target's existence is not what is intentionally checked.

Other few side changes:

  • I've added logic to fail the case where the deployment does exist, and a cpu/memory trigger references a non-existent containerName, which seems to be a reasonable check. A corresponding unit test is also added.
  • Some reordering of the unit tests in the file (the diff is a bit confusing 😄)
  • A redundant unit test have been removed, the one of the two that were added for the LimitRange case. The first of which (where no LimitRange exist) is already covered in the above tests.
  • An inner if condition on the trigger type within the verifyCPUMemoryScalers function is removed since it is duplicated.

The autogenerated code changes are not part of my changes, it just seems that they weren't generated for the latest change on main.

Checklist

@y-rabie y-rabie requested a review from a team as a code owner June 17, 2025 21:57
@y-rabie y-rabie force-pushed the fix-scaledobject-webhook branch from 9e7dfcb to 8f97d35 Compare June 17, 2025 21:59
Copy link
Member

@JorTurFer JorTurFer left a comment

Choose a reason for hiding this comment

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

I like this improvement, but I think that we should expose the new behavior via configuration, WDYT @wozniakjan ?

@y-rabie y-rabie force-pushed the fix-scaledobject-webhook branch from 8f97d35 to 6e66524 Compare June 29, 2025 10:38
@y-rabie y-rabie force-pushed the fix-scaledobject-webhook branch from 6e66524 to 044ed34 Compare June 29, 2025 10:58
@JorTurFer
Copy link
Member

PTAL @kedacore/keda-core-contributors

@stale
Copy link

stale bot commented Sep 21, 2025

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale All issues that are marked as stale due to inactivity label Sep 21, 2025
@JorTurFer
Copy link
Member

PTAL @kedacore/keda-core-contributors @kedacore/keda-core-maintainers

@stale stale bot removed the stale All issues that are marked as stale due to inactivity label Sep 22, 2025
@zroubalik
Copy link
Member

I like this improvement, but I think that we should expose the new behavior via configuration, WDYT @wozniakjan ?

Yeah, I tend to agree with this.

@y-rabie
Copy link
Contributor Author

y-rabie commented Sep 25, 2025

@JorTurFer Configuration as in a field in ScaledObject, or a controller command-line option? I'm okay with the latter, the former not tbh because it'd just add clutter

@JorTurFer
Copy link
Member

@JorTurFer Configuration as in a field in ScaledObject, or a controller command-line option? I'm okay with the latter, the former not tbh because it'd just add clutter

I think that as starting point, we can enable it as controller option at that's all, if end-users request more granular control, we can add by annotation or so in the future

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.

3 participants