Conversation
Contributor
Author
|
This PR plans to fix this issue [Question] Hard-Coding --host=127.0.01 in mysql_source.py · Issue #184 · twindb/backup If you would rather split these changes into two separate PRs, please let me know. |
Contributor
Author
|
I would also like to discuss adding support for building on enterprise linux (in general) 7, 8 and 9. I created these branches in a personal repo that I have been building our images with. These docker images use Oracle Linux as a base, it's frequently updated and JustWorks. We could possibly create another repo in twindb, or look at merging with twindb/omnibus-centos |
Contributor
Author
|
@akuzminsky - Could I please get a review/approve for this PR? |
ernstae
approved these changes
Nov 22, 2024
ernstae
left a comment
ernstae
left a comment
Contributor
There was a problem hiding this comment.
I've reviewed this code internally, and we are running this successfully against Azure Blob storage. Looking to get this merged in, and a further story on RPM and other package builds. We should consider whether there's a path forward for Oracle Linux, Rocky Linux or Alma Linux build processes to get new RPMs out into the world (or at least in the Releases artifacts of this repo).
Cheers!
Contributor
Author
|
@akuzminsky - bumping this thread once more.. We have been running this version in production for almost a year now without any issues. |
Introduces auth_mode=managed_identity for the Azure Blob destination so
the backup process can obtain an access token from the VM's managed
identity instead of using connection strings or SAS tokens.
- Accepts new [az] options:
* auth_mode: connection_string | managed_identity
(default: connection_string; preserves existing behavior)
* account_url: required when auth_mode=managed_identity
* managed_identity_client_id: optional, for a user-assigned identity
* managed_identity_resource_id: optional, for a user-assigned identity
selected by its ARM resource ID
- ManagedIdentityCredential is used when client_id or resource_id is
set; DefaultAzureCredential is used otherwise (system-assigned MI or
local development via az login / environment credentials).
- client_id and resource_id are mutually exclusive and validated at
config load time.
- Adds unit coverage for the new field, the mutual-exclusivity rule,
and the three credential code paths (resource_id, client_id, neither).
- Updates docs/usage.rst and the example twindb-backup.cfg files.
- Adds docs/azure_worm_compatibility.rst capturing the follow-on work
required for Azure immutable/WORM containers, which is out of scope
for this change.
Covers the new Azure managed-identity auth_mode and the managed_identity_resource_id field added in the preceding commit.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Documentation | 6 minor |
| ErrorProne | 1 high |
| Security | 1 medium 92 high |
🟢 Metrics 447 complexity · 16 duplication
Metric Results Complexity 447 Duplication 16
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Introduces a new ``[source] server_name`` config option that overrides the per-source segment of the remote backup path (previously hardcoded to ``socket.gethostname()``). When multiple replicas of a cluster back up to the same destination, pointing every replica at the same ``server_name`` collapses the backups into a single shared tree instead of one fan-out per hostname. On Azure Blob destinations, ``BaseDestination.cluster_lock`` is also overridden to acquire an Azure blob lease before running the backup. Only one replica holding the lease proceeds; the rest skip the run cleanly. A background thread renews the lease for the duration of the backup so long-running xtrabackup streams stay protected within the Azure 60s lease maximum. On other destinations ``cluster_lock`` is a no-op and always reports the lock as acquired, preserving existing behaviour. The ``server_name`` value also drives the status file directory and the ``--hostname`` defaults in the ``status`` and ``verify mysql`` CLI commands so listing and verification use the same namespacing. Adds unit tests covering the new config parsing, the ``BaseSource`` override path, and the Azure lease behaviour (happy path, existing blob, ``LeaseAlreadyPresent`` skip, transient-error fail-open, TTL clamping, release-on-exception).
When multiple replicas share a cluster-wide server_name, we previously gated every backup kind (full, incremental, binlogs) behind the same Azure blob lease and wrote every replica's binlog progress into a single shared binlog-status blob. Two issues surfaced: 1. binlog jobs (firing every few minutes) would briefly acquire and release the cluster lock, racing with the top-of-hour full/incremental jobs. The loser of the race skipped, occasionally producing empty hourly windows even though local flock already serializes jobs on each replica. 2. Because binlog filenames are prefixed with the MySQL hostname and binlogs_to_backup compares them as strings against the shared latest_backup, only the replica with the lexicographically highest hostname ever had its binlogs uploaded. The other replicas saw their local binlogs as "older" and uploaded nothing, turning binlog coverage into a single-host single point of failure for PITR. Changes: - Bypass the cluster lock on the binlogs_only path. Full and incremental runs still acquire the lock so only one replica produces the big xtrabackup/file payloads. - Store binlog-status at <server_name>/<hostname>/binlog-status so each replica tracks its own progress independently and uploads its own binlog stream into the shared <server_name>/binlog/ tree (filenames are already per-host so blobs don't collide). - Seed the new per-host status from the legacy shared blob on the first post-upgrade run, filtering entries by the local hostname prefix so we don't re-upload every existing blob (Azure's upload_blob without overwrite errors on duplicates). A corrupt legacy blob is logged and skipped rather than blocking the upgrade. - Teach 'twindb-backup status --type binlog' to read the per-host path, defaulting the replica segment to the local hostname. - Document the split lock/status semantics in docs/usage.rst. - Bump to 3.6.1. Adds unit tests for: the per-host status path, the lock-scope invariants of backup_everything (binlogs_only bypasses the lock; full run acquires it; contended lock skips), and the legacy-seed helper (no-op when per-host populated or legacy empty, filters by local hostname, tolerates no matches, swallows corrupt legacy blob). All 435 unit tests pass.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request includes a version update for TwinDB Backup from 3.3.0 to 3.4.0, along with several other enhancements and fixes. The most important changes include updating version references, adding a new hostname configuration option for MySQL, and improving the handling of remote paths in Azure Blob Storage.
Version Update:
3.3.0to3.4.0inREADME.rst,docs/installation.rst,omnibus/config/projects/twindb-backup.rb,setup.cfg,setup.py, andtwindb_backup/__init__.py. [1] [2] [3] [4] [5] [6] [7]MySQL Configuration Enhancements:
hostnameoption to MySQL configuration with a default value of127.0.0.1indocs/usage.rst,support/twindb-backup.cfg,twindb_backup/configuration/mysql.py, andvagrant/environment/puppet/modules/profile/files/twindb-backup.cfg. [1] [2] [3] [4] [5]backup_mysqlandbackup_binlogsfunctions to use the newhostnameconfiguration intwindb_backup/backup.py. [1] [2] [3] [4]Azure Blob Storage Improvements:
twindb_backup/destination/az.py,tests/unit/destination/az/test_init.py,tests/unit/destination/az/test_list_files.py, andtests/unit/destination/az/util.py. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11]Unit Tests:
tests/unit/configuration/test_mysql.py.tests/unit/destination/az/test_list_files.py.Miscellaneous:
get_streammethod intwindb_backup/source/mysql_source.pyto use the newhostnameconfiguration.