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

Prober targets service instead of pods directly #2112

Merged
merged 1 commit into from May 5, 2022

Conversation

pierDipi
Copy link
Member

@pierDipi pierDipi commented Apr 21, 2022

Istio doesn't work well when there is pod to pod communication.

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

Fixes #2107

Proposed Changes

  • Prober targets service instead of pods directly

Release Note

Receiver's prober targets services instead of pods directly to allow components to be part of an Istio mesh

@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 size/M Denotes a PR that changes 30-99 lines, ignoring generated files. area/data-plane approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Apr 21, 2022
@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #2112 (93b3108) into main (b2c0499) will increase coverage by 0.04%.
The diff coverage is 95.23%.

@@             Coverage Diff              @@
##               main    #2112      +/-   ##
============================================
+ Coverage     66.55%   66.60%   +0.04%     
- Complexity      676      677       +1     
============================================
  Files           142      142              
  Lines          9138     9147       +9     
  Branches        196      196              
============================================
+ Hits           6082     6092      +10     
+ Misses         2651     2650       -1     
  Partials        405      405              
Flag Coverage Δ
java-unittests 82.58% <ø> (+0.04%) ⬆️

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

Impacted Files Coverage Δ
control-plane/pkg/prober/async_prober.go 88.88% <88.88%> (+0.13%) ⬆️
control-plane/pkg/prober/prober.go 67.74% <100.00%> (+4.77%) ⬆️
control-plane/pkg/reconciler/broker/controller.go 82.55% <100.00%> (+0.20%) ⬆️
control-plane/pkg/reconciler/channel/controller.go 89.02% <100.00%> (+0.13%) ⬆️
...ol-plane/pkg/reconciler/channel/v2/controllerv2.go 100.00% <100.00%> (ø)
control-plane/pkg/reconciler/sink/controller.go 77.94% <100.00%> (+0.32%) ⬆️
...a/broker/dispatcher/impl/RecordDispatcherImpl.java 89.88% <0.00%> (-0.57%) ⬇️
...ispatcher/impl/http/WebClientCloudEventSender.java 62.79% <0.00%> (+2.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2c0499...93b3108. Read the comment docs.

@pierDipi
Copy link
Member Author

[ERROR] Plugin org.apache.maven.plugins:maven-shade-plugin:3.3.0 or one of its dependencies could not be resolved: Failed to read artifact descriptor for org.apache.maven.plugins:maven-shade-plugin:jar:3.3.0: Could not transfer artifact org.apache.maven.plugins:maven-shade-plugin:pom:3.3.0 from/to central (https://repo.maven.apache.org/maven2): transfer failed for https://repo.maven.apache.org/maven2/org/apache/maven/plugins/maven-shade-plugin/3.3.0/maven-shade-plugin-3.3.0.pom: Connection reset -> [Help 1]

/test unit-tests_eventing-kafka-broker_main

Signed-off-by: Pierangelo Di Pilato <pierdipi@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 Apr 21, 2022
@pierDipi pierDipi added the kind/bug Categorizes issue or PR as related to a bug. label Apr 21, 2022
@matzew
Copy link
Contributor

matzew commented Apr 29, 2022

LGTM

but, I'd like to see some feedback on if the patch worked in your environment, @LeonardAukea

@LeonardAukea
Copy link

@markhulia how is it going with the tests?

@pierDipi pierDipi changed the title [WIP] Prober targets service instead of pods directly Prober targets service instead of pods directly May 4, 2022
@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 May 4, 2022
@pierDipi
Copy link
Member Author

pierDipi commented May 4, 2022

The patch worked #2107 (comment)

/cherry-pick release-v1.4

@knative-prow-robot
Copy link
Contributor

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-v1.4 in a new PR and assign it to you.

In response to this:

The patch worked #2107 (comment)

/cherry-pick release-v1.4

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.

@pierDipi
Copy link
Member Author

pierDipi commented May 4, 2022

/cherry-pick release-1.4

@knative-prow-robot
Copy link
Contributor

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.4 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.4

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.

@pierDipi
Copy link
Member Author

pierDipi commented May 4, 2022

/cherry-pick release-1.3

@knative-prow-robot
Copy link
Contributor

@pierDipi: once the present PR merges, I will cherry-pick it on top of release-1.3 in a new PR and assign it to you.

In response to this:

/cherry-pick release-1.3

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.

@pierDipi
Copy link
Member Author

pierDipi commented May 4, 2022

/cc @matzew

@knative-prow knative-prow bot requested a review from matzew May 4, 2022 15:34
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 May 5, 2022
@knative-prow
Copy link

knative-prow bot commented May 5, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: matzew, 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 c4264b9 into knative-extensions:main May 5, 2022
@knative-prow-robot
Copy link
Contributor

@pierDipi: new pull request created: #2158

In response to this:

/cherry-pick release-1.4

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.

@knative-prow-robot
Copy link
Contributor

@pierDipi: new pull request created: #2159

In response to this:

/cherry-pick release-1.3

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.

@knative-prow-robot
Copy link
Contributor

@pierDipi: cannot checkout release-v1.4: error checking out release-v1.4: exit status 1. output: error: pathspec 'release-v1.4' did not match any file(s) known to git

In response to this:

The patch worked #2107 (comment)

/cherry-pick release-v1.4

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.

pierDipi added a commit to pierDipi/eventing-kafka-broker that referenced this pull request May 9, 2022
)

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
pierDipi added a commit to pierDipi/eventing-kafka-broker that referenced this pull request May 10, 2022
)

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
openshift-merge-robot pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request May 10, 2022
…sions#2112) (#250)

* Prober targets service instead of pods directly (knative-extensions#2112)

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

* Run make generate-release

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
openshift-cherrypick-robot pushed a commit to openshift-cherrypick-robot/eventing-kafka-broker that referenced this pull request May 10, 2022
)

Signed-off-by: Pierangelo Di Pilato <pierdipi@redhat.com>
openshift-merge-robot pushed a commit to openshift-knative/eventing-kafka-broker that referenced this pull request May 10, 2022
…ve-extensions#2112) (#257)

* Prober targets service instead of pods directly (knative-extensions#2112)

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

* Run make generate-release

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

Co-authored-by: Pierangelo Di Pilato <pierdipi@redhat.com>
@pierDipi
Copy link
Member Author

/cherry-pick release-1.2

@knative-prow-robot
Copy link
Contributor

@pierDipi: #2112 failed to apply on top of branch "release-1.2":

Applying: Prober targets service instead of pods directly
Using index info to reconstruct a base tree...
M	control-plane/pkg/reconciler/broker/controller.go
M	control-plane/pkg/reconciler/channel/controller.go
A	control-plane/pkg/reconciler/channel/v2/controllerv2.go
M	data-plane/config/broker/500-receiver.yaml
M	data-plane/config/channel/500-receiver.yaml
M	data-plane/config/sink/500-receiver.yaml
Falling back to patching base and 3-way merge...
Auto-merging data-plane/config/sink/500-receiver.yaml
Auto-merging data-plane/config/channel/500-receiver.yaml
Auto-merging data-plane/config/broker/500-receiver.yaml
CONFLICT (modify/delete): control-plane/pkg/reconciler/channel/v2/controllerv2.go deleted in HEAD and modified in Prober targets service instead of pods directly. Version Prober targets service instead of pods directly of control-plane/pkg/reconciler/channel/v2/controllerv2.go left in tree.
Auto-merging control-plane/pkg/reconciler/channel/controller.go
Auto-merging control-plane/pkg/reconciler/broker/controller.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Prober targets service instead of pods directly
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-1.2

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.

@pierDipi pierDipi deleted the KNATIVE-2107 branch May 11, 2022 08:50
@aliok
Copy link
Member

aliok commented May 11, 2022

Manual cherry pick into 1.2 branch: #2180

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 kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Simple Broker does not become Ready, 502
5 participants