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

Methods to determine deprecated features in use report incomplete/inaccurate information #12619

Closed
Zerpet opened this issue Oct 30, 2024 · 10 comments
Assignees
Labels
Milestone

Comments

@Zerpet
Copy link
Contributor

Zerpet commented Oct 30, 2024

Describe the bug

The command rabbitmq-diagnostics check_if_any_deprecated_features_are_used and the API endpoint /api/deprecated-features/used do not report accurate information on actual state of the system. The command/endpoint only seem to check for classic queue mirroring, and it doesn't report the state of other deprecated features in use.

Reproduction steps

  1. Start RabbitMQ 4.0.3 (latest at this time): docker run --name bunny -p 15672:15672 --rm rabbitmq:4.0-management
  2. List deprecated features: docker exec bunny rabbitmq-diagnostics --formatter=pretty_table list_deprecated_features
    Observe transient_nonexcl_queues as deprecated feature, permitted by default
  3. Create a transient non-exclusive classic queue:
    curl -v -u guest:guest -H "Content-Type: application/json" -d '{"auto_delete":false,"durable":false}' -X PUT http://localhost:15672/api/queues/%2F/myqueue
    
  4. Query the state of deprecated features used: curl -v -u guest:guest http://localhost:15672/api/deprecated-features/used
  5. Observe that none [] are reported
  6. Observe that server logs print Deprecated features: transient_nonexcl_queues: Feature transient_nonexcl_queues is deprecated.
  7. Run docker exec bunny rabbitmq-diagnostics --formatter=pretty_table check_if_any_deprecated_features_are_used
  8. Observe the reported output: Cluster reported no deprecated features in use

Expected behavior

The command rabbitmq-diagnostics --formatter=pretty_table check_if_any_deprecated_features_are_used reports that transient_nonexcl_queues deprecated feature is used.

The API endpoint /api/deprecated-features/used reports that transient_nonexcl_queues deprecated feature is used.

Additional context

Setting deprecated_features.permit.transient_nonexcl_queues = false causes the queue declare operation to fail right away.

@Zerpet Zerpet added the bug label Oct 30, 2024
@michaelklishin
Copy link
Member

It's the CLI command that specifically only checks for CMQs.

@Zerpet
Copy link
Contributor Author

Zerpet commented Oct 30, 2024

It's the CLI command that specifically only checks for CMQs.

Strange, I can reproduce the issue using the HTTP API as well 😕

@michaelklishin
Copy link
Member

Maybe the HTTP API endpoints do the same thing, the CLI command was the first thing I've taken a look at.

@dumbbell
Copy link
Member

dumbbell commented Nov 6, 2024

In the specific case of transient_nonexcl_queues, the deprecated feature doesn’t define a is_feature_used() callback. Therefore the subsystem can’t tell if it is used or not.

This is the first thing to add if we want something accurate. I will prepare a patch.

@dumbbell
Copy link
Member

dumbbell commented Nov 6, 2024

Adding a is_feature_used callback to transient_nonexcl_queues is enough to have it listed in the API enpoint:

[
  {
    "name": "transient_nonexcl_queues",
    "desc": "",
    "deprecation_phase": "permitted_by_default",
    "doc_url": "https://blog.rabbitmq.com/posts/2021/08/4.0-deprecation-announcements/#removal-of-transient-non-exclusive-queues",
    "provided_by": "rabbit"
  }
]

It is not enough for the CLI command though, so it must not call the deprecated feature subsystem correctly.

Edit: Indeed, as Michael, the CLI is not calling the subsystem at all. It does its own thing specifically for classic mirrored queues.

dumbbell added a commit that referenced this issue Nov 6, 2024
…_queues` depr. feature

[Why]
Without this callback, the deprecated features subsystem can't report if
the feature is used or not.

This reduces the usefulness of the HTTP API endpoint or the CLI command
that help verify if a cluster is using deprecated features.

[How]
The callback counts transient non-exclusive queues and return `true` if
there are one or more of them.

References #12619.
dumbbell added a commit that referenced this issue Nov 6, 2024
[Why]
The previous implementation bypassed the deprecated features subsystem.
It only cared about classic mirrored queues and called some
queue-related code directly to determine if this specific feature was
used.

[How]
The command code is simplified by calling the deprecated subsystem to
list used deprecated features instead.

References #12619.
@dumbbell dumbbell self-assigned this Nov 6, 2024
dumbbell added a commit that referenced this issue Nov 11, 2024
[Why]
The previous implementation bypassed the deprecated features subsystem.
It only cared about classic mirrored queues and called some
queue-related code directly to determine if this specific feature was
used.

[How]
The command code is simplified by calling the deprecated subsystem to
list used deprecated features instead.

References #12619.
dumbbell added a commit that referenced this issue Nov 11, 2024
…_queues` depr. feature

[Why]
Without this callback, the deprecated features subsystem can't report if
the feature is used or not.

This reduces the usefulness of the HTTP API endpoint or the CLI command
that help verify if a cluster is using deprecated features.

[How]
The callback counts transient non-exclusive queues and return `true` if
there are one or more of them.

References #12619.
dumbbell added a commit that referenced this issue Nov 11, 2024
…_queues` depr. feature

[Why]
Without this callback, the deprecated features subsystem can't report if
the feature is used or not.

This reduces the usefulness of the HTTP API endpoint or the CLI command
that help verify if a cluster is using deprecated features.

[How]
The callback counts transient non-exclusive queues and return `true` if
there are one or more of them.

References #12619.
@dumbbell
Copy link
Member

Both pull requests are ready for review! @Zerpet: could you please give them a try? You will need both to fix the problem entirely.

@Zerpet
Copy link
Contributor Author

Zerpet commented Nov 14, 2024

@dumbbell I built the broker after merging both PRs locally, run the same commands as in the OP, and I got the expected result 👍 I'll defer the approval of the PRs to other folks more experienced with Erlang code.

@michaelklishin
Copy link
Member

@dumbbell any reason not to backport #12675 #12674 to v4.0.x?

@michaelklishin michaelklishin added this to the 4.1.0 milestone Nov 14, 2024
michaelklishin pushed a commit that referenced this issue Nov 14, 2024
…_queues` depr. feature

[Why]
Without this callback, the deprecated features subsystem can't report if
the feature is used or not.

This reduces the usefulness of the HTTP API endpoint or the CLI command
that help verify if a cluster is using deprecated features.

[How]
The callback counts transient non-exclusive queues and return `true` if
there are one or more of them.

References #12619.
michaelklishin pushed a commit that referenced this issue Nov 14, 2024
[Why]
The previous implementation bypassed the deprecated features subsystem.
It only cared about classic mirrored queues and called some
queue-related code directly to determine if this specific feature was
used.

[How]
The command code is simplified by calling the deprecated subsystem to
list used deprecated features instead.

References #12619.
@dumbbell
Copy link
Member

@dumbbell any reason not to backport #12675 #12674 to v4.0.x?

To me, it’s bug and should be backported. I will set the labels accordingly.

@dumbbell
Copy link
Member

Thank you for reviewing and merging!

mergify bot pushed a commit that referenced this issue Nov 15, 2024
[Why]
The previous implementation bypassed the deprecated features subsystem.
It only cared about classic mirrored queues and called some
queue-related code directly to determine if this specific feature was
used.

[How]
The command code is simplified by calling the deprecated subsystem to
list used deprecated features instead.

References #12619.

(cherry picked from commit ddaea6f)
mergify bot pushed a commit that referenced this issue Nov 15, 2024
…_queues` depr. feature

[Why]
Without this callback, the deprecated features subsystem can't report if
the feature is used or not.

This reduces the usefulness of the HTTP API endpoint or the CLI command
that help verify if a cluster is using deprecated features.

[How]
The callback counts transient non-exclusive queues and return `true` if
there are one or more of them.

References #12619.

(cherry picked from commit 638e3a4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants