Skip to content

Conversation

@fabienarchambault
Copy link
Contributor

@fabienarchambault fabienarchambault commented Dec 3, 2025

Enhancement: Add Ubuntu support

Reason: Can be needed

Result: activate and enable features in kdump config for Ubuntu.

maybe this can work for Debian but this is not tested.

Issue Tracker Tickets (Jira or BZ if any): None

Summary by Sourcery

Add Ubuntu support to the kdump Ansible role while keeping existing Red Hat–style behavior intact.

Enhancements:

  • Generalize kdump service handling via a distro-specific __kdump_service variable and per-distro vars files.
  • Add Ubuntu-specific package and service configuration for kdump using a dedicated vars/Ubuntu.yml file.
  • Generate the appropriate kdump configuration file depending on the detected distribution (Red Hat–style kdump.conf vs Ubuntu /etc/default/kdump-tools).

Documentation:

  • Document the Ubuntu-specific kdump configuration file and provide an example kdump_kdump_tools configuration block in the README.

@sourcery-ai
Copy link
Contributor

sourcery-ai bot commented Dec 3, 2025

Reviewer's Guide

Adds Ubuntu-specific support to the kdump Ansible role by introducing Ubuntu vars and template handling, switching service management to use a distro-specific service name, and documenting Ubuntu configuration usage.

Flow diagram for distro-specific kdump configuration and service handling

flowchart TD
  Start[[Start kdump role]] --> LoadVars[Load vars_main_yml]
  LoadVars --> LoadUbuntuVars{Target_distribution == Ubuntu?}

  LoadUbuntuVars -- No --> SetServiceDefault[Set __kdump_service = kdump]
  LoadUbuntuVars -- Yes --> SetServiceUbuntu[Set __kdump_service = kdump-tools]

  SetServiceDefault --> EnableService[service: name = __kdump_service, enabled = true]
  SetServiceUbuntu --> EnableService

  EnableService --> BranchDistro{Distribution group}

  BranchDistro -- RedHat_CentOS_Fedora --> TemplRH[Template /etc/kdump.conf
  from kdump_conf_j2]
  BranchDistro -- Ubuntu --> TemplUbuntu[Template /etc/default/kdump-tools
  from kdump-tools_j2]

  TemplRH --> NotifyRestart[Notify handler Restart_kdump]
  TemplUbuntu --> NotifyRestart

  NotifyRestart --> StartService[service: name = __kdump_service, state = started]

  StartService --> Handler[Handler Restart_kdump
  service: name = __kdump_service, state = restarted]

  Handler --> End[[Role completed]]
Loading

File-Level Changes

Change Details Files
Make kdump service management distro-aware via a variable.
  • Replace hard-coded 'kdump' service name in main tasks with a templated variable for enabling and starting the service
  • Replace hard-coded 'kdump' service name in the restart handler with the same templated variable
  • Define a default __kdump_service value and override it in a distro-specific vars file
tasks/main.yml
handlers/main.yml
vars/main.yml
vars/Ubuntu.yml
Add Ubuntu-specific package and service configuration.
  • Introduce an Ubuntu-specific vars file declaring the required kdump-related packages
  • Set the Ubuntu kdump service name to 'kdump-tools' in the Ubuntu vars file
vars/Ubuntu.yml
Add Ubuntu-specific kdump configuration templating.
  • Split kdump configuration generation so that RedHat-like systems still manage /etc/kdump.conf while Ubuntu manages /etc/default/kdump-tools
  • Add a new kdump-tools.j2 template used to generate /etc/default/kdump-tools on Ubuntu
tasks/main.yml
templates/kdump-tools.j2
Update documentation to describe Ubuntu configuration and config file paths.
  • Document that RedHat-like systems use /etc/kdump.conf and Ubuntu uses /etc/default/kdump-tools
  • Add an example Ubuntu kdump_kdump_tools configuration block and usage notes
README.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link
Contributor

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

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

Hey there - I've reviewed your changes - here's some feedback:

  • In templates/kdump-tools.j2, the variable is referenced as kdump_kdump-tools, which is not a valid Ansible variable name and does not match the README example (kdump_kdump_tools); this will fail at runtime and should be corrected.
  • The when conditions use ansible_facts['distribution'] while other parts of the role rely on ansible_distribution/__kdump_is_rh_distro; consider using a single, consistent fact/variable for distribution checks to avoid subtle mismatches.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `templates/kdump-tools.j2`, the variable is referenced as `kdump_kdump-tools`, which is not a valid Ansible variable name and does not match the README example (`kdump_kdump_tools`); this will fail at runtime and should be corrected.
- The `when` conditions use `ansible_facts['distribution']` while other parts of the role rely on `ansible_distribution`/`__kdump_is_rh_distro`; consider using a single, consistent fact/variable for distribution checks to avoid subtle mismatches.

## Individual Comments

### Comment 1
<location> `templates/kdump-tools.j2:3` </location>
<code_context>
+# {{ ansible_managed }}
+
+{% for key, value in kdump_kdump-tools.items() %}
+{{ key }} = {{ value }}
+{% endfor %}
</code_context>

<issue_to_address>
**issue (bug_risk):** The template references a variable name containing a hyphen, which is invalid in Jinja/Ansible and will likely fail at render time.

In Jinja/Ansible, `kdump_kdump-tools` is parsed as `kdump_kdump` minus `tools`, so this will fail at render time. If the intended var is `kdump_kdump_tools`, please rename it consistently in vars and template. If you truly need a key with a dash, access it via `vars` or bracket notation instead, e.g. `{% for key, value in vars['kdump_kdump-tools'].items() %}`.
</issue_to_address>

### Comment 2
<location> `tasks/main.yml:34-43` </location>
<code_context>
     backup: true
     mode: "{{ __kdump_conf.stat.mode | d('0644') }}"
   notify: Restart kdump
+  when: ansible_facts['distribution'] in ['RedHat', 'CentOS', 'Fedora']
+
+- name: Generate /etc/default/kdump-tools
+  template:
+    src: kdump-tools.j2
+    dest: /etc/default/kdump-tools
+    backup: true
+    mode: "{{ __kdump_conf.stat.mode | d('0644') }}"
+  notify: Restart kdump
+  when: ansible_facts['distribution'] in ['Ubuntu']

 - name: Find out reserved memory for the crash kernel
</code_context>

<issue_to_address>
**suggestion:** The distro checks here are inconsistent with the derived distro flags already defined in vars, which could make maintenance harder.

Since `vars/main.yml` already exposes booleans like `__kdump_is_rh_distro_fedora`, it would be better to reuse those here instead of checking `ansible_facts['distribution']` directly. That way all distro-specific logic stays in one place and won’t diverge if we add or rename supported distros. For example, use `when: __kdump_is_rh_distro_fedora` for `/etc/kdump.conf` and a corresponding Ubuntu flag for the kdump-tools task.

Suggested implementation:

```
  notify: Restart kdump
  when: __kdump_is_rh_distro_fedora

```

```
  notify: Restart kdump
  when: __kdump_is_ubuntu_distro

```

You’ll need to ensure that the Ubuntu boolean used in the second replacement (`__kdump_is_ubuntu_distro` in this example) matches the actual variable defined in `vars/main.yml`. If the existing Ubuntu flag has a different name (e.g. `__kdump_is_ubuntu`, `__kdump_is_debian_ubuntu`, etc.), update the `when:` condition to use that exact variable instead. No other changes in `tasks/main.yml` should be required as long as those vars are already set correctly in `vars/main.yml`.
</issue_to_address>

### Comment 3
<location> `README.md:15` </location>
<code_context>
 following configuration file:

-* `/etc/kdump.conf`
+* `/etc/kdump.conf` (RedHat like OS)
+* `/etc/default/kdump-tools` (Ubuntu only yet)

</code_context>

<issue_to_address>
**suggestion (typo):** Consider correcting the spelling and phrasing of "RedHat like OS".

Please use the correct product name "Red Hat" and a clearer phrase such as "Red Hat–like OS" or "Red Hat–like operating systems."

```suggestion
* `/etc/kdump.conf` (Red Hat–like operating systems)
```
</issue_to_address>

### Comment 4
<location> `README.md:16` </location>
<code_context>

-* `/etc/kdump.conf`
+* `/etc/kdump.conf` (RedHat like OS)
+* `/etc/default/kdump-tools` (Ubuntu only yet)

 ## Requirements
</code_context>

<issue_to_address>
**issue (typo):** The phrase "Ubuntu only yet" is grammatically awkward.

Consider changing this to either "Ubuntu only" or, if you want to imply it may change later, "Ubuntu only, for now" for more natural phrasing.

```suggestion
* `/etc/default/kdump-tools` (Ubuntu only, for now)
```
</issue_to_address>

### Comment 5
<location> `README.md:101-103` </location>
<code_context>

+## Ubuntu
+
+To configure the file over-writing default, use for example:
+```yaml
+kdump_kdump_tools:
</code_context>

<issue_to_address>
**suggestion (typo):** The wording "file over-writing default" is unclear and "over-writing" should likely be "overwriting".

Consider rephrasing this for clarity, e.g. "To configure the default file-overwriting behavior, use for example:" or "To configure the default that overwrites the file, use for example:". Also, change "over-writing" to "overwriting".

```suggestion
## Ubuntu

To configure the default file-overwriting behavior, use for example:
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

fabienarchambault and others added 3 commits December 3, 2025 18:26
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
@fabienarchambault
Copy link
Contributor Author

Thanks for the initial review. I have updated the code with your comments.

@richm richm changed the title Ubuntu support feat: Ubuntu support Dec 4, 2025
Co-authored-by: Richard Megginson <[email protected]>
@richm
Copy link
Contributor

richm commented Dec 4, 2025

[citest]

@richm
Copy link
Contributor

richm commented Dec 4, 2025

@fabienarchambault thank you for the contribution!

@richm richm merged commit 691b0fa into linux-system-roles:main Dec 4, 2025
25 of 27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants