Skip to content

listener: reset the file event in framework instead of listener filter doing itself#17227

Merged
mattklein123 merged 3 commits intoenvoyproxy:mainfrom
soulxu:reset_file_event_in_framework
Jul 7, 2021
Merged

listener: reset the file event in framework instead of listener filter doing itself#17227
mattklein123 merged 3 commits intoenvoyproxy:mainfrom
soulxu:reset_file_event_in_framework

Conversation

@soulxu
Copy link
Copy Markdown
Member

@soulxu soulxu commented Jul 3, 2021

Commit Message: listener: reset the file event in framework instead of listener filter doing itself
Additional Description:
The previous fix of the issue #16951 (#16952) was done by
reset the file event inside the listener filter destruction. But that isn't good enough since it given the burden
to the listener filter developer, and people still may forget to reset the file event. So this PR is going to
reset the file event inside ActiveTcpSocket.
Risk Level: low
Testing: unitest and integration test
Docs Changes: n/a
Release Notes: n/a
Improve the Fixes #16951

Signed-off-by: He Jie Xu hejie.xu@intel.com

…r doing itself

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu soulxu requested review from asraa, ggreenway and lizan as code owners July 3, 2021 04:05
@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Jul 3, 2021

cc @mattklein123 @antoniovicente

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Jul 3, 2021

Do we need release note for this PR? if yes, I guess it should be in the section 'Bug fixes'?

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Jul 3, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit isn't fully completed, but will still attempt retrying.
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17227 (comment) was created by @soulxu.

see: more, trace.

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Jul 4, 2021

The PR failed on direct source address filter matching test https://dev.azure.com/cncf/envoy/_build/results?buildId=81052&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881&l=18024

This probably related to #17118

But I can't reproduce the failure locally. So let me add some log to debug it.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Jul 4, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17227 (comment) was created by @soulxu.

see: more, trace.

@soulxu
Copy link
Copy Markdown
Member Author

soulxu commented Jul 4, 2021

The PR failed on direct source address filter matching test https://dev.azure.com/cncf/envoy/_build/results?buildId=81052&view=logs&j=bbe4b42d-86e6-5e9c-8a0b-fea01d818a24&t=e00c5a13-c6dc-5e9a-6104-69976170e881&l=18024

This probably related to #17118

But I can't reproduce the failure locally. So let me add some log to debug it.

Emm....after I add log, the failure disappears...

This reverts commit bdf45ee.

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
@mattklein123 mattklein123 self-assigned this Jul 6, 2021
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks for the follow up. IMO this is a more resilient fix until we do something better. @ggreenway @antoniovicente any objections to this per previous discussions?

@ggreenway
Copy link
Copy Markdown
Member

No objection from me; I agree that this is an improvement from having every listener filter do it.

Copy link
Copy Markdown
Contributor

@antoniovicente antoniovicente left a comment

Choose a reason for hiding this comment

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

Thanks for the followup cleanup.

@mattklein123 mattklein123 merged commit 6ef4705 into envoyproxy:main Jul 7, 2021
baojr added a commit to baojr/envoy that referenced this pull request Jul 7, 2021
* main:
  listener: match rebalancer to listener IP family type (envoyproxy#16914)
  jwt_authn: implementation of www-authenticate header (envoyproxy#16216)
  listener: reset the file event in framework instead of listener filter doing itself (envoyproxy#17227)
  Small typo fix (envoyproxy#17247)
  Doc: Clarify request/response attributes are http-only (envoyproxy#17204)
  bazel/README.md: add aspell comment (envoyproxy#17072)
  docs: Fix broken URL links in HTTP upgrades doc (envoyproxy#17225)
  remove the wrong comment on test (envoyproxy#17233)
  upstream: allow clusters to skip waiting on warmup for initialization (envoyproxy#17179)
  JwtAuthn: support completing padding on forward jwt payload header (envoyproxy#16752)
  remove support for v2 UNSUPPORTED_REST_LEGACY (envoyproxy#16968)
  metrics service: fix wrong argument arrange on MetricsServiceSink (envoyproxy#17127)
  deps: update cel-cpp to 0.6.1 (envoyproxy#16293)
  Add ability to filter ConfigDump. (envoyproxy#16774)
  examples: fix Wasm example. (envoyproxy#17218)
  upstream: update host's socket factory when metadata is updated. (envoyproxy#16708)

Signed-off-by: Garrett Bourg <bourg@squareup.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
…r doing itself (envoyproxy#17227)

Signed-off-by: He Jie Xu <hejie.xu@intel.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

envoy still try to create file event twice on the same fd when continue_on_listener_filters_timeout is true

4 participants