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

witness_node uses two incompatible parsers for config.ini #149

Closed
vikramrajkumar opened this issue Jan 18, 2017 · 22 comments

Comments

Projects
8 participants
@vikramrajkumar
Copy link
Member

commented Jan 18, 2017

From @pmconrad on January 14, 2016 17:14

The config.ini file used by the witness_node is parsed twice, with different parsers:

  1. By bpo::parse_config_file to get the basic options
  2. By boost::property_tree::ini_parser::read_ini for the logging configuration

These two parsers are not entirely compatible, which sometimes leads to the second parse failing. This results in the default logging config being used instead of that in the config file.

Specifically, the ini_parser does not support repeating options. This conflicts with the documented (in the default config file) behaviour wrt the track-account option "Account ID to track history for (may specify multiple times)".

The problem is difficult to diagnose, because the reason for the parse failure is not logged.

Copied from original issue: cryptonomex/graphene#521

CORE TEAM TASK LIST

  • Evaluate / Prioritize Feature Request
    • Assignment: @ihla
    • Effort Estimation: 4 hours
  • Refine User Stories / Requirements
  • Define Test Cases
  • Design / Develop Solution
  • Perform QA/Testing
  • Update Documentation
@abitmore

This comment has been minimized.

Copy link
Member

commented Oct 27, 2017

@pmconrad: does 0011d5f fixes this?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Oct 27, 2017

No, that commit handles the situation when two plugins share a common option (like witness and debug_witness in this case).
The original ticket is about an option occurring more than once in the config file.

@abitmore abitmore added this to the Future Non-Consensus-Changing Release milestone Nov 27, 2017

@abitmore abitmore added the bug label Feb 5, 2018

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Feb 15, 2018

Apparently steem has solved this by moving to a json format for the logging config: https://github.com/steemit/steem/releases/tag/v0.19.4rc1

@abitmore abitmore added the logging label May 25, 2018

@ryanRfox ryanRfox added this to New -Awaiting Core Team Evaluation in Project Backlog via automation Jun 2, 2018

@ryanRfox ryanRfox moved this from New -Awaiting Core Team Evaluation to Unassigned - Bugs in Project Backlog Jun 2, 2018

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2018

Ideas I can think of for the config.ini "repeating option" problem:

  1. Switch config file format (i.e. JSON)
  2. Change formatting for .ini entries to accept arrays of values
  3. Parse .ini at startup, throwing logging options into a separate file, to be read by the logging parser in a later step.
  4. Add an option in .ini to point to a separate file for logging settings, and have the logging parser read it.

ATM, I'm leaning towards 4, as we could implement that without affecting existing configurations. If the new option is there, set a flag and have the logging configuration parser look there for the file.

1 could be implemented where it does not affect existing installations. Read JSON, if parser fails, read the old way.

@clockworkgr

This comment has been minimized.

Copy link
Member

commented Jun 4, 2018

I suggest either option 1 as a complete overhaul.

or option 4 seeing as that method is already used with api-access.json

@ihla

This comment has been minimized.

Copy link

commented Jun 5, 2018

we have implemented the option 3, I can offer the PR if you wish

@abitmore

This comment has been minimized.

Copy link
Member

commented Jun 5, 2018

@ihla please do! thanks.

Before this is fixed, the workaround for node admins is to specify the required options as command line arguments instead. E.G.

./witness_node --witness-id="1.6.1" --witness-id="1.6.2"

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

Thanks @ihla

If we use Lubos' fix, we need to consider these questions:

A. Do we migrate existing users' configuration settings to the new file, or tell them to do it themselves?
B. If we do the migration, do we remove the logging configuration sections from the config.ini to avoid confusion?

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

I like automation, so if possible, please save my exiting config.ini file to config-.ini and rewrite a new one in proper format from legacy file.

If that is too much effort, please fail on launch with exceedingly informative instr uctions to refactor the config.ini file manually.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

Automatic migration would be a one-time benefit for a few dozen (?) node operators. IMO not worth the effort.

IMO we can apply simple logic

  • if config.ini and logging.ini both exists, use logging.ini only for logging config
  • if config.ini exists but logging.ini does not, assume config.ini contains both
  • if config.ini does not exist, create new config.ini (and also new logging.ini if logging.ini does not exist)
@ryanRfox

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

I agree with @pmconrad that the number of impacted users for this one time event does not warrent investment of time for automation.

What is intended behavior at startup when: "config.ini exists but logging.ini does not"?

  • Create default logging.ini file, parse config.ini but ignore any logging values therein, parse logging.ini, launch
  • Fail to launch
    • Warn user about new ini file configuration
    • User must manually create properly configured config.ini
      • Recommend to save config.ini to config.old
      • Start/Stop node to create default config.ini and logging.ini files
      • Copy/paste relevant values into config.ini from config.old (manually)
  • Other?
@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jun 7, 2018

IMO parse config ini like we do now, i. e. for both "normal" config and logging config. No need to change anything if it's working for the node operator.

@ryanRfox

This comment has been minimized.

Copy link
Member

commented Jun 7, 2018

That works as well. New features/parameters added into a new Release typically appear in the config.ini file. My practice has been to allow the software to create a new default config.ini file. I then copy/paste my values into the new .ini file and start it back up.

@ryanRfox ryanRfox removed this from Unassigned - Bugs in Project Backlog Jul 16, 2018

@ryanRfox ryanRfox added this to In Development in Community Claims Jul 16, 2018

@ihla

This comment has been minimized.

Copy link

commented Jul 24, 2018

I have some more questions regrading the implementation details: I believe the requirement is specified in this pseudo-code by @jmjatlanta

If (config.ini does not exist)
{
create config.ini;
create logging.ini;
}

Parse config.ini for application configuration;

If (logging.ini exists)
{
Parse logging.ini for logging configuration;
}
else
{
Parse config.ini for logging configuration;
}

I am not sure about this: should we replace the legacy config.ini with the new config.ini + backup the legacy config.ini? I have modified the pseudo-code to provide backup and new file creation:

If (config.ini does not exist) // => fresh installation
{
create config.ini;
create logging.ini;
}

parse config.ini for application configuration;

If (logging.ini exists)
{
parse logging.ini for logging configuration;
}
else // => legacy config.ini
{
parse config.ini for logging configuration;
backup legacy config.ini;
create config.ini;
create logging.ini;

}

Please clarify the requirements so that I can proceed with the implementation, thx.

@jmjatlanta

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2018

My opinion was to allow the legacy config.ini to exist, and read the settings within it. Do not attempt to create the new ones. I was thinking that way so as to avoid the extra work.

It would be great to read their settings and create a new one based on their old one. If you would like to tackle that, it would be appreciated. And in that case, yes, backing up the old ones would be important. Verifying the new ones exist and are readable would also be important.

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2018

See my comment above:

Automatic migration would be a one-time benefit for a few dozen (?) node operators. IMO not worth the effort.

IOW if a config.ini exists but logging.ini does not, use only config.ini and the logging config contained therein.

@ihla

This comment has been minimized.

Copy link

commented Jul 27, 2018

I am going to implement the option presented in the original @jmjatlanta pseudo-code according to the @pmconrad guidance above. If I find the effort for the "automatic migration" is within my 4 hour estimate, I can offer the enhancement in other PR.

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 17, 2018

Now #1024 has been merged. But I'm not sure whether it completely fixed this issue. Originally,

These two parsers are not entirely compatible, which sometimes leads to the second parse failing. This results in the default logging config being used instead of that in the config file.

Specifically, the ini_parser does not support repeating options. This conflicts with the documented (in the default config file) behaviour wrt the track-account option "Account ID to track history for (may specify multiple times)".

With the patch, if I have multiple "track-account" entries in config.ini, what's the behavior? Still unable to parse logging config?

@pmconrad

This comment has been minimized.

Copy link
Contributor

commented Aug 18, 2018

For existing installations the behaviour will not change, i. e. if a config.ini exists and contains multiple entries with the same key, then the contained logging config will be ignored. This is deliberate, see discussion in #1024 (comment) - node operators must manually fix such broken installations, for example by adding a logging.ini file.

New installations will have a config.ini and logging.ini generated.

@abitmore

This comment has been minimized.

Copy link
Member

commented Aug 23, 2018

2.0.180823 Windows build only generates one config.ini file for me.

Update: I thought the fix was included in 2.0.180823 release. Apparently I was wrong.

@jmjatlanta jmjatlanta moved this from In progress to In Testing in Feature release (201810) Sep 3, 2018

@abitmore abitmore self-assigned this Sep 14, 2018

@abitmore

This comment has been minimized.

Copy link
Member

commented Sep 14, 2018

I think we can close this now. Related PR #1024 #1325.

@abitmore abitmore closed this Sep 14, 2018

Feature release (201810) automation moved this from In Testing to Done Sep 14, 2018

@bitfag

This comment has been minimized.

Copy link

commented Apr 16, 2019

I'm running a private testnet and constantly getting logging parsing error

1513040ms th_a       config_util.cpp:239           load_logging_config_ ] Error parsing logging config from logging config file /var/lib/bitshares/config.ini, using default config

config.ini contains multiple witnesses

witness-id = "1.6.1"
witness-id = "1.6.2"
witness-id = "1.6.3"
witness-id = "1.6.4"
witness-id = "1.6.5"
witness-id = "1.6.6"
witness-id = "1.6.7"
witness-id = "1.6.8"
witness-id = "1.6.9"
witness-id = "1.6.10"
witness-id = "1.6.11"

If I'm commenting out all witnesses except the first, logging begins to work.

Bitshares version is recent, docker image 26b7a6685e66

Edit: logging configuration is in logging.ini is working, but it's not symlinked inside docker CT, will open a new issue for that.

Opened #1723

@pmconrad pmconrad removed this from In Development in Community Claims Apr 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.