Repository: zalando/patroni
Pull request #2585 (2023-03-06 14:10:33)

Make K8s retriable HTTP status code configurable
Configuration parameter is `kubernetes.retriable_http_codes` or `PATRONI_KUBERNETES_RETRIABLE_HTTP_CODES` environment variable.

These status codes are added to the default list of 500, 503, 504.

Close https://github.com/zalando/patroni/issues/2536

coveralls: ## Pull Request Test Coverage Report for [Build 4344319694](https://coveralls.io/builds/57569171)

* **18** of **18**   **(100.0%)**  changed or added relevant lines in **2** files are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage increased (+**0.0001%**) to **99.852%**

---



|  Totals | [![Coverage Status](https://coveralls.io/builds/57569171/badge)](https://coveralls.io/builds/57569171) |
| :-- | --: |
| Change from base [Build 4312101654](https://coveralls.io/builds/57457984): |  0.0001% |
| Covered Lines: | 12105 |
| Relevant Lines: | 12123 |

---
##### 💛  - [Coveralls](https://coveralls.io)

hughcapet: 👍 

Patch:
diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md
deleted file mode 100644
index f0f6ca04..00000000
--- a/.github/ISSUE_TEMPLATE/bug_report.md
+++ /dev/null
@@ -1,48 +0,0 @@
----
-name: Bug report
-about: Create a report to help us improve
-title: ''
-labels: ''
-assignees: ''
-
----
-
-**Describe the bug**
-A clear and concise description of what the bug is.
-
-**To Reproduce**
-Steps to reproduce the behavior:
-
-**Expected behavior**
-A clear and concise description of what you expected to happen.
-
-**Screenshots**
-If applicable, add screenshots to help explain your problem.
-
-**Environment**
--   Patroni version:
--   PostgreSQL version:
--   DCS (and its version):
-
-**Patroni configuration file**
-```
-Please copy&paste your Patroni configuration file here
-```
-
-**patronictl show-config**
-```
-Please copy&paste the output of "patronictl show-config" command here
-```
-
-**Have you checked Patroni logs?**
-Please provide a snippet of Patroni log files here
-
-**Have you checked PostgreSQL logs?**
-Please provide a snippet here
-
-**Have you tried to use GitHub issue search?**
-Maybe there is already a similar issue solved.
-
-
-**Additional context**
-Add any other context about the problem here.
diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml
new file mode 100644
index 00000000..f8f14c58
--- /dev/null
+++ b/.github/ISSUE_TEMPLATE/bug_report.yml
@@ -0,0 +1,97 @@
+name: Bug Report
+description: Create a report to help us improve
+labels:
+- bug
+body:
+- type: markdown
+  attributes:
+    value: |
+      If you have a question please post it on channel [#patroni](https://postgresteam.slack.com/archives/C9XPYG92A) in the [PostgreSQL Slack](https://postgresteam.slack.com).
+      Before reporting a bug please make sure to **reproduce it with the latest Patroni version**!
+      Please fill the form below and provide as much information as possible.
+      Not doing so may result in your bug not being addressed in a timely manner.
+
+- type: textarea
+  id: problem
+  attributes:
+    label: What happened?
+  validations:
+    required: true
+
+- type: textarea
+  id: repro
+  attributes:
+    label: How can we reproduce it (as minimally and precisely as possible)?
+  validations:
+    required: true
+
+- type: textarea
+  id: expected
+  attributes:
+    label: What did you expect to happen?
+  validations:
+    required: true
+
+- type: textarea
+  id: environment
+  attributes:
+    label: Patroni/PostgreSQL/DCS version
+    value: |
+      -   Patroni version: 
+      -   PostgreSQL version: 
+      -   DCS (and its version): 
+  validations:
+    required: true
+
+- type: textarea
+  id: patroniConfig
+  attributes:
+    label: Patroni configuration file
+    description: Please copy and paste Patroni configuration file here. This will be automatically formatted into code, so no need for backticks.
+    render: yaml
+  validations:
+    required: true
+
+- type: textarea
+  id: globalConfig
+  attributes:
+    label: patronictl show-config
+    description: Please copy and paste `patronictl show-config` output here. This will be automatically formatted into code, so no need for backticks.
+    render: yaml
+  validations:
+    required: true
+
+- type: textarea
+  id: patroniLogs
+  attributes:
+    label: Patroni log files
+    description: Please copy and paste any relevant Patroni log output. This will be automatically formatted into code, so no need for backticks.
+    render: shell
+  validations:
+    required: true
+
+- type: textarea
+  id: postgresLogs
+  attributes:
+    label: PostgreSQL log files
+    description: Please copy and paste any relevant PostgreSQL log output. This will be automatically formatted into code, so no need for backticks.
+    render: shell
+  validations:
+    required: true
+
+- type: checkboxes
+  id: issueSearch
+  attributes:
+    label: Have you tried to use GitHub issue search?
+    description: Maybe there is already a similar issue solved.
+    options:
+    - label: 'Yes'
+      required: true
+  validations:
+    required: true
+
+- type: textarea
+  id: additional
+  attributes:
+    label: Anything else we need to know?
+    description: Add any other context about the problem here.
diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml
index 3ba13e0c..e1d4265c 100644
--- a/.github/ISSUE_TEMPLATE/config.yml
+++ b/.github/ISSUE_TEMPLATE/config.yml
@@ -1,1 +1,5 @@
 blank_issues_enabled: false
+contact_links:
+- name: Question
+  url: https://postgresteam.slack.com/archives/C9XPYG92A
+  about: Please ask question about Patroni on Slack
diff --git a/docs/ENVIRONMENT.rst b/docs/ENVIRONMENT.rst
index f0c56227..edb62c77 100644
--- a/docs/ENVIRONMENT.rst
+++ b/docs/ENVIRONMENT.rst
@@ -117,6 +117,7 @@ Kubernetes
 -  **PATRONI\_KUBERNETES\_POD\_IP**: (optional) IP address of the pod Patroni is running in. This value is required when `PATRONI_KUBERNETES_USE_ENDPOINTS` is enabled and is used to populate the leader endpoint subsets when the pod's PostgreSQL is promoted.
 -  **PATRONI\_KUBERNETES\_PORTS**: (optional) if the Service object has the name for the port, the same name must appear in the Endpoint object, otherwise service won't work. For example, if your service is defined as ``{Kind: Service, spec: {ports: [{name: postgresql, port: 5432, targetPort: 5432}]}}``, then you have to set ``PATRONI_KUBERNETES_PORTS='[{"name": "postgresql", "port": 5432}]'`` and Patroni will use it for updating subsets of the leader Endpoint. This parameter is used only if `PATRONI_KUBERNETES_USE_ENDPOINTS` is set.
 -  **PATRONI\_KUBERNETES\_CACERT**: (optional) Specifies the file with the CA_BUNDLE file with certificates of trusted CAs to use while verifying Kubernetes API SSL certs. If not provided, patroni will use the value provided by the ServiceAccount secret.
+-  **PATRONI\_RETRIABLE\_HTTP\_CODES**: (optional) list of HTTP status codes from K8s API to retry on. By default Patroni is retrying on ``500``, ``503``, and ``504``, or if K8s API response has ``retry-after`` HTTP header.
 
 Raft (deprecated)
 -----------------
diff --git a/docs/SETTINGS.rst b/docs/SETTINGS.rst
index fae83030..4577f15c 100644
--- a/docs/SETTINGS.rst
+++ b/docs/SETTINGS.rst
@@ -218,6 +218,7 @@ Kubernetes
 -  **pod\_ip**: (optional) IP address of the pod Patroni is running in. This value is required when `use_endpoints` is enabled and is used to populate the leader endpoint subsets when the pod's PostgreSQL is promoted.
 -  **ports**: (optional) if the Service object has the name for the port, the same name must appear in the Endpoint object, otherwise service won't work. For example, if your service is defined as ``{Kind: Service, spec: {ports: [{name: postgresql, port: 5432, targetPort: 5432}]}}``, then you have to set ``kubernetes.ports: [{"name": "postgresql", "port": 5432}]`` and Patroni will use it for updating subsets of the leader Endpoint. This parameter is used only if `kubernetes.use_endpoints` is set.
 -  **cacert**: (optional) Specifies the file with the CA_BUNDLE file with certificates of trusted CAs to use while verifying Kubernetes API SSL certs. If not provided, patroni will use the value provided by the ServiceAccount secret.
+-  **retriable\_http\_codes**: (optional) list of HTTP status codes from K8s API to retry on. By default Patroni is retrying on ``500``, ``503``, and ``504``, or if K8s API response has ``retry-after`` HTTP header.
 
 
 .. _raft_settings:
diff --git a/patroni/config.py b/patroni/config.py
index 91b14186..d69ff55a 100644
--- a/patroni/config.py
+++ b/patroni/config.py
@@ -363,8 +363,8 @@ class Config(object):
                               'CACERT', 'CERT', 'KEY', 'VERIFY', 'TOKEN', 'CHECKS', 'DC', 'CONSISTENCY',
                               'REGISTER_SERVICE', 'SERVICE_CHECK_INTERVAL', 'SERVICE_CHECK_TLS_SERVER_NAME',
                               'SERVICE_TAGS', 'NAMESPACE', 'CONTEXT', 'USE_ENDPOINTS', 'SCOPE_LABEL', 'ROLE_LABEL',
-                              'POD_IP', 'PORTS', 'LABELS', 'BYPASS_API_SERVICE', 'KEY_PASSWORD', 'USE_SSL', 'SET_ACLS',
-                              'GROUP', 'DATABASE') and name:
+                              'POD_IP', 'PORTS', 'LABELS', 'BYPASS_API_SERVICE', 'RETRIABLE_HTTP_CODES', 'KEY_PASSWORD',
+                              'USE_SSL', 'SET_ACLS', 'GROUP', 'DATABASE') and name:
                     value = os.environ.pop(param)
                     if name == 'CITUS':
                         if suffix == 'GROUP':
@@ -373,7 +373,7 @@ class Config(object):
                             continue
                     elif suffix == 'PORT':
                         value = value and parse_int(value)
-                    elif suffix in ('HOSTS', 'PORTS', 'CHECKS', 'SERVICE_TAGS'):
+                    elif suffix in ('HOSTS', 'PORTS', 'CHECKS', 'SERVICE_TAGS', 'RETRIABLE_HTTP_CODES'):
                         value = value and _parse_list(value)
                     elif suffix in ('LABELS', 'SET_ACLS'):
                         value = _parse_dict(value)
diff --git a/patroni/dcs/kubernetes.py b/patroni/dcs/kubernetes.py
index 80afb0ff..506066b8 100644
--- a/patroni/dcs/kubernetes.py
+++ b/patroni/dcs/kubernetes.py
@@ -19,6 +19,7 @@ from urllib3 import Timeout
 from urllib3.exceptions import HTTPError
 from six.moves.http_client import HTTPException
 from threading import Condition, Lock, Thread
+from typing import Any, Dict, List, Optional
 
 from . import AbstractDCS, Cluster, ClusterConfig, Failover, Leader, Member, SyncState,\
         TimelineHistory, CITUS_COORDINATOR_GROUP_ID, citus_group_re
@@ -220,7 +221,7 @@ class K8sClient(object):
 
         _API_URL_PREFIX = '/api/v1/namespaces/'
 
-        def __init__(self, bypass_api_service=False):
+        def __init__(self, bypass_api_service: Optional[bool] = False) -> None:
             self._bypass_api_service = bypass_api_service
             self.pool_manager = urllib3.PoolManager(**k8s_config.pool_config)
             self._base_uri = k8s_config.server
@@ -488,11 +489,15 @@ class KubernetesRetriableException(k8s_client.rest.ApiException):
 
 
 class CoreV1ApiProxy(object):
+    """Proxy class to work with k8s_client.CoreV1Api() object"""
 
-    def __init__(self, use_endpoints=False, bypass_api_service=False):
+    _DEFAULT_RETRIABLE_HTTP_CODES = frozenset([500, 503, 504])
+
+    def __init__(self, use_endpoints: Optional[bool] = False, bypass_api_service: Optional[bool] = False) -> None:
         self._api_client = k8s_client.ApiClient(bypass_api_service)
         self._core_v1_api = k8s_client.CoreV1Api(self._api_client)
         self._use_endpoints = bool(use_endpoints)
+        self._retriable_http_codes = set(self._DEFAULT_RETRIABLE_HTTP_CODES)
 
     def configure_timeouts(self, loop_wait, retry_timeout, ttl):
         # Normally every loop_wait seconds we should have receive something from the socket.
@@ -504,10 +509,21 @@ class CoreV1ApiProxy(object):
         self._api_client.set_read_timeout(retry_timeout)
         self._api_client.set_api_servers_cache_ttl(loop_wait)
 
+    def configure_retriable_http_codes(self, retriable_http_codes: List[int]) -> None:
+        self._retriable_http_codes = self._DEFAULT_RETRIABLE_HTTP_CODES | set(retriable_http_codes)
+
     def refresh_api_servers_cache(self):
         self._api_client.refresh_api_servers_cache()
 
-    def __getattr__(self, func):
+    def __getattr__(self, func: str):
+        """Intercepts calls to `CoreV1Api` methods.
+
+        Handles two important cases:
+        1. Depending on whether Patroni is configured to work with `ConfigMaps` or `Endpoints`
+           it remaps "virtual" method names from `*_kind` to `*_endpoints` or `*_config_map`.
+        2. It handles HTTP error codes and raises `KubernetesRetriableException`
+           if the given error is supposed to be handled with retry."""
+
         if func.endswith('_kind'):
             func = func[:-4] + ('endpoints' if self._use_endpoints else 'config_map')
 
@@ -515,7 +531,7 @@ class CoreV1ApiProxy(object):
             try:
                 return getattr(self._core_v1_api, func)(*args, **kwargs)
             except k8s_client.rest.ApiException as e:
-                if e.status in (500, 503, 504) or e.headers and 'retry-after' in e.headers:  # XXX
+                if e.status in self._retriable_http_codes or e.headers and 'retry-after' in e.headers:
                     raise KubernetesRetriableException(e)
                 raise
         return wrapper
@@ -775,10 +791,24 @@ class Kubernetes(AbstractDCS):
     def set_retry_timeout(self, retry_timeout):
         self._retry.deadline = retry_timeout
 
-    def reload_config(self, config):
+    def reload_config(self, config: Dict[str, Any]) -> None:
+        """Handles dynamic config changes.
+
+        Either cause by changes in the local configuration file + SIGHUP or by changes of dynamic configuration"""
+
         super(Kubernetes, self).reload_config(config)
         self._api.configure_timeouts(self.loop_wait, self._retry.deadline, self.ttl)
 
+        # retriable_http_codes supposed to be either int, list of integers or comma-separated string with integers.
+        retriable_http_codes = config.get('retriable_http_codes', [])
+        if not isinstance(retriable_http_codes, list):
+            retriable_http_codes = [c.strip() for c in str(retriable_http_codes).split(',')]
+
+        try:
+            self._api.configure_retriable_http_codes([int(c) for c in retriable_http_codes])
+        except Exception as e:
+            logger.warning('Invalid value of retriable_http_codes = %s: %r', config['retriable_http_codes'], e)
+
     @staticmethod
     def member(pod):
         annotations = pod.metadata.annotations or {}
diff --git a/patroni/validator.py b/patroni/validator.py
index 0aad3f51..5eb7b7f7 100644
--- a/patroni/validator.py
+++ b/patroni/validator.py
@@ -365,6 +365,7 @@ schema = Schema({
           Optional("use_endpoints"): bool,
           Optional("pod_ip"): Or(is_ipv4_address, is_ipv6_address),
           Optional("ports"): [{"name": str, "port": int}],
+          Optional("retriable_http_codes"): Or(int, [int]),
           },
       }),
   Optional("citus"): {


Test patch:
diff --git a/tests/test_config.py b/tests/test_config.py
index 6b607c1f..172e8562 100644
--- a/tests/test_config.py
+++ b/tests/test_config.py
@@ -61,6 +61,7 @@ class TestConfig(unittest.TestCase):
             'PATRONI_KUBERNETES_LABELS': 'a: b: c',
             'PATRONI_KUBERNETES_SCOPE_LABEL': 'a',
             'PATRONI_KUBERNETES_PORTS': '[{"name": "postgresql"}]',
+            'PATRONI_KUBERNETES_RETRIABLE_HTTP_CODES': '401',
             'PATRONI_ZOOKEEPER_HOSTS': "'host1:2181','host2:2181'",
             'PATRONI_EXHIBITOR_HOSTS': 'host1,host2',
             'PATRONI_EXHIBITOR_PORT': '8181',
diff --git a/tests/test_kubernetes.py b/tests/test_kubernetes.py
index 7e13210f..eacdcea1 100644
--- a/tests/test_kubernetes.py
+++ b/tests/test_kubernetes.py
@@ -317,6 +317,17 @@ class TestKubernetesConfigMaps(BaseTestKubernetes):
     def test_set_history_value(self):
         self.k.set_history_value('{}')
 
+    @patch('patroni.dcs.kubernetes.logger.warning')
+    def test_reload_config(self, mock_warning):
+        self.k.reload_config({'loop_wait': 10, 'ttl': 30, 'retry_timeout': 10, 'retriable_http_codes': '401, 403 '})
+        self.assertEqual(self.k._api._retriable_http_codes, self.k._api._DEFAULT_RETRIABLE_HTTP_CODES | set([401, 403]))
+        self.k.reload_config({'loop_wait': 10, 'ttl': 30, 'retry_timeout': 10, 'retriable_http_codes': 402})
+        self.assertEqual(self.k._api._retriable_http_codes, self.k._api._DEFAULT_RETRIABLE_HTTP_CODES | set([402]))
+        self.k.reload_config({'loop_wait': 10, 'ttl': 30, 'retry_timeout': 10, 'retriable_http_codes': [405, 406]})
+        self.assertEqual(self.k._api._retriable_http_codes, self.k._api._DEFAULT_RETRIABLE_HTTP_CODES | set([405, 406]))
+        self.k.reload_config({'loop_wait': 10, 'ttl': 30, 'retry_timeout': 10, 'retriable_http_codes': True})
+        mock_warning.assert_called_once()
+
 
 class TestKubernetesEndpoints(BaseTestKubernetes):
 
diff --git a/tests/test_validator.py b/tests/test_validator.py
index 244cd342..9f4d261c 100644
--- a/tests/test_validator.py
+++ b/tests/test_validator.py
@@ -59,6 +59,7 @@ config = {
         "use_endpoints": False,
         "pod_ip": "127.0.0.1",
         "ports": [{"name": "string", "port": 1000}],
+        "retriable_http_codes": [401],
     },
     "postgresql": {
         "listen": "127.0.0.2,::1:543",