Add missing regex to backup manager

What does this MR do?

!10901 (merged) introduced a new naming scheme for backups, but the code which cleans up old backups wasn't updated accordingly. In order to maintain backward compatibility, we need to account for 3 naming schemes.

http://rubular.com/r/dUx1D42p3i

What are the relevant issue numbers?

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/32669

@axil Can you set this MR to MWPS if changes are reviewed? I can hold off before we tag 9.2.1 to ensure this MR is picked.

/cc @timothyandrew

@kushalpandya it needs one approval, I'll ask around.

changed the description

@axil: Could you please add a test case to lib/gitlab/backup/manager_spec.rb that fails without this change?

/cc @kushalpandya

added 1 commit

• 6fe2744d - Add tests for removing old backups with the new timestamp

Compare with previous version

added 1 commit

• e4d9be0e - Fix rubocop

Compare with previous version

The new regex seems to be a little safer:

demovars = [
  "1495488287_2017_05_22_gitlab_backup.tar",
  "1495488287_2017_05_22_9.2.0_gitlab_backup.tar",
  "1495488287_2017_05_22_9.2.0_412_gitlab_backup.tar",
  "1495527122_gitlab_backup.tar",
  "1495527068_2017_05_23_gitlab_backup.tar",
  "12345_someunrelatedfile"
]

demovars.each do |file|
  if file =~ /(\d+)(?:_\d{4}_\d{2}_\d{2})?_gitlab_backup\.tar/
    timestamp = $1.to_i
    print "OLD Will look at #{file} as timestamp #{timestamp}\n"
  else
    timestamp = $1.to_i
    print "OLD Will ignore  #{file} as timestamp #{timestamp}\n"
  end
end
demovars.each do |file|
  if file =~ /(\d+)(?:_\d{4}_\d{2}_\d{2}(_\d+\.\d+\.\d+.*)?)?_gitlab_backup\.tar$/
    timestamp = $1.to_i
    print "NEW Will look at #{file} as timestamp #{timestamp}\n"
  else
    timestamp = $1.to_i
    print "NEW Will ignore  #{file} as timestamp #{timestamp}\n"
  end
end

Output:

OLD Will look at 1495488287_2017_05_22_gitlab_backup.tar as timestamp 1495488287
OLD Will look at 1495488287_2017_05_22_9.2.0_gitlab_backup.tar as timestamp 0
OLD Will look at 1495488287_2017_05_22_9.2.0_412_gitlab_backup.tar as timestamp 412
OLD Will look at 1495527122_gitlab_backup.tar as timestamp 1495527122
OLD Will look at 1495527068_2017_05_23_gitlab_backup.tar as timestamp 1495527068
OLD Will ignore  12345_someunrelatedfile as timestamp 0
NEW Will look at 1495488287_2017_05_22_gitlab_backup.tar as timestamp 1495488287
NEW Will look at 1495488287_2017_05_22_9.2.0_gitlab_backup.tar as timestamp 1495488287
NEW Will look at 1495488287_2017_05_22_9.2.0_412_gitlab_backup.tar as timestamp 1495488287
NEW Will look at 1495527122_gitlab_backup.tar as timestamp 1495527122
NEW Will look at 1495527068_2017_05_23_gitlab_backup.tar as timestamp 1495527068
NEW Will ignore  12345_someunrelatedfile as timestamp 0

The integer type casting (to_i) causing the old regex's inner match to become '0' may be worthy of special attention. to_i giving a "0" or a "1" as output is going to cause the file being matched to be deleted.

A proper unix timestamp will always be 9-10 digits for the foreseeable future. For a belt & suspenders approach, the timestamp will match the date too, i.e. 1495488287 should represent a timestamp sometime during 2017-05-22 (this sanity check only works with some of the filename formats).

approved this merge request

enabled an automatic merge when the pipeline for e4d9be0e succeeds

@axil: Thanks for adding tests! I've approved this and set it to MWPS.

/cc @kushalpandya

Could you please add a test case to lib/gitlab/backup/manager_spec.rb that fails without this change?

@timothyandrew I think this is handled by https://gitlab.com/gitlab-org/gitlab-ce/blob/d71577468cbf54bdd2621fac5f5a62f93193d4ea/spec/tasks/gitlab/backup_rake_spec.rb#L354

I added some examples in spec/lib/gitlab/backup/manager_spec.rb though.

@axil: I tried running the spec file locally, and it fails.

<snipped rspec output>

EDIT: This could be local quirks though. Let's see if this build passes.

I think this is handled by https://gitlab.com/gitlab-org/gitlab-ce/blob/d71577468cbf54bdd2621fac5f5a62f93193d4ea/spec/tasks/gitlab/backup_rake_spec.rb#L354

@axil: If that spec covered this issue, wouldn't it have failed the 9-2-stable build and prevented https://gitlab.com/gitlab-org/gitlab-ce/issues/32669 from occurring?

mentioned in issue #32669 (closed)

Well, there is no test that actually tests the backups expect https://gitlab.com/gitlab-org/gitlab-ce/blob/d71577468cbf54bdd2621fac5f5a62f93193d4ea/spec/tasks/gitlab/backup_rake_spec.rb#L342 which actually runs the backup raketask.

Things in spec/lib/gitlab/backup/manager_spec.rb are just stubs if I understand correctly.

@axil: Yes they are stubs, but they are valuable since they test which files get picked up and which ones get left behind for the given regex. They are a version of http://rubular.com/r/dUx1D42p3i but running within our test suite.

Essentially, if this regex is changed in the future, we need to be confident that https://gitlab.com/gitlab-org/gitlab-ce/issues/32669 will not happen again as long as the build for spec/lib/gitlab/backup/manager_spec.rb is green.

@axil I clarified my remarks on #32669 (closed) and also provided some example test cases on this merge request which might be worthy of both special consideration as well as test coverage.

E.g. if the function thinks the timestamp is "0" or "1", it probably shouldn't delete the file.

mentioned in issue #32437 (closed)

added 1 commit

• 4ee79839 - Fix tests

Compare with previous version

@timothyandrew ok I fixed the tests, and tested with setting the regex to the old one which failed.

The integer type casting (to_i) causing the old regex's inner match to become '0' may be worthy of special attention. to_i giving a "0" or a "1" as output is going to cause the file being matched to be deleted.

@DanielDent thanks! I'll leave your comment to the experts :)

mentioned in issue #32711 (closed)

assigned to @rymai

@DanielDent: Thanks for the detailed report of the problem. While the timestamp mis-parsing is a serious issue and should be fixed/tightened immediately, it isn't a regression for %9.2 - the issue is present for the earlier version of the regex as well.

This fix should be merged into the stable branch now. I'll create a separate bug issue to track the potential mis-parsing of the timestamp and make sure it gets merged as soon as possible.

Thanks again!

/cc @axil @selfup @kushalpandya @ClemMakesApps

approved this merge request

merged

mentioned in commit b00c268b

assigned to @timothyandrew

mentioned in issue #32796 (closed)

mentioned in commit 8b46dfdc

Picked into 9-2-stable, will go in 9-2-1.

mentioned in merge request gitlab-com/www-gitlab-com!6146 (merged)

resolved all discussions

mentioned in issue #33125 (closed)

mentioned in merge request !14333 (merged)