feat: allows users to specify host ports when creating a cluster#9892
feat: allows users to specify host ports when creating a cluster#9892
Conversation
|
Auto Cherry-pick Instructions |
|
/pick release-1.0 |
225d524 to
b7ab0ed
Compare
apis/apps/v1/types.go
Outdated
| // Therefore, it is the user's responsibility to specify all container ports that need to be bound to host ports. | ||
| // Check @cmpd.spec.hostNetwork to obtain all container ports that need to be bound. | ||
| // | ||
| // !!!!! When you specify the host ports, you must specify two additional ports for the kbagent sidecar of KB: 'http', 'streaming'. |
There was a problem hiding this comment.
@leon-inf can you please clarify why the kbagent ports must also be defined? in theory they could be randomly assigned, no?
There was a problem hiding this comment.
It may conflict with the host ports used by the database service. (It occupies the host ports assigned to the database service if it starts up first.)
There was a problem hiding this comment.
If we can remove those predefined host ports from the DefaultPortManager it wouldn't conflict, and you wouldn't have to define them manually.
Additionally, there's no actual need for the kbagent to use a HostPort, right? The database has the need of latency, but that is not true for the kbagent. Hence, if we could selectively expose only the database, the kbagent wouldn't need a hostport at all.
There was a problem hiding this comment.
What you actually expect is the node-port service. The database service can use the host ports while other services still run in the container network.
When the host-network is enabled, pods will share the host network's namespace, and there will inevitably be port conflict issues and management requirements.
There was a problem hiding this comment.
Yes, but pods sharing the host network's namespace make sense when it comes to the database service. That's the only thing that should require "manual" port management on the user side.
The other sidecars running on the same pod shouldn't by default be exposed by HostPort. Meaning that the hostPort configuration should be at the container level, not pod level.
For example:
spec:
containers:
- name: database
ports:
- containerPort: 8080
hostPort: 8080 # this will be bound to the node’s network namespace
- name: kbagent
ports:
- containerPort: 9000 # no hostPort
There was a problem hiding this comment.
I think the first one.
In my mind the first version gives me control to expose only the database container in a HostPort, while the second forces me to have all containers in the HostNetwork, which will require more ports than actually necessary.
There was a problem hiding this comment.
I see. What the system provides currently is the second one, while what you want is a completely different feature, the cost is that there will be a DNAT for the traffic of all data requests.
There was a problem hiding this comment.
I'll update this PR later to support it.
There was a problem hiding this comment.
@dudizimber Please review the updated API again to check if the behavior meets your requirements.
b7ab0ed to
03fd33c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #9892 +/- ##
==========================================
+ Coverage 50.79% 51.13% +0.34%
==========================================
Files 539 541 +2
Lines 58214 58389 +175
==========================================
+ Hits 29568 29857 +289
+ Misses 25745 25602 -143
- Partials 2901 2930 +29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
a5cbe4b to
e6d6bde
Compare
e6d6bde to
d0e11c6
Compare
21ce06c to
dedf53f
Compare
| for i, c := range synthesizedComp.PodSpec.Containers { | ||
| for j, p := range c.Ports { | ||
| if hostPort, ok := ports[p.Name]; ok { | ||
| synthesizedComp.PodSpec.Containers[i].Ports[j].HostPort = hostPort |
There was a problem hiding this comment.
Will the HostPort allocated here be conflict with the ports automatically allocated by portmanager?
There was a problem hiding this comment.
This should be guaranteed by the user. Either turn off the default port manager or assign a separate port range to it.
There was a problem hiding this comment.
Ok. Another question. What if two containers have defined ports with a same name?
There was a problem hiding this comment.
K8s requires that each named port in a Pod must have a unique name.
There was a problem hiding this comment.
This should be guaranteed by the user. Either turn off the default port manager or assign a separate port range to it.
It looks like when using hostNetwork, ports defined in hostPorts spec also won't be managed by portmanager?
cjc7373
left a comment
There was a problem hiding this comment.
I suggest mention in the API that user should be aware of the potential port conflict when using both default portmanager and hostPorts.
Other things LGTM.
|
/cherry-pick release-1.0 |
|
🤖 says: Error cherry-picking. |
|
🤖 says: |
|
/nopick |
close #9818