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.

Sign up for GitHub

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

Jump to bottom

Make sure amq.rabbitmq.log is accessible to client connections #1974

Merged
merged 3 commits into from
Apr 10, 2019
Merged

Make sure amq.rabbitmq.log is accessible to client connections #1974

merged 3 commits into from
Apr 10, 2019

Conversation

michaelklishin
Copy link
Member

Proposed Changes

amq.rabbitmq.log must not be declared as internal since internal exchanges cannot be used by client connections.

Types of Changes

  • Bug fix (non-breaking change which fixes issue amq.rabbitmq.log exchange in 3.7.x can be unavailable #1973)
  • 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)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Further Comments

Closes #1973, references #1456.

[#165243321]

@michaelklishin
Make sure amq.rabbitmq.log is accessible to client connections
12a8ca6 
That is, not an internal exchange.

Closes #1973, references #1456.

[#165243321]
michaelklishin
michaelklishin commented Apr 10, 2019
View reviewed changes
src/lager_exchange_backend.erl
#exchange{} = rabbit_exchange:declare(VHost, topic, true, false, true, [], ?INTERNAL_USER),
{ok, #resource{virtual_host=DefaultVHost, kind=exchange, name=?LOG_EXCH_NAME}}
%% durable
#exchange{} = rabbit_exchange:declare(Exchange, topic, true, false, false, [], ?INTERNAL_USER),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried a rabbit_exchange:update/2 here to simplify upgrades but it requires a Mnesia transaction and at logger initialisation stage this would not be safe. So users who already had the exchange must delete it manually during upgrades. Failures are handled in this function so node startup is not at risk.

@lukebakken lukebakken self-requested a review April 10, 2019 16:04
@lukebakken
Copy link
Collaborator

Testing this, one second.

lukebakken
lukebakken approved these changes Apr 10, 2019
View reviewed changes
Copy link
Collaborator

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested with the attached script

consume_from_amq.rabbitmq.log.py.gz

@lukebakken lukebakken merged commit e52513a into master Apr 10, 2019
@lukebakken lukebakken deleted the rabbitmq-server-1973 branch April 10, 2019 16:13
lukebakken added a commit that referenced this pull request Apr 10, 2019
@lukebakken
Merge pull request #1974 from rabbitmq/rabbitmq-server-1973
3af4299 
Make sure amq.rabbitmq.log is accessible to client connections

(cherry picked from commit e52513a)
@lukebakken
Copy link
Collaborator

Backported to v3.7.x

@michaelklishin
Copy link
Member Author

This will be partially reverted with an alternative solution as we now have new evidence in #1973 that changes our root cause hypothesis.

@michaelklishin
Copy link
Member Author

Largely superseded by #1976.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lukebakken lukebakken lukebakken approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

amq.rabbitmq.log exchange in 3.7.x can be unavailable
2 participants
@michaelklishin @lukebakken