Rabbitmq vhost and user support #1052
Conversation
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/190f4bb7eca7441d8e42567b23824058 ✔️ openstack-meta-content-provider SUCCESS in 3h 29m 35s |
|
recheck |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/e49f704cb10943c295aeec1583a8186e ✔️ openstack-meta-content-provider SUCCESS in 3h 20m 36s |
|
recheck |
lmiccini
force-pushed
the
rabbitmq_vhosts
branch
from
047781d to
c449b78
Compare
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
lmiccini
force-pushed
the
rabbitmq_vhosts
branch
from
c449b78 to
b5594d1
Compare
| errors = append(errors, field.Forbidden( | ||
| basePath.Child("apiMessageBusInstance"), | ||
| "apiMessageBusInstance is deprecated and cannot be changed. Please use messagingBus.cluster instead")) | ||
| } |
There was a problem hiding this comment.
so we should allow them to null it out if its set
but we could reject other changes How that is a breaking api change so im not sold on doing that.
it defeats the reason for ahvaing a deprecation period
what woudl be more correct is to not allow setting both at the same time.
i.e you can either use the old field or new filed but not both.
i woudl condier it an error if
cellTemplate.MessagingBus.Cluster and APIMessageBusInstance were ever diffent unless APIMessageBusInstance was nil or ""
we should exicltiy check for an reject that.
| } | ||
| } | ||
|
|
||
| errors = append(errors, spec.ValidateCellTemplates(basePath, namespace)...) |
There was a problem hiding this comment.
so i shoudl point out that
explicit block 2 cells form sharing a rabbit instance since that is not supproted when not using seperat vhost per cell.
we need to enhance ValidateCellTemplates to account for vhost and more imporatnly the new cellTemplate.MessagingBus.Cluster field.
the quetion i have is do we want to enforce that you cant share the same rabbit cluster btween cells via the webhook or jsut document that as unsupproted in our product.
three isnt a reasonable usecase for sharign beyond testing/dev by levrageing vhosts as cells only exist to shard your cloud to supprot scaling beyond the limitats fo a sincel db/rabbit cluster.
my inclindation is to block 2 cells form using the same rabbit cluster in the webhook to ensure that unsupproted configuration are not possible rather then supproting it only for testing.
| cellDatabaseInstance: openstack | ||
| cellDatabaseAccount: nova-cell0 | ||
| hasAPIAccess: true | ||
| memcachedInstance: memcached |
There was a problem hiding this comment.
so in this case we expect cell 0 to be using the the rabbitmq rabit cluster
based on teh default of CellMessageBusInstance in the cell template.
// +kubebuilder:validation:Optional
// +kubebuilder:default=rabbitmq
// CellMessageBusInstance is the name of the RabbitMqCluster CR to select
// the Message Bus Service instance used by the nova services to
// communicate in this cell. For cell0 it is unused.
CellMessageBusInstance string `json:"cellMessageBusInstance"`
relying on the implict defaulting is nova very clear.
| messagingBus: | ||
| cluster: rabbitmq | ||
| notificationsBus: | ||
| cluster: rabbitmq |
There was a problem hiding this comment.
while this is possibel to do that goes against best practices so this is nto somethin we shoudl supprot downstream
unlike with cells im ok with not blocking this in the webhook since it does not fundementally unermine how cells are desgien to work.
it generally recomemnted upstream for moderate to large clouds that each service uses its own rabbit instnace and notifications are kept sepreate form rpc trafic.
that using seperate notification really shoudl be the only deployment model we supprot downstream but its technially valid so we can supprot it in the operator. in a very small cloud i.e. less then 50 comptues its viable but its not somethign we shoudl ever default too or recommend for most clouds.
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/ca4ac87ba47c49098e4da429204a102a ✔️ openstack-meta-content-provider SUCCESS in 5h 06m 17s |
lmiccini
force-pushed
the
rabbitmq_vhosts
branch
from
b5594d1 to
a55d41f
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: lmiccini The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
lmiccini
force-pushed
the
rabbitmq_vhosts
branch
from
a55d41f to
f5fe07a
Compare
Add new messagingBus and notificationsBus interfaces to hold cluster,
user and vhost names for optional usage.
The controller adds these values to the TransportURL create request when present.
Additionally, we migrate RabbitMQ cluster name to RabbitMq config struct
using DefaultRabbitMqConfig from infra-operator to automatically
populate the new Cluster field from legacy RabbitMqClusterName.
Example usage:
spec:
messagingBus:
cluster: rpc-rabbitmq
user: rpc-user
vhost: rpc-vhost
notificationsBus:
cluster: notifications-rabbitmq
user: notifications-user
vhost: notifications-vhost
Finally, we add the rabbitmquser crs to the secret so they can be
stored for dataplane finalizers management and do auto cleanup of orphaned
users after credential rotations.
Jira: https://issues.redhat.com/browse/OSPRH-22697
lmiccini
force-pushed
the
rabbitmq_vhosts
branch
from
f5fe07a to
d5bf4bf
Compare
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
|
@lmiccini: The following tests failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
3 participants
Add new messagingBus and notificationsBus interfaces to hold cluster, user and vhost names for optional usage.
The controller adds these values to the TransportURL create request when present.
Additionally, we migrate RabbitMQ cluster name to RabbitMq config struct using DefaultRabbitMqConfig from infra-operator to automatically populate the new Cluster field from legacy RabbitMqClusterName.
Example usage:
Jira: https://issues.redhat.com/browse/OSPRH-22697