By hashing the config values and files, only restart services on differences #31

addyess · 2024-08-22T13:51:02Z

Overview

calculate a hash for each config item on a kube service, and store the hash values on disk
each reconcile loop calculates the differences, if there are none -- don't restart the service

Details

store a hash file in _snap_common_path(service) / "args-hash.txt"
the hash file has a list of all the args and their sha256 hashed values so no secrets are leaked
the hash file also contains a sha256 of any file contents if the configured value is an on disk file that may have changed
if the hash for each item matches -- don't restart the service -- this saves 3x time on every reconcile

…nfig changes

addyess · 2024-08-22T13:52:36Z

charms/kubernetes_snaps.py

+    config_files = set(
+        map(
+            api_opts.get,
+            (
+                "tls-cert-file",
+                "tls-private-key-file",
+                "kubelet-certificate-authority",
+                "kubelet-client-certificate",
+                "kubelet-client-key",
+                "authentication-token-webhook-config-file",
+                "service-account-signing-key-file",
+                "service-account-key-file",
+                "encryption-provider-config",
+                "etcd-cafile",
+                "etcd-keyfile",
+                "etcd-certfile",
+                "authorization-webhook-config-file",
+                "requestheader-client-ca-file",
+                "proxy-client-cert-file",
+                "proxy-client-key-file",
+                "client-ca-file",
+                "audit-policy-file",
+                "audit-webhook-config-file",
+            ),
+        )
+    )


these keys point to config files that may have been overwritten during the reconcile period for the api server

Out of curiosity, how do you know that you didn't miss one?

I know... that's what i'm SO afraid of. Missing one would be bad

@eaudetcobello do you have any ideas/opinions on how to not miss a file? I could "scan all the arguments looking for file like strings" but that sounds not great

Honestly, I think this could become unmanageable in the future and might come back to bite us. Looking at the API server options, there seem to be a few common patterns:

Options with the file suffix

Options with the config suffix

Options that include certificate in the name

Options that include key in the name

It might be a good idea to check if an option has any of these indicators before trying to process the file. Here's an example: https://regex101.com/r/m5Gq9m/1

the full list of args that can be files are listed here:
https://kubernetes.io/docs/reference/command-line-tools-reference/kube-apiserver/

Some with comments like:
File with
Path to
File containing
file used

And the hits just keep coming. The docs and arguments names aren't consistent. Maybe in the go code there's a list -- but with every k8s release -- more and more arguments may or may not be config files. Yes -- each config file will need to be ensured it gets added to the list and the regex could be a cool way, but there's no guarantee upstream k8s will follow it.

actually, there's another argument here which I expressly excluded because it's not a config file but a log file: audit-log-path

We do not want to watch for changes in this file b/c it's a log -- always changing means the api server reboots every time.

I think this list requires brain-power or some other comprehensive list of arguments which are absolutely treated as "read" by the api-server when restarted

addyess · 2024-08-22T13:53:04Z

charms/kubernetes_snaps.py

+    config_files = set(
+        map(
+            controller_opts.get,
+            (
+                "root-ca-file",
+                "kubeconfig",
+                "authorization-kubeconfig",
+                "authentication-kubeconfig",
+                "service-account-private-key-file",
+                "tls-cert-file",
+                "tls-private-key-file",
+            ),
+        )
+    )
+


these keys point to config files that may have been overwritten during the reconcile period for the controller-manager

addyess · 2024-08-22T13:53:30Z

charms/kubernetes_snaps.py

+    config_files = {
+        kube_proxy_opts["config"],
+        kubeconfig,
+    }


these are config files that may have been overwritten during the reconcile period for the kube-proxy service

addyess · 2024-08-22T14:01:14Z

charms/kubernetes_snaps.py

-def configure_kubernetes_service(service, base_args, extra_args_config):
-    extra_args = parse_extra_args(extra_args_config)
-
-    args = {}
-    args.update(base_args)
-    args.update(extra_args)
-
-    # TODO: CIS arg handling???
-    # CIS benchmark action may inject kv config to pass failing tests. Merge
-    # these after the func args as they should take precedence.
-    # cis_args_key = "cis-" + service
-    # cis_args = db.get(cis_args_key) or {}
-    # args.update(cis_args)
-
-    # Remove any args with 'None' values (all k8s args are 'k=v') and
-    # construct an arg string for use by 'snap set'.
-    args = {k: v for k, v in args.items() if v is not None}
-    args = ['--%s="%s"' % arg for arg in args.items()]
-    args = " ".join(args)
-
-    cmd = ["snap", "set", service, f"args={args}"]
-    check_call(cmd)
-    service_restart(f"snap.{service}.daemon")
-
-


this is moved down the page...

charms/kubernetes_snaps.py

addyess · 2024-08-22T17:54:10Z

Events times from the same control-plane charm with and without these changes:

total time install -> active/idle
with -- 15min
without -- 36min

mateoflorido · 2024-08-22T19:52:53Z

charms/kubernetes_snaps.py

+    config_files = set(
+        map(
+            api_opts.get,
+            (
+                "tls-cert-file",
+                "tls-private-key-file",
+                "kubelet-certificate-authority",
+                "kubelet-client-certificate",
+                "kubelet-client-key",
+                "authentication-token-webhook-config-file",
+                "service-account-signing-key-file",
+                "service-account-key-file",
+                "encryption-provider-config",
+                "etcd-cafile",
+                "etcd-keyfile",
+                "etcd-certfile",
+                "authorization-webhook-config-file",
+                "requestheader-client-ca-file",
+                "proxy-client-cert-file",
+                "proxy-client-key-file",
+                "client-ca-file",
+                "audit-policy-file",
+                "audit-webhook-config-file",
+            ),
+        )
+    )


Honestly, I think this could become unmanageable in the future and might come back to bite us. Looking at the API server options, there seem to be a few common patterns:

Options with the file suffix

Options with the config suffix

Options that include certificate in the name

Options that include key in the name

It might be a good idea to check if an option has any of these indicators before trying to process the file. Here's an example: https://regex101.com/r/m5Gq9m/1

charms/kubernetes_snaps.py

mateoflorido · 2024-08-22T20:06:34Z

charms/kubernetes_snaps.py

+        log.debug("%s:   Added config value %s", service.capitalize(), key)
+    for key in removed:
+        log.debug("%s: Dropped config value %s", service.capitalize(), key)
+    for key in modified:
+        log.debug("%s: Updated config value %s", service.capitalize(), key)


Suggested change

log.debug("%s: Added config value %s", service.capitalize(), key)

for key in removed:

log.debug("%s: Dropped config value %s", service.capitalize(), key)

for key in modified:

log.debug("%s: Updated config value %s", service.capitalize(), key)

log.debug("%s: Added config value %s", service.capitalize(), key)

for key in removed:

log.debug("%s: Dropped config value %s", service.capitalize(), key)

for key in modified:

log.debug("%s: Updated config value %s", service.capitalize(), key)

addyess

@mateoflorido I think you'll like this. We can use this env var to control from the charm whether or not we want to use this caching feature.

addyess · 2024-09-06T21:47:13Z

charms/kubernetes_snaps.py

+def _enable_config_hashing() -> bool:
+    """This is a debug option to enable config hashing."""
+    env = os.environ.get("JUJU_FEATURE_KUBERNETES_SNAP_CONFIG_HASHING") or ""
+    return env.lower() == "on"
+
+


This allows the charm at runtime (per hook even!!!) decide whether or not it wants to enable hashing.

Now an upgrade action could restart the service even if there aren't any config changes. We could even expose via charm config (though i think that'd be going too far).

addyess · 2024-09-06T21:48:13Z

charms/kubernetes_snaps.py

+    if not _enable_config_hashing():
+        log.debug("Config hashing disabled, skipping config change detection")
+        yield True
+        args_hash.unlink(missing_ok=True)
+        return


if hashing isn't enabled, this method declares there are changes, and we should reconfigure the service (as before this feature existed) and it also clears the cache file if one exists.

By hashing the config values and files, only restart services when co…

a97748f

…nfig changes

addyess commented Aug 22, 2024

View reviewed changes

charms/kubernetes_snaps.py Outdated Show resolved Hide resolved

eaudetcobello reviewed Aug 22, 2024

View reviewed changes

charms/kubernetes_snaps.py Outdated Show resolved Hide resolved

addyess force-pushed the akd/config-hashing-speedup branch 2 times, most recently from b1dd359 to fa9a3db Compare August 22, 2024 15:44

Confirm changes to a config file produce different has values

5c7fe83

addyess force-pushed the akd/config-hashing-speedup branch from fa9a3db to 5c7fe83 Compare August 22, 2024 15:50

check file existence with Path.is_file

39d841e

mateoflorido reviewed Aug 22, 2024

View reviewed changes

addyess added 2 commits August 22, 2024 15:37

improved set operations

768eb06

enable/disable cache calculation feature with env var

c1ebc5e

addyess commented Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

By hashing the config values and files, only restart services on differences #31

By hashing the config values and files, only restart services on differences #31

addyess commented Aug 22, 2024

addyess Aug 22, 2024

eaudetcobello Aug 22, 2024

addyess Aug 22, 2024

addyess Aug 22, 2024

mateoflorido Aug 22, 2024

addyess Aug 22, 2024 •

edited

Loading

addyess Aug 22, 2024 •

edited

Loading

addyess Aug 22, 2024

addyess Aug 22, 2024

addyess Aug 22, 2024

addyess commented Aug 22, 2024

mateoflorido Aug 22, 2024

mateoflorido Aug 22, 2024

addyess left a comment

addyess Sep 6, 2024

addyess Sep 6, 2024

By hashing the config values and files, only restart services on differences #31

Are you sure you want to change the base?

By hashing the config values and files, only restart services on differences #31

Conversation

addyess commented Aug 22, 2024

Overview

Details

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

addyess Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

addyess Aug 22, 2024 • edited Loading

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

addyess commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addyess left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addyess Aug 22, 2024 •

edited

Loading

addyess Aug 22, 2024 •

edited

Loading