Skip to content

Fix regression by socket tuple when c/s deployed in same node#1378

Merged
kmesh-bot merged 5 commits intokmesh-net:mainfrom
hzxuzhonghu:fix-regress
May 9, 2025
Merged

Fix regression by socket tuple when c/s deployed in same node#1378
kmesh-bot merged 5 commits intokmesh-net:mainfrom
hzxuzhonghu:fix-regress

Conversation

@hzxuzhonghu
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu commented May 8, 2025

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #1370

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Copilot AI review requested due to automatic review settings May 8, 2025 12:37
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a regression in socket tuple handling when client and server are deployed on the same node. Key changes include:

  • Adding a new "direction" field to the connection source/destination structure.
  • Renaming variables (e.g. from "connectData" to "rawStats") for clarity.
  • Removing the monitoring enabled check in the Run loop.

@hzxuzhonghu
Copy link
Copy Markdown
Member Author

Fix #1370

@hzxuzhonghu
Copy link
Copy Markdown
Member Author

@yp969803

@yp969803
Copy link
Copy Markdown
Contributor

yp969803 commented May 8, 2025

unit tests are also be updated

@hzxuzhonghu
Copy link
Copy Markdown
Member Author

Sure, will do fix it

@hzxuzhonghu
Copy link
Copy Markdown
Member Author

@yp969803 can you help add ut coverage for same src/dst but different direction case after this pr

@codecov
Copy link
Copy Markdown

codecov bot commented May 9, 2025

Codecov Report

Attention: Patch coverage is 53.12500% with 75 lines in your changes missing coverage. Please review.

Project coverage is 46.27%. Comparing base (15ee68c) to head (5d434e0).
Report is 47 commits behind head on main.

Files with missing lines Patch % Lines
pkg/controller/telemetry/metric.go 52.94% 68 Missing and 4 partials ⚠️
pkg/controller/telemetry/accesslog.go 57.14% 3 Missing ⚠️
Files with missing lines Coverage Δ
pkg/controller/telemetry/accesslog.go 84.84% <57.14%> (ø)
pkg/controller/telemetry/metric.go 59.61% <52.94%> (+0.15%) ⬆️

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@LiZhenCheng9527
Copy link
Copy Markdown
Contributor

/lgtm

data.conSrcDstInfo.srcPort = connectData.SrcPort
reqMetric.conSrcDstInfo.src = rawStats.SrcAddr
reqMetric.conSrcDstInfo.dst = rawStats.DstAddr
reqMetric.conSrcDstInfo.direction = rawStats.Direction
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is this the root cause of the problem? Too many renames swamping critical changes.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

right

Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@LiZhenCheng9527
Copy link
Copy Markdown
Contributor

/lgtm
/approve

@kmesh-bot kmesh-bot added the lgtm label May 9, 2025
@kmesh-bot
Copy link
Copy Markdown
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LiZhenCheng9527

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

The pull request process is described here

Details 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

@kmesh-bot kmesh-bot merged commit 410c184 into kmesh-net:main May 9, 2025
11 checks passed
@kmesh-bot
Copy link
Copy Markdown
Collaborator

In response to a cherrypick label: new pull request created: #1387

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Metric sent_bytes received_bytes not consistent with accesslog

6 participants