Skip to content

Conversation

@dumbbell
Copy link
Collaborator

@dumbbell dumbbell commented Jan 5, 2023

This patch makes the v2 code path the only code path. Therefore the feature_flags_v2 feature flag is marked as required. At the same time, we remove the v1 code path because it is dead code after the first part.

For end users, it means they will have to upgrade from RabbitMQ 3.10 (or older) to RabbitMQ 3.11 first, before they can upgrade to whatever RabbitMQ 3.12.0+. Of course rolling upgrades are still supported in the process.

After making feature flags v2 required, the patch does a lot of code cleanup:

  • It removes all the v1-related code
  • It removes support for migration_fun migration functions; only callbacks is supported now
  • It drops code to handle errors with upgrade from pre-3.7.x.

It depends on #6682.

@dumbbell dumbbell self-assigned this Jan 5, 2023
@mergify mergify bot added the bazel label Jan 5, 2023
@dumbbell dumbbell force-pushed the make-feature_flags_v2-required branch 3 times, most recently from 3a3c278 to 4f5b6e6 Compare January 9, 2023 13:24
@dumbbell dumbbell marked this pull request as ready for review January 9, 2023 13:41
@dumbbell dumbbell force-pushed the make-feature_flags_v2-required branch 2 times, most recently from 4d770cb to af686f4 Compare January 9, 2023 20:44
@dumbbell dumbbell marked this pull request as draft January 10, 2023 11:37
This means users of RabbitMQ 3.10.x or older will have to upgrade to
RabbitMQ 3.11.x and enable `feature_flags_v2` before they can upgrade to
a more recent release.
`feature_flags_v2` is required now, so we can drop the old code path.
…ntory/0`

With the way the registry is used in the feature flags controller, the
registry was never initialized if `feature_flags_v2` was enabled
out-of-the-box (i.e. once marked as required).

The bug was discovered after marking `feature_flags_v2` as required and
removing the v1 code: the `feature_flags_v2_SUITE` testsuite failed
because the controller got an empty inventory when it called
`rabbit_ff_registry:inventory()`.
@dumbbell dumbbell force-pushed the make-feature_flags_v2-required branch from af686f4 to f5eabc6 Compare January 10, 2023 12:52
@dumbbell dumbbell added this to the 3.12.0 milestone Jan 10, 2023
All existing feature flags either use v2 callbacks or don't have any
associated migration code. Future flags will have to use v2 callbacks if
they need to.

v1 migration functions were deprecated with the introduction of the
feature flags controller in RabbitMQ 3.11.0. Removing support for v1
migration functions simplifies the code.
We used it to prevent a node breakage when RabbitMQ was updated from
e.g. 3.6.x to 3.7.0 when the package manager uncompresses the files of
the package while the service is running, overwriting existing files. In
this situation, a 3.6.x node could be queried for its feature flags
state and the `rabbit_feature_flags` module would be loaded (because it
appeared during the upgrade but before the restart) and this would fail
because the node really didn't support feature flags.

With some feature flags being required as of RabbitMQ 3.11.0, upgrading
directly from e.g. 3.6.x is unsupported now. Therefore, we can drop this
`-on_load()` failsafe.
They were useful to diagnose failures when upgrading from e.g. 3.6.x to
3.8.x, but this is no longer supported. Therefore, drop this part of the
code.
@dumbbell dumbbell force-pushed the make-feature_flags_v2-required branch from f5eabc6 to f3b0fe6 Compare January 10, 2023 15:32
... instead of the internal `rabbit_log_feature_flags` module.
…_ff_file_is_unwritable` testcase

The testcase was moved to another testsuite but called functions from
its initial testsuite. That code code relies on the name of the
testsuite so it broke.

At the same time, make it depend on the testsuite's feature flag, not
stream queues. The stream queues feature flag will become required at
some point and break the testcase.
…gs_SUITE

