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

[WIP] HAProxy config check #640

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gcs278
Copy link
Contributor

@gcs278 gcs278 commented Nov 27, 2024

Swap TemplatePlugin and StatusAdmitter ordering so that StatusAdmitter is the last plugin so that the TemplatePlugin can reject a route based on whether HAProxy rejects the configuration.

When TemplatePlugin adds a route, it also writes the config file without reloading, and runs the configuration check script. If the configuration check script fails with the new route, the route will be removed from the state and it will be rejected.

Example failure for https://issues.redhat.com//browse/OCPBUGS-27741 looks like:

status:
  ingress:
  - conditions:
    - lastTransitionTime: "2024-11-27T16:40:59Z"
      message: Failed to validate HAProxy config with route. Review the openshift-router
        logs for more details on the validation failure.
      reason: HAProxyCheckConfigFailed
      status: "False"
      type: Admitted
    host: route-service1-default.apps.gspence-2024-11-27-0943.devcluster.openshift.com
    routerCanonicalHostname: router-default.apps.gspence-2024-11-27-0943.devcluster.openshift.com
    routerName: default
    wildcardPolicy: None

WIP:

  • Additional unit testing coverage
  • Test & confirm that writing the config before reloading is OKAY
  • Research impact of swapping StatusAdmitter and TemplatePlugin ordering
  • Understand performance impact of running haproxy -c on every route add or change.

Copy link
Contributor

openshift-ci bot commented Nov 27, 2024

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 27, 2024
Copy link
Contributor

openshift-ci bot commented Nov 27, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from gcs278. For more information see the Kubernetes Code Review Process.

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

@gcs278 gcs278 force-pushed the haproxy-config-check branch 8 times, most recently from e7ffdb8 to 8d03c79 Compare November 27, 2024 19:55
Swap TemplatePlugin and StatusAdmitter ordering so that StatusAdmitter
is the last plugin so that the TemplatePlugin can reject a route based
on whether HAProxy rejects the configuration.

When TemplatePlugin adds a route, it also writes the config file without
reloading, and runs the configuration check script. If the configuration
check script fails with the new route, the route will be removed from the
state and it will be rejected.
@gcs278 gcs278 force-pushed the haproxy-config-check branch from 8d03c79 to 4980806 Compare November 27, 2024 20:04
@gcs278 gcs278 marked this pull request as ready for review November 27, 2024 20:16
@openshift-ci openshift-ci bot requested review from candita and frobware November 27, 2024 20:17
Copy link
Contributor

openshift-ci bot commented Nov 28, 2024

@gcs278: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-metal-ipi-ovn-ipv6 4980806 link false /test e2e-metal-ipi-ovn-ipv6
ci/prow/okd-scos-e2e-aws-ovn 4980806 link false /test okd-scos-e2e-aws-ovn
ci/prow/e2e-agnostic 4980806 link true /test e2e-agnostic
ci/prow/e2e-upgrade 4980806 link true /test e2e-upgrade
ci/prow/e2e-aws-serial 4980806 link true /test e2e-aws-serial

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant