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.

Sign up for GitHub

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

Jump to bottom

fix(core): Ensure only the leader's license manager handles renewal #9210

Closed
wants to merge 4 commits into from
Closed

fix(core): Ensure only the leader's license manager handles renewal #9210

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ab87463
fix(core): Add leader check to license renewal
ivov Apr 24, 2024
2b30b43
Add logger calls
ivov Apr 24, 2024
e4a744c
Move early return
ivov Apr 24, 2024
2955ac3
Add tests
ivov Apr 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions packages/cli/src/License.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,19 @@ export class License {
return;
}

const isMainInstance = instanceType === 'main';
const isLeaderMain = instanceType === 'main' && this.orchestrationService.isLeader;

const server = config.getEnv('license.serverUrl');
const autoRenewEnabled = isMainInstance && config.getEnv('license.autoRenewEnabled');
const offlineMode = !isMainInstance;
const autoRenewEnabled = isLeaderMain && config.getEnv('license.autoRenewEnabled');
Copy link
Contributor

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?

const offlineMode = !isLeaderMain;
const autoRenewOffset = config.getEnv('license.autoRenewOffset');
const saveCertStr = isMainInstance
const saveCertStr = isLeaderMain
? async (value: TLicenseBlock) => await this.saveCertStr(value)
: async () => {};
const onFeatureChange = isMainInstance
const onFeatureChange = isLeaderMain
? async (features: TFeatures) => await this.onFeatureChange(features)
: async () => {};
const collectUsageMetrics = isMainInstance
const collectUsageMetrics = isLeaderMain
? async () => await this.usageMetricsService.collectUsageMetrics()
: async () => [];

Expand Down
8 changes: 3 additions & 5 deletions packages/cli/src/commands/start.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,10 +173,10 @@ export class Start extends BaseCommand {
await super.init();
this.activeWorkflowRunner = Container.get(ActiveWorkflowRunner);

await this.initLicense();
Copy link
Contributor

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.

Copy link
Contributor Author

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.


await this.initOrchestration();
this.logger.debug('Orchestration init complete');
console.log('Orchestration init complete');
await this.initLicense();
console.log('License init complete');
await this.initBinaryDataService();
this.logger.debug('Binary data service init complete');
await this.initExternalHooks();
Expand All @@ -192,8 +192,6 @@ export class Start extends BaseCommand {
}

async initOrchestration() {
if (config.getEnv('executions.mode') !== 'queue') return;

if (
config.getEnv('multiMainSetup.enabled') &&
!Container.get(License).isMultipleMainInstancesLicensed()
Expand Down
3 changes: 2 additions & 1 deletion packages/cli/src/services/orchestration.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,8 @@ export class OrchestrationService {
}

/**
* Whether this instance is the leader in a multi-main setup. Always `false` in single-main setup.
* Whether this instance is the leader. This is always `true` in single-main setup,
* once `OrchestrationService` has been initialized.
Comment on lines +47 to +48
Copy link
Contributor Author

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.

*/
get isLeader() {
return config.getEnv('multiMainSetup.instanceType') === 'leader';
Expand Down