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 backup/restore plans #339

Merged
merged 121 commits into from
Jun 28, 2024
Merged

Add backup/restore plans #339

merged 121 commits into from
Jun 28, 2024

Conversation

timidri
Copy link
Contributor

@timidri timidri commented Feb 17, 2023

Summary

Adding backup/restore functionality

Checklist

  • 🟢 Spec tests.
  • 🟢 Acceptance tests.

Changes include test coverage?

  • Yes
  • Not needed

Have you updated the documentation?

  • Yes, I've updated the appropriate docs
  • Not needed

@davidsandilands
Copy link
Contributor

@cdenneen
Copy link
Contributor

Would like to leverage backup/restore for few pieces:

  • Have existing 2019.8.12 stack (ie. pe-2019.8.12-(primary,replica).us-east-1.domain.com)
  • Backup and restore, increment serial, upgrade pe-2021.7.4-(primary,replica).us-east-1.domain.com (Assuming the host/certnames would need to change in the Node Classification.

Trying to work out the steps if that can be done as:

  • new 2021.7.4 infra with a CA copy, DB copy.
  • build new stack (2021.7.4) but as (2019.8.12), backup/restore, increment serial, perform upgrade

After flipping stacks from old -> new we can do another task to rsync the remaining client certs from old CA to new (with incremented serial this should avoid any overlap (Thanks @Sharpie)).

@@ -5,42 +5,33 @@ on:
workflow_dispatch:
inputs:
image:
description: 'GCP image for test cluster'
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is all the quoting changed? I think the change isn't related to adding the backup/restore plan. If it's required it should be done in another PR, to reduce the diff here.

Copy link
Contributor Author

@timidri timidri Nov 21, 2023

Choose a reason for hiding this comment

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

@bastelfreak The integration testing is actually part of this PR since that is needed to make backup/restore releasable.
The quoting change is an unfortunate side-effect of auto-formatting I'm afraid... We will work on synchronizing auto-formatting rules since this is annoying, thanks for catching this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

now this was merged with the quote changes. Now the style in the repo is very inconsistent and the diff in this PR is unnecessary big.

Copy link
Member

Choose a reason for hiding this comment

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

Inconsistency was inevitable from this change, and will be synchronised in a separate PR.

@ragingra ragingra marked this pull request as ready for review May 31, 2024 10:04
@ragingra ragingra requested review from a team as code owners May 31, 2024 10:04
ragingra and others added 4 commits June 3, 2024 11:43
This looks like it should work with the `pull_request` event
* Update backup_restore.md

* fix typo

* Update documentation/backup_restore.md

* Update backup_restore.md

Integrating the extra note (added to SOLARCH 1160 branch) that was causing the conflict into my changes.

* Update backup_restore.md

* [ITHELP-87329] Update test-backup-restore.yaml (#446)

This looks like it should work with the `pull_request` event

---------

Co-authored-by: Ben Ford <[email protected]>
@ragingra ragingra merged commit 9cb918e into main Jun 28, 2024
48 checks passed
@ragingra ragingra deleted the SOLARCH-1160 branch June 28, 2024 08:45
NGROK_AUTH_TOKEN: ${{ secrets.NGROK_AUTH_TOKEN }}
SSH_PASS: ${{ secrets.SSH_PASS }}

# - name: Download artifacts
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was that committed if it isn't used?

Copy link
Member

Choose a reason for hiding this comment

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

This could have been moved to another branch until migration was picked up, but wasn't doing any harm for the time being.

@@ -35,7 +35,7 @@ group :development do
gem "rubocop-performance", '= 1.16.0', require: false
gem "rubocop-rspec", '= 2.19.0', require: false
gem "rb-readline", '= 0.5.5', require: false, platforms: [:mswin, :mingw, :x64_mingw]
gem "bolt", '>= 3.10.0', require: false
gem "bolt", '>= 3.27.2', require: false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is bolt updated? Do old versions not work anymore with PEADM? That's not documented in the backup_restore.md nor the README.md

Copy link
Member

Choose a reason for hiding this comment

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

We felt that it was good to increment the version to keep it within the last couple of years. Not that is wont work anymore, but just difficult for us to sign off support for every version.

@@ -0,0 +1,9 @@
# This Puppetfile is managed by Bolt. Do not edit.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was The Puppetfile committed?

Copy link
Member

Choose a reason for hiding this comment

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

As I'd mentioned elsewhere, this was for the intention of improving instructions for installation and usage.

@@ -0,0 +1,9 @@
---
Copy link
Collaborator

Choose a reason for hiding this comment

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

why was this file added?

Copy link
Member

Choose a reason for hiding this comment

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

Similar to the Puppetfile

@@ -0,0 +1,12 @@
function peadm::migration_opts_default () {
Copy link
Collaborator

Choose a reason for hiding this comment

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

none of the new functions are documented :(

Copy link
Member

Choose a reason for hiding this comment

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

I think these are relatively simple functions for just holding an object, and didn't feel they needed documented.

Peadm::Recovery_opts $backup = {},

# Where to put the backup folder
String $output_directory = '/tmp',
Copy link
Collaborator

@bastelfreak bastelfreak Jul 1, 2024

Choose a reason for hiding this comment

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

are relative paths okay, or should this be Stdlib::Absolutepath? WHen relative paths are okay, this should at least be String[1].

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good idea, and I'll change to this


## shutdown services
run_command(@("CMD"/L), $primary_target)
systemctl stop pe-console-services pe-nginx pxp-agent pe-puppetserver \
Copy link
Collaborator

@bastelfreak bastelfreak Jul 1, 2024

Choose a reason for hiding this comment

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

Is this supposed to stop all services? Because PE provides more. The dead Jira also had a feature request for PE to provide a pe.target that would allow us to reliable stop all services.

Copy link
Member

Choose a reason for hiding this comment

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

This is stopping all the services that we require for the time being.

@@ -0,0 +1,91 @@
plan peadm::util::init_db_server(
Copy link
Collaborator

Choose a reason for hiding this comment

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

puppet-strings documentation is missing :(

Copy link
Member

Choose a reason for hiding this comment

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

I think this should maybe be private. I will update documentation for this.


# Install PE. We need to pass "y" through stdin since the flag -y requires pe.conf to be present.
cd $INSTALLER
echo 'y' | ./puppet-enterprise-installer
Copy link
Collaborator

Choose a reason for hiding this comment

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

trailing newline is missing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants