implicitly handle load balancers #274
Conversation
k8s-ci-robot
added
priority/important-soon
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: neolit123 If they are not already assigned, you can assign the PR to them by writing 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 |
k8s-ci-robot
added
cncf-cla: yes
There was a problem hiding this comment.
@neolit123 thanks for this PR!
I think this can help users, but I'm wondering if we should change cfg or if we should prefer to keep cfg as it is (config is config) and instead add the load balancer node to the derived list of nodes only.
Wdyt?
it would be much better to have it in the derived config, indeed. |
|
@neolit123 good point. |
ok, will update based on that value. |
This preserves load balancers as being part of the node lists, but makes it possible to handle the case where the user did not provide a LB in the config. The LB is added in the derived config. Includes minor cleanup in types.go and validate.go.
k8s-ci-robot
added
size/S
|
updated and tested. |
|
/test pull-kind-conformance-parallel-1-13 |
k8s-ci-robot
added
the
lgtm
|
This PR is somehow related to #269 as it makes the LB optional. |
|
@pablochacin that's true; i forgot about that ticket. |
| Role: config.ExternalLoadBalancerRole, | ||
| Image: d.BootStrapControlPlane().Image, // should always return non-nil in this branch | ||
| } | ||
| log.Info("Automatically creating a load balancer node") |
There was a problem hiding this comment.
Call me picky, but I would log this as "Default load balancer node added". Just to stress that the user did not provide a LB configuration
There was a problem hiding this comment.
the current message seems better to me.
also we use present continuous tense.
There was a problem hiding this comment.
No problem with tense. I wanted to stress the fact that the LB is created using default values, as it was not specified. "automatically" doesn't convey that information.
k8s-ci-robot
added
the
needs-rebase
|
@neolit123: PR needs rebase. 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. |
|
this overlaps with the plan for the next config version: |
This preserves load balancers as being part of the node lists,
but makes it possible to handle the case where the user
did not provide a LB in the config.
In such a case the function populateLoadBalancer() adds this "kind node"
automatically.
Includes minor cleanup in types.go and validate.go.
/assign @fabriziopandini @BenTheElder
/priority important-soon
/kind feature
i'm not particularly happy about this change as is, yet it's not config breaking at this point.
my goal was to get this to unblock my other PR:
#267
related to: #269
so consider this a temporary solution and we can iterate on config changes later.