OPNET-678: Make VIPs optional with external loadbalancer #1803
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@cybertron: This pull request references OPNET-678 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the epic to target the "4.20.0" version, but no target version was set. 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 openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/cc @mkowalski @emy |
|
After first pass all good, I do not see anything particularly concerning |
cybertron
force-pushed
the
optional-vips-with-extlb
branch
from
9d931c1 to
429561d
Compare
|
/cc @JoelSpeed |
|
|
||
| ### Implementation Details/Notes/Constraints | ||
|
|
||
| A configuration to use this new functionality would look like the following: |
There was a problem hiding this comment.
What configuration is this? Who is creating it? What is reading it?
There was a problem hiding this comment.
This snippet comes from the install-config.yaml
There was a problem hiding this comment.
Can we clarify this in the enhancement?
| Note the lack of the apiVIPs and ingressVIPs fields, which previously would | ||
| have been mandatory with this configuration. |
There was a problem hiding this comment.
What happens if I provide only one of those fields? Is that a valid configuration? Are there any risks in place if a user manually specifies either or both of the VIPs fields and uses one of these more advanced external loadbalancers?
There was a problem hiding this comment.
if a user manually specifies [...] both of the VIPs fields and uses one of these more advanced external loadbalancers?
This is what happens today, so what we want to have is that you are no longer forced to specify VIP(s) when you use external loadbalancer
There was a problem hiding this comment.
What happens if I provide only one of those fields? Is that a valid configuration?
We should have a syntax check in place that you either provide both of them or none
There was a problem hiding this comment.
A syntax check sounds reasonable.
I'm also wondering if it would make sense to have a new loadbalancer.type field (assuming this is a discriminated union) that just removes the ability to set those options altogether?
I'm not very familiar with the existing API, so if we could also link to the existing API in the enhancement, that would be helpful for further review.
There was a problem hiding this comment.
We already do: https://github.com/openshift/installer/blob/fe225d16eef71880ba4cd8027f6f9e5b2b0d3063/pkg/types/validation/installconfig.go#L983 and https://github.com/openshift/installer/blob/fe225d16eef71880ba4cd8027f6f9e5b2b0d3063/pkg/types/validation/installconfig.go#L1034
However, after looking through that validation logic I have realized we already allow 0 VIPs, which is problematic for this proposal because it conflicts with the behavior I was planning to implement. A pretty significant re-work of this enhancement is on the way.
| This is a variation on an existing supported deployment scenario, so we don't | ||
| plan to pursue a graduation process. It will be GA from initial implementation. |
There was a problem hiding this comment.
Why should this new variation not go through the proper promotion channels before being supported?
There was a problem hiding this comment.
Honestly, we've had pretty bad luck with tech preview features for baremetal customers. They won't even try them.
However, given that I think this is going to be a more substantial change than I initially expected I'm discussing this with the field team asking for it.
@everettraven Is taking the first pass from an API review perspective, he will ping me for review once he's happy from the API review perspective |
It turns out we already supported providing zero VIPs, so we can't use that as the determining factor for this feature.
| Note the lack of the apiVIPs and ingressVIPs fields, which previously would | ||
| have been mandatory with this configuration. |
There was a problem hiding this comment.
We already do: https://github.com/openshift/installer/blob/fe225d16eef71880ba4cd8027f6f9e5b2b0d3063/pkg/types/validation/installconfig.go#L983 and https://github.com/openshift/installer/blob/fe225d16eef71880ba4cd8027f6f9e5b2b0d3063/pkg/types/validation/installconfig.go#L1034
However, after looking through that validation logic I have realized we already allow 0 VIPs, which is problematic for this proposal because it conflicts with the behavior I was planning to implement. A pretty significant re-work of this enhancement is on the way.
|
|
||
| ### Implementation Details/Notes/Constraints | ||
|
|
||
| A configuration to use this new functionality would look like the following: |
| This is a variation on an existing supported deployment scenario, so we don't | ||
| plan to pursue a graduation process. It will be GA from initial implementation. |
There was a problem hiding this comment.
Honestly, we've had pretty bad luck with tech preview features for baremetal customers. They won't even try them.
However, given that I think this is going to be a more substantial change than I initially expected I'm discussing this with the field team asking for it.
|
Inactive enhancement proposals go stale after 28d of inactivity. See https://github.com/openshift/enhancements#life-cycle for details. Mark the proposal as fresh by commenting If this proposal is safe to close now please do so with /lifecycle stale |
openshift-ci
bot
added
the
lifecycle/stale
|
@everettraven I've had some discussions with the customer that asked for this and they agreed to try it in tech preview, so I've included details about that. I also did some prototyping of the new API and modified the proposal to better reflect the OpenShift API guidelines. I have a working implementation now so if we can get the API details worked out I think we'll be able to move forward pretty quickly with this. Thanks! |
|
@cybertron: all tests passed! Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
|
/remove-lifecycle stale |
openshift-ci
bot
removed
the
lifecycle/stale
No description provided.