[AWS] Add LB Type in the infrastructure cluster object via install-config yaml#6478
Conversation
|
/assign r4f4 |
|
I just removed the vendoring and openshift/api bump as the openshift/api version in master ( |
pkg/types/aws/platform.go
Outdated
There was a problem hiding this comment.
Classic will be created
Is there any blocker to use NLB as default?
There was a problem hiding this comment.
Yup - it's documented here Shall we use NLB for default ingress controller ?
Specifically, we have this BZ https://bugzilla.redhat.com/show_bug.cgi?id=2023681 to figure out before going default.
pkg/asset/manifests/ingress.go
Outdated
There was a problem hiding this comment.
Nit: this line should be in the installer imports block.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: r4f4 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
There was a problem hiding this comment.
Nit: Some double spaces in the description fileld.
There was a problem hiding this comment.
Hmm I see that, but I think this is generated from https://github.com/openshift/installer/pull/6478/files#diff-7941d8da46384fc43b6eeae7d5c3319065b16940457822346c9ccb08cbcf47b4R66, which uses a hanging indent and a bulleted list.
Should I remove the hanging indent and extra spaces from there?
There was a problem hiding this comment.
I propose we do that as a follow-up.
|
/lgtm |
In the ingress config cluster object we need to add api for AWS LBType which will store and persist the information of the desired LB type mentioned by the installer config API so that the ingress operator will refer the LB type from this object whenever someone accidentally deletes the ingress controller. Currently, the default LB type i.e CLB/ELB gets set if you delete the ingress controller object even when the desired the LB type for default ingress controller was set to NLB as ingress operator does not have a way to check the LB type as we don't store it. This commit adds LB Type for AWS mentioned in the install-config.yaml in the ingress cluster object. `data/data/install.openshift.io_installconfigs.yaml` A generated new field called lbType will be available to add load balancer type in the install-config.yaml for AWS `pkg/asset/manifests/ingress.go` The ingress cluster object will be patched with this lbType provided by user so that we have the information related to the lbType persistent which can be referred by ingress operator when the default ingress controller is deleted. `pkg/asset/manifests/ingress_test.go` Unit test to test setting of lbType in the ingress cluster object's spec and status as per the value specified in the install-config.yaml `pkg/explain/printer_test.go` A new field to test called lbType which will be available to add the load balancer type in the install-config.yaml for AWS `pkg/types/aws/platform.go` A new field called lbType will be available to add the load balancer type in the install-config.yaml for AWS Enhacement proposal - openshift/enhancements#1148 OpenShift API - openshift/api#1209 Fixes Jira ticket - https://issues.redhat.com/browse/NE-942 Modified-by: Grant Spence <gspence@redhat.com>
|
@sadasu Just modified the commit message to include my name in |
|
/lgtm |
|
/retest |
|
/override ci/prow/e2e-vsphere-ovn |
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-vsphere-ovn DetailsIn 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. |
|
/test e2e-vsphere-ovn |
|
@r4f4 @sadasu @jhixson74 @mtulio @patrickdillon can we get this merged ASAP. We want this to get merged by today. This is a critical epic after having discussed with the PM and stakeholders. |
|
/test e2e-vsphere-ovn |
|
/override ci/prow/e2e-vsphere-ovn |
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-vsphere-ovn DetailsIn 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. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/skip |
|
/override ci/prow/e2e-aws-ovn ci/prow/e2e-azure-ovn |
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-aws-ovn, ci/prow/e2e-azure-ovn DetailsIn 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. |
|
/skip |
|
/hold Revision 5d12adc was retested 3 times: holding |
openshift-ci
bot
added
the
do-not-merge/hold
|
@gcs278: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. I understand the commands that are listed here. |
|
/override ci/prow/e2e-aws-ovn ci/prow/e2e-vsphere-ovn |
|
@r4f4: Overrode contexts on behalf of r4f4: ci/prow/e2e-aws-ovn, ci/prow/e2e-vsphere-ovn DetailsIn 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. |
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
In the loadBalancer:
platform:
aws: {}
type: AWSWhereas |
|
Yup, |
8 participants
Note: PR cloned from #6074 so I could modify. This is @miheer's commit.
In the ingress config cluster object we need to add api for AwsLBType which will store and persist the information of the desired LB type mentioned by the installer config API so that the ingress operator will refer the LB type from this object whenever someone accidentally deletes the ingress controller.
Currently, the default LB type i.e CLB/ELB gets set if you delete the ingress controller object even when the desired the LB type for default ingress controller
was set to NLB as ingress operator does not have a way to check the LB type as we don't store it.
This commit adds LB Type for AWS mentioned in the install-config.yaml in the infrastructure cluster object.
data/data/install.openshift.io_installconfigs.yamlA generated new field called lbType will be available to add load balancer type in the install-config.yaml for AWSpkg/asset/manifests/ingress.goThe ingress cluster object will be patched with this lbType provided by user so that we have the information related to the lbType persistent which can be referred by ingress operator when the default ingress controller is deleted.pkg/asset/manifests/ingress_test.goUnit test to test setting of lbType in the ingress cluster object's spec and status as per the value specified in the install-config.yamlpkg/explain/printer_test.goA new field to test called lbType which will be available to add the load balancer type in the install-config.yaml for AWSpkg/types/aws/platform.goA new field called lbType will be available to add the load balancer type in the install-config.yaml for AWSEnhacement proposal - openshift/enhancements#1148
OpenShift API - openshift/api#1209
Jira ticket - https://issues.redhat.com/browse/NE-942