-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
🐛 Delegate CAPD port selection to the container runtime #8642
🐛 Delegate CAPD port selection to the container runtime #8642
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as duplicate.
This comment was marked as duplicate.
/test pull-cluster-api-e2e-full-main |
Passing e2e full. @sbueringer @chrischdi - WDYT about merging this to see if docker is better at picking an open port for the nodes in the Cluster? |
Should be cherry picked to help with flakes on older supported branches. /cherry-pick release-1.3 |
@killianmuldoon: once the present PR merges, I will cherry-pick it on top of release-1.3 in a new PR and assign it to you. 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. |
/cherry-pick release-1.4 |
@killianmuldoon: once the present PR merges, I will cherry-pick it on top of release-1.4 in a new PR and assign it to you. 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. |
That should be way more stable 👍 /lgtm |
LGTM label has been added. Git tree hash: 74da53540a462de29945e5ec26f3bf8dd4306917
|
@@ -56,15 +54,6 @@ type nodeCreateOpts struct { | |||
|
|||
// CreateControlPlaneNode will create a new control plane container. | |||
func (m *Manager) CreateControlPlaneNode(ctx context.Context, name, image, clusterName, listenAddress string, port int32, mounts []v1alpha4.Mount, portMappings []v1alpha4.PortMapping, labels map[string]string, ipFamily clusterv1.ClusterIPFamily) (*types.Node, error) { |
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 found this comment for HostPort in the kind API:
// NOTE: if you set the special value of `-1` then the node backend
// (docker, podman...) will be left to pick the port instead.
// This is potentially useful for remote hosts, BUT it means when the container
// is restarted it will be randomized. Leave this unset to allow kind to pick it.
This got me thinking about what happens in our case when Docker picks a random port. So I tried what happens with the variant on this PR vs main.
This PR: After restart containers get a new random port
main: Ports are stable across restarts
As far as I can see we only have host ports for the haproxy and the control plane containers.
Restarting a control plane node breaks connectivity to the apiserver for a few seconds but it recovers. So that's fine.
Restarting haproxy does not break connectivity from mgmt to workload cluster (i.e. the kubeconfig we store still works (endpoint: https://172.18.0.4:6443
)). It only breaks the kubeconfig from kind get kubeconfig
(endpoint: https://0.0.0.0:32776
)
So overall this only breaks kubeconfigs from kind after haproxy restarts.
For me this tradeoff is fine.
It would be just really good to document this behavior that we now have in combination with the impact it has (as I described here). Maybe godoc comments for the CreateControlPlaneNode
and CreateExternalLoadBalancerNode
funcs. "If the port is set to 0, the host port will be picked by the container runtime and is not stable across container restarts. This has the following consequences ..."
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.
Thanks for finding that - it seems like a reasonable tradeoff, given these clusters aren't production ready and users will be able to fix the kubeconfig if they really need to.
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.
Sounds good!
Signed-off-by: killianmuldoon <kmuldoon@vmware.com>
b6e99a4
to
f726a2e
Compare
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
Just thought about if we should also add a similar note somewhere around here in quickstart:
cluster-api/docs/book/src/user/quick-start.md
Line 1359 in b474354
<aside class="note warning"> |
But OK for me without having it.
LGTM label has been added. Git tree hash: 4612b3dd8c8d2a72deb89262be3e71685b05736c
|
I think it's fine. restarting the container is an edge case |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
@killianmuldoon: new pull request created: #8654 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. |
@killianmuldoon: new pull request created: #8655 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. |
/area provider/infrastructure-docker |
Delegate port selection to the container runtime by passing 0 directly into the runtime on container creation.
Related to #8641