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

Add env to disable worker restart #10664

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Add env to disable worker restart #10664

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
037a57d
refactor(cosmic-swingset): plumb maxVatsOnline explicitely
mhofman Dec 10, 2024
462c786
feat(cosmic-swingset): set restartWorkerOnSnapshot policy from env
mhofman Dec 10, 2024
d050fe0
ci: disable restart on snapshot to verify deterministic snapshots
mhofman Dec 10, 2024
153ab38
fixup! refactor(cosmic-swingset): plumb maxVatsOnline explicitely
mhofman Dec 10, 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
8 changes: 7 additions & 1 deletion packages/cosmic-swingset/src/chain-main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -302,6 +302,7 @@ export default async function main(
const swingsetConfig = harden(initAction.resolvedConfig || {});
validateSwingsetConfig(swingsetConfig);
const {
maxVatsOnline,
slogfile,
vatSnapshotRetention,
vatTranscriptRetention,
Expand All @@ -315,6 +316,10 @@ export default async function main(
? vatTranscriptRetention !== 'operational'
: false;

const restartWorkerOnSnapshot = processValue.getBoolean({
envName: 'XSNAP_RESTART_ON_SNAPSHOT',
Copy link
Member

Choose a reason for hiding this comment

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

This should be documented in env.md.

Copy link
Member Author

Choose a reason for hiding this comment

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

Since this config is fairly experimental, it is not currently documented anywhere. This is in line with similar XSNAP env.

Copy link
Member

Choose a reason for hiding this comment

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

Then let's document it as experimental. I don't like having environment variable sensitivity that can only be discovered by reading the code.

});

// As a kludge, back-propagate selected configuration into environment variables.
// eslint-disable-next-line dot-notation
if (slogfile) env['SLOGFILE'] = slogfile;
Expand Down Expand Up @@ -558,7 +563,8 @@ export default async function main(
archiveSnapshot,
archiveTranscript,
afterCommitCallback,
swingsetConfig,
maxVatsOnline,
restartWorkerOnSnapshot,
});

const { blockingSend, shutdown } = s;
Expand Down
13 changes: 7 additions & 6 deletions packages/cosmic-swingset/src/launch-chain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,10 +307,8 @@ export async function buildSwingset(
* @property {ReturnType<typeof import('@agoric/swing-store').makeArchiveSnapshot>} [archiveSnapshot]
* @property {ReturnType<typeof import('@agoric/swing-store').makeArchiveTranscript>} [archiveTranscript]
* @property {() => object | Promise<object>} [afterCommitCallback]
* @property {import('./chain-main.js').CosmosSwingsetConfig} swingsetConfig
* TODO refactor to clarify relationship vs. import('@agoric/swingset-vat').SwingSetConfig
* --- maybe partition into in-consensus "config" vs. consensus-independent "options"?
* (which would mostly just require `bundleCachePath` to become a `buildSwingset` input)
* @property {number} [maxVatsOnline]
* @property {boolean} [restartWorkerOnSnapshot]
Comment on lines -310 to +311
Copy link
Member

Choose a reason for hiding this comment

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

This feels like a step in the wrong direction—I'd really like to move towards better categorization of launch inputs than just a flat object with way too many properties.

Copy link
Member Author

@mhofman mhofman Dec 10, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree but right now passing the config itself for just picking a single consensus-independent option felt wrong too, especially because I didn't want this new option to be part of the cosmos driven config.

Also that same file used swingsetConfig to designate something else entirely, so removing stops the ambiguity

Copy link
Member

Choose a reason for hiding this comment

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

That's fair, but the CosmosSwingsetConfig vs. SwingSetConfig problem still exists. I guess I'm fine with this refactoring if it doesn't lose the comment (or a variation thereof).

*/

/**
Expand Down Expand Up @@ -340,7 +338,8 @@ export async function launch({
archiveSnapshot,
archiveTranscript,
afterCommitCallback = async () => ({}),
swingsetConfig,
maxVatsOnline,
restartWorkerOnSnapshot,
}) {
console.info('Launching SwingSet kernel');

Expand Down Expand Up @@ -418,8 +417,10 @@ export async function launch({
});

console.debug(`buildSwingset`);
/** @type {import('@agoric/swingset-vat').VatWarehousePolicy} */
const warehousePolicy = {
maxVatsOnline: swingsetConfig.maxVatsOnline,
maxVatsOnline,
restartWorkerOnSnapshot,
};
const {
coreProposals: bootstrapCoreProposals,
Expand Down
1 change: 0 additions & 1 deletion packages/cosmic-swingset/src/sim-chain.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,6 @@ export async function connectToFakeChain(basedir, GCI, delay, inbound) {
debugName: GCI,
metricsProvider,
slogSender,
swingsetConfig: {},
});

const { blockingSend, savedHeight } = s;
Expand Down
3 changes: 0 additions & 3 deletions packages/cosmic-swingset/tools/test-kit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@ const baseConfig = harden({
* @param {Replacer<SwingSetConfig>} [options.fixupConfig] a final opportunity
* to make any changes
* @param {import('@agoric/telemetry').SlogSender} [options.slogSender]
* @param {import('../src/chain-main.js').CosmosSwingsetConfig} [options.swingsetConfig]
* @param {import('@agoric/swing-store').SwingStore} [options.swingStore]
* defaults to a new in-memory store
* @param {SwingSetConfig['vats']} [options.vats] extra static vat configuration
Expand Down Expand Up @@ -170,7 +169,6 @@ export const makeCosmicSwingsetTestKit = async (
fixupInitMessage,
fixupConfig,
slogSender,
swingsetConfig = {},
swingStore,
vats,

Expand Down Expand Up @@ -251,7 +249,6 @@ export const makeCosmicSwingsetTestKit = async (
env,
debugName,
slogSender,
swingsetConfig,
});
const { blockingSend, shutdown: shutdownKernel } = launchResult;
/** @type {(options?: { kernelOnly?: boolean }) => Promise<void>} */
Expand Down
1 change: 1 addition & 0 deletions packages/deployment/scripts/integration-test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ if [ -n "$LOADGEN" ]; then
"$AG_SETUP_COSMOS_HOME/faucet-helper.sh" add-egress loadgen "$SOLO_ADDR"
SLOGSENDER=@agoric/telemetry/src/otel-trace.js SOLO_SLOGSENDER="" \
SLOGSENDER_FAIL_ON_ERROR=1 SLOGSENDER_AGENT=process \
XSNAP_RESTART_ON_SNAPSHOT=false \
AG_CHAIN_COSMOS_HOME=$HOME/.agoric \
SDK_BUILD=0 MUST_USE_PUBLISH_BUNDLE=1 SDK_SRC=$SDK_SRC OUTPUT_DIR="$RESULTSDIR" ./start.sh \
--no-stage.save-storage \
Expand Down
Loading