Skip to content

Commit

Permalink
feat(pd): special handling of seed nodes in join
Browse files Browse the repository at this point in the history
When joining a network via `pd testnet join`, pd already inspects
the peers known to the target bootstrap node, and includes those peers
in the generated CometBFT config. Technically it's not correct to assume
that these peers are seeds, but limiting to a threshold (#3063) ensures
that only a few nodes will be miscategorized this way.

The new behavior introduced here is that `pd testnet join` will inspect
monikers of peers learned from the bootstrap node, and if the string
"seed" appears in the moniker, guarantee that peer lands in the `seeds`
field of the CometBFT config.

To complement this change, we update the deployment logic to create a
seed node, and ensure that seed node is excluded from the loadbalanced
RPC, to avoid hitting it via the join request (in which case it won't
report many peers, because seed nodes drop peer connections after
serving address info).

Refs #3056.
  • Loading branch information
conorsch committed Oct 10, 2023
1 parent e85f527 commit 0d62255
Show file tree
Hide file tree
Showing 12 changed files with 95 additions and 36 deletions.
41 changes: 32 additions & 9 deletions crates/bin/pd/src/testnet/join.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,9 +155,14 @@ pub async fn fetch_peers(tm_url: &Url) -> anyhow::Result<Vec<TendermintAddress>>
// We'll look for a handful of peers and use those in our config.
// We don't want to naively add all of the bootstrap node's peers,
// instead trusting gossip to find peers dynamically over time.
// We'll also special-case nodes that contain the string "seed" in their moniker:
// those nodes should be assumed to seed nodes. This is optimistic, but should result
// in more solid peering behavior than the previous strategy of declaring all fullnodes
// as seeds within the joining node's CometBFT config.
let threshold = 5;

let mut peers = Vec::new();
let mut seeds = Vec::new();

for raw_peer in net_info_peers {
tracing::debug!(?raw_peer, "Analyzing whether to include candidate peer");
let node_id: tendermint::node::Id = raw_peer
Expand All @@ -177,6 +182,14 @@ pub async fn fetch_peers(tm_url: &Url) -> anyhow::Result<Vec<TendermintAddress>>
// Remove it so we can treat it as a SocketAddr when checking for internal/external.
.replace("tcp://", "");

let moniker = raw_peer
.get("node_info")
.and_then(|v| v.get("moniker"))
.and_then(|v| v.as_str())
.ok_or_else(|| {
anyhow::anyhow!("Could not parse node_info.moniker from JSON response")
})?;

// Filter out addresses that are obviously not external addresses.
if !address_could_be_external(&listen_addr) {
tracing::debug!(
Expand All @@ -185,7 +198,6 @@ pub async fn fetch_peers(tm_url: &Url) -> anyhow::Result<Vec<TendermintAddress>>
);
continue;
}

// The API returns a str formatted as a SocketAddr; prepend protocol so we can handle
// as a URL. The Tendermint config template already includes the tcp:// prefix.
let laddr = format!("tcp://{}", listen_addr);
Expand All @@ -194,19 +206,30 @@ pub async fn fetch_peers(tm_url: &Url) -> anyhow::Result<Vec<TendermintAddress>>
listen_addr
))?;
let peer_tm_address = parse_tm_address(Some(&node_id), &listen_url)?;
peers.push(peer_tm_address);
if peers.len() >= threshold {
tracing::debug!(?threshold, "Found desired number of initial peers");
break;
// A bit of optimism: any node with "seed" in its moniker gets to be a seed.
if moniker.contains("seed") {
tracing::debug!(
?peer_tm_address,
moniker,
"Found self-described seed node in candidate peers"
);
seeds.push(peer_tm_address)
// Otherwise, we check if we've found enough.
} else {
if peers.len() <= threshold {
peers.push(peer_tm_address)
}
}
}
if peers.len() < threshold {
if peers.len() < threshold && seeds.is_empty() {
tracing::warn!(
"bootstrap node reported only {} peers; we may have trouble peering",
"bootstrap node reported only {} peers, and 0 seeds; we may have trouble peering",
peers.len(),
);
}
Ok(peers)
// TODO handle seeds and peers differently. For now, all peers are used as seeds.
seeds.extend(peers);
Ok(seeds)
}

/// Check whether SocketAddress spec is likely to be externally-accessible.
Expand Down
6 changes: 4 additions & 2 deletions deployments/charts/penumbra-node/templates/NOTES.txt
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
Penumbra full node has been deployed!
{{- if .Values.ingressRoute.enabled }}
Penumbra full node config has been deployed!
{{- $count := (.Values.nodes | len | int) }}
Total count of fullnodes: {{ $count }}
{{ if .Values.ingressRoute.enabled }}
You can access the pd gRPC service here:

https://{{.Values.ingressRoute.hosts.pd }}
Expand Down
9 changes: 8 additions & 1 deletion deployments/charts/penumbra-node/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,9 @@ spec:
{{- end }}
{{- include "penumbra-node.selectorLabels" $ | nindent 8 }}
spec:
# Force the pods to different k8s nodes, so that egress ip is unique per Tendermint node.
# Force the pods to different k8s nodes, so that egress ip is unique per CometBFT node.
# Effectively limits the number of Penumbra nodes to the number of k8s nodes in the cluster.
# Setting `allow_duplicate_ip=true` in CometBFT config removes this constraint.
affinity:
podAntiAffinity:
requiredDuringSchedulingIgnoredDuringExecution:
Expand Down Expand Up @@ -116,6 +117,12 @@ spec:
sed -i -e 's/max_num_inbound_peers.*/max_num_inbound_peers = {{ $.Values.cometbft.config.p2p.max_num_inbound_peers | int }}/' /penumbra-config/testnet_data/node0/cometbft/config/config.toml
sed -i -e 's/max_num_outbound_peers.*/max_num_outbound_peers = {{ $.Values.cometbft.config.p2p.max_num_outbound_peers | int }}/' /penumbra-config/testnet_data/node0/cometbft/config/config.toml
# configure seed node, defaulting to false if unspecified.
{{- $seed_mode := (index $.Values.nodes $i).seed_mode | default false -}}
{{- with $moniker }}
sed -i -e 's/^seed_mode.*/seed_mode = {{ $seed_mode }}/' /penumbra-config/testnet_data/node0/cometbft/config/config.toml
{{- end }}
# set ownership for cometbft configs to match cometbft container "tmuser" uid/gid
chown -R 100:1000 /penumbra-config/testnet_data/node0/cometbft
Expand Down
12 changes: 12 additions & 0 deletions deployments/charts/penumbra-node/templates/ingressroute.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -15,19 +15,31 @@ spec:
- kind: Rule
match: Host(`{{ .Values.ingressRoute.hosts.pd }}`)
services:
{{- /*
Skip nodes with seed_mode=true when looping over nodes, to exclude from LB RPCs.
Otherwise, RPC can return surprising results, like very low numbers of peers.
*/}}
{{- range $i,$e := until $count }}
{{- $seed_mode := (index $.Values.nodes $i).seed_mode | default false }}
{{- if $seed_mode }}
{{- else }}
{{ $fn_name := printf "%s-fn-%d" $.Release.Name $i }}
- name: {{ $fn_name }}
port: 8080
scheme: h2c
{{- end }}
{{- end }}
- kind: Rule
match: Host(`{{ .Values.ingressRoute.hosts.tm }}`)
services:
{{- range $i,$e := until $count }}
{{- $seed_mode := (index $.Values.nodes $i).seed_mode | default false }}
{{- if $seed_mode }}
{{- else }}
{{ $fn_name := printf "%s-fn-%d" $.Release.Name $i }}
- name: {{ $fn_name }}
port: 26657
{{- end }}
{{- end }}
tls:
domains:
Expand Down
3 changes: 3 additions & 0 deletions deployments/charts/penumbra-node/values.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,13 @@ cometbft:
#
# - moniker
# - external_address
# - seed_mode
#
# Nodes with seed_mode=true will be excluded from the ingress.
nodes:
- moniker: ididntedittheconfig
external_address: ""
seed_mode: false

# Custom label for aggregating network, nodes, and metrics into a cohesive deployment.
# Maps to the 'app.kubernetes.io/part-of' label.
Expand Down
7 changes: 1 addition & 6 deletions deployments/helmfile.d/penumbra-devnet.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,6 @@ releases:
tag: main
# Communicate intra-cluster to the private validator rpc address.
- penumbra_bootstrap_node_cometbft_rpc_url: "http://penumbra-devnet-val-0:26657"
- nodes:
# We intentionally exclude external IPs in this declaration, but include them
# in the external vars/ file, which is generated via `./scripts/get-lb-ips penumbra-devnet`.
- moniker: phobos
- moniker: deimos
- persistence:
enabled: true
size: 10G
Expand All @@ -59,7 +54,7 @@ releases:
tag: main
- scrape_configs:
# Must match settings from "penumbra-node" chart
numFullNodes: 2
numFullNodes: 3
fmtFullNodeSvc: "penumbra-devnet-nodes-fn-%d"
# Must match settings from "penumbra-network" chart
numValidators: 2
Expand Down
9 changes: 1 addition & 8 deletions deployments/helmfile.d/penumbra-preview.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,11 @@ releases:
tag: main
# Communicate intra-cluster to the private validator rpc address.
- penumbra_bootstrap_node_cometbft_rpc_url: "http://penumbra-preview-val-0:26657"
- nodes:
# We intentionally exclude external IPs in this declaration, but include them
# in the external vars/ file, which is generated via `./scripts/get-lb-ips penumbra-devnet`.
- moniker: phobos
- moniker: deimos
- moniker: naiad
- moniker: thalassa
- persistence:
enabled: true
size: 50G
- part_of: penumbra-preview
# empty vars file for storing external ips
# Node config info, including ip address, monikers, and seed-mode status.
- vars/penumbra-preview-nodes-ips.yml

- name: penumbra-preview-metrics
Expand Down
9 changes: 1 addition & 8 deletions deployments/helmfile.d/penumbra-testnet.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -36,18 +36,11 @@ releases:
tag: latest
# Communicate intra-cluster to the private validator rpc address.
- penumbra_bootstrap_node_cometbft_rpc_url: "http://penumbra-testnet-val-0:26657"
- nodes:
# We intentionally exclude external IPs in this declaration, but include them
# in the external vars/ file, which is generated via `./scripts/get-lb-ips penumbra-devnet`.
- moniker: phobos
- moniker: deimos
- moniker: naiad
- moniker: thalassa
- persistence:
enabled: true
size: 100G
- part_of: penumbra-testnet
# empty vars file for storing external ips
# Node config info, including ip address, monikers, and seed-mode status.
- vars/penumbra-testnet-nodes-ips.yml

- name: penumbra-testnet-metrics
Expand Down
5 changes: 5 additions & 0 deletions deployments/helmfile.d/vars/penumbra-devnet-nodes-ips.yml
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
nodes:
- external_address: 35.202.100.199:26656
moniker: phobos-seed
seed_mode: true
- external_address: 34.16.34.194:26656
moniker: deimos
- external_address: 34.173.166.32:26656
moniker: naid
5 changes: 5 additions & 0 deletions deployments/helmfile.d/vars/penumbra-preview-nodes-ips.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
nodes:
- external_address: 34.135.6.235:26656
moniker: phobos-seed
seed_mode: true
- external_address: 34.28.180.178:26656
moniker: deimos
- external_address: 34.42.196.153:26656
moniker: naid
- external_address: 35.239.76.154:26656
moniker: thalassa
5 changes: 5 additions & 0 deletions deployments/helmfile.d/vars/penumbra-testnet-nodes-ips.yml
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
nodes:
- external_address: 35.225.116.144:26656
moniker: phobos-seed
seed_mode: true
- external_address: 35.224.80.161:26656
moniker: deimos
- external_address: 34.68.200.112:26656
moniker: naid
- external_address: 35.192.219.42:26656
moniker: thalassa
20 changes: 18 additions & 2 deletions deployments/scripts/get-lb-ips
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ if [[ $# -lt 1 ]] ; then
exit 1
fi

# Declare monikers for nodes on the network.
# These monikers will be added to the generated vars file,
# alongside the IP info.
node_names=(phobos-seed deimos naid thalassa)

HELM_RELEASE="${1:-}"
shift 1
vars_file="${PWD}/helmfile.d/vars/${HELM_RELEASE}-ips.yml"
Expand Down Expand Up @@ -42,8 +47,19 @@ printf ' done!\n'
# This format is very specific to values format required by the given Helm chart.
function generate_yaml_penumbra_nodes() {
printf 'nodes:\n'
while read -r i ; do
printf ' - external_address: %s:26656\n' "$i"
counter=0
while read -r ip_addr ; do
node_name="${node_names[$counter]}"
cat <<EOF
- external_address: ${ip_addr}:26656
moniker: ${node_names[$counter]}
EOF
# Special handling for seed nodes: if moniker declares it's a seed,
# it's a seed.
if [[ $node_name =~ seed ]] ; then
printf ' seed_mode: true\n'
fi
counter="$(( counter + 1 ))"
done <<< "$ip_info"
}

Expand Down

0 comments on commit 0d62255

Please sign in to comment.