Skip to content

Conversation

@coro
Copy link
Contributor

@coro coro commented Dec 6, 2021

Note to reviewers: remember to look at the commits in this PR and consider if they can be squashed
Note to contributors: remember to re-generate client set if there are any API changes

Summary Of Changes

  • Adds two new CRDs:
    • SuperStream
      • This object represents a partitioned stream queue - one can specify the number of partitions per SuperStream
      • For n partitions, creates 1 exchange, n stream queues (a.k.a. 'partitions') and n bindings
      • n can be increased on an existing SuperStream; this will cause new partitions and bindings to be created
        • n cannot be decreased as this could lead to data loss
      • Routing keys for each of the bindings can be specified, or default to the index of the binding
        • Bindings are immutable in the topology operator, so these routing keys cannot be modified once set
        • If n is increased, additional routing keys must be specified if not left as the index of the binding
    • SuperStreamConsumer
      • Represents a PodSpec or set of PodSpecs to use to create consumer client Pods for a given SuperStream
      • Does not need be in the same namespace as the RabbitmqCluster or SuperStream
      • Spins up new consumer Pods for every partition in the SuperStream
        • If the SuperStream scales up in size, new Pods are created for the new partitions
        • If a consumer Pod dies or crashes, recreates the Pod or container to ensure there is always one active consumer per partition (with some downtime of consumption)
      • Can define a default podSpec to use for all partitions, or specify a different podSpec for each routingKey in the super stream
        • This allows for different business logic per-partition
        • The name of the stream to consume from is provided to the pod as a kubernetes label rabbitmq.com/super-stream-partition
          • The podSpec provided to a SuperStreamConsumer will likely consume this label as a mounted volume - see documented example in this PR
      • If the podSpec for a consumer changes, a new pod will be spun up for that consumer and the old one deleted (again with some downtime of consumption)

Additional Context

These CRDs are compatible with any 3.9 RabbitMQ image where the stream plugin is enabled.

This PR adds a system test which uses Chaos Mesh to simulate a container error in a consumer pod. If Chaos Mesh is not installed on the test system, the test is skipped. For our GKE test clusters, it's pretty simple to install chaos mesh: helm install chaos-mesh chaos-mesh/chaos-mesh --namespace=chaos-testing --version 2.0.4 --set dashboard.securityMode=false --set chaosDaemon.runtime=containerd --set chaosDaemon.socketPath=/run/containerd/containerd.sock, and I've done this on the CI cluster.

Copy link
Contributor

@ChunyiLyu ChunyiLyu left a comment

Choose a reason for hiding this comment

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

Maybe these are by design, but noted them here for potential improvement if possible.

  1. superStreamReference can be updated

To reproduce, create all objects from messaging-topology-operator/docs/examples/singleactiveconsumer

After objects are all created, modify spec.superstreamConsumerReference.name to something else and apply. Update goes through (there are unit tests to prevent this update if I'm not mistaken).

  1. Orders in list spec.routingKeys matters when comparing the updated list with the current list.

Create superstream.rabbitmq.com/orders from the example with less than 3 routingKeys defined, swatch the position of a pair of keys and update will fail with Resource: "rabbitmq.com/v1beta1, Resource=superstreams", GroupVersionKind: "rabbitmq.com/v1beta1, Kind=SuperStream" Name: "orders", Namespace: "rabbitmq-system" for: "superstream.yaml": admission webhook "vsuperstream.kb.io" denied the request: SuperStream.rabbitmq.com "orders" is forbidden: spec.routingKeys: Forbidden: updates may only add to the existing list of routing keys

Comment on lines +38 to +46
// +kubebuilder:rbac:groups=rabbitmq.com,resources=exchanges,verbs=get;create;update;patch;delete
// +kubebuilder:rbac:groups=rabbitmq.com,resources=queues,verbs=get;create;update;patch;delete
// +kubebuilder:rbac:groups=rabbitmq.com,resources=bindings,verbs=get;create;update;patch;delete
// +kubebuilder:rbac:groups=rabbitmq.com,resources=superstreams,verbs=get;list;watch;create;update;patch;delete
// +kubebuilder:rbac:groups=rabbitmq.com,resources=superstreams/status,verbs=get;update;patch
// +kubebuilder:rbac:groups=rabbitmq.com,resources=rabbitmqclusters,verbs=get;list;watch
// +kubebuilder:rbac:groups=rabbitmq.com,resources=rabbitmqclusters/status,verbs=get
// +kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch
// +kubebuilder:rbac:groups="",resources=events,verbs=get;create;patch
Copy link
Contributor

Choose a reason for hiding this comment

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

I think most of these are defined in other controllers already. I suggest only define the ones that's new to this controller.

@coro
Copy link
Contributor Author

coro commented Dec 14, 2021

@ChunyiLyu Thanks for the review! On your points:

1: Totally agree, that field should be immutable IMO.
2: Order does matter, and it's because of an underlying detail in the binding: each binding is allocated an additional argument x-stream-partition-order which is the numerical index of the routing key in the list of routing keys. Hence, if you add to the list of keys, a new binding will be created ffor the new index, but the previous ones are untouched.

I can see how that provides a confusing API, though, as you say. It would be good to know what the impact of changing the value of x-stream-partition-order for a given partition would be. If we can freely change it, we could simply delete surplus bindings and create new ones for the new keys, meaning we could potentiall also do away with the restriction that we can only append routing keys. WDYT?

Copy link
Member

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

I'm half way through the PR, the refactor and the API types make sense, however I'd like to call for a stop here, as I see we are becoming a Pod controller.

I'm unsure if becoming a Pod controller is something we want to get into, and if so, I'd very much encourage a design document, specially highlighting the reasons to chose our Pod controller over the good ol' Deployment or StatefulSet controllers.


detailMsg := "updates on superStreamReference are forbidden"

if s.Spec.SuperStreamReference != oldSuperStreamConsumer.Spec.SuperStreamReference {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be a DeepEqual ?

@Zerpet
Copy link
Member

Zerpet commented Dec 15, 2021

Based on the conversation we had this morning during standup, I'll be happy with the current state of the PR once we mark the api version as alpha, something like v1alpha1.

Would be nice to have an opt-out option to not deploy these CRDs and the role with CRUD permissions on Pod objects, but it shouldn't block this PR.

@coro coro mentioned this pull request Jan 10, 2022
@coro
Copy link
Contributor Author

coro commented Jan 10, 2022

Closing in favour of #281

@coro coro closed this Jan 10, 2022
@ChunyiLyu ChunyiLyu deleted the super-streams branch January 19, 2023 17:56
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.

4 participants