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.

Sign up for GitHub

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

Jump to bottom

Install KEDA in tests #3285

Merged
merged 7 commits into from
Aug 24, 2023
Merged

Install KEDA in tests #3285

merged 7 commits into from
Aug 24, 2023

Conversation

Cali0707
Copy link
Member

Fixes #3237

Proposed Changes

  • Add KEDA setup script
  • Add KEDA update script
  • Call KEDA setup script in the ./hack/run.sh deploy-infa` command

@Cali0707
Added install keda script
9040789 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Added update keda script
13f7465 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow
Copy link

knative-prow bot commented Aug 10, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@knative-prow knative-prow bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 10, 2023
@knative-prow knative-prow bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. area/test labels Aug 10, 2023
@codecov
Copy link

codecov bot commented Aug 10, 2023 •
edited
Loading

Codecov Report

Merging #3285 (08daa71) into main (8c81822) will decrease coverage by 0.01%.
Report is 1 commits behind head on main.
The diff coverage is 40.00%.

@@             Coverage Diff              @@
##               main    #3285      +/-   ##
============================================
- Coverage     61.71%   61.70%   -0.01%     
  Complexity      769      769              
============================================
  Files           182      182              
  Lines         12304    12312       +8     
  Branches        268      268              
============================================
+ Hits           7593     7597       +4     
- Misses         4118     4121       +3     
- Partials        593      594       +1
Flag Coverage Δ
java-unittests 71.12% <ø> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...lane/pkg/reconciler/consumergroup/consumergroup.go 54.54% <25.00%> (-0.66%) ⬇️
...l-plane/pkg/reconciler/consumergroup/controller.go 58.62% <100.00%> (+0.48%) ⬆️

... and 2 files with indirect coverage changes

@Cali0707
Added keda install to ./hack/run.sh deploy-infra
0f7fbe1 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707 Cali0707 marked this pull request as ready for review August 11, 2023 14:28
@knative-prow knative-prow bot requested review from creydr and pierDipi August 11, 2023 14:28
@Cali0707 Cali0707 changed the title [WIP] Install KEDA in tests Install KEDA in tests Aug 11, 2023
@knative-prow knative-prow bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 11, 2023
@Cali0707
Copy link
Member Author

/cc @Leo6Leo

@knative-prow knative-prow bot requested a review from Leo6Leo August 11, 2023 14:28
@Cali0707
Copy link
Member Author

/hold

@knative-prow knative-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 11, 2023
@Cali0707 @pierDipi
Fixed consumergroup subscriber resolving
2129de1 
Signed-off-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@Cali0707
Copy link
Member Author

/test upgrade-tests

@Cali0707
Updated scaling lag threshold and polling interval
be0ef93 
Signed-off-by: Calum Murray <cmurray@redhat.com>
Leo6Leo
Leo6Leo approved these changes Aug 11, 2023
View reviewed changes
Copy link
Contributor

@Leo6Leo Leo6Leo left a comment

Choose a reason for hiding this comment

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

Tested in my local env, keda is installed properly.

All pods are up:
keda-admission-5d4d8c5587-cf68n          1/1   Running   0             47s
keda-metrics-apiserver-8f8b47f98-vcfjv   1/1   Running   0             46s
keda-operator-5cf9f89475-2f5bh           1/1   Running   1 (33s ago)   46s

Also compared with Pier's PR, lgtm
/lgtm

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 11, 2023
@Cali0707
Copy link
Member Author

/retest-required

@Cali0707
Copy link
Member Author

@pierDipi I think this these errors are what are causing the tests to fail:

failed to bind resource to pod: Operation cannot be fulfilled on configmaps "kafka-source-dispatcher-0": the object has been modified; please apply your changes to the latest version and try again

failed to schedule consumers: vpod to schedule has resource version different from one in indexer

Any ideas on how I can approach fixing this?

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 22, 2023
@pierDipi
Copy link
Member

@Cali0707 I think it would be better to disable the autoscaling feature [1], merge this PR and then follow up on:

  • setting up a separate job where we test the autoscaling feature separately
  • debug tests and add more if needed

[1]

eventing-kafka-broker/test/config/100-config-kafka-features.yaml

Line 9 in 8c81822

controller.autoscaler: "enabled"

@Cali0707
Merged main into keda-test-install
a25bb95 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 23, 2023
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 23, 2023
@Cali0707
Disabled autoscaler in tests
08daa71 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

setting up a separate job where we test the autoscaling feature separately

@pierDipi do you think #3067 captures this work or should I make a new issue?

@Cali0707
Copy link
Member Author

/cc @Leo6Leo @pierDipi

@Cali0707 Cali0707 requested a review from Leo6Leo August 23, 2023 14:28
@Cali0707
Copy link
Member Author

/retest-required

@pierDipi
Copy link
Member

setting up a separate job where we test the autoscaling feature separately

@pierDipi do you think #3067 captures this work or should I make a new issue?

Yes, it does

pierDipi
pierDipi approved these changes Aug 24, 2023
View reviewed changes
Copy link
Member

@pierDipi pierDipi left a comment

Choose a reason for hiding this comment

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

Thanks @Cali0707

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Aug 24, 2023
@knative-prow
Copy link

knative-prow bot commented Aug 24, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, Leo6Leo, pierDipi

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 24, 2023
@Cali0707
Copy link
Member Author

/unhold

@knative-prow knative-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 24, 2023
@Cali0707
Copy link
Member Author

/retest-required

2 similar comments
@Cali0707
Copy link
Member Author

/retest-required

@Cali0707
Copy link
Member Author

/retest-required

@knative-prow knative-prow bot merged commit 4445a21 into knative-extensions:main Aug 24, 2023
18 of 22 checks passed
Cali0707 added a commit to Cali0707/eventing-kafka-broker that referenced this pull request Aug 28, 2023
@Cali0707 @pierDipi
Install KEDA in tests (knative-extensions#3285)
56511af 
* Added install keda script

Signed-off-by: Calum Murray <cmurray@redhat.com>

* Added update keda script

Signed-off-by: Calum Murray <cmurray@redhat.com>

* Added keda install to ./hack/run.sh deploy-infra

Signed-off-by: Calum Murray <cmurray@redhat.com>

* Fixed consumergroup subscriber resolving

Signed-off-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Updated scaling lag threshold and polling interval

Signed-off-by: Calum Murray <cmurray@redhat.com>

* Disabled autoscaler in tests

Signed-off-by: Calum Murray <cmurray@redhat.com>

---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@Cali0707 Cali0707 mentioned this pull request Aug 28, 2023
Merged
openshift-merge-robot pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request Sep 1, 2023
@Cali0707 @pierDipi
Install KEDA in tests (knative-extensions#3285) (#793)
ff5aa23 
* Added install keda script



* Added update keda script



* Added keda install to ./hack/run.sh deploy-infra



* Fixed consumergroup subscriber resolving




* Updated scaling lag threshold and polling interval



* Disabled autoscaler in tests



---------

Signed-off-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@Cali0707 Cali0707 mentioned this pull request Sep 7, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pierDipi pierDipi pierDipi approved these changes

@aliok aliok Awaiting requested review from aliok

@odacremolbap odacremolbap Awaiting requested review from odacremolbap

@creydr creydr Awaiting requested review from creydr

@Leo6Leo Leo6Leo Awaiting requested review from Leo6Leo

Assignees

@pierDipi pierDipi

@Leo6Leo Leo6Leo

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane area/test lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Install KEDA for testing KEDA autoscaling for ConsumerGroup
4 participants
@Cali0707 @pierDipi @Leo6Leo @knative-prow-robot