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

feat: regex matching for scaler requirements #433

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jamesstocktonj1
Copy link

Feature or Problem

This features allows for scaler configurations to match a regex expression rather than the exact label value. All spread requirements will now be treated as regex with (most) existing wadm files being backwards compatible in matching the labels. This gives more flexible deployment and can be used in addition to leaf nodes to keep traffic within the same area.

Related Issues

Consumer Impact

  • escape characters may need to be used with hosts which contain regex-specific characters (e.g. *, +, ? ...). For example to match the label star-* you would need the regex star-\*

Unit Test(s)

  • existing unit tests remain untouched to ensure backwards compatability
  • additional unit tests for both component and provider
  • additional e2e test for regex matching
  • additional serialisation and deserialisation tests

Manual Verification

  • various wadm files tested on a simple 4 host setup

@jamesstocktonj1 jamesstocktonj1 requested a review from a team as a code owner September 28, 2024 21:48
@brooksmtownsend
Copy link
Member

Hey @jamesstocktonj1 ! I've been chatting it over with some of the other wadm maintainers, and I think there's a general concern with Regex being implemented as-is here today and how long we could support that feature. I wanted to give you a heads up that we're likely going to ask for a bit of a refactor in this PR so that the Regex functionality can fit more into a model like the Nomad constraint feature, for example:

        - type: spreadscaler
          properties:
            instances: 1
            constraints:
              - operation: "=="
                label: "foo"
                value: "bar"
              - operation: "regex"
                attribute: "${labels.foo}"
                value: ".*"

This would be a holistic improvement to our spread logic and isn't a fault of your PR. I didn't want to request changes just yet because the maintainers need to get together and decide on the right structure before we ask you to change anything, but I wanted to give you an update. Do you have any thoughts/concerns/questions for right now?

Thank you so much for taking the initiative to implement this + add tests to verify that it works, this is really a fantastic contribution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants