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

GMC: add UT for reconcile filters #383

Merged
merged 10 commits into from
Sep 4, 2024
Merged

GMC: add UT for reconcile filters #383

merged 10 commits into from
Sep 4, 2024

Conversation

KfreeZ
Copy link
Collaborator

@KfreeZ KfreeZ commented Sep 3, 2024

Description

  1. add UT for reconcile filters
  2. in order to do the UT, extract the filter out of SetupWithManager

Issues

n/a.

Type of change

List the type of change like below. Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds new functionality)
  • Breaking change (fix or feature that would break existing design and interface)

Dependencies

n/a.

Tests

CI/CD covers the regression tests
controller coverage increased
ok github.com/opea-project/GenAIInfra/microservices-connector/internal/controller 22.452s coverage: 78.2% of statements

@KfreeZ KfreeZ requested review from irisdingbj, zhlsunshine and mkbhanda and removed request for irisdingbj and zhlsunshine September 3, 2024 07:55
@@ -137,7 +137,7 @@ function validate_audioqa() {
pods_count=$(kubectl get pods -n $AUDIOQA_NAMESPACE -o jsonpath='{.items[*].metadata.name}' | wc -w)
# expected_ready_pods, expected_external_pods, expected_total_pods
# pods_count-1 is to exclude the client pod in this namespace
check_gmc_status $AUDIOQA_NAMESPACE 'audioqa' $((pods_count-1)) 0 7
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @KfreeZ, why do you use precise number instead pods_count ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hi @zhlsunshine because the pod count is not accurate sometimes when terminating pod is not cleared but counted

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay, I through that it should be okay after calling wait_until_all_pod_ready, if yes, could we just fix the bug for this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the latest wait_until_all_pod_ready has excluded the "Terminating" pods, so there is possibility that the pods are still existing as "Terminating" pods after wait_until_all_pod_ready. Yes I can also count the pods by excluding the "Terminating" pods, but for these test case, the ready counts and total counts are fixed numbers, for chatqa for instances, after a deploy, "8/0/8" is exactly we expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

okay, then let's exclude the Terminating pods when calculating the pods_count.

Copy link
Collaborator

@mkbhanda mkbhanda left a comment

Choose a reason for hiding this comment

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

Guess check_gmc_status function could check and post the error message in its body...

Copy link
Collaborator

@zhlsunshine zhlsunshine left a comment

Choose a reason for hiding this comment

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

LGTM

@mkbhanda mkbhanda merged commit 6442127 into opea-project:main Sep 4, 2024
7 of 9 checks passed
@lianhao
Copy link
Collaborator

lianhao commented Sep 4, 2024

Do we have explanation of this CI test failure: https://github.com/opea-project/GenAIInfra/actions/runs/10697764146/job/29655792762

KfreeZ added a commit to KfreeZ/GenAIInfra that referenced this pull request Sep 4, 2024
@KfreeZ KfreeZ mentioned this pull request Sep 4, 2024
3 tasks
KfreeZ added a commit to KfreeZ/GenAIInfra that referenced this pull request Sep 4, 2024
KfreeZ added a commit to KfreeZ/GenAIInfra that referenced this pull request Sep 4, 2024
zhlsunshine pushed a commit that referenced this pull request Sep 4, 2024
* fix a badcommit in #383

Signed-off-by: KfreeZ <[email protected]>

* fix a badcommit in #383

Signed-off-by: KfreeZ <[email protected]>

* fix a badcommit in #383

Signed-off-by: KfreeZ <[email protected]>

---------

Signed-off-by: KfreeZ <[email protected]>
Yugar-1 pushed a commit to Yugar-1/GenAIInfra that referenced this pull request Sep 5, 2024
* add UT for reconcile filters

Signed-off-by: KfreeZ <[email protected]>

* remove useless code

Signed-off-by: KfreeZ <[email protected]>

* update tests

Signed-off-by: KfreeZ <[email protected]>

* debug CI/CD failed

Signed-off-by: KfreeZ <[email protected]>

* skip use pod_count in e2e's check_gmc_status, the pod_count is not acurrate sometimes when terninating pod is not cleared but counted

Signed-off-by: KfreeZ <[email protected]>

* better than hardcode

Signed-off-by: KfreeZ <[email protected]>

* count the pods right

Signed-off-by: KfreeZ <[email protected]>

---------

Signed-off-by: KfreeZ <[email protected]>
Yugar-1 pushed a commit to Yugar-1/GenAIInfra that referenced this pull request Sep 5, 2024
* fix a badcommit in opea-project#383

Signed-off-by: KfreeZ <[email protected]>

* fix a badcommit in opea-project#383

Signed-off-by: KfreeZ <[email protected]>

* fix a badcommit in opea-project#383

Signed-off-by: KfreeZ <[email protected]>

---------

Signed-off-by: KfreeZ <[email protected]>
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.

5 participants