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.

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

Istio/XDS support #1804

Merged
merged 21 commits into from Apr 6, 2022
Merged

Istio/XDS support #1804

merged 21 commits into from Apr 6, 2022

Conversation

LaurenceLiZhixin
Copy link
Contributor

@LaurenceLiZhixin LaurenceLiZhixin commented Mar 31, 2022

What this PR does:

Added Istio/XDS support

  • Added isTIod-based service discovery
  • Added XDS circuit breaker
  • Added basic isTIO-RDS routing rules and traffic governance capabilities

We Refered xdsClient impl of gRPC, and retained copy right of gRPC developers.

增加了Istio/XDS 支持

  • 增加了基于istiod 的服务发现
  • 增加了xds circuit breaker
  • 增加了基本的适配于 istio-rds 的路由规则和流量治理能力

我们引用了gRPC的xdsClient impl,并保留了gRPC开发者的版权。

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

To Developers

  • Developers are recommended to use xds feature with dubbogo-cli, which would be release soon.
  • Samples and more add-on features of sevice mesh ecology are welcomend for contributers.

You should pay attention to items below to ensure your pr passes our ci test
We do not merge pr with ci tests failed

  • All ut passed (run 'go test ./...' in project root)
  • After go-fmt ed , run 'go fmt project' using goland.
  • Golangci-lint passed, run 'sudo golangci-lint run' in project root.
  • After import formatted, (using imports-formatter to run 'imports-formatter .' in project root, to format your import blocks, mentioned in CONTRIBUTING.md above)
  • Your new-created file needs to have apache license at the top, like other existed file does.
  • All integration test passed. You can run integration test locally (with docker env). Clone our dubbo-go-samples project and replace the go.mod to your dubbo-go, and run 'sudo sh start_integration_test.sh' at root of samples project root. (M1 Slice is not Support)

@@ -14,7 +14,7 @@ jobs:
# If you want to matrix build , you can append the following list.
matrix:
go_version:
- 1.15
- 1.17
Copy link
Contributor

Choose a reason for hiding this comment

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

我们要升级这个版本吗

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我认为升级吧。
1.15 版本不支持一些基础函数,比如os.ReadFile
开发者本地如果使用高版本,ci的时候会报错
@zhaoyunxing92

Copy link
Contributor

Choose a reason for hiding this comment

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

嗯嗯

Copy link
Contributor

Choose a reason for hiding this comment

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

As we discussed yesterday night, pls recover the go version.

@codecov-commenter
Copy link

codecov-commenter commented Apr 1, 2022

Codecov Report

Merging #1804 (e41ea18) into 3.0 (c12c1d6) will decrease coverage by 0.40%.
The diff coverage is 34.85%.

❗ Current head e41ea18 differs from pull request most recent head 53a86dd. Consider uploading reports for the commit 53a86dd to get more accurate results

@@            Coverage Diff             @@
##              3.0    #1804      +/-   ##
==========================================
- Coverage   47.17%   46.76%   -0.41%     
==========================================
  Files         298      298              
  Lines       17401    17157     -244     
==========================================
- Hits         8209     8024     -185     
+ Misses       8339     8288      -51     
+ Partials      853      845       -8     
Impacted Files Coverage Δ
common/url.go 59.31% <0.00%> (-1.36%) ⬇️
protocol/dubbo3/dubbo3_invoker.go 55.65% <ø> (+0.47%) ⬆️
protocol/invocation/rpcinvocation.go 12.92% <0.00%> (-2.87%) ⬇️
remoting/xds/xds_client_factory.go 0.00% <0.00%> (ø)
xds/balancer/clusterimpl/logging.go 0.00% <0.00%> (ø)
xds/balancer/clusterimpl/picker.go 0.00% <0.00%> (ø)
xds/balancer/priority/balancer_child.go 0.00% <0.00%> (ø)
xds/balancer/priority/balancer_priority.go 0.00% <0.00%> (ø)
xds/balancer/priority/ignore_resolve_now.go 0.00% <0.00%> (ø)
xds/balancer/priority/logging.go 0.00% <0.00%> (ø)
... and 42 more

Continue to review full report at Codecov.

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

ctx := invocation.ToContext()
matcher, err := resource.RouteToMatcher(r)
if err != nil {
panic(err)
Copy link
Contributor

Choose a reason for hiding this comment

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

panic 不能随便用,这里确实有必要使用 panic 吗?

go.mod Outdated
@@ -1,6 +1,6 @@
module dubbo.apache.org/dubbo-go/v3

go 1.15
go 1.17
Copy link
Contributor

Choose a reason for hiding this comment

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

欢迎 go 版本。

go.mod Outdated
google.golang.org/grpc v1.45.0
google.golang.org/protobuf v1.28.0
gopkg.in/yaml.v2 v2.4.0
k8s.io/apimachinery v0.22.4
k8s.io/client-go v0.16.9
)

require (
Copy link
Contributor

Choose a reason for hiding this comment

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

加这么多 require 块作甚?

Copy link
Contributor

Choose a reason for hiding this comment

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

合并成一个吧。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

这个是go 1.17的特性,切回去go版本就好了。

@@ -57,4 +58,6 @@ type Invocation interface {
SetAttribute(key string, value interface{})
GetAttribute(key string) (interface{}, bool)
GetAttributeWithDefaultValue(key string, defaultValue interface{}) interface{}

ToContext() context.Context
Copy link
Contributor

Choose a reason for hiding this comment

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

GetContext() may be better?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed to 'GetAttachmentAsContext', which is more clear.

}

func isProvider(url *common.URL) bool {
return getCategory(url) == "providers"
Copy link
Contributor

Choose a reason for hiding this comment

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

"providers" 在 如下地方有定义:
./common/constant/default.go:77: ProviderCategory = "providers"

不要在这里直接使用字符串。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

"strings"
)

type Addr struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

我反对你重新封装这么一个 struct, net.Addr 不能用吗?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

官方库的net.Addr只是一个接口,并不是具体类型。
第三房库也有类似的实现,我认为不如自己定义一个用的方便和清晰。 @AlexStocks

Copy link
Contributor

Choose a reason for hiding this comment

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

1 继承 Addr 接口
2 看是否有必要放入 common

Port string
}

func NewAddr(addr string) Addr {
Copy link
Contributor

Choose a reason for hiding this comment

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

net. 包内部有 net.SplitHostPort

}
}

func (a *Addr) String() string {
Copy link
Contributor

Choose a reason for hiding this comment

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

net 包内部有 net.JoinHostPort

从上面分析来看,你没必要封装这么一个东西。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

我认为还是需要封装一个自己的Addr,用来以字符串形式存储“地址 + 端口”
官方提供的net.TCPAddr, 其ip必须是真实的ip,并不是字符串封装的地址例如"istiod.istio-system.svc.cluster.local",不能在我的场景使用。
上面几个评论,使用官方库来进行地址和ip+port的转换,这个我会修复。
但我还是希望保留这个Addr 对象。 @AlexStocks

Copy link
Contributor

Choose a reason for hiding this comment

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

那你把名字清晰化,譬如取名 IstioXDSAddr 之类的

* limitations under the License.
*/

package interfaceMapping
Copy link
Contributor

Choose a reason for hiding this comment

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

interfaceMapping 这个 package name 是从 xds 里面搞过来的吗?如果不是,直接改用 mapping 就可以了。

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

func (i *InterfaceMapHandlerImpl) GetHostAddrMap(serviceUniqueKey string) (string, error) {
i.interfaceNameHostAddrMapLock.RLock()
if hostAddr, ok := i.interfaceNameHostAddrMap[serviceUniqueKey]; ok {
return hostAddr, nil
Copy link
Contributor

Choose a reason for hiding this comment

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

OMG,deadlock

if str, ok := v.([]string); ok {
gRPCMD.Set(k, str...)
continue
}
Copy link
Contributor

Choose a reason for hiding this comment

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

这里如果是其他类型,是不是得几个错误 log?

@@ -312,9 +314,28 @@ func (dir *RegistryDirectory) toGroupInvokers() []protocol.Invoker {
return groupInvokersList
}

func (dir *RegistryDirectory) uncacheInvokerWithClusterId(clusterId string) []protocol.Invoker {
Copy link
Contributor

Choose a reason for hiding this comment

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

ClusterId --> ClusterID

Copy link
Contributor

Choose a reason for hiding this comment

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

函数名称和变量名称都改下

*
*/

package clusterresolver
Copy link
Contributor

Choose a reason for hiding this comment

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

clusterresolver 这里的 package 和 路径名字改为 resolver 如何?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Codes in xds/* are refered form grpc @AlexStocks


"google.golang.org/grpc/resolver"
)

Copy link
Contributor

Choose a reason for hiding this comment

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

下面这个 struct 还有本文件名称命名很奇怪,Resolve 是个动词。文件名称里面带有 now,这很奇怪。你是要表达什么意思?详细说下,我给你换个名字。

@zhaoyunxing92 zhaoyunxing92 merged commit b84027f into apache:3.0 Apr 6, 2022
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.

None yet

5 participants