Skip to content

apollo_deployments,deployment: set storage-reader-server ports as constants#14607

Open
nimrod-starkware wants to merge 1 commit into
nimrod/jsonnet/add-cli-argfrom
nimrod/jsonnet/storage-reader-ports
Open

apollo_deployments,deployment: set storage-reader-server ports as constants#14607
nimrod-starkware wants to merge 1 commit into
nimrod/jsonnet/add-cli-argfrom
nimrod/jsonnet/storage-reader-ports

Conversation

@nimrod-starkware

Copy link
Copy Markdown
Contributor

The three storage-reader-server ports (batcher 55011, class_manager 55210,
state_sync 55014) are fixed infra constants, not per-deployment overrides. Bake
them into the app_configs and applicative_config jsonnet instead of templatizing
them as $$$...$$$ placeholders, and restrict the generic port/url
auto-templatizing in replace_pred to components.* keys only. Add
state_sync_config...network_config.port to KEYS_TO_BE_REPLACED to preserve its
existing overridability. Drop the now-redundant storage-reader port overrides
from the common and testing overlay YAMLs.

Co-Authored-By: Claude Opus 4.8 (1M context) noreply@anthropic.com

@reviewable-StarkWare

Copy link
Copy Markdown

This change is Reviewable

nimrod-starkware commented Jun 23, 2026

Copy link
Copy Markdown
Contributor Author

@nimrod-starkware nimrod-starkware left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nimrod-starkware made 1 comment.
Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion.


crates/apollo_deployments/src/service.rs line 369 at r1 (raw file):

    // auto-templatized — an overridable one must be listed explicitly in `KEYS_TO_BE_REPLACED`;
    // the rest (e.g. the fixed storage-reader-server ports) keep their literal app_config
    // value.

shrink comment

Code quote:

    // Only `components.*` infra fields are auto-templatized: the layout assigns each component's
    // remote URL and port per deployment. Ports/URLs OUTSIDE `components.*` are NOT
    // auto-templatized — an overridable one must be listed explicitly in `KEYS_TO_BE_REPLACED`;
    // the rest (e.g. the fixed storage-reader-server ports) keep their literal app_config
    // value.

@nimrod-starkware nimrod-starkware left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nimrod-starkware resolved 1 discussion.
Reviewable status: 0 of 10 files reviewed, all discussions resolved.

@nimrod-starkware nimrod-starkware left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nimrod-starkware made 1 comment.
Reviewable status: 0 of 10 files reviewed, 1 unresolved discussion.


crates/apollo_deployments/src/service.rs line 103 at r2 (raw file):

    "state_sync_config.static_config.central_sync_client_config.sync_config.store_sierras_and_casms_block_threshold",
    "state_sync_config.static_config.network_config.#is_none",
    "state_sync_config.static_config.network_config.port",

is this needed?

Code quote:

 "state_sync_config.static_config.network_config.port"

@nimrod-starkware nimrod-starkware self-assigned this Jun 25, 2026
@nimrod-starkware
nimrod-starkware marked this pull request as ready for review June 25, 2026 06:54
@cursor

cursor Bot commented Jun 25, 2026

Copy link
Copy Markdown

PR Summary

Medium Risk
Wrong listener ports would break storage-reader connectivity; narrowing replace_pred changes which config keys become deployment placeholders.

Overview
Treats batcher (55011), class_manager (55210), and state_sync (55014) storage-reader-server ports as fixed infra constants instead of per-deployment replacer placeholders.

Those values are set in applicative jsonnet and static/replacer app_configs (replacing shared 8091 / $$$_..._$$$ entries). Hybrid overlay sequencerConfig lines that only repeated those ports are removed; Kubernetes service.ports for the storage-reader listeners are unchanged.

replace_pred in service.rs now auto-templatizes only components.* .port / .url keys (plus KEYS_TO_BE_REPLACED). state_sync_config.static_config.network_config.port is added to KEYS_TO_BE_REPLACED so P2P network port stays explicitly overridable like before.

Reviewed by Cursor Bugbot for commit 5747cee. Bugbot is set up for automated code reviews on this repo. Configure here.

@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/add-cli-arg branch from 982abe5 to 9122ce3 Compare July 6, 2026 08:03
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from 9896355 to 8f6b34c Compare July 6, 2026 08:03
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/add-cli-arg branch from 9122ce3 to d444ae2 Compare July 6, 2026 08:10
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from 8f6b34c to e7ae755 Compare July 6, 2026 08:10
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/add-cli-arg branch from d444ae2 to 895dac1 Compare July 6, 2026 08:29
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from e7ae755 to 347c7b5 Compare July 6, 2026 08:29
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from 347c7b5 to ff89371 Compare July 6, 2026 17:19
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from ff89371 to dc46e17 Compare July 9, 2026 10:29
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/add-cli-arg branch from 02745d0 to c5f1afb Compare July 12, 2026 08:50
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch 6 times, most recently from eaf1937 to e0f5a55 Compare July 14, 2026 06:16
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/add-cli-arg branch from 387c812 to bde6f0d Compare July 14, 2026 09:16
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from e0f5a55 to 8ce57de Compare July 14, 2026 09:16
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from 8ce57de to cacef4a Compare July 14, 2026 14:59
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/add-cli-arg branch from bde6f0d to 89f0a5a Compare July 14, 2026 14:59
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from cacef4a to c77eb44 Compare July 14, 2026 16:05
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from c77eb44 to d9f7579 Compare July 15, 2026 11:10
…stants

The three storage-reader-server ports (batcher 55011, class_manager 55210,
state_sync 55014) are fixed infra constants, not per-deployment overrides. Bake
them into the app_configs and applicative_config jsonnet instead of templatizing
them as $$$_..._$$$ placeholders, and restrict the generic port/url
auto-templatizing in replace_pred to components.* keys only. Add
state_sync_config...network_config.port to KEYS_TO_BE_REPLACED to preserve its
existing overridability. Drop the now-redundant storage-reader port overrides
from the common and testing overlay YAMLs.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/add-cli-arg branch from a1266de to d71f1a4 Compare July 15, 2026 11:26
@nimrod-starkware
nimrod-starkware force-pushed the nimrod/jsonnet/storage-reader-ports branch from d9f7579 to 5747cee Compare July 15, 2026 11:26

@nimrod-starkware nimrod-starkware left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@nimrod-starkware resolved 1 discussion.
Reviewable status: 0 of 13 files reviewed, all discussions resolved (waiting on dan-starkware and matanl-starkware).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants