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.

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

[FIX] app status inconsistencies when running multiple instances in a cluster #29219

Merged
merged 4 commits into from May 12, 2023

Conversation

d-gubert
Copy link
Member

Proposed changes (including videos or screenshots)

App status inconsistencies between multiple instances in a cluster boil down to the fact that the Apps-Engine is currently responsible for orchestrating when these events are triggered and is overly verbose in doing so.

Upon analysis, the framework itself should not have the concept of "other instances" - this is a deployment detail of the host system, and as such should be controlled by the host. The correct solution for this problem is to review this notification system, potentially removing it from the framework and leaving the responsibility solely for Rocket.Chat.

However, this is hindering the current app management experience for workspaces, so this PR cuts the control of some notifications that come from the framework (the more problematic ones) and moves the control over to RC in a short and practical way.

This is done by turning the methods of the most problematic events in the AppActivationBridge into no-ops, and instead triggering the AppServerNotifier directly in the api endpoints that are applicable.

It is not the most correct solution to the problem, but due to time constraints and urgency this will be applied first so we can move with the correct solution in a future point.

Issue(s)

Steps to test or reproduce

1 - Setup a clustered deployment (6.x), either micro-services or high availability;
2 - Install an app on instance A
3 - App will not be available in other instances

Further comments

@d-gubert d-gubert requested a review from a team as a code owner May 12, 2023 00:11
@d-gubert d-gubert changed the title fix: app status inconsistencies when running multiple instances in a cluster [FIX] app status inconsistencies when running multiple instances in a cluster May 12, 2023
@d-gubert d-gubert requested a review from a team as a code owner May 12, 2023 00:38
@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #29219 (0d41395) into release-6.0.7 (3f44035) will decrease coverage by 0.04%.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##           release-6.0.7   #29219      +/-   ##
=================================================
- Coverage          45.02%   44.99%   -0.04%     
=================================================
  Files                772      772              
  Lines              14997    14997              
  Branches            2094     2094              
=================================================
- Hits                6753     6748       -5     
- Misses              7942     7950       +8     
+ Partials             302      299       -3     
Flag Coverage Δ
e2e 44.95% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Copy link
Contributor

@AllanPazRibeiro AllanPazRibeiro left a comment

Choose a reason for hiding this comment

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

LGTM

@casalsgh casalsgh requested a review from a team May 12, 2023 16:58
@dionisio-bot dionisio-bot bot added stat: ready to merge PR tested and approved waiting for merge and removed stat: needs QA labels May 12, 2023
@kodiakhq kodiakhq bot merged commit 9e1166a into release-6.0.7 May 12, 2023
34 checks passed
@kodiakhq kodiakhq bot deleted the fix/app-installation branch May 12, 2023 18:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA skipped stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants