[Misc] Add Integration Test Utilities for PodAutoscaler Controller #1682
Conversation
Summary of ChangesHello @zhenyu-02, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the testing infrastructure for the PodAutoscaler controller by introducing a suite of dedicated integration test utilities. These utilities streamline the process of validating the controller's behavior, ensuring its robustness and correctness across various scenarios, including interactions with HorizontalPodAutoscalers and custom resource scaling for StormServices. The new framework allows for more efficient and comprehensive testing, ultimately improving the reliability of the autoscaling mechanisms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive set of integration test utilities and tests for the PodAutoscaler controller. The use of helper libraries for validation and a fluent wrapper for test object creation is excellent practice and greatly improves the readability and maintainability of the tests. The test coverage is extensive. I've identified a few areas for improvement, including a minor typo, replacing time.Sleep with more robust Eventually blocks to prevent flaky tests, and removing a duplicated helper function. Overall, this is a high-quality contribution that significantly improves the testing infrastructure.
| const ( | ||
| ConditionReady = "Ready" | ||
| ConditionValidSpec = "ValidSpec" | ||
| ConditionConflict = "MutilPodAutoscalerConflict" |
There was a problem hiding this comment.
There's a typo in the constant name ConditionConflict. "Mutil" should be "Multi". This should be corrected to MultiPodAutoscalerConflict for clarity and consistency. This constant appears to be copied from the controller logic, so it should be updated there as well for consistency across the codebase.
| ConditionConflict = "MutilPodAutoscalerConflict" | |
| ConditionConflict = "MultiPodAutoscalerConflict" |
| time.Sleep(time.Second * 3) // Wait for initial reconcile | ||
| fetched := validation.GetPodAutoscaler(ctx, k8sClient, pa) | ||
| minReplicas := int32(2) | ||
| fetched.Spec.MinReplicas = &minReplicas | ||
| fetched.Spec.MaxReplicas = 10 | ||
| gomega.Expect(k8sClient.Update(ctx, fetched)).To(gomega.Succeed()) | ||
| time.Sleep(time.Second * 3) // Wait for update to propagate |
There was a problem hiding this comment.
Using time.Sleep in tests can make them flaky and slow. Instead of waiting for a fixed duration, it's better to use gomega.Eventually to retry operations until they succeed or a timeout is reached. This is especially useful for handling "the object has been modified" errors that can occur when the test and the controller race to update the same object. The current implementation with time.Sleep is not a guaranteed fix for these race conditions and makes the test suite slower. This pattern of replacing time.Sleep with Eventually should be applied throughout the test file where applicable.
| time.Sleep(time.Second * 3) // Wait for initial reconcile | |
| fetched := validation.GetPodAutoscaler(ctx, k8sClient, pa) | |
| minReplicas := int32(2) | |
| fetched.Spec.MinReplicas = &minReplicas | |
| fetched.Spec.MaxReplicas = 10 | |
| gomega.Expect(k8sClient.Update(ctx, fetched)).To(gomega.Succeed()) | |
| time.Sleep(time.Second * 3) // Wait for update to propagate | |
| gomega.Eventually(func() error { | |
| fetched := validation.GetPodAutoscaler(ctx, k8sClient, pa) | |
| minReplicas := int32(2) | |
| fetched.Spec.MinReplicas = &minReplicas | |
| fetched.Spec.MaxReplicas = 10 | |
| return k8sClient.Update(ctx, fetched) | |
| }, time.Second*10, time.Millisecond*250).Should(gomega.Succeed()) |
| // ValidatePodAutoscalerScalingEventually validates scaling status and waits for eventual consistency. | ||
| func ValidatePodAutoscalerScalingEventually(ctx context.Context, | ||
| k8sClient client.Client, | ||
| pa *autoscalingv1alpha1.PodAutoscaler, | ||
| expectedDesired, expectedActual int32) { | ||
|
|
||
| gomega.Eventually(func(g gomega.Gomega) { | ||
| fetched := &autoscalingv1alpha1.PodAutoscaler{} | ||
| err := k8sClient.Get(ctx, client.ObjectKeyFromObject(pa), fetched) | ||
| g.Expect(err).ToNot(gomega.HaveOccurred()) | ||
|
|
||
| g.Expect(fetched.Status.DesiredScale).To(gomega.Equal(expectedDesired), | ||
| "DesiredScale should be %d", expectedDesired) | ||
| g.Expect(fetched.Status.ActualScale).To(gomega.Equal(expectedActual), | ||
| "ActualScale should be %d", expectedActual) | ||
| }, time.Second*10, time.Millisecond*250).Should(gomega.Succeed()) | ||
| } |
There was a problem hiding this comment.
The function ValidatePodAutoscalerScalingEventually is an exact duplicate of ValidatePodAutoscalerScaling. This duplication should be removed to improve maintainability. Since ValidatePodAutoscalerScaling already uses gomega.Eventually, it serves the purpose of waiting for eventual consistency, and the ...Eventually suffix is redundant.
|
@zhenyu-02 thanks 😄 |
| // Note: In envtest, HPA cascade deletion via OwnerReference doesn't work | ||
| // because garbage collector controller is not running. In real K8s, | ||
| // the HPA would be automatically deleted due to OwnerReference. | ||
| // We already verified OwnerReference is set correctly in the creation test. |
zhenyu-02
force-pushed
the
feat-add-integrate-test-pa
branch
from
4285d0e to
6809163
Compare
fixed |
|
@zhenyu-02 please fix the ci-lint |
fixed |
Signed-off-by: Wang Zhenyu <ts-zhenyu.b.wang@rakuten.com>
Signed-off-by: Wang Zhenyu <ts-zhenyu.b.wang@rakuten.com>
Signed-off-by: Wang Zhenyu <ts-zhenyu.b.wang@rakuten.com>
Signed-off-by: Wang Zhenyu <ts-zhenyu.b.wang@rakuten.com>
googs1025
force-pushed
the
feat-add-integrate-test-pa
branch
from
4b3bff1 to
7eba636
Compare
|
@googs1025 If there are any other issue need help, I am very willing to contribute. |
3 participants
Pull Request Title
[Misc] Add Integration Test Utilities for PodAutoscaler Controller
Pull Request Description
This PR introduces test utilities to support integration testing for the PodAutoscaler controller. The changes include three new helper libraries that provide a clean, reusable API for:
These utilities enable 31 comprehensive integration tests covering HPA strategy, spec validation, conflict detection, status management, boundary enforcement, StormService scaling, annotation-based configuration, and advanced scenarios.
Related Issues
issue 1650
Changes
Files Added
test/utils/validation/hpa.go
test/utils/validation/podautoscaler.go
test/utils/wrapper/podautoscaler.go
Test Coverage
The integration test suite (
podautoscaler_test.go) covers 31 test scenarios across the following categories:1. HPA Strategy - Resource Lifecycle (3 tests)
2. Spec Validation Logic (3 tests)
3. Conflict Detection Mechanism (2 tests)
4. Status and Condition Management (3 tests)
5. Scale Target Management (2 tests)
6. Boundary Enforcement (3 tests)
7. Scaling History Management (1 test)
8. StormService Scaling (3 tests)
9. Annotation-Based Configuration (3 tests)
10. Advanced Scenarios (2 tests)
Additional Tests (6 tests)
Test Results
Note: transient “the object has been modified” warnings are expected under concurrent reconciles; all specs passed.
Additional Information
pkg/controller/autoscaling/podautoscaler_controller.go