A plugin's stable feature flag will now be enabled on initial node
start with `feature_flags_v2` required.
This is required for mixed-version testing where all nodes need to start
with the required feature flags enabled.
@dumbbell dumbbell force-pushed the make-feature_flags_v2-required branch from f3b0fe6 to c590db6 Compare January 10, 2023 15:38
@dumbbell dumbbell marked this pull request as ready for review January 10, 2023 16:10
@dumbbell dumbbell merged commit 103efec into main Jan 10, 2023
@dumbbell dumbbell deleted the make-feature_flags_v2-required branch January 10, 2023 16:29
@ansd ansd mentioned this pull request Jan 23, 2023
ansd added a commit that referenced this pull request Feb 8, 2023
RabbitMQ 3.12 requires feature flag feature_flags_v2 which got
introduced in 3.11.0 (see
#6810).

Therefore, we can mark all feature flags that got introduced in 3.11.0
or before 3.11.0 as required because users will have to upgrade to
3.11.x first, before upgrading to 3.12.x

The advantage of marking these feature flags as required is that we can
start deleting any compatibliy code for these feature flags, similarly
as done in #5215

This list shows when a given feature flag was first introduced:

classic_mirrored_queue_version 3.11.0
stream_single_active_consumer 3.11.0
direct_exchange_routing_v2 3.11.0
listener_records_in_ets 3.11.0
tracking_records_in_ets 3.11.0

empty_basic_get_metric 3.8.10
drop_unroutable_metric 3.8.10
ansd added a commit that referenced this pull request Feb 8, 2023
RabbitMQ 3.12 requires feature flag `feature_flags_v2` which got
introduced in 3.11.0 (see
#6810).

Therefore, we can mark all feature flags that got introduced in 3.11.0
or before 3.11.0 as required because users will have to upgrade to
3.11.x first, before upgrading to 3.12.x

The advantage of marking these feature flags as required is that we can
start deleting any compatibliy code for these feature flags, similarly
as done in #5215

This list shows when a given feature flag was first introduced:

```
classic_mirrored_queue_version 3.11.0
stream_single_active_consumer 3.11.0
direct_exchange_routing_v2 3.11.0
listener_records_in_ets 3.11.0
tracking_records_in_ets 3.11.0

empty_basic_get_metric 3.8.10
drop_unroutable_metric 3.8.10
```

In this commit, we also force all required feature flags to be enabled
in mixed version cluster testing and delete any tests that were about a
feature flag starting as disabled.

Furthermore, this commit already deletes the callback (migration) functions
given they do not run anymore in 3.12.x.

All other clean up (i.e. branching depending on whether a feature flag
is enabled) will be done in separate commits.
ansd added a commit that referenced this pull request Feb 8, 2023
RabbitMQ 3.12 requires feature flag `feature_flags_v2` which got
introduced in 3.11.0 (see
#6810).

Therefore, we can mark all feature flags that got introduced in 3.11.0
or before 3.11.0 as required because users will have to upgrade to
3.11.x first, before upgrading to 3.12.x

The advantage of marking these feature flags as required is that we can
start deleting any compatibliy code for these feature flags, similarly
as done in #5215

This list shows when a given feature flag was first introduced:

```
classic_mirrored_queue_version 3.11.0
stream_single_active_consumer 3.11.0
direct_exchange_routing_v2 3.11.0
listener_records_in_ets 3.11.0
tracking_records_in_ets 3.11.0

empty_basic_get_metric 3.8.10
drop_unroutable_metric 3.8.10
```

In this commit, we also force all required feature flags to be enabled
in mixed version cluster testing and delete any tests that were about a
feature flag starting as disabled.

Furthermore, this commit already deletes the callback (migration) functions
given they do not run anymore in 3.12.x.

All other clean up (i.e. branching depending on whether a feature flag
is enabled) will be done in separate commits.
ansd added a commit that referenced this pull request Feb 8, 2023
RabbitMQ 3.12 requires feature flag `feature_flags_v2` which got
introduced in 3.11.0 (see
#6810).

Therefore, we can mark all feature flags that got introduced in 3.11.0
or before 3.11.0 as required because users will have to upgrade to
3.11.x first, before upgrading to 3.12.x

The advantage of marking these feature flags as required is that we can
start deleting any compatibliy code for these feature flags, similarly
as done in #5215

This list shows when a given feature flag was first introduced:

```
classic_mirrored_queue_version 3.11.0
stream_single_active_consumer 3.11.0
direct_exchange_routing_v2 3.11.0
listener_records_in_ets 3.11.0
tracking_records_in_ets 3.11.0

empty_basic_get_metric 3.8.10
drop_unroutable_metric 3.8.10
```

In this commit, we also force all required feature flags in Erlang
application `rabbit` to be enabled in mixed version cluster testing
and delete any tests that were about a feature flag starting as disabled.

Furthermore, this commit already deletes the callback (migration) functions
given they do not run anymore in 3.12.x.

All other clean up (i.e. branching depending on whether a feature flag
is enabled) will be done in separate commits.
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