-
Notifications
You must be signed in to change notification settings - Fork 128
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
reorganize the edgemesh project catalog #19
Conversation
/assign @Poorunga |
@@ -111,10 +115,10 @@ clean: | |||
endif | |||
|
|||
|
|||
ARCH ?= amd64 |
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.
where is this parameter used
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.
Maybe it will be used in the future :)
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.
I think this belong to useless code and will cause confusion to others.
) | ||
|
||
func main() { | ||
command := app.NewEdgeMeshCommand() | ||
rand.Seed(time.Now().UnixNano()) |
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.
where is this rand used
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.
Maybe it will be used in the future :)
AddFunc: c.drAdd, UpdateFunc: c.drUpdate, DeleteFunc: c.drDelete}) | ||
} | ||
|
||
//func (c *controller) epAdd(obj interface{}) {} |
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.
please delete useless code
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.
fixed
agent/pkg/gateway/manager/manager.go
Outdated
) | ||
|
||
// Manager is gateway manager | ||
type Manager struct { | ||
ipArray []net.IP | ||
lock sync.RWMutex |
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 seems that sync.Mutex is enough
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.
fixed
agent/pkg/gateway/manager/manager.go
Outdated
// UpdateGateway update a gateway server | ||
func (mgr *Manager) UpdateGateway(gw *istioapi.Gateway) { | ||
mgr.lock.Lock() | ||
defer mgr.lock.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.
why do we need to lock the total function ? can we just lock the mgr.serversByGateway[key] = newGatewayServers
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.
AddGateway
can be modified as you said, but UpdaeGateway
have multiple critical sections.
/lgtm |
Init() | ||
GetSvcPorts(ip string) string | ||
GetSvcIP(svcName string) 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.
Why here need a set of interface, this interface only have one implement. in other module like dns, I found the similar code
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.
Adopt your suggestion
agent/pkg/proxy/proxy.go
Outdated
svcPorts += svcName | ||
return svcPorts | ||
} | ||
const SoOriginalDst = 80 |
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.
constants puts on the front of file looks better
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, it's more standardized like that
agent/pkg/common/modules/modules.go
Outdated
const ( | ||
EdgeDNSModuleName = "edgedns" | ||
EdgeProxyModuleName = "edgeproxy" | ||
EdgeGatewayModuleName = "edgegateway" |
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.
ModuleName and ModuleGroupName are better defined as two constants even if the two values are the same.
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.
I think we can modify it later when necessary
proxy = &EdgeProxy{Config: c} | ||
if !proxy.Config.Enable { | ||
return proxy, 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.
if !c.Enable {
return
}
proxy = &EdgeProxy{Config: c}
judge {enable} in the front of function avoid creating EdgeProxy
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 should be coded like this:
proxy = &EdgeProxy{Config: c}
if !c.Enable {
return proxy, nil
}
Otherwise, registryModules will terminate as an error.
Enable: true, | ||
ListenInterface: "docker0", | ||
ListenPort: 53, | ||
}, |
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.
EdgeDNSConfig: dnsconfig.NewEdgeDNSConfig(), EdgePorxyConfig: proxyconfig.NewEdgeProxyConfig(),
every module have self default config function may be better
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.
Adopt your suggestion
} | ||
|
||
type controller struct { | ||
svcLister listers.ServiceLister |
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.
what's the usage of svcLister, I just find the initialization code
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.
Alright, svcLister has been deleted.
IPBySvc map[string]string // key: svcName.svcNamespace, value: clusterIP | ||
} | ||
|
||
func New(ifm *informers.Manager) *controller { |
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.
I think function New should return a singleton, if other package want to use the interface of controller, now have to import it in the package proxy, and new a proxy to call it.
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.
Adopt your suggestion
func (c *controller) GetSvcIP(svcName string) string { | ||
c.RLock() | ||
defer c.RUnlock() | ||
ip := c.IPBySvc[svcName] |
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.
If controller is a singleton and you want user use function GetScvIP to get ClusterIP, then IPBySvc should be ipBySvc, SvcPortsByIP as the same
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.
Adopt your suggestion
@ZBoIsHere: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/assign @ZBoIsHere |
@ZBoIsHere: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/lgtm |
/lgtm cancel |
Signed-off-by: Poorunga <2744323@qq.com>
Signed-off-by: Poorunga <2744323@qq.com>
Signed-off-by: Poorunga <2744323@qq.com>
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED Approval requirements bypassed by manually added approval. This pull-request has been approved by: fisherxu 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 |
edgemesh will later have two parts: edgemesh-server and edgemesh-agent.
edgemesh-server is usually deployed in the cloud and acts as a relay server in the LibP2P mode or assist the agent to establish p2p hole punching. It is specifically responsible for forwarding data across subnets.
edgemesh-agent is usually deployed in the edge to act as an agent for communication between
pods
at different locations. It is internally divided into the following modules: