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

Fix channel finalizer logic #3295

Merged
merged 1 commit into from
Aug 28, 2023
Merged

Fix channel finalizer logic #3295

merged 1 commit into from
Aug 28, 2023

Conversation

Cali0707
Copy link
Member

Fixes #3289

Proposed Changes

  • Use the old channel topic name function to figure out which topic to delete when no topic is annotated

Release Note

:bug: Fixed bug where deleting channels shortly after upgrading from pre 1.10 Knative threw an error.

@Cali0707
Fix channel finalizer logic
e4ae8b9 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@knative-prow knative-prow bot requested review from aliok and creydr August 21, 2023 13:24
@knative-prow knative-prow bot added area/control-plane size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 21, 2023
@Cali0707
Copy link
Member Author

/cc @pierDipi @mgencur

@knative-prow knative-prow bot requested review from mgencur and pierDipi August 21, 2023 13:24
@codecov
Copy link

codecov bot commented Aug 21, 2023 •
edited
Loading

Codecov Report

Merging #3295 (e4ae8b9) into main (21c92e7) will increase coverage by 0.03%.
Report is 8 commits behind head on main.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #3295      +/-   ##
==========================================
+ Coverage   58.53%   58.56%   +0.03%     
==========================================
  Files          91       91              
  Lines        9159     9159              
==========================================
+ Hits         5361     5364       +3     
+ Misses       3377     3375       -2     
+ Partials      421      420       -1
Files Changed Coverage Δ
control-plane/pkg/reconciler/channel/channel.go 69.10% <0.00%> (ø)

... and 1 file with indirect coverage changes

@matzew
Copy link
Contributor

matzew commented Aug 22, 2023

/test channel-integration-tests-ssl

1 similar comment
@matzew
Copy link
Contributor

matzew commented Aug 22, 2023

/test channel-integration-tests-ssl

@matzew
Copy link
Contributor

matzew commented Aug 22, 2023

Annotation was introduced via #3162

matzew
matzew approved these changes Aug 22, 2023
View reviewed changes
Copy link
Contributor

@matzew matzew 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 Aug 22, 2023
@knative-prow
Copy link

knative-prow bot commented Aug 22, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, matzew

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 22, 2023
@Cali0707
Copy link
Member Author

/retest-required

@matzew
Copy link
Contributor

matzew commented Aug 23, 2023

/test channel-integration-tests-sasl-ss

@knative-prow
Copy link

knative-prow bot commented Aug 23, 2023

@matzew: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

  • /test build-tests
  • /test channel-integration-tests-sasl-plain
  • /test channel-integration-tests-sasl-ssl
  • /test channel-integration-tests-ssl
  • /test channel-reconciler-tests-sasl-plain
  • /test channel-reconciler-tests-sasl-ssl
  • /test channel-reconciler-tests-ssl
  • /test integration-tests
  • /test reconciler-tests
  • /test reconciler-tests-namespaced-broker
  • /test unit-tests
  • /test upgrade-tests

Use /test all to run all jobs.

In response to this:

/test channel-integration-tests-sasl-ss

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@Cali0707
Copy link
Member Author

/retest-required

2 similar comments
@matzew
Copy link
Contributor

matzew commented Aug 28, 2023

/retest-required

@Cali0707
Copy link
Member Author

/retest-required

@knative-prow knative-prow bot merged commit ffd66cf into knative-extensions:main Aug 28, 2023
matzew pushed a commit to matzew/eventing-kafka-broker that referenced this pull request Aug 29, 2023
@Cali0707 @matzew
Fix channel finalizer logic (knative-extensions#3295)
500a1d4 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@matzew matzew mentioned this pull request Aug 29, 2023
Closed
matzew pushed a commit to matzew/eventing-kafka-broker that referenced this pull request Aug 29, 2023
@Cali0707 @matzew
Fix channel finalizer logic (knative-extensions#3295)
59e807a 
Signed-off-by: Calum Murray <cmurray@redhat.com>
@matzew matzew mentioned this pull request Aug 29, 2023
Merged
openshift-merge-robot pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request Aug 30, 2023
@matzew @Cali0707 @pierDipi
Fix channel finalizer logic (knative-extensions#3295) (#795)
8197862 
Signed-off-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
pierDipi added a commit to pierDipi/eventing-kafka-broker that referenced this pull request Sep 20, 2023
@matzew @Cali0707 @pierDipi
Fix channel finalizer logic (knative-extensions#3295) (knative-extens…
db94536 
…ions#795)

Signed-off-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
openshift-merge-robot pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request Sep 20, 2023
@pierDipi @mgencur
@matzew @Cali0707 @openshift-cherrypick-robot
[release v1.11] Batch of backports (#834)
90c61a5 
* Make SecretSpec field of consumers Auth omitempty (#780)

* Expose init offset and schedule metrics for ConsumerGroup reconciler (#790) (#791)

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Fix channel finalizer logic (knative-extensions#3295) (#795)

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

* [release-v1.10] SRVKE-958: Cache init offsets results (#817)

* Cache init offsets results

When there is high load and multiple consumer group schedule calls,
we get many `dial tcp 10.130.4.8:9092: i/o timeout` errors when
trying to connect to Kafka.
This leads to increased "time to readiness" for consumer groups.

The downside of caching is that, in the case, partitions
increase while the result is cached we won't initialize
the offsets of the new partitions.

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

* Add autoscaler leader log patch

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>

---------

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

* Scheduler handle overcommitted pods (#820)

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

* Set consumer and consumergroups finalizers when creating them (#823)

It is possible that a delete consumer or consumergroup might
be reconciled and never finalized when it is deleted before
the finalizer is set.
This happens because the Knative generated reconciler uses
patch (as opposed to using update) for setting the finalizer
and patch doesn't have any optimistic concurrency controls.

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

* Clean up reserved from resources that have been scheduled (#830)

In a recent testing run, we've noticed we have have a scheduled
`ConsumerGroup` [1] (see placements) being considered having
reserved replicas in a different pod [2].

That makes the scheduler think that there is no space but the
autoscaler says we have enough space to hold every virtual
replica.

[1]
```
$ k describe consumergroups -n ks-multi-ksvc-0 c9ee3490-5b4b-4d11-87af-8cb2219d9fe3
Name:         c9ee3490-5b4b-4d11-87af-8cb2219d9fe3
Namespace:    ks-multi-ksvc-0
...
Status:
  Conditions:
    Last Transition Time:  2023-09-06T19:58:27Z
    Reason:                Autoscaler is disabled
    Status:                True
    Type:                  Autoscaler
    Last Transition Time:  2023-09-06T21:41:13Z
    Status:                True
    Type:                  Consumers
    Last Transition Time:  2023-09-06T19:58:27Z
    Status:                True
    Type:                  ConsumersScheduled
    Last Transition Time:  2023-09-06T21:41:13Z
    Status:                True
    Type:                  Ready
  Observed Generation:     1
  Placements:
    Pod Name:      kafka-source-dispatcher-6
    Vreplicas:     4
    Pod Name:      kafka-source-dispatcher-7
    Vreplicas:     4
  Replicas:        8
  Subscriber Uri:  http://receiver5-2.ks-multi-ksvc-0.svc.cluster.local
Events:            <none>
```

[2]
```
    "ks-multi-ksvc-0/c9ee3490-5b4b-4d11-87af-8cb2219d9fe3": {
      "kafka-source-dispatcher-3": 8
    },
```

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

* Ignore unknown fields in data plane contract (knative-extensions#3335) (#828)

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

---------

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
Signed-off-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Martin Gencur <mgencur@redhat.com>
Co-authored-by: Matthias Wessendorf <mwessend@redhat.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: OpenShift Cherrypick Robot <openshift-cherrypick-robot@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matzew matzew matzew approved these changes

@aliok aliok Awaiting requested review from aliok

@creydr creydr Awaiting requested review from creydr

@mgencur mgencur Awaiting requested review from mgencur

@pierDipi pierDipi Awaiting requested review from pierDipi

Assignees

@matzew matzew

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

Successfully merging this pull request may close these issues.

Finalizer for KafkaChannel throws "no topic annotated on channel"
2 participants
@Cali0707 @matzew