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

Configure agent nodes based on jenkins instance type #513

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+113 −55
Open

Configure agent nodes based on jenkins instance type #513

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
53e37bd
Configure agent nodes based on jenkins instane type
Divyaasm Dec 5, 2024
8d0f60a
Add requested changes
Divyaasm Jan 6, 2025
226d564
Resolve merge conflicts
Divyaasm Jan 6, 2025
6c3d912
Add variable type changes
Divyaasm Jan 9, 2025
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
32 changes: 10 additions & 22 deletions lib/ci-stack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ export interface CIStackProps extends StackProps {
readonly enableViews?: boolean;
/** Use Production Agents */
readonly useProdAgents?: boolean;
/** Specify jenkins instance type */
readonly jenkinsType?: string;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: jenkinsDeploymentType?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Or jenkinsInstanceType, Which name would you recommend?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm good with both.

}

function getServerAccess(serverAccessType: string, restrictServerAccessTo: string): IPeer {
Expand Down Expand Up @@ -113,6 +115,11 @@ export class CIStack extends Stack {
useProdAgents = 'false';
}

let jenkinsType = `${props?.jenkinsType ?? this.node.tryGetContext('jenkinsType')}`;
if (useProdAgents.toString() === 'true' && jenkinsType.toString() === 'undefined') {
jenkinsType = 'default';
}

const serverAccessType = this.node.tryGetContext('serverAccessType');
const restrictServerAccessTo = this.node.tryGetContext('restrictServerAccessTo');
const serverAcess = props?.restrictServerAccessTo ?? getServerAccess(serverAccessType, restrictServerAccessTo);
Expand Down Expand Up @@ -146,34 +153,15 @@ export class CIStack extends Stack {
const listenerCertificate = ListenerCertificate.fromArn(certificateArn.secretValue.toString());
const agentNode = new AgentNodes(this);

if (useProdAgents.toString() === 'true') {
// eslint-disable-next-line no-console
console.warn('Please note that if you have decided to use the provided production jenkins agents then '
// eslint-disable-next-line no-console
console.warn('Please note that if you have decided to use the provided production jenkins agents then '
Copy link
Member

Choose a reason for hiding this comment

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

Isn't the conditional useProdAgents required here?
The warning was only relevant if the ProdAgents parameter is enabled.

+ 'please make sure that you are deploying the stack in US-EAST-1 region as the AMIs used are only publicly '
+ 'available in US-EAST-1 region. '
+ 'If you want to deploy the stack in another region then please make sure you copy the public AMIs used '
+ 'from us-east-1 region to your region of choice and update the ami-id in agent-nodes.ts file accordingly. '
+ 'If you do not copy the AMI in required region and update the code then the jenkins agents will not spin up.');

this.agentNodes = [
agentNode.AL2023_X64,
agentNode.AL2_X64_DOCKER_HOST,
agentNode.AL2023_X64_DOCKER_HOST,
agentNode.AL2023_ARM64,
agentNode.AL2_ARM64_DOCKER_HOST,
agentNode.AL2023_ARM64_DOCKER_HOST,
agentNode.AL2023_X64_BENCHMARK_TEST,
agentNode.UBUNTU2004_X64_GRADLE_CHECK,
agentNode.UBUNTU2004_X64_DOCKER_BUILDER,
agentNode.MACOS13_X64_MULTI_HOST,
agentNode.MACOS13_ARM64_MULTI_HOST,
agentNode.WINDOWS2019_X64_DOCKER_HOST,
agentNode.WINDOWS2019_X64_DOCKER_BUILDER,
agentNode.WINDOWS2019_X64_GRADLE_CHECK,
];
} else {
this.agentNodes = [agentNode.AL2_X64_DEFAULT_AGENT, agentNode.AL2_ARM64_DEFAULT_AGENT];
}
this.agentNodes = agentNode.getRequiredAgentNodes(jenkinsType.toString());

const mainJenkinsNode = new JenkinsMainNode(this, {
vpc,
Expand Down
11 changes: 11 additions & 0 deletions lib/compute/agent-nodes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,4 +281,15 @@ export class AgentNodes {
remoteFs: '/home/ec2-user',
};
}

public getRequiredAgentNodes(type: string): any[] {
return Object.keys(this)
.filter((key) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly we are just checking if default exists in the key name and then return the list of keys.
To make it extendable, do you think it will be better to add another label to workerLabelString and use that to filter out agent nodes per use case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We are doing similar thing with jenkinsType variable.
If jenkinsType value is not passed we assume it's default(this option is mostly used for testing), in all other cases we retrieve the agent nodes list with the value passed in jenkinsType(Ex: benchmark, gradle). But for BTR we need to exclude the agentnode names having the values ('default', 'benchmark', 'gradle').

The Drawback is agentnodes used by BTR instance cannot be differentiated from their names like we do with benchmark and gradle. We can think of updating agentnodes names used by BTR with a key_name(RELEASE) if it's straightforward.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think since we have a defined number of instance type, better have a check, may be enums, to verify if a valid instance type name is passed.
If invalid name is passed then throw an error.

Copy link
Member

@gaiksaya gaiksaya Dec 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed. Have enums for instance types. Also what is BTR? 😅
Is it build/test/release?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes Sayali

if (type === 'BTR') {
return !key.toLowerCase().includes('default');
}
return key.toLowerCase().includes(type.toLowerCase());
})
.map((key) => (this as any)[key]);
}
}
43 changes: 41 additions & 2 deletions test/ci-stack.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,17 @@ import { App } from 'aws-cdk-lib';
import { Template } from 'aws-cdk-lib/assertions';
import { Peer } from 'aws-cdk-lib/aws-ec2';
import { CIStack } from '../lib/ci-stack';
import { AgentNodes } from '../lib/compute/agent-nodes';

test('CI Stack Basic Resources', () => {
const app = new App({
context: {
useSsl: 'true', runWithOidc: 'true', serverAccessType: 'ipv4', restrictServerAccessTo: '10.10.10.10/32', additionalCommands: './test/data/hello-world.py',
useSsl: 'true',
runWithOidc: 'true',
serverAccessType: 'ipv4',
restrictServerAccessTo: '10.10.10.10/32',
additionalCommands: './test/data/hello-world.py',
jenkinsType: 'BTR',
},
});

Expand Down Expand Up @@ -45,7 +51,7 @@ test('CI Stack Basic Resources', () => {
test('External security group is open', () => {
const app = new App({
context: {
useSsl: 'true', runWithOidc: 'true', serverAccessType: 'ipv4', restrictServerAccessTo: 'all',
useSsl: 'true', runWithOidc: 'true', serverAccessType: 'ipv4', restrictServerAccessTo: 'all', jenkinsType: 'BTR',
},
});

Expand Down Expand Up @@ -364,6 +370,39 @@ test('LoadBalancer Access Logging', () => {
});
});

describe('AgentNodes', () => {
let agentNodes: AgentNodes;
const app = new App({
context: {
useSsl: 'false', runWithOidc: 'false', serverAccessType: 'ipv4', restrictServerAccessTo: '10.10.10.10/32',
},
});

// WHEN
const stack = new CIStack(app, 'MyTestStack', {
env: { account: 'test-account', region: 'us-east-1' },
});

beforeEach(() => {
agentNodes = new AgentNodes(stack);
});

it('should exclude "default" keys when type is "BTR"', () => {
const result = agentNodes.getRequiredAgentNodes('BTR');
expect(result.length).toBe(14);
});

it('should return keys containing "benchmark" when type is "benchmark"', () => {
const result = agentNodes.getRequiredAgentNodes('benchmark');
expect(result).toHaveLength(1);
});

it('should return keys containing "gradle" when type is "gradle"', () => {
const result = agentNodes.getRequiredAgentNodes('default');
expect(result).toHaveLength(2);
});
});

test('WAF rules', () => {
const app = new App({
context: {
Expand Down
4 changes: 2 additions & 2 deletions test/compute/agent-node-config.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { CIStack } from '../../lib/ci-stack';
test('Agents Resource is present', () => {
const app = new App({
context: {
useSsl: 'true', runWithOidc: 'true', serverAccessType: 'ipv4', restrictServerAccessTo: '10.10.10.10/32',
useSsl: 'true', runWithOidc: 'true', serverAccessType: 'ipv4', restrictServerAccessTo: '10.10.10.10/32', jenkinsType: 'BTR',
},
});
const stack = new CIStack(app, 'TestStack', {
Expand Down Expand Up @@ -83,7 +83,7 @@ test('Agents Resource is present', () => {
test('Agents Node policy with assume role Resource is present', () => {
const app = new App({
context: {
useSsl: 'true', runWithOidc: 'true', serverAccessType: 'ipv4', restrictServerAccessTo: '10.10.10.10/32',
useSsl: 'true', runWithOidc: 'true', serverAccessType: 'ipv4', restrictServerAccessTo: '10.10.10.10/32', jenkinsType: 'BTR',
},
});
const stack = new CIStack(app, 'TestStack', {
Expand Down
Loading