-
Notifications
You must be signed in to change notification settings - Fork 227
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
MGMT-19500: Enable the installation of a baremetal cluster with a user-managed load balancer, where nodes are in the same subnet, and the load balancer IP is within the subnet #7096
Conversation
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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 APPROVED This pull-request has been approved by: danmanor 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 |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
@danmanor: This pull request references MGMT-19500 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 task to target the "4.19.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. |
/hold |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7096 +/- ##
==========================================
- Coverage 67.86% 67.60% -0.27%
==========================================
Files 291 296 +5
Lines 39831 40202 +371
==========================================
+ Hits 27030 27177 +147
- Misses 10361 10580 +219
- Partials 2440 2445 +5
|
type: | ||
x-go-custom-tag: gorm:"not null;check:load_balancer_type in ('cluster-managed', 'user-managed');default:'cluster-managed'" | ||
description: | | ||
Indicates if the load balancer will be managed by the cluster or by the user. This is optional and The |
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.
Is this only to be used in "User Managed Networking" mode or does this somehow work in "cluster managed" networking, with the only exception being load balancer config?
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.
Yes
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 assume that's a "yes" to "This only works for UMN"
Do we verify that the networking is UMN when allowing this to be set?
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.
Oh no I meant this work in "cluster managed" networking, with the only exception being load balancer config
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.
Sorry
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.
What is the effect of user-managed networking + user-managed LB?
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 blocked it
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.
It is simpler to manage this feature as part of cluster-managed-networking rather then part of user-managed-networking
The title of this issue contains "....with user managed load balancer and nodes are part of the same subnet". Is there a validation performed before the installation starts to ensure that the user LB is reachable? |
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
/override ci/prow/edge-e2e-oci-assisted-4-18 ci/prow/edge-e2e-oci-assisted-4-14 |
@danmanor: Overrode contexts on behalf of danmanor: ci/prow/edge-e2e-oci-assisted-4-14, ci/prow/edge-e2e-oci-assisted-4-18 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-sigs/prow repository. |
/override ci/prow/edge-e2e-oci-assisted-4-14 |
@danmanor: Overrode contexts on behalf of danmanor: ci/prow/edge-e2e-oci-assisted-4-14 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-sigs/prow repository. |
LoadBalancerTypeClusterManaged LoadBalancerType = "ClusterManaged" | ||
|
||
// LoadBalancerTypeUserManaged is a load balancer managed outside of the cluster by the customer. When this is | ||
// used no virtual IP addresses should be specified. Note that this is only allowed for the bare metal and |
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.
Currently just baremetal (no vSphere)
@@ -126,6 +134,11 @@ func VerifyVip(hosts []*models.Host, machineNetworkCidr string, vip string, vipN | |||
if ipIsBroadcast(vip, machineNetworkCidr) { | |||
return models.VipVerificationFailed, errors.Errorf("%s <%s> is the broadcast address of machine-network-cidr <%s>", vipName, vip, machineNetworkCidr) | |||
} | |||
|
|||
if !verifyVipFree { |
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.
Consider renameing to shouldVerifyIfVipIsFree
, current name is too similar to VerifyVipFree
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.
Agree
/test edge-e2e-metal-assisted-kube-api-umlb-4-18 edge-e2e-metal-assisted-umlb-4-18 |
/override ci/prow/edge-e2e-ai-operator-disconnected-capi ci/prow/edge-e2e-nutanix-assisted-4-18 ci/prow/okd-scos-e2e-aws-ovn |
/hold |
@danmanor: Overrode contexts on behalf of danmanor: ci/prow/edge-e2e-ai-operator-disconnected-capi, ci/prow/edge-e2e-nutanix-assisted-4-18, ci/prow/okd-scos-e2e-aws-ovn 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-sigs/prow repository. |
/override ci/prow/edge-e2e-nutanix-assisted-4-14 |
@danmanor: Overrode contexts on behalf of danmanor: ci/prow/edge-e2e-nutanix-assisted-4-14 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-sigs/prow repository. |
/test edge-e2e-ai-operator-ztp |
/test edge-e2e-metal-assisted-kube-api-umlb-4-18 |
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.
Assuming that the user accepts responsibility for setting up their load balancer and that we do not need to verify the provided VIP then I think this works fine.
Do E2E tests currently make use of the API VIP in any way, if so then would it be worth the extra step of setting up a "user defined" load balancer for the sake of making sure everything still works when the VIP is not the one we have already set up in the test?
@paul-maidment I created two new tests that test this exact scenario -
They both passed, there is a small patch I need to add to |
/unhold |
/test edge-e2e-metal-assisted-kube-api-umlb-4-18 |
/lgtm |
/test edge-subsystem-kubeapi-aws |
@danmanor: The following test failed, say
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. |
6cdfd59
into
openshift:master
[ART PR BUILD NOTIFIER] Distgit: ose-agent-installer-api-server |
This patch adds support for using an user managed load balancer. It will only be available for OpenShift 4.16 or newer. To enable it the user will have to use the new
load_balancer.type
field of thecluster
type, for example:To preserve backwards compatibility this will be optional, and the
default value will be
cluster-managed
.When the value is
user-managed
the following will be added to thegenerated
install-config.yaml
file:Note that when using an user managed load balancer the VIPs will be
used to specify the load balancer IP (not actually virtual).
More info can be found here - https://docs.redhat.com/en/documentation/openshift_container_platform/4.17/html/deploying_installer-provisioned_clusters_on_bare_metal/ipi-install-installation-workflow#nw-osp-configuring-external-load-balancer_ipi-install-installation-workflow
The database will be automatically migrated so that for existing
clusters the value of the
load_balancer_type
column will automaticallybe set to
cluster-managed
.This patch adds this support only for the
baremetal
platform. a follow-up PR should implement this logic forvsphere
as well.Notes
List all the issues related to this PR
What environments does this code impact?
How was this code tested?
Checklist
docs
, README, etc)Reviewers Checklist