Removed alphanumeric enforcement for node IDs

[kaiwetlesen]

This removes the node ID check requiring that a node identifier be solely composed of A through Z, 0 through 0, or an underscore (_) character. My reasoning for removing this restriction is that it enables easier node provisioning by enabling the use of generated Kubernetes host names to name index and query nodes. This also removes confusion when a new indexer deploys the indexer stack.

[kaiwetlesen]

CC: @chriswessels

[lutter]

My reasoning for removing this restriction is that it enables easier node provisioning by enabling the use of generated Kubernetes host names to name index and query nodes. This also removes confusion when a new indexer deploys the indexer stack.

Can you explain in more detail the problems this causes? Can that be avoided with better error messages? Since the change also renames nodes (from graph-node's perspective) we'll need some strategy how to avoid breaking people's existing setups. For example, this change as-is would make everything in the hosted service not index anything.

[kaiwetlesen]

Sure thing. Typically a new indexer will assign the hostname to the node ID parameter for simplicity and clarity within the cluster. Existing indexers (myself included), will use the same hostname for identifying additional index nodes. The symbol transform from - to _ gets done transparently in the startup script (removed as seen in the diff) and without notice to the user. Furthermore, this transformation and character requirement is not documented anywhere, except for this removed code snippet. This causes confusion and significantly hinders usability.

To address your second point in conceiving a strategy to preserve legacy behaviour, it is certainly possible to add a flag to either enable opt in to the new behaviour disabling the silent transform, or opt in to enabling the legacy behaviour. What are your thoughts @lutter?

[kaiwetlesen]

I'd like to add further reasoning why this case enforcement should be removed. When bootstrapping the node with the start script in the docker folder, it checks $node_id then sets $DISABLE_BLOCK_INGESTOR according to a comparison between $node_id and what value is set for $BLOCK_INGESTOR. If node ID is set to the host name provided by K8S, then the block ingestor is silently disabled. If one is to assume that the $BLOCK_INGESTOR follows the enforced naming convention, then this silent disablement will always occur. This degrades the user experience significantly.

[chriswessels]

Thank you for submitting @kaiwetlesen! Would very much like to see this merged. The existing silent behaviour doesn't play well with Kubernetes, and was a significant gotcha when I first started on the stack, and even once you know about it, the mismatch between names in infrastructure land (e.g. Kubernetes) and graph-node land is an unnecessary inconvenience. No doubt this is true for new Indexers too.

[kaiwetlesen]

Update: to address any potential backwards compatibility issues until user data can be updated, I've added an environment variable to opt into the new behavior.

[kaiwetlesen]

@lutter any way we can work this into the next release?

[azf20]

Hey @kaiwetlesen - thanks for adding the environment variable as a config. Are there any other edge cases or considerations that we should be aware of here?

[kaiwetlesen]

Not really that I can think of @azf20. As a matter of fact there should be no issues once an indexer reassigns their deployments. Issues should just go away.

[kaiwetlesen]

(rebased this branch onto current master)

[lutter]

This should be called something more specific, say GRAPH_NODE_LITERAL_NAME; once this is out there, we can't add to the kind of things that this changes since we'll have a mix of people with this on and off who may or may not want to also use any behavior we would add.

The docs should say something like

(Docker only) Use the literal `node_id` provided to the docker start script instead of replacing hyphens (-) in names with underscores (_) Changing this for an existing `graph-node` installation requires also changing it in the `subgraphs.subgraph_deployment_assignment` table in the database.

It's a little weird in that this variable only has an effect with our Dockerfile where everything else is built into the graph-node binary but I don't have a better idea where to document this.

[kaiwetlesen]

My apologies for the delayed response. Family life.

With regards to the variable name, I agree, a more succinct variable name is called for. I propose GRAPH_NODE_ID_USE_LITERAL_VALUE. It's a bit longer, but a bit more clear. Ideally the effect of this variable could be removed as people (hopefully) pick up and adopt the changes.

And the behaviour isn't even in the Dockerfile, it's in that little bootstrapping script in the same directory. Its function would better be served by an entrypoint that does all the environmental stuff.

[lutter]

Looks good now!