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

[RFC]: Configuration Overhaul #3795

Closed
wants to merge 11 commits into from

Conversation

0x009922
Copy link
Contributor

@0x009922 0x009922 commented Aug 11, 2023

I think this place is the most convenient to gather the team's feedback.

Checklist

  • Internal review
  • Library authors & users review
  • Tech writers review
  • DevOps review

Moved here: hyperledger-iroha/iroha-rfcs#3

@0x009922 0x009922 added iroha2-dev The re-implementation of a BFT hyperledger in RUST Documentation Documentation changes config-changes Changes in configuration and start up of the Iroha labels Aug 11, 2023
@0x009922 0x009922 self-assigned this Aug 11, 2023
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Outdated Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Show resolved Hide resolved
docs/source/rfc-configuration-overhaul.md Show resolved Hide resolved
Copy link
Contributor

@Erigara Erigara left a comment

Choose a reason for hiding this comment

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

Don't have any objections.

@0x009922 0x009922 changed the title [documentation]: Add Configuration RFC document [RFC]: Configuration Overhaul Aug 22, 2023
0x009922 and others added 8 commits August 28, 2023 09:03
Signed-off-by: Dmitry Balashov <[email protected]>
Co-authored-by: Aleksandr Petrosyan <[email protected]>
Signed-off-by: 0x009922 <[email protected]>
Signed-off-by: Dmitry Balashov <[email protected]>
@0x009922
Copy link
Contributor Author

Addressed all of the comments as of now. The document looks finalised.

Co-authored-by: Ilia Churin <[email protected]>
Signed-off-by: 0x009922 <[email protected]>

This is not a helpful error message as it does not mention what exactly is wrong, why and where the parsing failed.

#### 7. Invalid `IROHA2_PUBLIC_KEY` environment variable
Copy link
Contributor

Choose a reason for hiding this comment

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

good research of the all potential cases 👍 wondering if there is other behavior for other combinations of the parameters etc 😱

Copy link
Contributor

@6r1d 6r1d Sep 5, 2023

Choose a reason for hiding this comment

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

I think a regular deployment with multiple SDKs making test requests to Iroha will help answer your question.
The configuration will change, and knowing what is broken early is essential.

If (for example) Tuesday's stand-up starts with the automatic news about the dev, stable or lts issues, it'll help both respond to the community (we know the actual state) and quickly resolve the potential problems.
(I am not sure triggering it only on each release is valid because Docker may change and influence the behavior of Iroha deployment.)


Providing a configuration file with extra fields that are not supposed to be there does not produce any errors at all.

This might lead to a bad user experience when user expects some options to apply, but doesn't have any idea that those
Copy link
Contributor

Choose a reason for hiding this comment

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

to preserve the same behavior in using both ENV and FILE we should just get rid of using default values if nothing is specified at all, otherwise a user might use an outdated ENV var not knowing why the option is not applied (falling back to the default), and Iroha obviously shouldn't scan all outdated ENV vars to produce a warning about that (which can be done easily with a file)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am confused, because the purpose of default values is exactly to be applied when nothing is specified at all.

As for accidentally having an outdated ENV var, this RFC includes defining a deprecation policy. According to it, when an ENV var renaming takes place, the user will have a window between versions when they will get a warning/error for the deprecated variable before it is deleted entirely.

Copy link
Contributor

Choose a reason for hiding this comment

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

In that case we should at least specify what fields have default values, I guess such a set should be as little as possible (i.e. log level etc only)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, from internal discussions I remember the opposite request: we should have as many defaults as possible.

Anyway, on the configuration reference writing stage we will decide in details which fields we should have, what default values should be etc. At that stage we will also collect feedback from external users.

Copy link
Contributor

Choose a reason for hiding this comment

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

In my opinion, both of you have a point.
I have a question regarding the deployment workflow (which may be covered by a part of the RFC and I'm missing something).
Let's assume a person who never used Iroha 2 performs the deployment.
How do they see the configuration parameters that were applied as defaults?
Maybe a verbose-running mode would help debug the deployment configurations at a later point.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mversic the suggestion is definitely valid, though we still have to determine what parameters should not be allowed to have the default value, e.g. things like metadata size limit could be risky if a user starts a network without proper attention to the consequences of not configuring that specifically to their use case or constraints.
On the other hand, if someone migrates to a newer version of Iroha, updating the corresponding configuration to match that is not a decision making factor overhead (esp. compared to a blockstore migration).

To conclude, I'd not insist on my view, either choice is fine to me. If we allow the defaults we just should be much more careful about this for every parameter now and in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Mingela, do I get you correctly: your point is that there are more parameters which should be provided by the user than we have now?

In other words, configuration like metadata size limits should be defined by the user anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

we can always decide post factum on the set of parameters that will be required vs the ones that will have a default

Copy link
Contributor

Choose a reason for hiding this comment

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

do I get you correctly: your point is that there are more parameters which should be provided by the user than we have now?

In other words, configuration like metadata size limits should be defined by the user anyway.

@0x009922, I'm afraid I'm not entirely aware of all the parameters which have defaults, but in my opinion things like metadata limits should be described in the docs properly and left up to a user.

I've just come to another idea to consider which is to introduce a config mode, i.e. strict vs relaxed (or like advanced/basic) based on which a user may omit specifying many parameters (prohibiting or enforcing defaults) keeping that decided by them still. On the other hand, we probably should not complicate things that much..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

things like metadata limits should be described in the docs

Everything should no doubt be described in the docs!

strict vs relaxed

It might be convenient. Let's leave this discussion to the stage of the actual documentation reference creation. It will be the best time to make decisions based on what might be the best for users.

Also, I had an idea of having extends mechanism. With it, users might extend existing presets. For example:

extends = "./base.toml"
# or even some built-in preset?
extends = "built-in:strict"

extends mechanism plays well with the "Trace Configuration Resolution" section.

The proposals outlined in this section are designed to collectively enhance the configuration system within Iroha.
However, they are not mutually required. Each proposal can be considered and implemented independently.

### Proposal 1 - Use TOML
Copy link
Contributor

Choose a reason for hiding this comment

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

how about yaml alternative proposal?
common for deployments

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, choice between TOML and YAML is more about a personal preference, I believe. However, we prefer TOML primarily because:

  • Simple and explicit syntax, more readable
  • Strong types, less unexpected results
  • Indentation doesn't matter
  • Better error messages
  • It is a standard for Rust ecosystem

As for making YAML/JSON/whatever an additional configuration format, it might badly affect the overall quality of the configuration system due to it being more generalised, thus e.g. less able to provide some details in error messages.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO TOML is the simplest of the three and is best suited for Iroha purposes

Copy link
Contributor

Choose a reason for hiding this comment

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

My main motivation is that configuration is rather related to deployment, whereas TOML is common for a project (development) configuration in Rust, though I don't deny there deployment tools relying on TOML may exist. DevOps input may be really valuable here.

Copy link
Contributor

Choose a reason for hiding this comment

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

While my current position is unrelated to DevOps, I'm confident I have enough experience to discuss DevOps issues.

For me, TOML is more readable, and the advantage of JSON/YAML/TOML is that we learn them quickly as human beings.

If we assume that people are configuring Iroha, they either know TOML or will know it as soon as they need to use it.
If we assume we're using GPT to configure it (for some reason), it knows the format and will most likely get stuck on the variable names. Real people may make the same mistake unless they have the config reference doc for the latest Iroha version with a lot of examples.

whereas TOML is common for a project (development) configuration in Rust, though I don't deny there deployment tools relying on TOML may exist

TOML is intended to be a configuration file format, according to Tom Preston-Werner at https://toml.io/en/:

A config file format for humans.

TOML aims to be a minimal configuration file format that's easy to read due to obvious semantics. TOML is designed to map unambiguously to a hash table. TOML should be easy to parse into data structures in a wide variety of languages.

Copy link
Contributor

@mversic mversic Sep 7, 2023

Choose a reason for hiding this comment

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

IMO yaml is unnecessarily complicated and it's a mess to deal with spaces. JSON would be fine except that it doesn't support comments, and we really want to be able to comment out parameters in the config file. On the other hand, TOML fits our needs perfectly

if you look at mysql.conf it's configuration format is quite similar to TOML.

Copy link
Contributor

@Mingela Mingela left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@Cre-eD Cre-eD left a comment

Choose a reason for hiding this comment

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

LGTM

0x009922 added a commit to 0x009922/iroha-rfcs that referenced this pull request Sep 11, 2023
@0x009922
Copy link
Contributor Author

The RFC is moved here:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config-changes Changes in configuration and start up of the Iroha Documentation Documentation changes iroha2-dev The re-implementation of a BFT hyperledger in RUST
Projects
None yet
Development

Successfully merging this pull request may close these issues.