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

refactor(core): Make instance role clearer (no-changelog) #10188

Merged
merged 2 commits into from
Jul 30, 2024
Merged

refactor(core): Make instance role clearer (no-changelog) #10188

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d29574a
refactor(core): Make instance role clearer (no-changelog)
ivov Jul 25, 2024
b902db9
Fix test
ivov Jul 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
2 changes: 1 addition & 1 deletion packages/cli/src/License.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ export class License {
* This ensures the mains do not cause a 429 (too many requests) on license init.
*/
if (config.getEnv('multiMainSetup.enabled')) {
return autoRenewEnabled && config.getEnv('multiMainSetup.instanceType') === 'leader';
return autoRenewEnabled && config.getEnv('instanceRole') === 'leader';
}

return autoRenewEnabled;
Expand Down
7 changes: 2 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 @@ -184,10 +184,7 @@ export class Start extends BaseCommand {
await this.initOrchestration();
this.logger.debug('Orchestration init complete');

if (
!config.getEnv('license.autoRenewEnabled') &&
config.getEnv('multiMainSetup.instanceType') === 'leader'
) {
if (!config.getEnv('license.autoRenewEnabled') && config.getEnv('instanceRole') === 'leader') {
this.logger.warn(
'Automatic license renewal is disabled. The license will not renew automatically, and access to licensed features may be lost!',
);
Expand All @@ -211,7 +208,7 @@ export class Start extends BaseCommand {

async initOrchestration() {
if (config.getEnv('executions.mode') === 'regular') {
config.set('multiMainSetup.instanceType', 'leader');
config.set('instanceRole', 'leader');
return;
}

Expand Down
11 changes: 6 additions & 5 deletions packages/cli/src/config/schema.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -917,12 +917,13 @@ export const schema = {
},
},

instanceRole: {
Copy link
Member

Choose a reason for hiding this comment

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

why are we keeping this state in the config though? can this not be a property on one of the services instead?
we should stop using config to store data that user can't configure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I wouldn't feel comfortable making that change without thorough retesting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was likely done following what was done with instanceType. I think there's value in making instanceRole clearer until we decide to remove all state from the schema. Let me know what you prefer.

Copy link
Member

Choose a reason for hiding this comment

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

does this make sense? https://github.com/n8n-io/n8n/compare/make-instance-role-clearer...10188-proposal?expand=1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! In principle it does, what I'm nervous about is merging this without thorough testing.

Copy link
Member

Choose a reason for hiding this comment

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

The diff looks safe to me. Maybe we can ask someone else to review as well?
or, we can merge this PR, and then I can open a second PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's bring it in. I'll do some spot checks testing tomorrow but I agree it looks safe.

Copy link
Member

Choose a reason for hiding this comment

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

#10242

doc: 'Always `leader` in single-main setup. `leader` or `follower` in multi-main setup.',
format: ['unset', 'leader', 'follower'] as const,
default: 'unset', // only until Start.initOrchestration
},

multiMainSetup: {
instanceType: {
doc: 'Type of instance in multi-main setup',
format: ['unset', 'leader', 'follower'] as const,
default: 'unset', // only until first leader key check
},
enabled: {
doc: 'Whether to enable multi-main setup for queue mode (license required)',
format: Boolean,
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/src/executions/__tests__/execution-recovery.service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ describe('ExecutionRecoveryService', () => {
});

beforeEach(() => {
config.set('multiMainSetup.instanceType', 'leader');
config.set('instanceRole', 'leader');
});

afterEach(async () => {
Expand Down Expand Up @@ -130,7 +130,7 @@ describe('ExecutionRecoveryService', () => {
/**
* Arrange
*/
config.set('multiMainSetup.instanceType', 'follower');
config.set('instanceRole', 'follower');
// @ts-expect-error Private method
const amendSpy = jest.spyOn(executionRecoveryService, 'amend');
const messages = setupMessages('123', 'Some workflow');
Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/executions/execution-recovery.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,7 @@ export class ExecutionRecoveryService {
private shouldScheduleQueueRecovery() {
return (
config.getEnv('executions.mode') === 'queue' &&
config.getEnv('multiMainSetup.instanceType') === 'leader' &&
config.getEnv('instanceRole') === 'leader' &&
!this.isShuttingDown
);
}
Expand Down
9 changes: 3 additions & 6 deletions packages/cli/src/services/orchestration.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,12 @@ export class OrchestrationService {
return config.getEnv('redis.queueModeId');
}

/**
* Whether this instance is the leader in a multi-main setup. Always `false` in single-main setup.
*/
get isLeader() {
return config.getEnv('multiMainSetup.instanceType') === 'leader';
return config.getEnv('instanceRole') === 'leader';
}

get isFollower() {
return config.getEnv('multiMainSetup.instanceType') !== 'leader';
return config.getEnv('instanceRole') !== 'leader';
}

sanityCheck() {
Expand All @@ -66,7 +63,7 @@ export class OrchestrationService {
if (this.isMultiMainSetupEnabled) {
await this.multiMainSetup.init();
} else {
config.set('multiMainSetup.instanceType', 'leader');
config.set('instanceRole', 'leader');
}

this.isInitialized = true;
Expand Down
12 changes: 6 additions & 6 deletions packages/cli/src/services/orchestration/main/MultiMainSetup.ee.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ export class MultiMainSetup extends EventEmitter {
async shutdown() {
clearInterval(this.leaderCheckInterval);

const isLeader = config.getEnv('multiMainSetup.instanceType') === 'leader';
const isLeader = config.getEnv('instanceRole') === 'leader';

if (isLeader) await this.redisPublisher.clear(this.leaderKey);
}
Expand All @@ -64,8 +64,8 @@ export class MultiMainSetup extends EventEmitter {
if (leaderId && leaderId !== this.instanceId) {
this.logger.debug(`[Instance ID ${this.instanceId}] Leader is other instance "${leaderId}"`);

if (config.getEnv('multiMainSetup.instanceType') === 'leader') {
config.set('multiMainSetup.instanceType', 'follower');
if (config.getEnv('instanceRole') === 'leader') {
config.set('instanceRole', 'follower');

this.emit('leader-stepdown'); // lost leadership - stop triggers, pollers, pruning, wait-tracking, queue recovery

Expand All @@ -80,7 +80,7 @@ export class MultiMainSetup extends EventEmitter {
`[Instance ID ${this.instanceId}] Leadership vacant, attempting to become leader...`,
);

config.set('multiMainSetup.instanceType', 'follower');
config.set('instanceRole', 'follower');

/**
* Lost leadership - stop triggers, pollers, pruning, wait tracking, license renewal, queue recovery
Expand All @@ -101,7 +101,7 @@ export class MultiMainSetup extends EventEmitter {
if (keySetSuccessfully) {
this.logger.debug(`[Instance ID ${this.instanceId}] Leader is now this instance`);

config.set('multiMainSetup.instanceType', 'leader');
config.set('instanceRole', 'leader');

await this.redisPublisher.setExpiration(this.leaderKey, this.leaderKeyTtl);

Expand All @@ -110,7 +110,7 @@ export class MultiMainSetup extends EventEmitter {
*/
this.emit('leader-takeover');
} else {
config.set('multiMainSetup.instanceType', 'follower');
config.set('instanceRole', 'follower');
}
}

Expand Down
2 changes: 1 addition & 1 deletion packages/cli/src/services/pruning.service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ export class PruningService {
if (
config.getEnv('multiMainSetup.enabled') &&
config.getEnv('generic.instanceType') === 'main' &&
config.getEnv('multiMainSetup.instanceType') === 'follower'
config.getEnv('instanceRole') === 'follower'
) {
return false;
}
Expand Down
4 changes: 2 additions & 2 deletions packages/cli/test/unit/services/orchestration.service.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ describe('Orchestration Service', () => {

describe('shouldAddWebhooks', () => {
beforeEach(() => {
config.set('multiMainSetup.instanceType', 'leader');
config.set('instanceRole', 'leader');
});
test('should return true for init', () => {
// We want to ensure that webhooks are populated on init
Expand All @@ -169,7 +169,7 @@ describe('Orchestration Service', () => {
});

test('should return false for update or activate when not leader', () => {
config.set('multiMainSetup.instanceType', 'follower');
config.set('instanceRole', 'follower');
const modes = ['update', 'activate'] as WorkflowActivateMode[];
for (const mode of modes) {
const result = os.shouldAddWebhooks(mode);
Expand Down
Loading