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 #29180

Merged
merged 8 commits into from
May 11, 2023

Conversation

d-gubert
Copy link
Member

@d-gubert d-gubert commented May 9, 2023

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

Depends on RocketChat/Rocket.Chat.Apps-engine#621
AECO-150
AECO-100

@d-gubert
Copy link
Member Author

d-gubert commented May 9, 2023

PR is in draft state while and updated version of the Apps-Engine including the required changes is not published

@d-gubert d-gubert marked this pull request as ready for review May 9, 2023 17:05
@d-gubert d-gubert requested a review from a team as a code owner May 9, 2023 17:05
@d-gubert
Copy link
Member Author

d-gubert commented May 9, 2023

Apps-Engine launched and PR updated

@codecov
Copy link

codecov bot commented May 9, 2023

Codecov Report

Merging #29180 (ccad4c0) into release-6.2.0 (a0509a4) will increase coverage by 1.45%.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##           release-6.2.0   #29180      +/-   ##
=================================================
+ Coverage          44.66%   46.11%   +1.45%     
=================================================
  Files                682      716      +34     
  Lines              12737    13789    +1052     
  Branches            2058     2178     +120     
=================================================
+ Hits                5689     6359     +670     
- Misses              6766     7122     +356     
- Partials             282      308      +26     
Flag Coverage Δ
e2e 46.08% <ø> (+1.42%) ⬆️

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

@dionisio-bot dionisio-bot bot added the stat: ready to merge PR tested and approved waiting for merge label May 11, 2023
@kodiakhq kodiakhq bot merged commit b9f23b0 into release-6.2.0 May 11, 2023
@kodiakhq kodiakhq bot deleted the fix/app-cluster-inconsistent-status branch May 11, 2023 15:53
@sampaiodiego sampaiodiego mentioned this pull request May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stat: QA tested stat: ready to merge PR tested and approved waiting for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants