Skip to content

Add tests for backup#588

Open
sjha4 wants to merge 3 commits into
theforeman:masterfrom
sjha4:backup_tests
Open

Add tests for backup#588
sjha4 wants to merge 3 commits into
theforeman:masterfrom
sjha4:backup_tests

Conversation

@sjha4

@sjha4 sjha4 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Why are you introducing these changes? (Problem description, related links)

Add tests for foremanctl backup

What are the changes introduced in this pull request?

  • Add tests for foremanctl backup

How to test this pull request

The CI should run the new tests.
Steps to reproduce:

  • Check CI

Checklist

  • Tests added/updated (if applicable)
  • Documentation updated (if applicable)

PS: I had some issues with the OBSAH_STATE_PATH running these locally in a repo checkout. The backup is supposed to work on a host with services deployed and with path as /var/lib/foremanctl. The repo setup is different from production-like installs where the quadlet is mapped to localhost. On dev setup, quadlet is actual VM so the test finds OBSAH_STATE_PATH as /.var/lib/foremanctl inside the VM. Hence the conditional test for foremanctl state archive which archives this path.

@sjha4 sjha4 force-pushed the backup_tests branch 5 times, most recently from 95d6d11 to 727cdd8 Compare June 23, 2026 05:55
name: postgresql.service
register: backup_postgres_status
until: backup_postgres_status.status.ActiveState == 'inactive'
until: backup_postgres_status.status.ActiveState in ['inactive', 'failed']

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Changed this to include failed as an acceptable state to continue with backup. This was happening in the CI test because postgresql.service was getting a SIGTERM and also makes sense for actual backup scenario where we may want to take a backup if state is failed.

@sjha4 sjha4 force-pushed the backup_tests branch 2 times, most recently from 359bea6 to 736e9d2 Compare June 24, 2026 20:03
Comment thread tests/backup_test.py Outdated
Comment thread tests/backup_test.py Outdated
Comment thread tests/backup_test.py
Comment thread tests/backup_test.py
BACKUP_DIR
]

result = subprocess.run(

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
result = subprocess.run(
result = server.run(

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The server.run runs it inside quadlet for quadlet based deploys like CI..We want the command to run locally hence the subprocess.run ..

Comment thread tests/backup_test.py
backup_dir = backup_result['backup_dir']

expected_iop_dumps = [
'iop_advisor.dump',

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Are these the right names?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We base these based on name :

- name: iop_advisor
, not database..Hence the names like iop_advisor.dump..

Comment thread tests/backup_test.py Outdated
Comment thread tests/backup_test.py Outdated
"""
Test that foremanctl state directory is archived.

NOTE: This test is conditional because foremanctl-state.tar.gz is only

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This test should not be conditional, that tells me there is a design flaw that should be fixed.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have updated backup to handle localhost and quadlet based deployments. Have kept it as a separate commit to isolate the change there..

Comment thread tests/backup_test.py Outdated
Comment thread tests/backup_test.py Outdated
Comment thread tests/backup_test.py Outdated
assert metadata['incremental'] is False, "Backup should not be incremental"
assert metadata['database_mode'] in ['internal', 'external'], "Database mode should be 'internal' or 'external'"

# Core databases should always be present

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will end up being wrong in the long run due to flavors.

Comment thread tests/backup_test.py Outdated
Comment thread tests/backup_test.py Outdated
Comment thread tests/backup_test.py Outdated
Comment thread tests/backup_test.py Outdated
@sjha4 sjha4 force-pushed the backup_tests branch 3 times, most recently from 574985c to 7a0eb4c Compare June 30, 2026 16:33
Comment thread src/roles/backup/tasks/main.yaml Outdated
path: "{{ obsah_state_path }}"
register: backup_state_path_stat

- name: Copy foremanctl state from controller to target (remote deployment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think you could consolidate remote vs. local by just always performing this action.

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