cephadm: add basic init containers support #52178

phlogistonjohn · 2023-06-23T19:38:44Z

Depends on #51863

Fixes: https://tracker.ceph.com/issues/62170

Add init_containers support to the cephadm binary and cephadm orchestrator module via the Custom Container service type.

init_containers supports a more formalized method of executing a container that runs to completion and exits prior to a long running deaemon container. These containers can be used to assert certain preconditions or configure the environment for the later daemon container or perform other setup actions.

init containers currently support fields:

image
entrypoint
entrypoint_args
volume_mounts
envs
privileged

init containers are currently part of the script that starts daemon containers (unit.run) and is limited by the systemd services start-up timeout of 200 seconds. In later work we will be expanding cephadm's use of systemd services and plan to seperate init_containers into a separate service such that limits can be placed on the init containers (or not) independently of the service and vice versa.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

github-actions · 2023-07-07T15:59:53Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

phlogistonjohn · 2023-07-10T17:02:48Z

As discussed in recent ceph orch weekly and standup meetings, I experimented with the behavior of the systemd unit and long running init containers. I can confirm that a single init container or a cumulative running for more than the TimeoutStartSec value will cause the entire unit to time out (and then restart).

Short of major changes to the systemd unit files I think we'll need to live with this restriction for now. When I add docs to this PR I can add a note covering this fact. When I work on planned changes to add sidecar containers, which will probably require changes to systemd unit files, I think then we can revisit this and either put a longer timeout, or even possibly allow custom timeout values. This will be discussed in an upcoming orch weekly.

adk3798

Generally LGTM, as long as all the teuthology tests still pass with the refactoring done here . The actual changes for the init container support itself all seem good.

src/cephadm/cephadm.py

src/python-common/ceph/deployment/service_spec.py

adk3798 · 2023-07-10T21:07:57Z

src/pybind/mgr/cephadm/serve.py

+        ic_params = params['init_containers'] = []
+        for ic in ics:
+            ic_meta.append(ic.to_json())
+            ic_params.append(ic.to_json(flatten_args=True))
+        return ic_meta


I'm confused why we're building this ic_params here when we don't end up returning it.

It's an alias ic_params = params['init_containers'] = [] that side effects the params dict passed in. This is similar to the behavior of _setup_extra_deployment_args as I was intentionally mimicking that function. Do you think the aliasing is too confusing?

adk3798 · 2023-07-10T21:13:09Z

src/pybind/mgr/cephadm/serve.py

@@ -1297,6 +1298,7 @@ async def _create_daemon(self,
                            extra_entrypoint_args=ArgumentSpec.map_json(
                                extra_entrypoint_args,
                            ),
+                            init_containers=init_containers,


do we also want to set these up like extra_container_args and extra_entrypoint_args where if they're modified we'll redeploy the daemon automatically?

Good point thanks, I'll look into it.

We discussed this in the weekly orch meeting and agreed to add it in a follow up PR.

src/pybind/mgr/cephadm/serve.py

github-actions · 2023-07-14T06:16:13Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

github-actions · 2023-07-25T13:21:48Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Rename argument to "container" from "c" because a one character variable at a large function's scope (or anywhere outside of a comprehension) is a understanding/readability issue (IMO). Signed-off-by: John Mulligan <jmulligan@redhat.com>

Refactor the _write_container_cmd_to_bash such that it now uses a new _bash_cmd helper function and tries to avoid non-DRY repetition of the same pattern of shell command expression. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Break up deploy_daemon_units which had a lot of various "sub-tasks" implemented within the main body of the function. Splitting up the function makes each function's scope smaller, more readily testable and more readable. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add a BasicContainer class that will act as a base class for CephContainer and any future XyzContainer classes. The class is currently a stub that provides no functionality. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Move a bunch of logic out of CephContainer and into BasicContainer. BasicContainer needs to be more general than CephContainer so it adds a few parameters that don't exist in CephContainer and CephContainer will provide defaults for those values. Signed-off-by: John Mulligan <jmulligan@redhat.com>

It's an extremely common pattern in cephadm to need a triple of (fsid, daemon_type, daemon_id). These triples are used to generate various names and paths etc. Add a new type, DaemonIdentity, that can represent these values in a more convient package. It has an additional optional field "subcomponent" that can represent a part of a daemon/service. It is expected to be used in an immutable manner like a namedtuple, but I didn't end using a namedtuple because cephadm doesn't make much use of them and the syntax to combine namedtuple and type hints is awkward. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Use the new DaemonIdentity class to help identify a container. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add an optional identity field based on the new DaemonIdentity type to help name/identify the CephContainers. I would have preferred to make this field mandatory but there are so many places in the code that call CephContainer now, I didn't want to have to touch them all at once. CephContainer objects created using the for_daemon classmethod *will* all have DaemonIdentity set. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add a new InitContainer class that is similar to CephContainer but will not assume certain defaults and is expected to run for a "short" period before exiting. These init containers will be used to preform tasks before a long running container is started. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add functions that write commands to clean up and execute init containers. These init containers will be executed before the main container as part of unit.run files (for now). Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add get_deployment_init_containers to extract any init containers from the deploy configuration context. Currently, all init containers are to be explicitly defined, not inferred from the service type. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Complete the deployment functions so that any custom containers defined with "init_containers" will add said init containers to the commands to be executed. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add a test case that covers the act of setting init_containers in the deploy config on a custom container instance. This test executes custom containers that both specify custom volume_mounts and not, which are expected to "inherit" the volume_mounts of the primary container's config. Signed-off-by: John Mulligan <jmulligan@redhat.com>

The InitContainerSpec type will be used to define explicit "init containers" - containers that are expected to run and then exit and are executed prior to running a primary container. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Use the "black" style of multi-line import with parens. Signed-off-by: John Mulligan <jmulligan@redhat.com>

This allows custom containers to run init containers before the primary container starts. Signed-off-by: John Mulligan <jmulligan@redhat.com>

This will permit serializing init_container information for sending to cephadm binary. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Signed-off-by: John Mulligan <jmulligan@redhat.com>

This will permit the transfer of init_containers from a spec to the structure that actually generates the deploy JSON. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Use the parenthesized format with each import on a line for readability. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Signed-off-by: John Mulligan <jmulligan@redhat.com>

…ctrs This adds a test case to explicitly check custom containers with init containers. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add documentation covering the new init_containers parameter and the parameters within each init container's configuration. Signed-off-by: John Mulligan <jmulligan@redhat.com>

Add a 2nd custom container yaml to the test_extra_daemon_features.yaml file that uses two init containers. Add verification that the init containers (and primary) ran successfully. Signed-off-by: John Mulligan <jmulligan@redhat.com>

adk3798 · 2023-08-09T19:28:22Z

https://pulpito.ceph.com/adking-2023-08-09_14:31:36-orch:cephadm-wip-adk4-testing-2023-08-08-1040-distro-default-smithi/

3 failures, 4 jobs that will time out (still "running" as of me writing this

1 failure is deploying jaeger-tracing, known issue - https://tracker.ceph.com/issues/59704
2 failures were test_nfs task, known failure - https://tracker.ceph.com/issues/62084
2 timed out jobs were mds_upgrade_sequence, known issue (not tracker currently, will see if bumping the starting version fixes this)
2 timed out jobs were nfs-ingress tests on rhel, known issue (no tracker currently, likely related to the ganesha build issues that occurred recently)

Overall, no failures that wouldn't be expected on a main branch run so nothing to block merging

github-actions bot added ceph-volume cephadm orchestrator pybind labels Jun 23, 2023

phlogistonjohn force-pushed the jjm-cephadm-init-ctrs branch from 98afaf2 to 8f48ab8 Compare June 26, 2023 14:20

github-actions bot added the needs-rebase label Jul 7, 2023

phlogistonjohn force-pushed the jjm-cephadm-init-ctrs branch from 8f48ab8 to e3df865 Compare July 10, 2023 14:46

github-actions bot removed the needs-rebase label Jul 10, 2023

adk3798 reviewed Jul 10, 2023

View reviewed changes

phlogistonjohn force-pushed the jjm-cephadm-init-ctrs branch from e3df865 to 752e02d Compare July 11, 2023 13:53

github-actions bot added documentation needs-rebase labels Jul 11, 2023

phlogistonjohn force-pushed the jjm-cephadm-init-ctrs branch from 752e02d to f91429d Compare July 22, 2023 19:21

github-actions bot removed the needs-rebase label Jul 22, 2023

phlogistonjohn force-pushed the jjm-cephadm-init-ctrs branch 2 times, most recently from a5e3d13 to 301f5df Compare July 24, 2023 15:44

github-actions bot added the needs-rebase label Jul 25, 2023

phlogistonjohn force-pushed the jjm-cephadm-init-ctrs branch from 301f5df to c263144 Compare July 25, 2023 17:18

github-actions bot removed the needs-rebase label Jul 25, 2023

phlogistonjohn changed the title ~~[RFC] cephadm: add basic init containers support~~ cephadm: add basic init containers support Jul 25, 2023

phlogistonjohn marked this pull request as ready for review July 25, 2023 17:28

phlogistonjohn requested review from a team as code owners July 25, 2023 17:28

phlogistonjohn requested a review from adk3798 July 25, 2023 17:28

phlogistonjohn force-pushed the jjm-cephadm-init-ctrs branch 2 times, most recently from 2849c6a to f2422f5 Compare July 27, 2023 20:38

phlogistonjohn added 25 commits August 9, 2023 13:48

cephadm: rename argument to container from c

25feb0c

Rename argument to "container" from "c" because a one character variable at a large function's scope (or anywhere outside of a comprehension) is a understanding/readability issue (IMO). Signed-off-by: John Mulligan <jmulligan@redhat.com>

cephadm: add BasicContainer stub class

02796a4

Add a BasicContainer class that will act as a base class for CephContainer and any future XyzContainer classes. The class is currently a stub that provides no functionality. Signed-off-by: John Mulligan <jmulligan@redhat.com>

cephadm: add an identity field to BasicContainer

8d0d094

Use the new DaemonIdentity class to help identify a container. Signed-off-by: John Mulligan <jmulligan@redhat.com>

cephadm: add function to write init container commands

0a1310e

Add functions that write commands to clean up and execute init containers. These init containers will be executed before the main container as part of unit.run files (for now). Signed-off-by: John Mulligan <jmulligan@redhat.com>

cephadm: support deploying with explicit init containers

079dccc

Complete the deployment functions so that any custom containers defined with "init_containers" will add said init containers to the commands to be executed. Signed-off-by: John Mulligan <jmulligan@redhat.com>

cephadm: use an f-string for readability

4480a20

Signed-off-by: John Mulligan <jmulligan@redhat.com>

python-common: add InitContainerSpec type

857a6fe

The InitContainerSpec type will be used to define explicit "init containers" - containers that are expected to run and then exit and are executed prior to running a primary container. Signed-off-by: John Mulligan <jmulligan@redhat.com>

python-common: reformat service_spec import line

1e0f45f

Use the "black" style of multi-line import with parens. Signed-off-by: John Mulligan <jmulligan@redhat.com>

python-common: add init_containers field to CustomContainerSpec

89bcfe0

This allows custom containers to run init containers before the primary container starts. Signed-off-by: John Mulligan <jmulligan@redhat.com>

pybind/mgr/cephadm: add init_containers field to exchange module

9c6047f

This will permit serializing init_container information for sending to cephadm binary. Signed-off-by: John Mulligan <jmulligan@redhat.com>

pybind/mgr/cephadm: add support for init_containers in deploy

51881d5

Signed-off-by: John Mulligan <jmulligan@redhat.com>

pybind/mgr/cephadm: add init_containers field to CephadmDaemonDeploySpec

8a547ab

This will permit the transfer of init_containers from a spec to the structure that actually generates the deploy JSON. Signed-off-by: John Mulligan <jmulligan@redhat.com>

pybind/mgr/cephadm: reformat a long input line

6247438

Use the parenthesized format with each import on a line for readability. Signed-off-by: John Mulligan <jmulligan@redhat.com>

pybind/mgr/cephadm: add test_deploy_custom_container test case

493a96c

Signed-off-by: John Mulligan <jmulligan@redhat.com>

pybind/mgr/cephadm/tests: add test_deploy_custom_container_with_init_…

5a024b0

…ctrs This adds a test case to explicitly check custom containers with init containers. Signed-off-by: John Mulligan <jmulligan@redhat.com>

doc/cephadm: document init_containers params for custom containers

ec269d4

Add documentation covering the new init_containers parameter and the parameters within each init container's configuration. Signed-off-by: John Mulligan <jmulligan@redhat.com>

phlogistonjohn force-pushed the jjm-cephadm-init-ctrs branch from 34e797b to 921f474 Compare August 9, 2023 17:48

adk3798 approved these changes Aug 9, 2023

View reviewed changes

adk3798 merged commit 142da14 into ceph:main Aug 10, 2023
10 of 12 checks passed

phlogistonjohn deleted the jjm-cephadm-init-ctrs branch August 10, 2023 13:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cephadm: add basic init containers support #52178

cephadm: add basic init containers support #52178

phlogistonjohn commented Jun 23, 2023 •

edited

github-actions bot commented Jul 7, 2023

phlogistonjohn commented Jul 10, 2023 •

edited

adk3798 left a comment

adk3798 Jul 10, 2023

phlogistonjohn Jul 11, 2023

adk3798 Jul 10, 2023

phlogistonjohn Jul 11, 2023

phlogistonjohn Jul 25, 2023

github-actions bot commented Jul 14, 2023

github-actions bot commented Jul 25, 2023

adk3798 commented Aug 9, 2023

cephadm: add basic init containers support #52178

cephadm: add basic init containers support #52178

Conversation

phlogistonjohn commented Jun 23, 2023 • edited

Contribution Guidelines

Checklist

github-actions bot commented Jul 7, 2023

phlogistonjohn commented Jul 10, 2023 • edited

adk3798 left a comment

Choose a reason for hiding this comment

adk3798 Jul 10, 2023

Choose a reason for hiding this comment

phlogistonjohn Jul 11, 2023

Choose a reason for hiding this comment

adk3798 Jul 10, 2023

Choose a reason for hiding this comment

phlogistonjohn Jul 11, 2023

Choose a reason for hiding this comment

phlogistonjohn Jul 25, 2023

Choose a reason for hiding this comment

github-actions bot commented Jul 14, 2023

github-actions bot commented Jul 25, 2023

adk3798 commented Aug 9, 2023

phlogistonjohn commented Jun 23, 2023 •

edited

phlogistonjohn commented Jul 10, 2023 •

edited