Return early when notification TransportURL is not ready#1082
Conversation
auniyal61
force-pushed
the
OSPRH-26922-fix
branch
from
5964e36 to
5d139b3
Compare
| )) | ||
| // return early - wait for notification MQ to be ready before continuing | ||
| // to avoid propagating empty transport_url to config generation | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
this feels liek a bug
if the transport url is not read you should aboort reconsiliation
you should not continue to reconcile without it present
weare setting NovaNotificationMQReadyCondition to false meaning the overall ready status of the nova CR will be false as well
There was a problem hiding this comment.
I think its ok to set nova condition false - yes notification is not directly affecting nova services so better not set whole nova condition false, but as user want to enable notifications but there is some issue (which should not come), so its a important messaging.
There was a problem hiding this comment.
if the transport url is not read you should aboort reconsiliation
This return statement basically aborts the current Reconciliation. As you noted the Nova CR status is set to not ready. We are not asking for a requeue or returning an error as we don't need to explicitly schedule a retry of the reconciliation here. When the TransportURL CR status changes Nova will automatically reconcile as we are owning that resource.
auniyal61
force-pushed
the
OSPRH-26922-fix
branch
from
5d139b3 to
1abec8b
Compare
| )) | ||
| // return early - wait for notification MQ to be ready before continuing | ||
| // to avoid propagating empty transport_url to config generation | ||
| return ctrl.Result{}, nil |
There was a problem hiding this comment.
if the transport url is not read you should aboort reconsiliation
This return statement basically aborts the current Reconciliation. As you noted the Nova CR status is set to not ready. We are not asking for a requeue or returning an error as we don't need to explicitly schedule a retry of the reconciliation here. When the TransportURL CR status changes Nova will automatically reconcile as we are owning that resource.
|
I tested it locally and it resolves the unnecessary nova reconfiguration. |
auniyal61
force-pushed
the
OSPRH-26922-fix
branch
2 times, most recently
from
ae07b33 to
1a1f812
Compare
gibizer
left a comment
gibizer
left a comment
There was a problem hiding this comment.
two small changes in the test otherwise this looks good to me
When a RabbitMQ notification pod is deleted or restarted, the transporturl status changes from ready to others(creating). This change make reconciler return early when rabbitmq status is creating or failed, this prevents empty transporturl from being propogated in configs. Adds tests to verify same Resolves: OSPRH-26269
auniyal61
force-pushed
the
OSPRH-26922-fix
branch
from
1a1f812 to
59a5600
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: auniyal61, gibizer The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit 320f409
into
openstack-k8s-operators:main
|
/cherry-pick 18.0-fr5 |
|
@gibizer: new pull request created: #1109 DetailsIn response to this:
Instructions 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. |
4 participants
When a RabbitMQ notification pod is deleted or restarted, the transporturl status changes from ready to others(creating).
This change make reconciler return early when rabbitmq status is creating or failed, this prevents empty transporturl from being propogated in configs.
Adds tests to verify same
Resolves: OSPRH-26269