Skip to content

Add templating, via jinja2, to cephadm#52492

Merged
adk3798 merged 15 commits intoceph:mainfrom
phlogistonjohn:jjm-cephadm-jinja
Nov 6, 2023
Merged

Add templating, via jinja2, to cephadm#52492
adk3798 merged 15 commits intoceph:mainfrom
phlogistonjohn:jjm-cephadm-jinja

Conversation

@phlogistonjohn
Copy link
Contributor

@phlogistonjohn phlogistonjohn commented Jul 17, 2023

Contribution Guidelines

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)
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

@phlogistonjohn
Copy link
Contributor Author

Open question: If we are going to add external deps to cephadm and will do so via pip, should we also include hashes in our requreiments file? Example:

jinja2==3.1.2 \
    --hash=sha256:31351a702a408a9e7595a8fc6150fc3f43bb6bf7e319770cbc0db9df9437e852
markupsafe==2.1.3 \
    --hash=sha256:af598ed32d6ae86f1b747b82783958b1a4ab8f617b06fe68795c7f026abbdcad

@github-actions
Copy link

github-actions bot commented Sep 5, 2023

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

@github-actions
Copy link

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

@github-actions
Copy link

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

Comment on lines +3238 to +3240
_TMPL_DAEMON_UNIT = """# generated by cephadm
[Unit]
Description=Ceph %i for {fsid}
Description=Ceph %i for {{fsid}}
Copy link
Contributor

@mgfritch mgfritch Oct 4, 2023

Choose a reason for hiding this comment

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

maybe now is a good time to formally move these into a .j2 (or .jinja) template file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that's doable. I was looking at the jinj2 docs (and source) recently and the package loader appears to support zipimport (which IIUC is the basis for zipapp).

What I'll do is try this out in a an experimental branch and if all goes smoothly I'll fold that branch back into this one. If things don't work out I'll report my findings here.

@github-actions
Copy link

github-actions bot commented Oct 5, 2023

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

@phlogistonjohn phlogistonjohn force-pushed the jjm-cephadm-jinja branch 2 times, most recently from 797de3e to fd86aa1 Compare October 18, 2023 18:20
@phlogistonjohn phlogistonjohn changed the title [WIP] Add templating, via jinja2, to cephadm Add templating, via jinja2, to cephadm Oct 24, 2023
@phlogistonjohn
Copy link
Contributor Author

jenkins test dashboard cephadm

Copy link
Contributor

@adk3798 adk3798 left a comment

Choose a reason for hiding this comment

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

Builds are now all working. But seeing some test failures.

@github-actions
Copy link

github-actions bot commented Nov 3, 2023

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

We can not rely on any particular python version (py 3.6+ is supported)
and can not assume any particular architecture. So using wheels
based on the build system is pointless. Installing binary .so files
compiled from C/C++ similarly so. Attempt to block the behaviors
when adding dependencies to the zipapp.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Unfortunately, a single simple call to pip does not work on all the
distributions that ceph is built on. In particular, Ubuntu 20.04 and
Ubuntu 22.04 come with pip versions that can not correctly handle
disabling wheels and installing Jinja2 (it tries to use the markupsafe
dependency before it is installed). This can be worked around by using a
virtual env and updating pip before proceeding. However, this is not
enough because CentOS/RHEL 8 uses python 3.6 and there is no version of
pip that supports 3.6 that we can update to that is new enough to  fix
the issue with disabling wheels. The workaround in this case is to
install each dependency one at a time through multiple calls to pip.

Because of this extra complexity is it simpler to eschew the use of a
requirements.txt file in build.py entirely. Thus the zipapp is built
using build.py only. Requirements files for cephadm are for setting up
the tox environments *only*.

For completeness a new option is added that gives the caller control
over when build.py uses a virtualenv or not. Thus the build.py script
requires at least one of: a working pip that handles disabling wheels;
or, a virtualenv (venv) and the ability to update to a working version
of pip. If the list of distros ceph supports (and the python versions
they use) ever becomes simpler/newer some of this complexity could be
removed from the build.py script.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Add tests that cover the four main distros that ceph is built on (in
the ceph infra). These tests should not be run by automation as they
are slow and have special requirements like a working podman.
Instead, these are provided to be run by a dev when build.py is updated.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Asserts more about the unit files.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Add tests that check the generation of the standard systemd unit
for cephadm services. This test ensures that non trivial changes
to the content of these files are noticed.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Add Jinja2 and MarkupSafe dependencies to a requirements.txt style file.
This file tracks the dependencies needed to run the cephadm libs
in the unit test framework. The actual dependencies that get added
to the ziapp are managed by build.py but mirrored here.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Add `-rzipapp-reqs.txt` to the unit tests and mypy environments in
tox.ini, enabling the use of dependencies outside the stdlib.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Add a simple wrapper function for templating from a string to a string.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
The somewhat complex string assembly of the main systemd unit file
for cephadm services can benefit from using a standard templating
approach.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Signed-off-by: John Mulligan <jmulligan@redhat.com>
Expand the templating module so that templates can be source from the
python package. Add (more) convenient to use methods.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Copy the current template strings into files under the `templates` dir.
Add a enum for holding the names of known template files.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
Switch off of the embedded template strings to using the recently
added template files.

Signed-off-by: John Mulligan <jmulligan@redhat.com>
@adk3798
Copy link
Contributor

adk3798 commented Nov 6, 2023

https://pulpito.ceph.com/phlogistonjohn-2023-11-02_14:19:20-orch:cephadm-wip-phlogistonjohn-testing-2023-11-01-2015-distro-default-smithi/

reruns of failed jobs: https://pulpito.ceph.com/adking-2023-11-05_15:58:41-orch:cephadm-wip-phlogistonjohn-testing-2023-11-01-2015-distro-default-smithi/

After reruns, 1 failure:

  • mgr-nfs-upgrade test, known failure for a while now, typically due to https://tracker.ceph.com/issues/62958 although this time it seems to have timed out instead of failing immediately

Copy link
Contributor

Choose a reason for hiding this comment

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

I would imagine at some point a user might want to override this file similar to the service templates in the mgr

@adk3798 adk3798 merged commit a9d1c62 into ceph:main Nov 6, 2023
@phlogistonjohn phlogistonjohn deleted the jjm-cephadm-jinja branch November 8, 2023 00:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants