Skip to content

Commit

Permalink
Fix race condition with concurrently installs of service
Browse files Browse the repository at this point in the history
  • Loading branch information
TJ Higgins committed Jun 3, 2019
1 parent e42a6e5 commit 84aa8a6
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 31 deletions.
7 changes: 6 additions & 1 deletion src/commands/build.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ export default class Build extends Command {
char: 'r',
default: false,
description: 'Whether or not to build images for the cited dependencies'
}),
verbose: flags.boolean({
char: 'v',
description: 'Verbose log output'
})
};

Expand Down Expand Up @@ -85,7 +89,8 @@ export default class Build extends Command {
root_service_path = path.resolve(args.context);
}

const tasks = new Listr(await Build.getTasks(root_service_path, flags.tag, flags.recursive), { concurrent: 2 });
const renderer = flags.verbose ? 'verbose' : 'default';
const tasks = new Listr(await Build.getTasks(root_service_path, flags.tag, flags.recursive), { concurrent: 2, renderer });
await tasks.run();
}
}
11 changes: 8 additions & 3 deletions src/commands/install.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,15 @@ export default class Install extends Command {
help: flags.help({ char: 'h' }),
prefix: flags.string({
char: 'p',
description: 'Path prefix indicating where the install command should execute from.'
description: 'Path prefix indicating where the install command should execute from'
}),
recursive: flags.boolean({
char: 'r',
description: 'Generate architect dependency files for all services in the dependency tree.'
description: 'Generate architect dependency files for all services in the dependency tree'
}),
verbose: flags.boolean({
char: 'v',
description: 'Verbose log output'
})
};

Expand Down Expand Up @@ -70,7 +74,8 @@ export default class Install extends Command {
path.join(process_path, flags.prefix);
}

const tasks = new Listr(await Install.getTasks(process_path, flags.recursive), { concurrent: 3 });
const renderer = flags.verbose ? 'verbose' : 'default';
const tasks = new Listr(await Install.getTasks(process_path, flags.recursive), { concurrent: 3, renderer });
await tasks.run();
}
}
7 changes: 6 additions & 1 deletion src/commands/push.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,10 @@ export default class Push extends Command {
char: 'r',
default: false,
description: 'Whether or not to build images for the cited dependencies'
}),
verbose: flags.boolean({
char: 'v',
description: 'Verbose log output'
})
};

Expand All @@ -48,7 +52,8 @@ export default class Push extends Command {
root_service_path = path.resolve(args.context);
}

const tasks = new Listr(await this.getTasks(root_service_path, flags.tag, flags.recursive), { concurrent: 2 });
const renderer = flags.verbose ? 'verbose' : 'default';
const tasks = new Listr(await this.getTasks(root_service_path, flags.tag, flags.recursive), { concurrent: 2, renderer });
await tasks.run();
}

Expand Down
30 changes: 15 additions & 15 deletions src/common/protoc-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,43 +19,43 @@ namespace ProtocExecutor {
if (!dependency_config.proto) {
throw new Error(`${dependency_config.name} has no .proto file configured.`);
}
const dependency_folder = ServiceConfig.convertServiceNameToFolderName(dependency_config.name);
const target_config = ServiceConfig.loadFromPath(target_path);
// Prevent race conditions when building the same service concurrently for different targets
const namespace = `${ServiceConfig.convertServiceNameToFolderName(target_config.name)}__${dependency_folder}`;

// Make the folder to store dependency stubs
const stubs_directory = path.join(target_path, MANAGED_PATHS.DEPENDENCY_STUBS_DIRECTORY);
if (!existsSync(stubs_directory)) {
mkdirSync(stubs_directory);
}

const stub_directory = path.join(stubs_directory, ServiceConfig.convertServiceNameToFolderName(dependency_config.name));
const stub_directory = path.join(target_path, MANAGED_PATHS.DEPENDENCY_STUBS_DIRECTORY, dependency_folder);
if (!existsSync(stub_directory)) {
mkdirSync(stub_directory);
mkdirSync(stub_directory, { recursive: true });
}

const tmpRoot = realpathSync(os.tmpdir());
const tmpDir = path.join(tmpRoot, ServiceConfig.convertServiceNameToFolderName(dependency_config.name));
if (!existsSync(tmpDir)) {
mkdirSync(tmpDir);
const tmp_root = realpathSync(os.tmpdir());
const tmp_dir = path.join(tmp_root, namespace);
const tmp_dependency_dir = path.join(tmp_dir, dependency_folder);
if (!existsSync(tmp_dependency_dir)) {
mkdirSync(tmp_dependency_dir, { recursive: true });
}
copyFileSync(
path.join(dependency_path, dependency_config.proto),
path.join(tmpDir, dependency_config.proto)
path.join(tmp_dependency_dir, dependency_config.proto)
);

const mount_dirname = '/opt/protoc';
const mounted_proto_path = path.posix.join(mount_dirname, ServiceConfig.convertServiceNameToFolderName(dependency_config.name), dependency_config.proto);
const mounted_proto_path = path.posix.join(mount_dirname, dependency_folder, dependency_config.proto);

await execa.shell([
'docker', 'run',
'-v', `${target_path}:/defs`,
'-v', `${tmpRoot}:${mount_dirname}`,
'-v', `${tmp_dir}:${mount_dirname}`,
'--user', process.platform === 'win32' ? '1000:1000' : '$(id -u):$(id -g)', // TODO figure out correct user for windows
'architectio/protoc-all',
'-f', `${mounted_proto_path}`,
'-i', mount_dirname,
'-l', target_language,
'-o', MANAGED_PATHS.DEPENDENCY_STUBS_DIRECTORY
].join(' '));
await execa.shell(`rm -rf ${tmpDir}`);
await execa.shell(`rm -rf ${tmp_dir}`);

_postHooks(stub_directory, target_language);
};
Expand Down
6 changes: 4 additions & 2 deletions src/common/service-config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,7 @@ export default class ServiceConfig {
const service_config = service_dependency.service_config;
service_dependencies.push(service_dependency);

if (recursive) {
if (root_service_path === service_dependency.service_path || recursive) {
const dependency_names = Object.keys(service_config.dependencies);
for (let dependency_name of dependency_names) {
const dependency_path = ServiceConfig.parsePathFromDependencyIdentifier(
Expand All @@ -83,7 +83,9 @@ export default class ServiceConfig {
service_config: dependency_config,
dependencies: []
};
queue.push(services_map[dependency_path]);
if (recursive) {
queue.push(services_map[dependency_path]);
}
}
service_dependency.dependencies.push(services_map[dependency_path]);
}
Expand Down
8 changes: 4 additions & 4 deletions test/commands/build.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ describe('build', () => {
test
.stdout()
.timeout(10000)
.command(['build', './test/calculator-example/addition-service/'])
.command(['build', './test/calculator-example/addition-service/', '--verbose'])
.it('builds docker image', ctx => {
const { stdout } = ctx;
expect(stdout).to.contain('Building docker image for addition-service');
Expand All @@ -18,7 +18,7 @@ describe('build', () => {
test
.stdout()
.timeout(10000)
.command(['build', '--tag', 'tag-override', './test/calculator-example/addition-service/'])
.command(['build', '--tag', 'tag-override', './test/calculator-example/addition-service/', '--verbose'])
.it('allows tag overrides', ctx => {
const { stdout } = ctx;
expect(stdout).to.contain('Building docker image for addition-service');
Expand All @@ -30,8 +30,8 @@ describe('build', () => {

test
.stdout()
.timeout(15000)
.command(['build', '--recursive', './test/calculator-example/python-subtraction-service/'])
.timeout(20000)
.command(['build', '--recursive', './test/calculator-example/python-subtraction-service/', '--verbose'])
.it('builds images recursively', ctx => {
const { stdout } = ctx;
expect(stdout).to.contain('Building docker image for subtraction-service');
Expand Down
11 changes: 6 additions & 5 deletions test/commands/install.test.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
import {expect, test} from '@oclif/test';
import { expect, test } from '@oclif/test';

describe('install', () => {
test
.stdout()
.command(['install', '--prefix', './test/calculator-example/addition-service/'])
.command(['install', '--prefix', './test/calculator-example/addition-service/', '--verbose'])
.it('installs dependency stubs', ctx => {
const {stdout} = ctx;
const { stdout } = ctx;
expect(stdout).to.contain('Installing dependencies for addition-service');
});

Expand All @@ -15,10 +15,11 @@ describe('install', () => {
.command([
'install',
'--recursive',
'--prefix', './test/calculator-example/test-script/'
'--prefix', './test/calculator-example/test-script/',
'--verbose'
])
.it('installs dependencies recursively', ctx => {
const {stdout} = ctx;
const { stdout } = ctx;
expect(stdout).to.contain('Installing dependencies for test-service');
expect(stdout).to.contain('Installing dependencies for division-service');
expect(stdout).to.contain('Installing dependencies for subtraction-service');
Expand Down

0 comments on commit 84aa8a6

Please sign in to comment.