Hilbert YAML configuration parser and Hilbert Server/Client-side tools #34

malex984 · 2016-11-09T11:48:57Z

This is the first iteration of implementing Configuration Design.

Major Parts:

minimal CLI for hilbert (server part)
minimal CLI for hilbert-station (client part)
Makefile supports documentation generation: make epydoc / make apidocs
Makefile supports automated testing: make check
Server-side tool uses $HOME/.ssh/config to execute remote actions via ssh. Currently it expects client tool: hilbert-station to be available remotely.
Hilbert client-side tool: hilbert-station (currently requires legacy scripts and older config files)
logging output: iterated -v adds verbosity whereas iterated -q removes verbosity. Default: WARNING level.
add automated testing pytest (maybe also tox / unittest later on). Currently for (Danger of silent overrides due to YAML #27): tests/test_loadwarnings.py, VersionValidator: tests/test_version.py and whole configuration validator (Hilbert) + (Configuration design: Wrong format: list vs mapping #17): tests/test_validate.py

FIX: many fixes CHG/ADD: changes and additions to the class hierarchie, e.g.: * DockerMachie variant for PowerOnMethod * expect default values for optional fields!

@porst17

NOTE: due to @porst17's comment #32 (comment) ADD: minor comments (TODOs for later)

…Scalar and BaseString CHG: only keep the processed data is there was no error TODO: check that every validator follows this! REN: Abstract -> AbstractValidator

Changed API: version is implicitly globally available, use of classmethod: `Validator.parse(input, parent=...)` Filled in Validators with actual checks (URI verification, ssh alias, docker-compose config etc) Added CLI with subcommands to hilbert-cli-config.py (arghandler & argparse) Added pickling with `dill/pickle` NOTE: all validators can be tested separately now! Infrastructure: Added unit-tests using py.test Added `Makefile check` to run unit-tests Added doxygen/epydoc/pylint/pep8 execution to the Makefile

… keys

…ith `Application::name`

NOTE: the resulting .data.pickle should be generated under python2 (to be understood also by python3)

NOTE: parent should also be a Validator...

* Station:type may be: standalone, server and default: standard + Station:client_settings is not required for standalone stations! TODO: implement the above + add to the design document * Profiles and Groups may not to be displayed on UI: no need in name/icon/description... Q: Tags are to be implemented via UI - outside of the general configuration file, right?

helpers, loader, validators + main tool: hilbert Changes to the subcommand names in hilbert

NOTE: it requires ../config/*.py Also a few fixes in subcommand's argument handling (e.g. input sources & default values)

…ded configuration

For instance: * printing * show * config library package may be anywhere (see adding path to /config/ in config/tests/test_validate.py and /tools/hilbert.py) Update: tests/data/Hilbert.yml.data.pickle contains Hilbert top class TODO: Validator.parse should return Validator (or throw an exception)

NOTE: use .parse instead of .validate on temporary objects

…sub-commands to the hilbert tool CHG: improved Version Validator (keep semantic Version object, but show a version string) ADD: switched to logging instead of printing

server-side hilbert tool: - poweron/shutdown <StationID> [<args>] - deploy <StationID> - start/finish <StationID> [<ServiceID/ApplicationID>] - app_switch <StationID> <new ApplicationID>

NOTE: see the legend to "General Hilbert Architecture" in the "Management back-end" google.document

porst17

This pull request was supposed to be about config validation. Now it is also about the new CLI, so you are mixing different things again. The whole pull request is huge, making it hard to focus my comments on one particular issue. I will start commenting on the new cli.

@elondaits Please add your thought on the cli.

porst17 · 2016-11-22T10:50:49Z

tools/hilbert.py

+def cmd_poweron(parser, context, args):
+    log.debug("Running '{}'" . format('cmd_poweron'))
+
+    parser.add_argument('-s', '--station', required=True, help="specify the station")


Why have an additional parameter -s when hilbert poweron should just expect a station as argument by default, e.g. hilbert [global-options] poweron [subcommand-options] station_id?

Sorry - for later

porst17 · 2016-11-22T10:52:21Z

tools/hilbert.py

+def cmd_shutdown(parser, context, args):
+    log.debug("Running '{}'" . format('cmd_shutdown'))
+
+    parser.add_argument('-s', '--station', required=True, help="specify the station")


Why not just hilbert shutdown station_id?

porst17 · 2016-11-22T10:52:55Z

tools/hilbert.py

+def cmd_deploy(parser, context, args):
+    log.debug("Running '{}'" . format('cmd_deploy'))
+
+    parser.add_argument('-s', '--station', required=True, help="specify the station")


hilbert deploy station_id

porst17 · 2016-11-22T10:53:49Z

tools/hilbert.py

+def cmd_start(parser, context, args):
+    log.debug("Running '{}'" . format('cmd_deploy'))
+
+    parser.add_argument('-s', '--station', required=True, help="specify the station")


hilbert start station_id app_or_service_id

porst17 · 2016-11-22T10:54:33Z

tools/hilbert.py

+def cmd_finish(parser, context, args):
+    log.debug("Running '{}'" . format('cmd_finish'))
+
+    parser.add_argument('-s', '--station', required=True, help="specify the station")


hilbert finish station_id app_or_service_id

porst17 · 2016-11-22T11:06:47Z

tools/hilbert.py

+def cmd_query(parser, context, args):
+    log.debug("Running '{}'" . format('cfg_query'))
+
+    parser.add_argument('-o', '--object', required=False, default='all',


Streamline the CLI: hilbert cfg_query [config_object].

Sorry - this is for later

porst17 · 2016-11-22T11:12:07Z

tools/hilbert.py

+    # 2 input sources: .YAML or .PICKLE
+    handler.add_argument('-if', '--inputfile', required=False, 
+                         help="specify input .YAML file (default: 'Hilbert.yml')")
+    handler.add_argument('-id', '--inputdump', required=False,


-id does not make sense for the `cfg_verify´ subcommand, so it should not be a global option.

Are --inputfile and --inputdump a relic of the config validation cli? It suggests that the config is the only valid form of input to hilbert, but I assume that other kinds of input might be necessary in the future. It would be better to call it --configfile and --configdump.

High level representations of data (e.g. a JSON file or SQL file) are also called dumps... in fact, if you have a database in binary format the dump is a SQL text file... so calling the "compiled" representation a dump is confusing.

In NORMAL operation the UI will be using one cfg, and the cli should use the same... and we said the UI server should restart if the cfg changes... so users should be careful not to use the cli with the wrong cfg. Having the option of running the "pickle" file or the YAML one seems error prone and "dangerous" in this way. I recommend the YAML and pickle dates be compared, if the YAML is newer it's automatically compiled to .pickle in the same location. If not, the pickle is used directly.

... But really... it's an optimization already where the program doesn't even do anything. We shouldn't optimize before we even have code.

PS: If the CLI runs one command with one cfg and one command with another it can be dangerous as well... it's not just a matter of UI vs CLI.

PS2: Pickle files will break if there are changes to the classes that you serialized... so you should ignore pickle files written by a different version of hilbert (ANY level of changes, even a patch). At the minimum you should add a key to the pickle file that corresponds to the version of hilbert or something like that...

porst17 · 2016-11-22T11:20:44Z

tools/hilbert.py

@@ -0,0 +1,589 @@
+#!/usr/bin/env python2


I would like to be able to actually call the hilbert command instead of calling

python[3] tools/hilbert.py

sure. you can do that.

well just like any script: e.g ./hilbert.py - it will use Python2 by default.
I can add a link named hilbert if .py should be avoided?

The .py exposes the implementation detail that the script is written in python (at least it suggests that). The user interface should not expose such things. Please add the link.

porst17 · 2016-11-22T11:25:50Z

tools/hilbert.py

+    return cfg.query(obj)
+
+
+@subcmd('cfg_query', help='Load Configuration and query some part of it')


It looks like Load Configuration is done by any subcommand except version (which is not really a subcommand and could also be replace by a global switch --version). If it is a common thing, it should be part of the global hilbert documentation and not of the subcommand documentation.

porst17 · 2016-11-22T11:27:59Z

tools/hilbert.py

+    return load_yaml_file(f)  # TODO: check that this is a dictionary!
+
+
+@subcmd('version', help='Display version info')


Should be replaced by global option --version similar to git --version.

i will add that later, Ok?

elondaits · 2016-11-22T15:29:28Z

I'll check it later today.
Alex, two things now:

As far as I remember argparse doesn't really support subcommands (git style) where you can have different number of positional commands depending on the subcommand passed. Check these out, they are popular Python argument parsing libraries that I think support our case:
- https://github.com/docopt/docopt
- http://click.pocoo.org/5/
PyCharm does on-the-fly PEP8 validation on the sidebar (the thin stripe next to the scrollbar where a red line indicates an error, yellow a warning, and a gray one a style error). We should support PEP8... static analysis of all sorts is very important with a dynamic language. For instance the spell checker in PyCharm will let you know when you've misspelled something and avoid finding out on runtime.

elondaits · 2016-11-22T18:04:36Z

The tests are not well structured. Each test should test something specific, so it needs a descriptive name and a descriptive comment. Also tests should not print anything... they're run from the test runner which is the only one that indicates whether things run OK or not. They're not for debugging, rather for indicating there's something wrong.

Also, you should test that the code you wrote is doing what it's supposed to do... assume that third party libraries were already tested and work OK (i.e. no need to test the semantic versioning class).

The test where you load the cfg and then the pickle version of the cfg (produced by dumping your binary representation of the loaded cfg) is tautological... any errors in loading the cfg are there in the pickle as well, so knowing they're the same doesn't give you any reassurances that things are working.

Anyway I'm not sure if @porst17 would want to block the pull request because of these issues, or if we can fix them later. Although I recommend developing the tests alongside the code (not necessarily TDD, but close) because it helps to verify the interfaces are decent, that one can understand the code and that it doesn't have errors. Adding testing at the end is much more painful and we'll have a lot to do next year with the hardware and dockering the apps.

elondaits · 2016-11-22T18:11:30Z

Finally: That our code supports both Python2 and Python3 is worse, not better. Because we're giving ourselves a bigger problem, twice the amount of testing, and all the incompatibility problems (things we can't do in 2 or that we can't do in 3) instead of half of them. Just choose one... use 3 if it's the same.

elondaits · 2016-11-26T04:56:01Z

I said I have no problem calling hilbert with -q, but I want verbose output... just not any logging. There's a difference between the output of the operation and any errors and logging. As long as -q only affects logging there's no problem.
app_start is your previous "start" as I understand. app_stop was not listed but I assume it's logical if start exists. If there's no stop we don't need a start. And it seems a good idea to allow the station to be without any running app without shutting it down, which might take time or could be undesirable for other reasons (faulty HW, etc.)

malex984 · 2016-11-26T17:17:13Z

As long as -q only affects logging there's no problem.

Yes. -v and -q only affect logging. -p affects the treatment of some cases: wrong ssh alias/missing file: in pedantic mode such cases will result in an error.

BTW: what about redirecting all of logging output into a separate log file and showing it in case of non-zero exit code?

app_start is your previous "start" as I understand. app_stop was not listed but I assume it's logical if start exists. If there's no stop we don't need a start. And it seems a good idea to allow the station to be without any running app without shutting it down, which might take time or could be undesirable for other reasons (faulty HW, etc.)

I think previously there were commands like start_all and finish_all to start/stop all services and an application on a station. But if we consider start_all as a part of poweron and finish_all as a part of shutdown - they not be necessary anymore.

I also think that app_stop and app_start would be good to have but we have to define what should be shown after the current top application was stopped. Desktop? Splash screen? Special dummy application?

Note that OMD server expects that there is a single top application running on a station at any moment and sending heartbeats...
@elondaits Q: do you really want to change this logic now?

Hence app_stop can only stop the currently running top application.
@elondaits Q: should i remove ApplicationID argument to app_stop?

I would suggest to implement app_stop as app_change SplashApplication and
app_start AppID as app_change AppID, with a special dummy application (SplashApplication) which would send heartbeats, disable any user input and take over the display to show some banner.
This way we do not need to change deep OMD logic yet but already have those actions.

elondaits · 2016-11-26T17:22:03Z

I don't mean to discuss important aspects of the system's working in this issue.

I mentioned app_start because just two days ago you had this:

@subcmd('start', help='start a service/application on a station')

... I just suggested changing "start" to "app_start" and "poweron" to "start" to simplify the syntax, not change the whole logic of the system.

But if there is a command to "start a service/application on a station" it's logical that there is one to stop it. Just that.

malex984 · 2016-11-26T17:57:12Z

I see. Sorry for confusion!

@elondaits Please check the updated Hilbert CLI.
Does it look minimal enough?

NOTE: output to docs/

NOTE: run `make tox` (or simply `tox`), provided Python2.7 and Python3.4 are available

Added remote execution via ssh/scp + Dummy client script

Usage output: hilbert-station - Station part for Linux systems Usage: hilbert-station -h Display this help message. hilbert-station -v Increase verbosity hilbert-station -q Decrease verbosity hilbert-station -p Pedantic mode hilbert-station -V Display version info. hilbert-station start Start everything hilbert-station stop Stop everything and shutdown hilbert-station prepare <new_cfg> Install a new local configuration hilbert-station app_switch <app_id> Change the currently running top application to specified one hilbert-station dm_start <vm_name> Start a VM using docker-machine

= changes in config/hilbert_cli_config.py: * _execute, + Host::ssh/Host::scp + Station::type: StationType: [hidden, server, standard?, standalone?] * fixes / logging = extended Hilbert::parse testing (test_validate.py): singleHostHilbert.yml

Added locking, SubCommands use legacy client scripts Updated client scripts: prepare/default/finishall/luncher seem to work TODO: check.fix topswitch.sh!

Feature/hilbert client scripts

NOTE: temporary include docker-compose.yml for completeness

hilbert Server-side tool: - Better general Logging in * disabled checking docker-compose-file during verification as it takes too long at HITS :-( * removed general timeout option to _execute Client tool: + More locations for Lockfile (special lock group on Fedora...) * Try sudo if simple shutdown fails... TODO: - repair x11vnc, - fix OGL.tgz generation (for NVIdia OpenGL) - When should we generate OGL.tgz?

Updates and Fixes due to (initial) testing installation and its presentation

malex984 · 2016-12-09T06:57:34Z

Note: remaining implementation-related issues will be collected in (#14)

Oleksandr Motsak added 5 commits November 7, 2016 08:05

Initial version of General Configuration Validation

8685e9a

Fixing first issues and extending

7360603

FIX: many fixes CHG/ADD: changes and additions to the class hierarchie, e.g.: * DockerMachie variant for PowerOnMethod * expect default values for optional fields!

Added supported_types field to Profile

711b100

NOTE: due to @porst17's comment #32 (comment) ADD: minor comments (TODOs for later)

Improved detection of parsed YAML data types, e.g. in GroupList, Base…

f9559e6

…Scalar and BaseString CHG: only keep the processed data is there was no error TODO: check that every validator follows this! REN: Abstract -> AbstractValidator

Updated the ConfigurationDD due to feedback

2b5c8ed

malex984 added this to the Configuration design milestone Nov 9, 2016

Oleksandr Motsak added 17 commits November 15, 2016 03:53

Added another unittest for checking warnings due to shaddowed mapping…

0e2cad6

… keys

Renamed name -> ref in DockerComposeService to avoid conflict w…

a6e0f04

…ith `Application::name`

Added complete configuration with docker-compose resources

3b778ee

Station::client_settings is optional due to standalone stations

e23485a

Added unit test for parsing the result

ab3922d

NOTE: the resulting .data.pickle should be generated under python2 (to be understood also by python3)

Minor fix

9bf3cf5

NOTE: parent should also be a Validator...

Starting to separate hilbert-cli-config.py into smaller units:

d3d52c1

helpers, loader, validators + main tool: hilbert Changes to the subcommand names in hilbert

Separated the main Hilbert tool from the library: /tools/hilbert.py

92b6179

NOTE: it requires ../config/*.py Also a few fixes in subcommand's argument handling (e.g. input sources & default values)

Added cfg_show [-o <obj>] subcommand to display (a part of) the loa…

850ec47

…ded configuration

Keep Validators to handle configuration objects

39cb7e2

NOTE: use .parse instead of .validate on temporary objects

Added generic Quering and Data-Dumping to Validators + corresponding …

e570cf5

…sub-commands to the hilbert tool CHG: improved Version Validator (keep semantic Version object, but show a version string) ADD: switched to logging instead of printing

Addded stubs of required sub-commands (for the Dashboard) to

a385fa7

server-side hilbert tool: - poweron/shutdown <StationID> [<args>] - deploy <StationID> - start/finish <StationID> [<ServiceID/ApplicationID>] - app_switch <StationID> <new ApplicationID>

Repair extends mechnismus for stations

710e24a

Updated General System Design Diagram

44bbadc

NOTE: see the legend to "General Hilbert Architecture" in the "Management back-end" google.document

porst17 requested changes Nov 22, 2016

View reviewed changes

Oleksandr Motsak added 3 commits November 22, 2016 14:02

Fixing hilbert CLI

0134132

Notes about missing things

1a84b6f

Minor data cleanup

c5409aa

Oleksandr Motsak added 2 commits November 26, 2016 18:38

Added Requirements + removed some unnecessary imports

d5934ef

Looking for a minimal CLI: no app_start/app_stop ATM

056ab1e

Oleksandr Motsak added 4 commits November 26, 2016 22:08

Better automated doc generation + source verification (pep8/pylint)

a3b3800

NOTE: output to docs/

Added simple initial setup.py and tox.ini

8d43978

NOTE: run `make tox` (or simply `tox`), provided Python2.7 and Python3.4 are available

Initial configuration for testing via TravisCI

fd0fefb

More .gitignore + fix a few minor warnings due to PEP8

31c32f5

porst17 approved these changes Nov 28, 2016

View reviewed changes

Oleksandr Motsak and others added 8 commits December 1, 2016 06:25

Added remote execution via ssh/scp + Dummy client script

78a5169

Merge pull request #39 from hilbert/feature/hilbert_client_usage

fd3fcaf

Added remote execution via ssh/scp + Dummy client script

Improved hilbert tool (Server-side)

7131db8

= changes in config/hilbert_cli_config.py: * _execute, + Host::ssh/Host::scp + Station::type: StationType: [hidden, server, standard?, standalone?] * fixes / logging = extended Hilbert::parse testing (test_validate.py): singleHostHilbert.yml

Client-side hilbert-station tool

7ddb638

Added locking, SubCommands use legacy client scripts Updated client scripts: prepare/default/finishall/luncher seem to work TODO: check.fix topswitch.sh!

Merge pull request #40 from hilbert/feature/hilbert_client_scripts

e7e8593

Feature/hilbert client scripts

Updated Station part of Hilbert

b9d2828

NOTE: temporary include docker-compose.yml for completeness

malex984 self-assigned this Dec 9, 2016

malex984 added the in progress label Dec 9, 2016

Merge pull request #43 from hilbert/feature/hilbert_client

6454c5d

Updates and Fixes due to (initial) testing installation and its presentation

malex984 changed the title ~~Configuration validator~~ Hilbert YAML configuration parser and Hilbert Server/Client-side tools Dec 9, 2016

porst17 mentioned this pull request Dec 9, 2016

[Super-Issue] Further implementation of Hilbert configuration parser and server/client-side CLI tools #14

Closed

19 tasks

malex984 merged commit b9c5031 into devel Dec 9, 2016

malex984 deleted the feature/config_validate branch December 9, 2016 06:56

This was referenced Dec 12, 2016

How does hilbert know from the config which ServiceRuntime to use? #32

Closed

Configuration Design (super issue) #13

Closed

Designing new command line interface (hilbert) hilbert/hilbert-docker-images#28

Closed

		return cfg.query(obj)


		@subcmd('cfg_query', help='Load Configuration and query some part of it')

		return load_yaml_file(f) # TODO: check that this is a dictionary!


		@subcmd('version', help='Display version info')

Hilbert YAML configuration parser and Hilbert Server/Client-side tools #34

Hilbert YAML configuration parser and Hilbert Server/Client-side tools #34

Conversation

malex984 commented Nov 9, 2016 • edited Loading

porst17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elondaits commented Nov 22, 2016

elondaits commented Nov 22, 2016

elondaits commented Nov 22, 2016

elondaits commented Nov 26, 2016

malex984 commented Nov 26, 2016

elondaits commented Nov 26, 2016

malex984 commented Nov 26, 2016

malex984 commented Dec 9, 2016

malex984 commented Nov 9, 2016 •

edited

Loading