search

sensu / sensu-transport

The Sensu transport abstraction library.
MIT License
14 stars 19 forks source link

Fix RabbitMQ exchange cleanup #54

Closed portertech closed 6 years ago

portertech commented 6 years ago

The pull request adds the use of AMQP "auto_delete" with subscription exchanges, the exchanges are now deleted when all queues have finished using them.

These changes are in response to an issue reported by Anatoliy D on the Sensu community slack.

Just found that Sensu does not delete rabbitmq exchange when client is deleted.  I see this in my RMQ cluster -  Exchanges: 285191, Queues: 1005 .  Queues corresponds with the number of active clients, exchange list contains all client names since the beginning. Any idea why exchanges are not deleted ?

THESE CHANGES ARE NOT BACKWARDS COMPATIBLE! In order to set "auto_delete", the RabbitMQ exchanges must be re-declared, requiring users first manually delete the exchanges or reset the RabbitMQ node(s). This makes the upgrade path more difficult.

The Sensu server be updated to use :auto_delete => true and :passive => true when publishing check requests to subscriptions.

portertech commented 6 years ago

The introduction of unique Sensu client subscriptions dramatically increased the number of RabbitMQ exchanges created. Currently, environments with high client churn will continue to grow the number of exchanges indefinitely, e.g. Exchanges: 285191, Queues: 1005, I think we should do this. We'll have to deal with the repercussions, i.e. upgrading.

portertech commented 6 years ago

Going to bring this up on the next community call and send it out to some mailing lists.

portertech commented 6 years ago

Should probably take https://github.com/sensu/sensu-transport/pull/37 into account, make a few changes to accommodate it.

majormoses commented 6 years ago

@portertech hmm if it's a breaking change then it needs a major version bump. This probably affects the messaging around sensu 2.0 re-write. I bet this will cause a lot of confusion.

portertech commented 6 years ago

@calebhailey @cwjohnston let's discuss this, this week.

portertech commented 6 years ago

@calebhailey @cwjohnston let's discuss this, this week. Fo real.

cwjohnston commented 6 years ago

Closes #53

portertech commented 6 years ago

Let's measure the technical impact of a growing list of exchanges (memory usage etc) and use this data to make a final decision.

majormoses commented 6 years ago

I don't see what the decision could be other than a major version release. Failure to follow semver leads to distrust in the semver model. Either we follow semver or not that is the only decision to make regarding the version change. The decision to me is how to message that what is currently referred to as sensu 2.x should be messaged. This again shows that we need a different code name for the different code bases as they honestly need to be versioned independently as they both need support for a year and if we have a breaking change we risk having version conflicts.

portertech commented 6 years ago

Closing this issue in favour of https://github.com/sensu/sensu/issues/1828 We will ship a tool to address the issue, avoiding the breaking change.