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

Autoscaling Workflow Enhancement - Part 2 #94

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

kr11
Copy link
Collaborator

@kr11 kr11 commented Aug 26, 2024

Pull Request Description

Major Updates:

  1. Improved Scale Object Retrieval: We have refined the process of fetching the Scale object. Now, we directly access appsv1.Deployment instead of the generalized autoscalingv1.Scale. For a detailed discussion, refer to [issue AutoScaler Workflow Part 2: Areas for Optimization and Completion #95].
  2. Initialization of KPAAutoScaler: We have streamlined the initialization of KPAAutoScaler objects. KPA-specific parameters are temporarily set using the NewDefaultDeciderKpaSpec() function to apply default values.
  3. RBAC Bug Fix in PodAutoscalerReconciler::Reconcile: fix RBAC error about Event, add event via r.EventRecorder.Eventf to confirm the normal running of KPA Reconciler. Relevant discussion in [issue Add make docker-build in Integration Testing #93].
  4. README for KPA: Updated a new section detailing KPA-specific functionalities and configurations.

Additional Updates:

  1. Simplified Constructor for NewKpaAutoscaler: Aligned with practices from Knative, we now require only readyPodsCount and a pre-constructed DeciderKpaSpec for initializing a new KPA autoscaler.
  2. KPA Algorithm Bug Fixes: Addressed several bugs in the KPA algorithm.
  3. Renaming of DeciderSpec to DeciderKpaSpec: To clarify usage, DeciderSpec has been renamed to DeciderKpaSpec, indicating that this specification is exclusively used by KPA.

Open Issues and Enhancements:

For unresolved issues and potential enhancements, please see the discussion in [issue #95].

This update aims to enhance the scalability and reliability of our autoscaling processes, ensuring better performance and maintainability. Your feedback and contributions to these changes are welcome.

Related Issues

Resolves: [issue #95]


Contribution Guidelines (Expand for Details)

We appreciate your contribution to aibrix! To ensure a smooth review process and maintain high code quality, please adhere to the following guidelines:

Pull Request Title Format

Your PR title should start with one of these prefixes to indicate the nature of the change:

  • [Bug]: Corrections to existing functionality
  • [CI]: Changes to build process or CI pipeline
  • [Docs]: Updates or additions to documentation
  • [API]: Modifications to aibrix's API or interface
  • [CLI]: Changes or additions to the Command Line Interface
  • [Misc]: For changes not covered above (use sparingly)

Note: For changes spanning multiple categories, use multiple prefixes in order of importance.

Submission Checklist

  • PR title includes appropriate prefix(es)
  • Changes are clearly explained in the PR description
  • New and existing tests pass successfully
  • Code adheres to project style and best practices
  • Documentation updated to reflect changes (if applicable)
  • Thorough testing completed, no regressions introduced

By submitting this PR, you confirm that you've read these guidelines and your changes align with the project's contribution standards.

} else if currentReplicas > pa.Spec.MaxReplicas {
desiredReplicas = pa.Spec.MaxReplicas
} else if currentReplicas < minReplicas {
desiredReplicas = minReplicas
} else {
// TODO: fix the compute replicas interface.
kpaScaler, err := scaler.NewKpaAutoscaler(
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we move the initialization earlier? for example in reconciler new, here is the reconcile workflow and we should use the reference directly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fix it on newest commit 562c5d1

Copy link
Collaborator

Choose a reason for hiding this comment

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

it looks good to me

@Jeffwan
Copy link
Collaborator

Jeffwan commented Aug 27, 2024

besides the initialization part, rest looks good to me

kr11 added 3 commits August 27, 2024 15:36
1. Relocate NewKPAAutoscaler from reconcileKPA to newReconciler.
2. remove attribute podCounter from KpaAutoscaler; dynamically fetch up-to-date readyPodCount in reconceilKPA and pass to Scale Algorithm.
@kr11 kr11 force-pushed the kangrong/features/autoscaling_workflow_part2 branch from a31c0e0 to 562c5d1 Compare August 27, 2024 07:36
@kr11
Copy link
Collaborator Author

kr11 commented Aug 27, 2024

besides the initialization part, rest looks good to me

@Jeffwan, I push a commit about KPA initialization, and rebase this branch onto PR #92

@Jeffwan
Copy link
Collaborator

Jeffwan commented Aug 27, 2024

@kr11 It looks good to me and feel free to merge it

@kr11 kr11 merged commit 1428139 into main Aug 27, 2024
2 checks passed
@kr11 kr11 deleted the kangrong/features/autoscaling_workflow_part2 branch August 27, 2024 09:24
gangmuk pushed a commit that referenced this pull request Jan 25, 2025
* add autoscaler pipeline part 2

* refactor the KPA  initialization:

1. Relocate NewKPAAutoscaler from reconcileKPA to newReconciler.
2. remove attribute podCounter from KpaAutoscaler; dynamically fetch up-to-date readyPodCount in reconceilKPA and pass to Scale Algorithm.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants