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

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

Merged
merged 10 commits into from
Jul 25, 2024
Merged

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

merged 10 commits into from
Jul 25, 2024

Conversation

hzxuzhonghu
Copy link
Member

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
Member Author

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 Codecov
Copy link

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
pkg/utils/enroll.go
@@ -0,0 +1,110 @@
package utils
Copy link
Member

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
Member

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
Member Author

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
Contributor

nlgwcy commented Jul 25, 2024

/lgtm
/approve

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

[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

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
7 checks passed
@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