Skip to content

Make kmesh cni and manage controller consitent during pod enrollment #623

Merged
kmesh-bot merged 10 commits intokmesh-net:mainfrom
hzxuzhonghu:align-manage
Jul 25, 2024
Merged

Make kmesh cni and manage controller consitent during pod enrollment #623
kmesh-bot merged 10 commits intokmesh-net:mainfrom
hzxuzhonghu:align-manage

Conversation

@hzxuzhonghu
Copy link
Copy Markdown
Member

@hzxuzhonghu hzxuzhonghu commented Jul 22, 2024

What type of PR is this?

/kind enhancement

What this PR does / why we need it:

This is not just a cleanup. It fix some corner cases. Like:

  1. retrying add/delete redirection annotation should check pod existence and current status
  2. waypoint check should check the "gateway.istio.io/managed" "istio.io-mesh-controller" label instead of waypoint name
  3. Support "istio.io/dataplane-mode: none" label

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

@hzxuzhonghu
Make kmesh cni and manage controller consitent when checking enroll
385371c 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@hzxuzhonghu
Keep kmesh cni and controller management logic consistent
f4e76f8 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@kmesh-bot kmesh-bot requested review from kwb0523 and nlgwcy July 22, 2024 09:11
@hzxuzhonghu
Copy link
Copy Markdown
Member Author

hzxuzhonghu commented Jul 22, 2024

cc @weli-l to keep an eye on the manage controller
and @lec-bit on the cert manager

@hzxuzhonghu
fix lint
4790042 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@hzxuzhonghu
Add manage controller ut
8dc8796 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@hzxuzhonghu
update
98f479a 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@hzxuzhonghu
update test
1f5c921 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@codecov
Copy link
Copy Markdown

codecov bot commented Jul 23, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 68.46847% with 35 lines in your changes missing coverage. Please review.

Project coverage is 50.27%. Comparing base (aec868a) to head (b0aba42).
Report is 23 commits behind head on main.

Files Coverage Δ
pkg/utils/istio/sidecar.go 100.00% <100.00%> (ø)
pkg/controller/bypass/bypass_controller.go 39.08% <50.00%> (+1.28%) ⬆️
pkg/utils/enroll.go 93.87% <93.87%> (ø)
pkg/cni/plugin/plugin.go 0.00% <0.00%> (-8.98%) ⬇️
pkg/controller/manage/manage_controller.go 43.93% <50.00%> (ø)

... and 2 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 5dbd201...b0aba42. Read the comment docs.

@hzxuzhonghu
test coverage
244aff1 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@hzxuzhonghu hzxuzhonghu changed the title WIP: Make kmesh cni and manage controller consitent during pod enrollment Make kmesh cni and manage controller consitent during pod enrollment Jul 23, 2024
YaoZengzeng
YaoZengzeng reviewed Jul 23, 2024
View reviewed changes
YaoZengzeng
YaoZengzeng reviewed Jul 23, 2024
View reviewed changes
pkg/utils/enroll.go
@@ -0,0 +1,110 @@
package utils
Copy link
Copy Markdown
Member

@YaoZengzeng YaoZengzeng Jul 23, 2024

Choose a reason for hiding this comment

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

copyright

@hzxuzhonghu
update
cc9a829 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
YaoZengzeng
YaoZengzeng reviewed Jul 23, 2024
View reviewed changes
pkg/utils/enroll.go
)

// ShouldEnroll checks whether a pod should be managed by kmesh.
// Kmesh manages a pod if a pod has "istio.io/dataplane-mode: kmesh" label
Copy link
Copy Markdown
Member

@YaoZengzeng YaoZengzeng Jul 23, 2024

Choose a reason for hiding this comment

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

Suggested change
// Kmesh manages a pod if a pod has "istio.io/dataplane-mode: kmesh" label
// Kmesh manages a pod if a pod has "istio.io/dataplane-mode: Kmesh" label

Copy link
Copy Markdown
Member Author

@hzxuzhonghu hzxuzhonghu Jul 23, 2024

Choose a reason for hiding this comment

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

it is case insensitive

@hzxuzhonghu
add ut
130649d 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@hzxuzhonghu
fix ut
b0aba42 
Signed-off-by: Zhonghu Xu <xuzhonghu@huawei.com>
@nlgwcy
Copy link
Copy Markdown
Contributor

nlgwcy commented Jul 25, 2024

/lgtm
/approve

@kmesh-bot kmesh-bot added the lgtm label Jul 25, 2024
@kmesh-bot
Copy link
Copy Markdown
Collaborator

kmesh-bot commented Jul 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nlgwcy

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 06c2e03 into kmesh-net:main Jul 25, 2024
@hzxuzhonghu hzxuzhonghu deleted the align-manage branch July 25, 2024 06:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@YaoZengzeng YaoZengzeng YaoZengzeng left review comments

@LiZhenCheng9527 LiZhenCheng9527 LiZhenCheng9527 left review comments

@kwb0523 kwb0523 Awaiting requested review from kwb0523

@nlgwcy nlgwcy Awaiting requested review from nlgwcy

Assignees

@nlgwcy nlgwcy

Labels

approved kind/enhancement New feature or request lgtm size/XXL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@hzxuzhonghu @nlgwcy @kmesh-bot @YaoZengzeng @LiZhenCheng9527