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 ability to set default vhost limits by pattern #6172

Merged
merged 1 commit into from
Oct 20, 2022

Conversation

illotum
Copy link

@illotum illotum commented Oct 18, 2022

Proposed Changes

A fairly straightforward implementation of #4999. Per ticket, vhost limit defaults can be set up with regex patterns in config, and the first matching is applied during vhost creation. Defaults come from a file and are not synced between cluster members.

Unlike the ticket, I do not allow negative limits in favor of omission.

Types of Changes

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)
  • Build system and/or CI

Checklist

Further Comments

This change is not documented, nor added to release-notes. LMK if this needs to be addressed.

Before I'm happy with it, several concerns and questions:

  • I'll admit I'm confused about the two places to store VHost limits: the record and the runtime params. Is one deprecated?
  • In my manual tests validators didn't error nor log anything on bad config inputs. Any advice?

@illotum
Copy link
Author

illotum commented Oct 18, 2022

Right, I see I failed to follow the breadcrumbs deep enough. Let me fix the snippets.

@illotum
Copy link
Author

illotum commented Oct 19, 2022

This time it feels like flaky tests.

deps/rabbit/priv/schema/rabbit.schema Outdated Show resolved Hide resolved
deps/rabbit/src/rabbit_vhost.erl Outdated Show resolved Hide resolved
deps/rabbit/test/config_schema_SUITE_data/rabbit.snippets Outdated Show resolved Hide resolved
@michaelklishin
Copy link
Member

michaelklishin commented Oct 19, 2022

@rin and I have noticed a genuine failure on this branch:

bazel test --config=local //deps/rabbitmq_management:rabbit_mgmt_rabbitmqadmin_SUITE

fails with

=== Ended at 2022-10-19 08:47:05
=== Location: [{rabbit_mgmt_rabbitmqadmin_SUITE,parameters,319]},
              {test_server,ts_tc,1782},
              {test_server,run_test_case_eval1,1291},
              {test_server,run_test_case_eval,1223}]
=== === Reason: no match of right hand side value 
                 {ok,[["test","good","/","123"],
                      ["vhost-limits","limits","/"]]}
  in function  rabbit_mgmt_rabbitmqadmin_SUITE:parameters/1 (rabbit_mgmt_rabbitmqadmin_SUITE.erl, line 319)

so the HTTP API response must have changed but the test hasn't.

@michaelklishin
Copy link
Member

There is also a CLI suite test failure:

bazel test --config=local //deps/rabbitmq_cli:all
SetParameterCommandTest [test/ctl/set_parameter_command_test.exs]
  * test banner [L#133]
  * test banner (0.4ms) [L#133]
  * test run: an invalid component_name returns a validation failed error [L#88]
  * test run: an invalid component_name returns a validation failed error (1.8ms) [L#88]

  1) test run: an invalid component_name returns a validation failed error (SetParameterCommandTest)
     test/ctl/set_parameter_command_test.exs:88
     Assertion with == failed
     code:  assert list_parameters(context[:vhost]) == []
     left:  [[component: "vhost-limits", name: "limits", value: "[]"]]
     right: []
     stacktrace:
       test/ctl/set_parameter_command_test.exs:96: (test)

--max-failures reached, aborting test suite
Finished in 228.4 seconds (14.6s async, 213.7s sync)
642 tests, 1 failure

Limits are defined in the instance config:

    default_limits.vhosts.1.pattern = ^device
    default_limits.vhosts.1.max_connections = 10
    default_limits.vhosts.1.max_queues = 10

    default_limits.vhosts.2.pattern = ^system
    default_limits.vhosts.2.max_connections = 100

    default_limits.vhosts.3.pattern = .*
    default_limits.vhosts.3.max_connections = 20
    default_limits.vhosts.3.max_queues = 20

Where pattern is a regular expression used to match limits to a newly
created vhost, and the limits are non-negative integers. First matching
set of limits is applied, only once, during vhost creation.
@illotum
Copy link
Author

illotum commented Oct 19, 2022

Addressed comments, squashed & rebased.

@michaelklishin
Copy link
Member

The example in the commit uses default_limits.vhosts. but the actual schema uses default_limits.vhost. which has resulted some head scratching on my end.

I will rename "vhosts" to be plural because this is a collection.

@michaelklishin
Copy link
Member

I have also noticed that if there are multiple patterns that match, the effective one is picked undeterministically. I suspect that it depends on collection iteration ordering. There are
other places in RabbitMQ that works this way, namely policies with equal priorities.

@illotum
Copy link
Author

illotum commented Oct 19, 2022

I could sort them, e.g. by the pattern hash, if you think it's worth it.

I feel the config file mismatch (between instances) makes this a minor issue by comparison.

@illotum
Copy link
Author

illotum commented Oct 19, 2022

I had two extra questions in the description: most importantly, I was able to boot with config values that violate cuttlefish validators. Should I file a separate issue?

@michaelklishin
Copy link
Member

If you can reproduce this outside of RabbitMQ, then sure. Cuttlefish is at Kyorai/cuttlefish.

@michaelklishin michaelklishin merged commit 27ebc04 into rabbitmq:main Oct 20, 2022
mergify bot pushed a commit that referenced this pull request Oct 20, 2022
References #6172

(cherry picked from commit 9192482)
@illotum illotum deleted the vhost-defaults branch October 20, 2022 04:15
michaelklishin added a commit that referenced this pull request Nov 2, 2022
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