-
Notifications
You must be signed in to change notification settings - Fork 91
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
handle hostname type waypoint in workload #1021
Conversation
d0c33bf
to
6d6f71e
Compare
Codecov ReportAttention: Patch coverage is
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
/retest |
@hzxuzhonghu PTAL |
return nil, nil | ||
} | ||
|
||
type associatedObjects struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
type associatedObjects struct { | |
type waypointAssociatedObjects struct { |
} | ||
|
||
func (w *associatedObjects) isResolved() bool { | ||
return w.address != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
donot you need to protect address?
// AddOrUpdateWorkload add or update workload in this cache, return true if the | ||
// workload's waypoint doesn't need to be resolved or resolved successfully. | ||
AddOrUpateWorkload(workload *workloadapi.Workload) bool | ||
DeleteWorkload(uid string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And they are duplicate with workload cache
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it's a tradeoff and duplicating a little code now maybe better than making further abstractions.
// Anyway, add svc to the association list. | ||
w.waypointAssociatedObjects[waypointResourceName].addService(resourceName, svc) | ||
|
||
return ret |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add ut coverage for this branch
} | ||
|
||
log.Debugf("Refreshing services associated with waypoint %s", resourceName) | ||
return associated.update(address) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add ut coverage for this
@YaoZengzeng please add some ut, then we can merge |
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
Signed-off-by: YaoZengzeng <yaozengzeng@huawei.com>
e5b1dda
to
b67b286
Compare
config/kmesh_marcos_def.h
Outdated
@@ -14,15 +14,15 @@ | |||
* and sidecar containers. The bpf_sk_original_addr auxiliary function is | |||
* used to obtain the original destination address. | |||
*/ | |||
#define MDA_NAT_ACCEL 1 | |||
#define MDA_NAT_ACCEL 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
retract
|
||
func (w *waypointCache) AddOrUpdateService(svc *workloadapi.Service) bool { | ||
w.mutex.Lock() | ||
defer w.mutex.Unlock() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need to lock the whole function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK, we should make it easy at first. If we find this is a performance bottleneck later, we can further optimize it and make more precise control.
@hzxuzhonghu updated |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hzxuzhonghu 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 |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Starting from istio 1.24, waypoint supports the address type of hostname and Kmesh need to support it.
Supplement to PR #995
Which issue(s) this PR fixes:
Fixes part of #984
Special notes for your reviewer:
Does this PR introduce a user-facing change?: