From 644b7419693db3d623700af4095b2bf946b3d798 Mon Sep 17 00:00:00 2001 From: Ants Aasma Date: Fri, 19 May 2017 17:25:21 +0300 Subject: [PATCH] Add config editing to patronictl (#428) Current UI to change cluster configuration is somewhat unfriendly, involving a curl command, knowing the REST API endpoint, knowing the specific syntax to call it with and writing a JSON document. I added two commands in this branch to make this a bit easier, `show-config` and `edit-config` (names are merely placeholders, any opinions on better ones?). * `patronictl show-config clustername` fetches the config from DCS, formats it as YAML and outputs it. * `patronictl edit-config clustername` fetches the config, formats it as YAML, invokes $EDITOR on it, then shows user the diff and after confirmation applies the changed config to DCS, guarding for concurrent modifications. * `patronictl edit-config clustername --set synchronous_mode=true --set postgresql.use_slots=true` will set the specific key-value pairs. There are also some UI capabilities I'm less sure of, but included them here as I already implemented them. * If output is a tty then the diffs are colored. I'm not sure if this feature is cool enough to pull the weight of adding a dependency on cdiff. Or maybe someone knows of another more task focused diff coloring library? * `patronictl edit-config clustername --pg work_mem=100MB` - Shorthand for `--set postgresql.parameters.work_mem=100MB` * `patronictl edit-config clustername --apply changes.yaml` - apply changes from a yaml file. * `patronictl edit-config clustername --replace new-config.yaml` - replace config with new version. --- patroni/ctl.py | 213 +++++++++++++++++++++++++++++++++++++++++++++- requirements.txt | 1 + tests/test_ctl.py | 44 +++++++++- 3 files changed, 256 insertions(+), 2 deletions(-) diff --git a/patroni/ctl.py b/patroni/ctl.py index 94b95a33..5c80d3ae 100644 --- a/patroni/ctl.py +++ b/patroni/ctl.py @@ -4,27 +4,36 @@ Patroni Control import base64 import click +import codecs import datetime import dateutil.parser +import cdiff +import copy +import difflib +import io import json import logging import os import psycopg2 import random import requests +import subprocess import sys +import tempfile import time import tzlocal import yaml from click import ClickException +from contextlib import contextmanager from patroni.config import Config from patroni.dcs import get_dcs as _get_dcs from patroni.exceptions import PatroniException from patroni.postgresql import Postgresql -from patroni.utils import is_valid_pg_version +from patroni.utils import is_valid_pg_version, patch_config from prettytable import PrettyTable from six.moves.urllib_parse import urlparse +from six import text_type CONFIG_DIR_PATH = click.get_app_dir('patroni') CONFIG_FILE_PATH = os.path.join(CONFIG_DIR_PATH, 'patronictl.yaml') @@ -819,3 +828,205 @@ def pause(obj, cluster_name): @click.pass_obj def resume(obj, cluster_name): return toggle_pause(obj, cluster_name, False) + + +@contextmanager +def temporary_file(contents, suffix='', prefix='tmp'): + """Creates a temporary file with specified contents that persists for the context. + + :param contents: binary string that will be written to the file. + :param prefix: will be prefixed to the filename. + :param suffix: will be appended to the filename. + :returns path of the created file. + """ + tmp = tempfile.NamedTemporaryFile(suffix=suffix, prefix=prefix, delete=False) + with tmp: + tmp.write(contents) + + try: + yield tmp.name + finally: + os.unlink(tmp.name) + + +def show_diff(before_editing, after_editing): + """Shows a diff between two strings. + + If the output is to a tty the diff will be colored. Inputs are expected to be unicode strings. + """ + def listify(string): + return [l+'\n' for l in string.rstrip('\n').split('\n')] + + unified_diff = difflib.unified_diff(listify(before_editing), listify(after_editing)) + + if sys.stdout.isatty(): + buf = io.StringIO() + for line in unified_diff: + # Force cast to unicode as difflib on Python 2.7 returns a mix of unicode and str. + buf.write(text_type(line)) + buf.seek(0) + + class opts: + side_by_side = False + width = 80 + tab_width = 8 + cdiff.markup_to_pager(cdiff.PatchStream(buf), opts) + else: + for line in unified_diff: + click.echo(line.rstrip('\n')) + + +def format_config_for_editing(data): + """Formats configuration as YAML for human consumption. + + :param data: configuration as nested dictionaries + :returns unicode YAML of the configuration""" + return yaml.safe_dump(data, default_flow_style=False, encoding=None, allow_unicode=True) + + +def apply_config_changes(before_editing, data, kvpairs): + """Applies config changes specified as a list of key-value pairs. + + Keys are interpreted as dotted paths into the configuration data structure. Except for paths beginning with + `postgresql.parameters` where rest of the path is used directly to allow for PostgreSQL GUCs containing dots. + Values are interpreted as YAML values. + + :param before_editing: human representation before editing + :param data: configuration datastructure + :param kvpairs: list of strings containing key value pairs separated by = + :returns tuple of human readable and parsed datastructure after changes + """ + changed_data = copy.deepcopy(data) + + def set_path_value(config, path, value, prefix=()): + # Postgresql GUCs can't be nested, but can contain dots so we re-flatten the structure for this case + if prefix == ('postgresql', 'parameters'): + path = ['.'.join(path)] + + if len(path) == 1: + if value is None: + config.pop(path[0], None) + else: + config[path[0]] = value + else: + key = path[0] + if key not in config: + config[key] = {} + set_path_value(config[key], path[1:], value, prefix + (key,)) + if config[key] == {}: + del config[key] + + for pair in kvpairs: + if not pair or "=" not in pair: + raise PatroniCtlException("Invalid parameter setting {0}".format(pair)) + key_path, value = pair.split("=", 1) + set_path_value(changed_data, key_path.strip().split("."), yaml.safe_load(value)) + + return format_config_for_editing(changed_data), changed_data + + +def apply_yaml_file(data, filename): + """Applies changes from a YAML file to configuration + + :param data: configuration datastructure + :param filename: name of the YAML file, - is taken to mean standard input + :returns tuple of human readable and parsed datastructure after changes + """ + changed_data = copy.deepcopy(data) + + if filename == '-': + new_options = yaml.safe_load(sys.stdin) + else: + with open(filename) as fd: + new_options = yaml.safe_load(fd) + + patch_config(changed_data, new_options) + + return format_config_for_editing(changed_data), changed_data + + +def invoke_editor(before_editing, data, cluster_name): + """Starts editor command to edit configuration in human readable format + + :param before_editing: human representation before editing + :param data: configuration datastructure + :returns tuple of human readable and parsed datastructure after changes + """ + editor_cmd = os.environ.get('EDITOR') + if not editor_cmd: + raise PatroniCtlException('EDITOR environment variable is not set') + + with temporary_file(contents=before_editing.encode('utf-8'), + suffix='.yaml', + prefix='{0}-config-'.format(cluster_name)) as tmpfile: + ret = subprocess.call([editor_cmd, tmpfile]) + if ret: + raise PatroniCtlException("Editor exited with return code {0}".format(ret)) + + with codecs.open(tmpfile, encoding='utf-8') as fd: + after_editing = fd.read() + + return after_editing, yaml.safe_load(after_editing) + + +@ctl.command('edit-config', help="Edit cluster configuration") +@click.argument('cluster_name') +@click.option('--quiet', '-q', is_flag=True, help='Do not show changes') +@click.option('--set', '-s', 'kvpairs', multiple=True, + help='Set specific configuration value. Can be specified multiple times') +@click.option('--pg', '-p', 'pgkvpairs', multiple=True, + help='Set specific PostgreSQL parameter value. Shorthand for -s postgresql.parameters. ' + 'Can be specified multiple times') +@click.option('--apply', 'apply_filename', help='Apply configuration from file. Use - for stdin.') +@click.option('--replace', 'replace_filename', help='Apply configuration from file, replacing existing configuration.' + ' Use - for stdin.') +@option_force +@click.pass_obj +def edit_config(obj, cluster_name, force, quiet, kvpairs, pgkvpairs, apply_filename, replace_filename): + dcs = get_dcs(obj, cluster_name) + cluster = dcs.get_cluster() + + before_editing = format_config_for_editing(cluster.config.data) + + after_editing = None # Serves as a flag if any changes were requested + changed_data = cluster.config.data + + if replace_filename: + after_editing, changed_data = apply_yaml_file({}, replace_filename) + + if apply_filename: + after_editing, changed_data = apply_yaml_file(changed_data, apply_filename) + + if kvpairs or pgkvpairs: + all_pairs = list(kvpairs) + ['postgresql.parameters.'+v.lstrip() for v in pgkvpairs] + after_editing, changed_data = apply_config_changes(before_editing, changed_data, all_pairs) + + # If no changes were specified on the command line invoke editor + if after_editing is None: + after_editing, changed_data = invoke_editor(before_editing, cluster.config.data, cluster_name) + + if cluster.config.data == changed_data: + if not quiet: + click.echo("Not changed") + return + + if not quiet: + show_diff(before_editing, after_editing) + + if (apply_filename == '-' or replace_filename == '-') and not force: + click.echo("Use --force option to apply changes") + return + + if force or click.confirm('Apply these changes?'): + if not dcs.set_config_value(json.dumps(changed_data), cluster.config.modify_index): + raise PatroniCtlException("Config modification aborted due to concurrent changes") + click.echo("Configuration changed") + + +@ctl.command('show-config', help="Show cluster configuration") +@click.argument('cluster_name') +@click.pass_obj +def show_config(obj, cluster_name): + cluster = get_dcs(obj, cluster_name).get_cluster() + + click.echo(format_config_for_editing(cluster.config.data)) diff --git a/requirements.txt b/requirements.txt index ba3fe65d..99c4c80a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,3 +12,4 @@ prettytable>=0.7 tzlocal python-dateutil enum34 +cdiff diff --git a/tests/test_ctl.py b/tests/test_ctl.py index 645e7a4f..67644756 100644 --- a/tests/test_ctl.py +++ b/tests/test_ctl.py @@ -7,7 +7,8 @@ import unittest from click.testing import CliRunner from mock import patch, Mock from patroni.ctl import ctl, members, store_config, load_config, output_members, request_patroni, get_dcs, parse_dcs, \ - get_all_members, get_any_member, get_cursor, query_member, configure, PatroniCtlException + get_all_members, get_any_member, get_cursor, query_member, configure, PatroniCtlException, apply_config_changes, \ + format_config_for_editing, show_diff from patroni.dcs.etcd import Client from psycopg2 import OperationalError from test_etcd import etcd_read, requests_get, socket_getaddrinfo, MockResponse @@ -438,3 +439,44 @@ class TestCtl(unittest.TestCase): with patch('requests.patch', Mock(side_effect=Exception)): result = self.runner.invoke(ctl, ['resume', 'dummy']) assert 'Can not find accessible cluster member' in result.output + + def test_apply_config_changes(self): + config = {"postgresql": {"parameters": {"work_mem": "4MB"}, "use_pg_rewind": True}, "ttl": 30} + + before_editing = format_config_for_editing(config) + + # Spaces are allowed and stripped, numbers and booleans are interpreted + after_editing, changed_config = apply_config_changes(before_editing, config, + ["postgresql.parameters.work_mem = 5MB", "ttl=15", "postgresql.use_pg_rewind=off"]) + self.assertEquals(changed_config, + {"postgresql": {"parameters": {"work_mem": "5MB"}, "use_pg_rewind": False}, "ttl": 15}) + + # postgresql.parameters namespace is flattened + after_editing, changed_config = apply_config_changes(before_editing, config, + ["postgresql.parameters.work_mem.sub = x"]) + self.assertEquals(changed_config, + {"postgresql": {"parameters": {"work_mem": "4MB", "work_mem.sub": "x"}, "use_pg_rewind": True}, "ttl": 30}) + + # Setting to null deletes + after_editing, changed_config = apply_config_changes(before_editing, config, + ["postgresql.parameters.work_mem=null"]) + self.assertEquals(changed_config, + {"postgresql": {"use_pg_rewind": True}, "ttl": 30}) + after_editing, changed_config = apply_config_changes(before_editing, config, + ["postgresql.use_pg_rewind=null", "postgresql.parameters.work_mem=null"]) + self.assertEquals(changed_config, + {"ttl": 30}) + + @patch('sys.stdout.isatty', return_value=False) + @patch('cdiff.markup_to_pager') + def test_show_diff(self, mock_markup_to_pager, mock_isatty): + show_diff("foo:\n bar: 1\n", "foo:\n bar: 2\n") + mock_markup_to_pager.assert_not_called() + + mock_isatty.return_value = True + show_diff("foo:\n bar: 1\n", "foo:\n bar: 2\n") + mock_markup_to_pager.assert_called_once() + + # Test that unicode handling doesn't fail with an exception + show_diff(b"foo:\n bar: \xc3\xb6\xc3\xb6\n".decode('utf-8'), + b"foo:\n bar: \xc3\xbc\xc3\xbc\n".decode('utf-8')) \ No newline at end of file