-
Notifications
You must be signed in to change notification settings - Fork 380
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
Optimize package organization to reduce antctl binary size #6037
Conversation
9abb34a
to
8735ba7
Compare
/test-all |
/test-ipv6-e2e |
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.
Very nice, I was thinking of the exact same improvement yesterday.
/test-ipv6-e2e |
@tnqn anything blocking this? |
I was waiting for other large PRs to merge to avoid their rework, e.g. NetworkPolicyEvaluation, K8s lib upgrade. |
e1ce909
to
3c5f96c
Compare
/test-all |
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
pkg/util/k8s/name.go
Outdated
// GetServiceDNSNames returns the DNS names that can be used to access the given Service. | ||
func GetServiceDNSNames(namespace, serviceName string) []string { | ||
antreaServerName := serviceName + "." + namespace + ".svc" | ||
// TODO: Although antrea-agent and kube-aggregator only verify the server name "antrea.<Namespace>.svc", | ||
// We should add the whole FQDN "antrea.<Namespace>.svc.<Cluster Domain>" as an alternate DNS name when | ||
// other clients need to access it directly with that name. | ||
return []string{antreaServerName} | ||
} |
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.
The location in this package and the name of the function make it sound like it is "generic", but then the variable name antreaServerName
and the TODO comment kind of make it Antrea-specific.
It's kind of a minor comment though. Maybe we could just rename the variable to something more appropriate.
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.
Good catch. I missed updating it when moving the function.
Updated.
As a client, antctl needs to know some well-known resource names and the structs of the API responses. However, the code placement makes antctl import a lot of packages which are supposed to be consumed by server side code, e.g. antrea-controller, antrea-agent. This patch refactors relevant packages, extracts well-known resources names and API structs to separate common packages, mostly apis packages. It reduces antctl binary size by 10MB, from 55MB to 45MB. The placement also makes it more explicit that these constants and structs are API related and should be changed more carefully. The major changes are as follows: 1. Move well-known CRD resource names to types.go files which declare these CRD structs. 2. Move other well-known resource names to package pkg/apis. 3. Move structs of API responses to package apis of components which provide the corresponding APIs. Signed-off-by: Quan Tian <[email protected]>
3c5f96c
to
4078c98
Compare
/test-all |
As a client, antctl needs to know some well-known resource names and the structs of the API responses. However, the code placement makes antctl import a lot of packages which are supposed to be consumed by server side code, e.g. antrea-controller, antrea-agent.
This patch refactors relevant packages, extracts well-known resources names and API structs to separate common packages, mostly apis packages. It reduces antctl binary size by 10MB, from 55MB to 45MB. The placement also makes it more explicit that these constants and structs are API related and should be changed more carefully.
The major changes are as follows:
types.go
files which declare these CRD structs.pkg/apis
.apis
of components which provide the corresponding APIs.For #5883
Before:
After: