Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for exec "service-context" #957

Merged
merged 11 commits into from
Jul 17, 2023
15 changes: 15 additions & 0 deletions ops/jujuversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,3 +128,18 @@ def supports_open_port_on_k8s(self) -> bool:
"""Report whether this Juju version supports open-port on Kubernetes."""
# Support added: https://bugs.launchpad.net/juju/+bug/1920960
return (self.major, self.minor, self.patch) >= (3, 0, 3)

@property
def supports_exec_service_context(self) -> bool:
"""Report whether this Juju version supports exec's service_context option."""
if self.major < 3:
return False # 2.9 doesn't support it
if self.major > 3:
return True # 4.x presumably will
if self.minor == 0:
return False # 3.0 doesn't support it
if self.minor == 1:
return self.patch >= 6 # 3.1.6+ supports it
if self.minor == 2:
return self.patch >= 2 # 3.2.2+ supports it
return True # 3.3+ will
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fwiw, 3.2.1 isn't "out" it just is a burned release number so nobody will ever run it.

Anyway, are we sure that this is a true statement? Have we already landed the pebble changes into the juju branches such that the next releases will have the service context?

Would this read cleaner as:

if (self.major, self.minor, self.patch) < (3,1,6):
  # First released in 3.1.6
  return False
if (self.major.self.minor,self.patch) == (3,2,0):
  # 3.2.0 was released before pebble was updated, but all other 3.2 releases have the change
  return False
return True

9 changes: 9 additions & 0 deletions ops/model.py
Original file line number Diff line number Diff line change
Expand Up @@ -2360,6 +2360,7 @@ def exec( # noqa
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2381,6 +2382,7 @@ def exec( # noqa
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2400,6 +2402,7 @@ def exec(
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2418,8 +2421,14 @@ def exec(
See :meth:`ops.pebble.Client.exec` for documentation of the parameters
and return value, as well as examples.
"""
if service_context is not None:
version = JujuVersion.from_environ()
if not version.supports_exec_service_context:
raise RuntimeError(
f'exec with service_context not supported on Juju version {version}')
return self._pebble.exec(
command,
service_context=service_context,
environment=environment,
working_dir=working_dir,
timeout=timeout,
Expand Down
34 changes: 31 additions & 3 deletions ops/pebble.py
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@
'user-id': Optional[int],
'group': str,
'group-id': Optional[int],
'working-dir': str,
'on-success': str,
'on-failure': str,
'on-check-failure': Dict[str, Any],
Expand All @@ -88,9 +89,25 @@
},
total=False)

HttpDict = typing.TypedDict('HttpDict', {'url': str})
TcpDict = typing.TypedDict('TcpDict', {'port': int})
ExecDict = typing.TypedDict('ExecDict', {'command': str})
HttpDict = typing.TypedDict('HttpDict',
{'url': str,
'headers': Dict[str, str]},
total=False)
TcpDict = typing.TypedDict('TcpDict',
{'port': int,
'host': str},
total=False)
ExecDict = typing.TypedDict('ExecDict',
{'command': str,
# see JujuVersion.supports_exec_service_context
'service-context': str,
'environment': Dict[str, str],
'user-id': Optional[int],
'user': str,
'group-id': Optional[int],
'group': str,
'working-dir': str},
total=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did these get so out of sync with what you can put into them? Presumably because they are just dicts (so you could have always passed additional information there).
But that also implies that we are missing test cases that the extra fields have any impact on the requests.
I suppose mostly these are just curried to Pebble, and all the testing should happen at that level.
It does feel like we are missing something about the contract with an external system
(for example, someone might spell it service_context vs service-context, and while the typeddict will catch with a linter, it would be nice to have some form of validation that it matches what pebble actually expects)

What if pebble had a "validate these arguments" mode, where it didn't actually run it, but at least validated the content, and then an ops test that if it doesn't see pebble in the path, just gets skipped, but otherwise asks to validate args that we want to pass?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These types only just became public (in ops 2.4.0), so that's probably the main reason. I wanted them to be public so people could use them to type check their pebble.LayerDicts in their charms. If people are using this new type annotations for their dicts, it will check them (though they don't have to).

Separately, we're planning to add a --dry mode for validating layer config to Pebble (canonical/pebble#214). But I think additional work here is out of scope, at least for now.


CheckDict = typing.TypedDict('CheckDict',
{'override': str,
Expand Down Expand Up @@ -800,6 +817,7 @@ def __init__(self, name: str, raw: Optional['ServiceDict'] = None):
self.user_id = dct.get('user-id')
self.group = dct.get('group', '')
self.group_id = dct.get('group-id')
self.working_dir = dct.get('working-dir', '')
self.on_success = dct.get('on-success', '')
self.on_failure = dct.get('on-failure', '')
self.on_check_failure = dict(dct.get('on-check-failure', {}))
Expand All @@ -823,6 +841,7 @@ def to_dict(self) -> 'ServiceDict':
('user-id', self.user_id),
('group', self.group),
('group-id', self.group_id),
('working-dir', self.working_dir),
('on-success', self.on_success),
('on-failure', self.on_failure),
('on-check-failure', self.on_check_failure),
Expand Down Expand Up @@ -2136,6 +2155,7 @@ def exec( # noqa
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2157,6 +2177,7 @@ def exec( # noqa
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand All @@ -2176,6 +2197,7 @@ def exec(
self,
command: List[str],
*,
service_context: Optional[str] = None,
environment: Optional[Dict[str, str]] = None,
working_dir: Optional[str] = None,
timeout: Optional[float] = None,
Expand Down Expand Up @@ -2260,6 +2282,11 @@ def exec(
Args:
command: Command to execute: the first item is the name (or path)
of the executable, the rest of the items are the arguments.
service_context: If specified, run the command in the context of
this service. Specifically, inherit its environment variables,
user/group settings, and working directory. The other exec
options will override the service context; ``environment``
will be merged on top of the service's.
environment: Environment variables to pass to the process.
working_dir: Working directory to run the command in. If not set,
Pebble uses the target user's $HOME directory (and if the user
Expand Down Expand Up @@ -2325,6 +2352,7 @@ def exec(

body = {
'command': command,
'service-context': service_context,
'environment': environment or {},
'working-dir': working_dir,
'timeout': _format_timeout(timeout) if timeout is not None else None,
Expand Down
2 changes: 1 addition & 1 deletion requirements-dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ flake8-builtins==2.1.0
pyproject-flake8==4.0.1
pep8-naming==0.13.2
pytest==7.2.1
pyright==1.1.313
pyright==1.1.316
pytest-operator==0.23.0
coverage[toml]==7.0.5
typing_extensions==4.2.0
Expand Down
2 changes: 2 additions & 0 deletions test/pebble_cli.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ def main():
p.add_argument('name', help='check name(s) to filter on', nargs='*')

p = subparsers.add_parser('exec', help='execute a command')
p.add_argument('--context', help='service context')
p.add_argument('--env', help='environment variables to set', action='append',
metavar='KEY=VALUE')
p.add_argument('--working-dir', help='working directory to run command in')
Expand Down Expand Up @@ -196,6 +197,7 @@ def main():

process = client.exec(
args.exec_command,
service_context=args.context,
environment=environment,
working_dir=args.working_dir,
timeout=args.timeout,
Expand Down
11 changes: 11 additions & 0 deletions test/test_jujuversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,17 @@ def test_supports_open_port_on_k8s(self):
self.assertFalse(ops.JujuVersion('3.0.2').supports_open_port_on_k8s)
self.assertFalse(ops.JujuVersion('2.9.30').supports_open_port_on_k8s)

def test_supports_exec_service_context(self):
self.assertFalse(ops.JujuVersion('2.9.30').supports_exec_service_context)
self.assertTrue(ops.JujuVersion('4.0.0').supports_exec_service_context)
self.assertFalse(ops.JujuVersion('3.0.0').supports_exec_service_context)
self.assertFalse(ops.JujuVersion('3.1.5').supports_exec_service_context)
self.assertTrue(ops.JujuVersion('3.1.6').supports_exec_service_context)
self.assertFalse(ops.JujuVersion('3.2.1').supports_exec_service_context)
self.assertTrue(ops.JujuVersion('3.2.2').supports_exec_service_context)
self.assertTrue(ops.JujuVersion('3.3.0').supports_exec_service_context)
self.assertTrue(ops.JujuVersion('3.4.0').supports_exec_service_context)

def test_parsing_errors(self):
invalid_versions = [
"xyz",
Expand Down
9 changes: 9 additions & 0 deletions test/test_model.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
from collections import OrderedDict
from test.test_helpers import fake_script, fake_script_calls
from textwrap import dedent
from unittest.mock import patch

import pytest

Expand Down Expand Up @@ -1777,10 +1778,12 @@ def raise_error():
self.assertEqual(len(cm.output), 1)
self.assertRegex(cm.output[0], r'WARNING:ops.model:.*: api error!')

@patch('model.JujuVersion.from_environ', new=lambda: ops.model.JujuVersion('3.1.6'))
def test_exec(self):
self.pebble.responses.append('fake_exec_process')
p = self.container.exec(
['echo', 'foo'],
service_context='srv1',
environment={'K1': 'V1', 'K2': 'V2'},
working_dir='WD',
timeout=10.5,
Expand All @@ -1796,6 +1799,7 @@ def test_exec(self):
)
self.assertEqual(self.pebble.requests, [
('exec', ['echo', 'foo'], dict(
service_context='srv1',
environment={'K1': 'V1', 'K2': 'V2'},
working_dir='WD',
timeout=10.5,
Expand All @@ -1812,6 +1816,11 @@ def test_exec(self):
])
self.assertEqual(p, 'fake_exec_process')

@patch('model.JujuVersion.from_environ', new=lambda: ops.model.JujuVersion('3.1.5'))
def test_exec_service_context_not_supported(self):
with self.assertRaises(RuntimeError):
self.container.exec(['foo'], service_context='srv1')

def test_send_signal(self):
with self.assertRaises(TypeError):
self.container.send_signal('SIGHUP')
Expand Down
6 changes: 5 additions & 1 deletion test/test_pebble.py
Original file line number Diff line number Diff line change
Expand Up @@ -681,6 +681,7 @@ def _assert_empty(self, service, name):
self.assertIs(service.user_id, None)
self.assertEqual(service.group, '')
self.assertIs(service.group_id, None)
self.assertEqual(service.working_dir, '')
self.assertEqual(service.on_success, '')
self.assertEqual(service.on_failure, '')
self.assertEqual(service.on_check_failure, {})
Expand Down Expand Up @@ -711,6 +712,7 @@ def test_dict(self):
'user-id': 1000,
'group': 'staff',
'group-id': 2000,
'working-dir': '/working/dir',
'on-success': 'restart',
'on-failure': 'ignore',
'on-check-failure': {'chk1': 'halt'},
Expand All @@ -732,6 +734,7 @@ def test_dict(self):
self.assertEqual(s.user_id, 1000)
self.assertEqual(s.group, 'staff')
self.assertEqual(s.group_id, 2000)
self.assertEqual(s.working_dir, '/working/dir')
self.assertEqual(s.on_success, 'restart')
self.assertEqual(s.on_failure, 'ignore')
self.assertEqual(s.on_check_failure, {'chk1': 'halt'})
Expand Down Expand Up @@ -2639,10 +2642,11 @@ def add_responses(self, change_id, exit_code, change_err=None):
return (stdio, stderr, control)

def build_exec_data(
self, command, environment=None, working_dir=None, timeout=None,
self, command, service_context=None, environment=None, working_dir=None, timeout=None,
user_id=None, user=None, group_id=None, group=None, combine_stderr=False):
return {
'command': command,
'service-context': service_context,
'environment': environment or {},
'working-dir': working_dir,
'timeout': f'{timeout:.3f}s' if timeout is not None else None,
Expand Down