Skip to content
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

Add unsupported config override for config manager #628

Conversation

Miciah
Copy link
Contributor

@Miciah Miciah commented Jun 23, 2021

Add an unsupported config override to enable OpenShift router's dynamic config manager for HAProxy.

  • pkg/operator/controller/ingress/deployment.go (RouterHAProxyConfigManager): New const.
    (desiredRouterDeployment): Add unsupported config override for ROUTER_HAPROXY_CONFIG_MANAGER.
  • pkg/operator/controller/ingress/deployment_test.go (TestDesiredRouterDeployment): Verify that desiredRouterDeployment sets ROUTER_HAPROXY_CONFIG_MANAGER as expected.
  • test/e2e/operator_test.go (TestDynamicConfigManagerUnsupportedConfigOverride): Verify that the operator sets ROUTER_HAPROXY_CONFIG_MANAGER on a router deployment for an IngressController that enables the unsupported config override.

@openshift-ci openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 23, 2021
@Miciah Miciah force-pushed the add-unsupported-config-override-for-config-manager branch from b5601bd to 74cec30 Compare July 19, 2021 14:20
@Miciah
Copy link
Contributor Author

Miciah commented Jul 19, 2021

Rebased.

@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 19, 2021
@Miciah Miciah changed the title WIP: Add unsupported config override for config manager Add unsupported config override for config manager Jul 19, 2021
t.Fatalf("failed to update ingresscontroller: %v", err)
}
if err := waitForDeploymentEnvVar(t, kclient, deployment, 1*time.Minute, "ROUTER_HAPROXY_CONFIG_MANAGER", "true"); err != nil {
t.Fatalf("expected initial deployment to set ROUTER_HAPROXY_CONFIG_MANAGER=true: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"expected updated deployment"?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks!

Add an unsupported config override to enable OpenShift router's dynamic
config manager for HAProxy.

* pkg/operator/controller/ingress/deployment.go
(RouterHAProxyConfigManager): New const.
(desiredRouterDeployment): Add unsupported config override for
ROUTER_HAPROXY_CONFIG_MANAGER.
* pkg/operator/controller/ingress/deployment_test.go
(TestDesiredRouterDeployment): Verify that desiredRouterDeployment sets
ROUTER_HAPROXY_CONFIG_MANAGER as expected.
* test/e2e/operator_test.go
(TestDynamicConfigManagerUnsupportedConfigOverride): Verify that the
operator sets ROUTER_HAPROXY_CONFIG_MANAGER on a router deployment for an
IngressController that enables the unsupported config override.
@Miciah Miciah force-pushed the add-unsupported-config-override-for-config-manager branch from 74cec30 to 6a8516a Compare July 19, 2021 15:47
@frobware
Copy link
Contributor

/lgtm

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: frobware, Miciah

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2021
@openshift-merge-robot openshift-merge-robot merged commit df87fb7 into openshift:master Jul 19, 2021
@showuon
Copy link

showuon commented Jan 13, 2023

@Miciah , we're wondering for this feature, other than this being explicitly unsupported, are the any drawbacks as to why we should not consider using this a way to address our issues with connection termination? And when it will be officially supported? Thanks.

@showuon
Copy link

showuon commented Jan 17, 2023

@Miciah @frobware , could you help answer the questions above when you're available? Since we're planning to adopt this dynamic config manager feature in our production environment, we'd really like to get your suggestions about it. Thank you!

@frobware
Copy link
Contributor

@Miciah , we're wondering for this feature, other than this being explicitly unsupported, are the any drawbacks as to why we should not consider using this a way to address our issues with connection termination? And when it will be officially supported? Thanks.

Enabling the override is not currently tested either manually or in any CI jobs that we have/use. I cannot vouch for its current behaviour.

@showuon
Copy link

showuon commented Jan 17, 2023

@frobware , thanks for the reply. So do you have plan when this feature will be "supported"?

@frobware
Copy link
Contributor

@showuon the RFE is tracked here: https://issues.redhat.com/browse/RFE-1439. Notably the spike https://issues.redhat.com/browse/NE-870 is still to be done. I don't have any answer for you as to "when'.

@showuon
Copy link

showuon commented Jan 17, 2023

Thank you for the comments. I see. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants