-
Notifications
You must be signed in to change notification settings - Fork 7.1k
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(core): Ensure only the leader's license manager handles renewal #9210
fix(core): Ensure only the leader's license manager handles renewal #9210
Conversation
* Whether this instance is the leader. This is always `true` in single-main setup, | ||
* once `OrchestrationService` has been initialized. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WaitTracker
is initialized as part of ExecutionService
, which is initialized as part of ActiveWorkflowRunner
, which is initialized before OrchestrationService
is initialized. This is why at #9100 WaitTracker.constructor
found isLeader
to be false
in single-main setup - because instanceType
is unset
until OrchestrationService
is initialized.
Ideally OrchestrationService
should be initialized before ActiveWorkflowRunner
but this original order has been in place for a long time - changing this may have unforeseen side effects, so I decided to preserve the order of OrchestrationService
and ActiveWorkflowRunner
for now.
1fdb750
to
2955ac3
Compare
@@ -173,10 +173,10 @@ export class Start extends BaseCommand { | |||
await super.init(); | |||
this.activeWorkflowRunner = Container.get(ActiveWorkflowRunner); | |||
|
|||
await this.initLicense(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License needs to be init first because orchestration depends on the enterprise features, so we cannot put it afterwards.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
License
depends on OrchestrationService
for onFeatureChange
and for the change in this PR. But OrchestrationService
depends on License
for checking if multi-main setup is licensed. Let me see how to untangle.
const server = config.getEnv('license.serverUrl'); | ||
const autoRenewEnabled = isMainInstance && config.getEnv('license.autoRenewEnabled'); | ||
const offlineMode = !isMainInstance; | ||
const autoRenewEnabled = isLeaderMain && config.getEnv('license.autoRenewEnabled'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think only autoRenewEnabled
needs to change, the others can stay as they are probably.
- saving a certificate only happens on renew, so if not renewing, it should be fine
- feature changes will happen following a change to license, which afaik only happens when license changes or leader asks followers for updates
- collectUsageMetrics is probably also something that should not pose any issues
The problem is then that we need to change the autoRenewEnabled
setting whenever a follower becomes a leader, right?
@@ -205,6 +203,8 @@ export class Start extends BaseCommand { | |||
|
|||
await orchestrationService.init(); | |||
|
|||
if (config.getEnv('executions.mode') !== 'queue') return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any specific reason why we do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This early return in its original position prevents OrchestrationService.init()
from being called in single-main setup, meaning that instanceType
remains unset
for this single main, causing isLeader
checks to return false
when used later on, forcing us to avoid checking isLeader
in single-main mode, e.g. in WaitTracker
.
Closing for now, will find better approach |
Ensure only the leader's license manager handles renewal, in both single- and multi-main setup.
Fixes: