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

Use triggerv2 controller in shared main #3558

Merged
merged 40 commits into from
Apr 3, 2024

Conversation

Cali0707
Copy link
Member

@Cali0707 Cali0707 commented Jan 2, 2024

As we move towards scaling triggers with KEDA, we first need to migrate triggers to use consumergroups.

Proposed Changes

  • Switch triggers to use the triggerv2 controller in shared main.
  • Update the deployments to use the contract-resources configmap, and for the broker-dispatcher to be deployed as a statefulset

Release Note

Triggers for Kafka brokers (but not namespaced brokers) now use consumergroups internally. IMPORTANT: to downgrade from this version, you will now need to: 1. delete all consumergroups with an owner reference pointing to a trigger and 2. delete statefulsets for broker dispatcher and receivers

Docs

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

knative-prow bot commented Jan 2, 2024

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 do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. area/control-plane labels Jan 2, 2024
@knative-prow knative-prow bot requested review from creydr and matzew January 2, 2024 15:48
@knative-prow knative-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 2, 2024
@Cali0707
Copy link
Member Author

Cali0707 commented Jan 2, 2024

/test all

@knative-prow knative-prow bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 2, 2024
Copy link

codecov bot commented Jan 2, 2024

Codecov Report

Attention: Patch coverage is 38.23529% with 126 lines in your changes are missing coverage. Please review.

Project coverage is 54.15%. Comparing base (648f4f0) to head (8b07dc2).
Report is 23 commits behind head on main.

Files Patch % Lines
...md/post-install/kafka_broker_deployment_deleter.go 0.00% 27 Missing ⚠️
...ontrol-plane/pkg/reconciler/testing/statefulset.go 0.00% 26 Missing ⚠️
control-plane/cmd/post-install/main.go 0.00% 21 Missing ⚠️
control-plane/pkg/reconciler/consumer/consumer.go 34.37% 21 Missing ⚠️
control-plane/pkg/security/config.go 0.00% 8 Missing ⚠️
...l-plane/pkg/reconciler/broker/namespaced_broker.go 76.66% 3 Missing and 4 partials ⚠️
...ol-plane/pkg/reconciler/trigger/v2/controllerv2.go 82.85% 3 Missing and 3 partials ⚠️
...ane/pkg/reconciler/broker/namespaced_controller.go 0.00% 3 Missing ⚠️
control-plane/cmd/kafka-controller/main.go 0.00% 2 Missing ⚠️
control-plane/pkg/kafka/clientpool/clientpool.go 50.00% 1 Missing and 1 partial ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #3558      +/-   ##
============================================
+ Coverage     52.43%   54.15%   +1.72%     
- Complexity      874      875       +1     
============================================
  Files           342      346       +4     
  Lines         21415    18193    -3222     
  Branches        284      288       +4     
============================================
- Hits          11229     9853    -1376     
+ Misses         9276     7415    -1861     
- Partials        910      925      +15     
Flag Coverage Δ
java-unittests 73.87% <ø> (-0.21%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added area/data-plane area/test size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jan 2, 2024
@Cali0707
Copy link
Member Author

Cali0707 commented Jan 2, 2024

/test all

1 similar comment
@Cali0707
Copy link
Member Author

Cali0707 commented Jan 3, 2024

/test all

Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 3, 2024
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

Cali0707 commented Jan 3, 2024

/test all

Signed-off-by: Calum Murray <cmurray@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

Cali0707 commented Jan 4, 2024

/test all

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

Cali0707 commented Jan 4, 2024

/test all

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

Cali0707 commented Jan 5, 2024

/test all

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

/test upgrade-tests

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.

/lgtm
/approve

One nit, we can fix it later

}

func (k *kafkaDeploymentDeleter) deleteDeployment(ctx context.Context, deploymentName string) error {
err := k.waiteStatefulSetExists(ctx, deploymentName)
Copy link
Member

Choose a reason for hiding this comment

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

typo waite.... unless it's a word I don't know about

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Mar 28, 2024
@pierDipi
Copy link
Member

/test reconciler-tests-namespaced-broker

@Cali0707
Copy link
Member Author

@pierDipi can I unhold this PR?

@Cali0707
Copy link
Member Author

It looks like the hold was originally because this PR was on hold because of the 1.13 branch cut, I'm unholding now since we are a month away from the branch cut for 1.14

/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 Mar 28, 2024
@Cali0707
Copy link
Member Author

/test reconciler-tests-namespaced-broker

@Cali0707
Copy link
Member Author

/retest-required

@Cali0707
Copy link
Member Author

/test reconciler-tests-namespaced-broker

@Cali0707
Copy link
Member Author

/retest-required

@pierDipi
Copy link
Member

/test reconciler-tests-namespaced-broker

@pierDipi
Copy link
Member

/test reconciler-tests

@Cali0707
Copy link
Member Author

Cali0707 commented Apr 2, 2024

/retest-required

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 Apr 2, 2024
@Cali0707
Copy link
Member Author

Cali0707 commented Apr 2, 2024

/cc @pierDipi

Would you mind taking a look at my most recent commit? I changed the namespaced_controller for the triggers to use a filtered global resync, and to resync the triggers instead of the brokers. This seems to have fixed the namespaced broker tests, but I'm not sure if there was a reason in the past for en-queuing the namespaced brokers instead of triggers when doing a global resync of the triggers

@knative-prow knative-prow bot requested a review from pierDipi April 2, 2024 20:00
@Cali0707
Copy link
Member Author

Cali0707 commented Apr 3, 2024

/retest-required

@pierDipi
Copy link
Member

pierDipi commented Apr 3, 2024

/test reconciler-tests

Signed-off-by: Calum Murray <cmurray@redhat.com>
@Cali0707
Copy link
Member Author

Cali0707 commented Apr 3, 2024

/retest-required

@Cali0707
Copy link
Member Author

Cali0707 commented Apr 3, 2024

/test reconciler-tests-namespaced-broker

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.

/lgtm
/approve

@knative-prow knative-prow bot added the lgtm Indicates that a PR is ready to be merged. label Apr 3, 2024
Copy link

knative-prow bot commented Apr 3, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, 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 merged commit de17c00 into knative-extensions:main Apr 3, 2024
33 of 37 checks passed
if err != nil {
return fmt.Errorf("failed to resolve auth context: %w", err)
}

Copy link
Member

Choose a reason for hiding this comment

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

Not sure why we dropped this here

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh yeah, I think this might have snuck in while trying to fix merge conflicts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/control-plane area/data-plane area/test lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants