Merge lp://qastaging/~gl-az/percona-xtrabackup/BT23557-2.1-lp1160778 into lp://qastaging/percona-xtrabackup/2.1

Proposed by George Ormond Lorch III
Status: Merged
Approved by: Alexey Kopytov
Approved revision: no longer in the source branch.
Merged at revision: 649
Proposed branch: lp://qastaging/~gl-az/percona-xtrabackup/BT23557-2.1-lp1160778
Merge into: lp://qastaging/percona-xtrabackup/2.1
Diff against target: 420 lines (+152/-144)
5 files modified
innobackupex (+136/-118)
test/t/xb_compress_encrypt.sh (+3/-7)
test/t/xb_encrypt.sh (+8/-6)
test/t/xb_parallel_compress_encrypt.sh (+3/-7)
test/t/xb_parallel_encrypt.sh (+2/-6)
To merge this branch: bzr merge lp://qastaging/~gl-az/percona-xtrabackup/BT23557-2.1-lp1160778
Reviewer Review Type Date Requested Status
Alexey Kopytov (community) Approve
Vlad Lesin (community) g2 Approve
Review via email: mp+174334@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2013-06-03.

Description of the change

Fix for lp1160778 : non-InnoDB files are not encrypted.

Refactored some innobackupex to make all files not copied/streamed by xtrabackup binary go through one of two common functions. These new functions handle all of the stream/encryption formatting for non-InnoDB files. This also simplifies and normalizes some code in innobackupex that has just been added onto over time.

Encryption test cases have also been fixed and improved. xb_encrypt.sh will now fail if there are any unencrypted files detected within the backup set.

23557

http://jenkins.percona.com/view/XtraBackup/job/percona-xtrabackup-2.1-param/375/
------

Rebased this MP on current trunk and fixed issues pointed out in last MP. Vlad if you could please re-review and think of it as a fresh review because of the re-basing. I only ran into two small conflicts but still I would like another of your great reviews if possible.

To post a comment you must log in.
Revision history for this message
George Ormond Lorch III (gl-az) wrote : Posted in a previous version of this proposal

I wonder though if we should make xtrabackup_checkpoints not encrypted so that incrementals are a little easier. Otherwise users will have to scrape the "The latest check point (for incremental):" from the innobackupex output and store it somewhere.

Revision history for this message
Stewart Smith (stewart) wrote : Posted in a previous version of this proposal

George Ormond Lorch III <email address hidden> writes:
> I wonder though if we should make xtrabackup_checkpoints not encrypted
> so that incrementals are a little easier. Otherwise users will have to
> scrape the "The latest check point (for incremental):" from the
> innobackupex output and store it somewhere.

I think we should keep everything encrypted. history on the server is
the correct way of better supporting incremental encrypted backups.

--
Stewart Smith

Revision history for this message
George Ormond Lorch III (gl-az) wrote : Posted in a previous version of this proposal

This also raises the question of compression...Should we not be
compressing all non-innodb files as well? Which also leads to parallel
compress/encrypt/copy of non-innodb files too...

On 6/6/2013 7:08 PM, Stewart Smith wrote:
> George Ormond Lorch III <email address hidden> writes:
>> I wonder though if we should make xtrabackup_checkpoints not encrypted
>> so that incrementals are a little easier. Otherwise users will have to
>> scrape the "The latest check point (for incremental):" from the
>> innobackupex output and store it somewhere.
> I think we should keep everything encrypted. history on the server is
> the correct way of better supporting incremental encrypted backups.
>

--
George O. Lorch III
Software Engineer, Percona
+1-888-401-3401 x542 US/Arizona (GMT -7)
skype: george.ormond.lorch.iii

Revision history for this message
Vlad Lesin (vlad-lesin) wrote : Posted in a previous version of this proposal

1) Lines 31-45: I see the logic is changed here because files are not copied if $option_rsync is true in the original code because they are copied earlier in backup()->backup_files() call with rsync. innobackupex:429 - backup_files() copies $buffer_pool_file_name with rsync (lines 2234-2241 includes it in copy-list), but innobackupex:450 copies it again in the case of $option_rsync is on. So it looks like double copying.

2) ib_buffer_pool.sh fails with the following message: "ib_buffer_pool.sh: Buffer pool dump has not been restored", but it passes when the changes are reverted.

review: Needs Fixing (g2)
Revision history for this message
Vlad Lesin (vlad-lesin) :
review: Approve (g2)
Revision history for this message
Alexey Kopytov (akopytov) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches