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.

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

feat: proposal pre/post test steps #210

Merged
merged 31 commits into from
Feb 7, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
2660fec
hooks
usmanmani1122 Jan 1, 2025
8e3f3e4
Merge branch "usman/prepare-test-hook" into branch "usman/before-afte…
usmanmani1122 Jan 1, 2025
df80864
nit
usmanmani1122 Jan 1, 2025
0554920
fixes
usmanmani1122 Jan 3, 2025
5d4e465
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Jan 3, 2025
2c94fef
document prepare-test hook
usmanmani1122 Jan 16, 2025
a689ecc
address comments
usmanmani1122 Jan 22, 2025
61371f5
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Jan 22, 2025
9f24947
increase test timeout
usmanmani1122 Jan 22, 2025
3d6780c
increase test timeout 2.0
usmanmani1122 Jan 22, 2025
5b4c029
fix for spawn
usmanmani1122 Jan 22, 2025
608e2da
:)
usmanmani1122 Jan 22, 2025
0ee2a77
increase test timeout 3.0
usmanmani1122 Jan 22, 2025
7ad7d92
revert
usmanmani1122 Jan 22, 2025
f55ef1f
address comments
usmanmani1122 Jan 23, 2025
8b22861
Empty
usmanmani1122 Jan 23, 2025
b89fe4f
Empty
usmanmani1122 Jan 23, 2025
6d5c227
Empty
usmanmani1122 Jan 23, 2025
f121f67
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Jan 24, 2025
679a083
Address comments
usmanmani1122 Jan 24, 2025
f983826
lint
usmanmani1122 Jan 24, 2025
36a2a20
use tmp directory
usmanmani1122 Jan 27, 2025
5d54940
nit
usmanmani1122 Jan 28, 2025
ef3d10b
address comments + fix bug
usmanmani1122 Jan 30, 2025
963e59f
lint
usmanmani1122 Jan 30, 2025
1a137a4
nit
usmanmani1122 Jan 31, 2025
12c5042
nit
usmanmani1122 Feb 4, 2025
ac2c07b
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Feb 4, 2025
b34bdb6
Merge branch 'main' into usman/before-after-test-scripts
usmanmani1122 Feb 7, 2025
73a9c4b
add test + host folder notes
usmanmani1122 Feb 7, 2025
9d67bc1
lint
usmanmani1122 Feb 7, 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
4 changes: 3 additions & 1 deletion packages/synthetic-chain/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,15 @@
"chalk": "^5.4.1",
"cosmjs-types": "^0.9.0",
"execa": "^9.5.2",
"glob": "^11.0.0"
"glob": "^11.0.0",
"tmp": "0.2.3"
},
"devDependencies": {
"@agoric/cosmic-proto": "0.5.0-u18.5",
"@types/better-sqlite3": "^7.6.11",
"@types/glob": "^8.1.0",
"@types/node": "^18.19.50",
"@types/tmp": "0.2.6",
"ava": "^6.2.0",
"ts-blank-space": "^0.5.0",
"tsup": "^8.3.5",
Expand Down
2 changes: 1 addition & 1 deletion packages/synthetic-chain/src/cli/cli.ts
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ switch (cmd) {
console.log(chalk.cyan.bold(`Testing ${proposal.proposalName}`));
const image = imageNameForProposal(proposal, 'test');
bakeTarget(image.target, values.dry);
runTestImage(proposal);
runTestImage({ proposal });
// delete the image to reclaim disk space. The next build
// will use the build cache.
execSync('docker system df', { stdio: 'inherit' });
Expand Down
85 changes: 74 additions & 11 deletions packages/synthetic-chain/src/cli/dockerfileGen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,12 @@ ENV UPGRADE_TO=${to}
RUN echo '. /usr/src/upgrade-test-scripts/env_setup.sh' >> ~/.bashrc

# copy scripts
COPY --link --chmod=755 ./upgrade-test-scripts/env_setup.sh ./upgrade-test-scripts/run_prepare_zero.sh /usr/src/upgrade-test-scripts/
${createCopyCommand(
[],
'./upgrade-test-scripts/env_setup.sh',
'./upgrade-test-scripts/run_prepare_zero.sh',
'/usr/src/upgrade-test-scripts/',
)}
SHELL ["/bin/bash", "-c"]
# this is the only layer that starts ag0
RUN /usr/src/upgrade-test-scripts/run_prepare_zero.sh
Expand Down Expand Up @@ -75,8 +80,18 @@ ENV \
UPGRADE_INFO=${JSON.stringify(encodeUpgradeInfo(upgradeInfo))} \
SKIP_PROPOSAL_VALIDATION=${skipProposalValidation}

COPY --exclude=host --exclude=test --exclude=test.sh --link --chmod=755 ./proposals/${path} /usr/src/proposals/${path}
COPY --link --chmod=755 ./upgrade-test-scripts/env_setup.sh ./upgrade-test-scripts/run_prepare.sh ./upgrade-test-scripts/start_to_to.sh /usr/src/upgrade-test-scripts/
${createCopyCommand(
['host', 'node_modules', 'test', 'test.sh'],
`./proposals/${path}`,
`/usr/src/proposals/${path}`,
)}
${createCopyCommand(
[],
'./upgrade-test-scripts/env_setup.sh',
'./upgrade-test-scripts/run_prepare.sh',
'./upgrade-test-scripts/start_to_to.sh',
'/usr/src/upgrade-test-scripts/',
)}
WORKDIR /usr/src/upgrade-test-scripts
SHELL ["/bin/bash", "-c"]
RUN ./run_prepare.sh ${path}
Expand All @@ -100,8 +115,19 @@ FROM ghcr.io/agoric/agoric-sdk:${sdkImageTag} as execute-${proposalName}
WORKDIR /usr/src/upgrade-test-scripts

# base is a fresh sdk image so set up the proposal and its dependencies
COPY --exclude=host --exclude=test --exclude=test.sh --link --chmod=755 ./proposals/${path} /usr/src/proposals/${path}
COPY --link --chmod=755 ./upgrade-test-scripts/env_setup.sh ./upgrade-test-scripts/run_execute.sh ./upgrade-test-scripts/start_to_to.sh ./upgrade-test-scripts/install_deps.sh /usr/src/upgrade-test-scripts/
${createCopyCommand(
['host', 'node_modules', 'test', 'test.sh'],
`./proposals/${path}`,
`/usr/src/proposals/${path}`,
)}
${createCopyCommand(
['test.sh'],
'./upgrade-test-scripts/env_setup.sh',
'./upgrade-test-scripts/run_execute.sh',
'./upgrade-test-scripts/start_to_to.sh',
'./upgrade-test-scripts/install_deps.sh',
'/usr/src/upgrade-test-scripts/',
)}
RUN --mount=type=cache,target=/root/.yarn ./install_deps.sh ${path}

COPY --link --from=prepare-${proposalName} /root/.agoric /root/.agoric
Expand All @@ -122,15 +148,27 @@ RUN ./run_execute.sh ${planName}
# EVAL ${proposalName}
FROM use-${lastProposal.proposalName} as eval-${proposalName}

COPY --exclude=host --exclude=test --exclude=test.sh --link --chmod=755 ./proposals/${path} /usr/src/proposals/${path}
${createCopyCommand(
['host', 'node_modules', 'test', 'test.sh'],
`./proposals/${path}`,
`/usr/src/proposals/${path}`,
)}

WORKDIR /usr/src/upgrade-test-scripts

# First stage of this proposal so install its deps.
COPY --link ./upgrade-test-scripts/install_deps.sh /usr/src/upgrade-test-scripts/
${createCopyCommand(
[],
'./upgrade-test-scripts/install_deps.sh',
'/usr/src/upgrade-test-scripts/',
)}
RUN --mount=type=cache,target=/root/.yarn ./install_deps.sh ${path}

COPY --link --chmod=755 ./upgrade-test-scripts/*eval* /usr/src/upgrade-test-scripts/
${createCopyCommand(
[],
'./upgrade-test-scripts/*eval*',
'/usr/src/upgrade-test-scripts/',
)}
SHELL ["/bin/bash", "-c"]
RUN ./run_eval.sh ${path}
`;
Expand All @@ -149,7 +187,12 @@ FROM ${previousStage}-${proposalName} as use-${proposalName}

WORKDIR /usr/src/upgrade-test-scripts

COPY --link --chmod=755 ./upgrade-test-scripts/run_use.sh ./upgrade-test-scripts/start_agd.sh /usr/src/upgrade-test-scripts/
${createCopyCommand(
[],
'./upgrade-test-scripts/run_use.sh',
'./upgrade-test-scripts/start_agd.sh',
'/usr/src/upgrade-test-scripts/',
)}
SHELL ["/bin/bash", "-c"]
RUN ./run_use.sh ${path}
ENTRYPOINT ./start_agd.sh
Expand All @@ -171,11 +214,19 @@ FROM use-${proposalName} as test-${proposalName}
# Previous stages copied excluding test files (see COPY above). It would be good
# to copy only missing files, but there may be none. Fortunately, copying extra
# does not invalidate other images because nothing depends on this layer.
COPY --exclude=host --link --chmod=755 ./proposals/${path} /usr/src/proposals/${path}
${createCopyCommand(
['host', 'node_modules'],
`./proposals/${path}`,
`/usr/src/proposals/${path}`,
)}

WORKDIR /usr/src/upgrade-test-scripts

COPY --link --chmod=755 ./upgrade-test-scripts/run_test.sh /usr/src/upgrade-test-scripts/
${createCopyCommand(
[],
'./upgrade-test-scripts/run_test.sh',
'/usr/src/upgrade-test-scripts/',
)}
SHELL ["/bin/bash", "-c"]
ENTRYPOINT ./run_test.sh ${path}
`;
Expand All @@ -196,6 +247,18 @@ FROM ${useImage} as latest
},
};

export const createCopyCommand = (
exclusionList: Array<string>,
...files: Array<string>
) =>
[
'COPY',
'--link',
'--chmod=755',
...exclusionList.map(excluded => `--exclude=${excluded}`),
...files,
].join(' ');

export function writeBakefileProposals(
allProposals: ProposalInfo[],
platforms?: Platform[],
Expand Down
125 changes: 70 additions & 55 deletions packages/synthetic-chain/src/cli/run.ts
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
import { spawnSync } from 'node:child_process';
import { existsSync, realpathSync } from 'node:fs';
import { resolve as resolvePath } from 'node:path';
import { basename, resolve as resolvePath } from 'node:path';
import { fileSync as createTempFile } from 'tmp';
import { ProposalInfo, imageNameForProposal } from './proposals.js';

const createMessageFile = () => {
const messageFileName = `${new Date().getTime()}.tmp`;
const messageFilePath = `/tmp/${messageFileName}`;
spawnSync('touch', [messageFilePath]);
return [messageFileName, messageFilePath];
};
const createMessageFile = (proposal: ProposalInfo) =>
createTempFile({ prefix: proposal.proposalName });

const executeHostScriptIfPresent = (
extraEnv: typeof process.env,
proposal: ProposalInfo,
scriptName: string,
) => {
Expand All @@ -19,7 +17,10 @@ const executeHostScriptIfPresent = (
console.log(
`Running script ${scriptName} for proposal ${proposal.proposalName}`,
);
spawnSync(scriptPath, { env: process.env, stdio: 'inherit' });
spawnSync(scriptPath, {
env: { ...process.env, ...extraEnv },
stdio: 'inherit',
});
}
};

Expand All @@ -45,77 +46,91 @@ const propagateSlogfile = env => {
];
};

export const runTestImage = (proposal: ProposalInfo) => {
const [messageFileName, messageFilePath] = createMessageFile();
export const runTestImage = ({
extraDockerArgs = [],
proposal,
removeContainerOnExit = true,
}: {
extraDockerArgs?: Array<string>;
proposal: ProposalInfo;
removeContainerOnExit?: boolean;
}) => {
const { name: messageFilePath, removeCallback: removeTempFileCallback } =
createMessageFile(proposal);
const messageFileName = basename(messageFilePath);

const containerFilePath = `/root/${messageFileName}`;
Copy link
Member

Choose a reason for hiding this comment

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

I thought we would have the message file in the container use a static name (not even related to the proposal name) to avoid having to plumb an extra env.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Container file doesn't necessarily need to use the proposal name in it's path, but we still need to pass the host file path to the host start script so that the follower can mount that file in it's container

Copy link
Member

Choose a reason for hiding this comment

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

Right but the code above will make the container file name dynamic, without any way for the container script to figure out what that name is. Or am I missing something?

process.env.MESSAGE_FILE_PATH = messageFilePath;

executeHostScriptIfPresent(proposal, 'before-test-run.sh');

console.log(`Running test image for proposal ${proposal.proposalName}`);
const { name } = imageNameForProposal(proposal, 'test');
spawnSync(
'docker',
[
'run',
'--env',
`MESSAGE_FILE_PATH=${containerFilePath}`,
'--mount',
`source=${messageFilePath},target=${containerFilePath},type=bind`,
'--network',
'host',
'--rm',
...propagateSlogfile(process.env),
name,
],
{ stdio: 'inherit' },
);
try {
executeHostScriptIfPresent(
{
MESSAGE_FILE_PATH: messageFilePath,
},
proposal,
'before-test-run.sh',
);

spawnSync('rm', ['--force', messageFilePath]);
console.log(`Running test image for proposal ${proposal.proposalName}`);
const { name } = imageNameForProposal(proposal, 'test');
spawnSync(
'docker',
[
'run',
'--env',
`MESSAGE_FILE_PATH=${containerFilePath}`,
'--mount',
`source=${messageFilePath},target=${containerFilePath},type=bind`,
'--network',
'host',
Copy link
Member

Choose a reason for hiding this comment

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

I'm pretty sure this is a bad idea. Why do we need to hook onto the host network interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we don't use host network, we would need individual container IPs
Using host network will make the RPC/Peer available on localhost (using different ports)

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand. How is different IPs different than different ports?
Also afaik, only the acceptance test needs to be connected to, so couldn't we only target that container?

Copy link
Member

Choose a reason for hiding this comment

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

I never submitted this last week, but for future reference:

We can get the IP address from inside the container using something like hostname -i (trimming trailing space). We already need to get the tendermint node-id and probably should get the chain_id (from genesis file). The setup test script can grab those and send over the message file.

removeContainerOnExit && '--rm',
Copy link
Member

Choose a reason for hiding this comment

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

To avoid the .filter(Boolean).map(String):

Suggested change
removeContainerOnExit && '--rm',
...(removeContainerOnExit ? ['--rm'] : []),

...propagateSlogfile(process.env),
...extraDockerArgs,
name,
]
.filter(Boolean)
.map(String),
{ stdio: 'inherit' },
);

executeHostScriptIfPresent(proposal, 'after-test-run.sh');
executeHostScriptIfPresent(

Choose a reason for hiding this comment

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

So at this point our tests are being executed inside container, right?

{
MESSAGE_FILE_PATH: messageFilePath,
},
proposal,
'after-test-run.sh',
);
} catch (err) {
removeTempFileCallback();
throw err;
}
mhofman marked this conversation as resolved.
Show resolved Hide resolved
};

export const debugTestImage = (proposal: ProposalInfo) => {
executeHostScriptIfPresent(proposal, 'before-test-run.sh');
const { name } = imageNameForProposal(proposal, 'test');
console.log(
`
Starting chain of test image for proposal ${proposal.proposalName}

To get an interactive shell in the container, use an IDE feature like "Attach Shell" or this command:'

docker exec -ti $(docker ps -q -f ancestor=${name}) bash

And within that shell:
cd /usr/src/proposals/${proposal.path} && ./test.sh

To edit files you can use terminal tools like vim, or mount the container in your IDE.
In VS Code the command is:
Dev Containers: Attach to Running Container...
`,
);

// start the chain with ports mapped
spawnSync(
'docker',
[
'run',
return runTestImage({
extraDockerArgs: [
'--entrypoint',
'/usr/src/upgrade-test-scripts/start_agd.sh',
'--interactive',
'--publish',
'1317:1317',
'--publish',
'9090:9090',
'--publish',
'26657:26657',
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 losing these port mappings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because of the host network these will still be available

'--tty',
...propagateSlogfile(process.env),
name,
],
{ stdio: 'inherit' },
);
executeHostScriptIfPresent(proposal, 'after-test-run.sh');
proposal,
removeContainerOnExit: false,
});
};
16 changes: 16 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@ __metadata:
"@types/better-sqlite3": "npm:^7.6.11"
"@types/glob": "npm:^8.1.0"
"@types/node": "npm:^18.19.50"
"@types/tmp": "npm:0.2.6"
ava: "npm:^6.2.0"
better-sqlite3: "npm:^11.8.0"
chalk: "npm:^5.4.1"
cosmjs-types: "npm:^0.9.0"
execa: "npm:^9.5.2"
glob: "npm:^11.0.0"
tmp: "npm:0.2.3"
ts-blank-space: "npm:^0.5.0"
tsup: "npm:^8.3.5"
typescript: "npm:^5.7.3"
Expand Down Expand Up @@ -634,6 +636,13 @@ __metadata:
languageName: node
linkType: hard

"@types/tmp@npm:0.2.6":
version: 0.2.6
resolution: "@types/tmp@npm:0.2.6"
checksum: 10c0/a11bfa2cd8eaa6c5d62f62a3569192d7a2c28efdc5c17af0b0551db85816b2afc8156f3ca15ac76f0b142ae1403f04f44279871424233a1f3390b2e5fc828cd0
languageName: node
linkType: hard

"@vercel/nft@npm:^0.27.5":
version: 0.27.10
resolution: "@vercel/nft@npm:0.27.10"
Expand Down Expand Up @@ -3326,6 +3335,13 @@ __metadata:
languageName: node
linkType: hard

"tmp@npm:0.2.3":
version: 0.2.3
resolution: "tmp@npm:0.2.3"
checksum: 10c0/3e809d9c2f46817475b452725c2aaa5d11985cf18d32a7a970ff25b568438e2c076c2e8609224feef3b7923fa9749b74428e3e634f6b8e520c534eef2fd24125
languageName: node
linkType: hard

"to-regex-range@npm:^5.0.1":
version: 5.0.1
resolution: "to-regex-range@npm:5.0.1"
Expand Down