Repository: canonical/operator

Issue:
`RelationDataContent.__setitem__` should dynamically dispatch to a file if it's too long
We've already seen this with Grafana Dashboards, which routinely overflow the maximum argument length from `subprocess`, but it was also observed that relating Prometheus to a very large number of targets could overflow and cause a strange looking `OSError` on a `RelationChangedEvent`

Ultimately, this is due to [`relation_set`](https://github.com/canonical/operator/blob/0a097748299506c7651bdef99524638146f9724a/ops/model.py#L951) calling back to `subprocess` to handle [`relation-set ...`](https://github.com/canonical/operator/blob/0a097748299506c7651bdef99524638146f9724a/ops/model.py#L2136). 

We already [split long log messages](https://github.com/canonical/operator/pull/632), and `relation-set` takes a `--file` parameter which reads in YAML, allowing the limit to be bypassed. If OF determines that the length of the relation data is anywhere near the limit, we could defer to something like:

```python
with tempfile.TempFile() as relation_data:
    with open(relation_data, "w") as f:
        f.write(yaml.dump({key: value})
    self._backend.relation_set(..., data_file=relation_data)
```

If an optarg were added to `relation_set` where, if present, data was loaded from a file. This seems easy enough to add, avoids requiring charm authors to carefully think about the size/length of their data bags and potentially destructure them to avoid it mapping back to a `map[string]string` on the backend, and yields the desired behavior.

Discussion:

Patch:
diff --git a/ops/model.py b/ops/model.py
index b4940d2..9682548 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -28,7 +28,7 @@ import typing
 import weakref
 from abc import ABC, abstractmethod
 from pathlib import Path
-from subprocess import PIPE, CalledProcessError, run
+from subprocess import PIPE, CalledProcessError, CompletedProcess, run
 from typing import (
     Any,
     BinaryIO,
@@ -2025,9 +2025,12 @@ class _ModelBackend:
         self._leader_check_time = None
         self._hook_is_running = ''
 
-    def _run(self, *args: str, return_output: bool = False, use_json: bool = False
+    def _run(self, *args: str, return_output: bool = False,
+             use_json: bool = False, input_stream: Optional[bytes] = None
              ) -> Union[str, 'JsonObject', None]:
-        kwargs = dict(stdout=PIPE, stderr=PIPE, check=True)
+        kwargs = dict(stdout=PIPE, stderr=PIPE, check=True)  # type: Dict[str, Any]
+        if input_stream:
+            kwargs.update({"input": input_stream})
         which_cmd = shutil.which(args[0])
         if which_cmd is None:
             raise RuntimeError('command not found: {}'.format(args[0]))
@@ -2036,6 +2039,10 @@ class _ModelBackend:
             args += ('--format=json',)
         try:
             result = run(args, **kwargs)
+
+            # pyright infers the first match when argument overloading/unpacking is used,
+            # so this needs to be coerced into the right type
+            result = typing.cast('CompletedProcess[bytes]', result)
         except CalledProcessError as e:
             raise ModelError(e.stderr)
         if return_output:
@@ -2133,12 +2140,14 @@ class _ModelBackend:
                 raise RuntimeError(
                     'setting application data is not supported on Juju version {}'.format(version))
 
-        args = ['relation-set', '-r', str(relation_id), '{}={}'.format(key, value)]
+        args = ['relation-set', '-r', str(relation_id)]
         if is_app:
             args.append('--app')
+        args.extend(["--file", "-"])
 
         try:
-            return self._run(*args)
+            content = yaml.safe_dump({key: value}, encoding='utf8')  # type: ignore
+            return self._run(*args, input_stream=content)
         except ModelError as e:
             if self._is_relation_not_found(e):
                 raise RelationNotFoundError() from e


Test patch:
diff --git a/test/test_model.py b/test/test_model.py
index 838f653..0d38eed 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -1969,17 +1969,17 @@ class TestModelBackend(unittest.TestCase):
             lambda: fake_script(self, 'relation-set', 'echo fooerror >&2 ; exit 1'),
             lambda: self.backend.relation_set(3, 'foo', 'bar', is_app=False),
             ops.model.ModelError,
-            [['relation-set', '-r', '3', 'foo=bar']],
+            [['relation-set', '-r', '3', '--file', '-']],
         ), (
             lambda: fake_script(self, 'relation-set', 'echo {} >&2 ; exit 2'.format(err_msg)),
             lambda: self.backend.relation_set(3, 'foo', 'bar', is_app=False),
             ops.model.RelationNotFoundError,
-            [['relation-set', '-r', '3', 'foo=bar']],
+            [['relation-set', '-r', '3', '--file', '-']],
         ), (
             lambda: None,
             lambda: self.backend.relation_set(3, 'foo', 'bar', is_app=True),
             ops.model.RelationNotFoundError,
-            [['relation-set', '-r', '3', 'foo=bar', '--app']],
+            [['relation-set', '-r', '3', '--app', '--file', '-']],
         ), (
             lambda: fake_script(self, 'relation-get', 'echo fooerror >&2 ; exit 1'),
             lambda: self.backend.relation_get(3, 'remote/0', is_app=False),
@@ -2027,15 +2027,25 @@ class TestModelBackend(unittest.TestCase):
     def test_relation_set_juju_version_quirks(self):
         self.addCleanup(os.environ.pop, 'JUJU_VERSION', None)
 
-        fake_script(self, 'relation-set', 'exit 0')
-
         # on 2.7.0+, things proceed as expected
         for v in ['2.8.0', '2.7.0']:
             with self.subTest(v):
-                os.environ['JUJU_VERSION'] = v
-                self.backend.relation_set(1, 'foo', 'bar', is_app=True)
-                calls = [' '.join(i) for i in fake_script_calls(self, clear=True)]
-                self.assertEqual(calls, ['relation-set -r 1 foo=bar --app'])
+                t = tempfile.NamedTemporaryFile()
+                try:
+                    fake_script(self, 'relation-set', dedent("""
+                        cat >> {}
+                        """).format(pathlib.Path(t.name).as_posix()))
+                    os.environ['JUJU_VERSION'] = v
+                    self.backend.relation_set(1, 'foo', 'bar', is_app=True)
+                    calls = [' '.join(i) for i in fake_script_calls(self, clear=True)]
+                    self.assertEqual(calls, ['relation-set -r 1 --app --file -'])
+                    t.seek(0)
+                    content = t.read()
+                finally:
+                    t.close()
+                self.assertEqual(content.decode('utf-8'), dedent("""\
+                    foo: bar
+                    """))
 
         # before 2.7.0, it just fails always (no --app support)
         os.environ['JUJU_VERSION'] = '2.6.9'