N°8458 - Improve backup/restore mechanisms

[Hipska]

Base information

Question	Answer
Related to a SourceForge thead / Another PR / Combodo ticket?	NA
Type of change?	Bug fix / Enhancement

Symptom (bug) / Objective (enhancement)

To have better reliability when issues occur when creating/restoring backup.

Reproduction procedure (bug)

These are different scenarios when problems might arise:

• Create backup on limited (almost full) filesystem on data/ while there is more than enough in system temporary directory.
• Try restoring a corrupt archive.
• Try restoring old ZIP archive.

Proposed solution (bug and enhancement)

• Create the temporary directory on the system designated location
• Also remove the temporary directory while encountering errors

Checklist before requesting a review

• I have performed a self-review of my code
• I have tested all changes I made on an iTop instance
• I have added a unit test, otherwise I have explained why I couldn't
• Is the PR clear and detailed enough so anyone can understand digging in the code?

Checklist of things to do before PR is ready to merge

• Make sure there is enough space on tmp dir before attempting to dump database backup
• Make sure a valid, not corrupt archive has been created. (Might be solved with _writeBlock always returns true pear/Archive_Tar#52 or implement workaround)

[jf-cbd]

Hello, thank you for your PR ;)

Concerning the use of the system temp folder, this is the way we did it first in iTop, before finally setting it to data folder, because some people met permissions issues.

I think the best way would be to have an option parameter that would allow choosing the desired path to store the temporary backup file, with the default value being the current one (in data folder). That would also allow choosing another disk or VM for example.

On my side I'll open an internal ticket, to clean the temporary backup when creating a new one, since there is generally no point of keeping them.

[Hipska]

because some people met permissions issues

If that would be really the case, all the other uses of SetupUtils::GetTmpDir() would not work. For example compiling the data-model..

It is really bad practice to use app/data directories for temporary files (disk wear, disk speeds, ...) hence this is why specific temp dirs exist in Linux systems.

About having an option for that, do you mean it to be specific for backups or for all uses of SetupUtils::GetTmpDir()?

On my side I'll open an internal ticket, to clean the temporary backup when creating a new one, since there is generally no point of keeping them.

What do you mean with that? Under normal circumstances the tmp files are removed after backup creation. This only didn't happen when encountering errors, this should now be solved by moving this removal to the finally block..

[jf-cbd]

It is really bad practice to use app/data directories for temporary files (disk wear, disk speeds, ...) hence this is why specific temp dirs exist in Linux systems.

Well, that's the point. Some iTops are not installed on Linux. And some are installed on Linux with some weird configurations. Even if we don't have to support the systems configurations of our users, we found a solution that works in most of the environments. You could also say the same thing for the logs - that could/should be written in /var/log - but the choice has been mode to store them inside iTop folder.
We made a meeting - with our sysadmin - to review your PR and think about the solution(s), and the result of it is the content of my first message.

About having an option for that, do you mean it to be specific for backups or for all uses of SetupUtils::GetTmpDir()?

It was specific to the backups.

this should now be solved by moving this removal to the finally block.

Yes, it should, indeed. I'll test it.

[Hipska]

@odain-cbd Why did You close this PR? And why did you remove the 3.2 branch?

[odain-cbd]

@odain-cbd Why did You close this PR? And why did you remove the 3.2 branch?

Sorry for this. Branch was removed by mistake. Lucky me ...github only closes the attached PRs. It could have bébé worst!
I will fix the PR status.

[Hipska]

@odain-cbd please reopen the PR

[Hipska]

@jf-cbd I made it into an config option, this gives indeed more flexibility. Default stays data/ dir for current and new customers.

[Hipska]

@jf-cbd Archive_Tar has been updated to 1.6.0 which includes checks for correct writing to the file..

Do you expect anything else from me?

[jf-cbd]

Hello @Hipska, thanks for your edition :) It sounds good to me. I'll do some tests and keep you updated.

[Hipska]

I have an additional use-case where the disk on data/ is much slower than the disk of the system temporary directory. This increases the time to take a backup significantly.

[jf-cbd]

Suggested change

	public static function GetTmpDir(Config $oConfig): string
	{
	$sTmpDir = @tempnam($oConfig->GetModuleSetting('itop-backup', 'backup_tmpdir', 'data/') , 'itop-backup-');
	unlink($sTmpDir); // I need a directory, not a file...
	SetupUtils::builddir($sTmpDir);
	return $sTmpDir;
	}
	public static function GetTmpDir(Config $oConfig): string
	{
	$sTmpDirConfig = $oConfig->GetModuleSetting('itop-backup', 'backup_tmpdir', 'data/');
	if (Utils::IsNullOrEmptyString($sTmpDirConfig) || $sTmpDirConfig === 'data/' || $sTmpDirConfig === 'data') {
	$sTmpDir = APPROOT.'data/itop-backup';
	} else {
	if (!is_dir($sTmpDirConfig)) {
	throw new InvalidParameterException("The configured temporary backup directory '$sTmpDirConfig' is not a valid directory.");
	}
	$sTmpDir = @tempnam($sTmpDirConfig, 'itop-backup-');
	@unlink($sTmpDir); // I need a directory, not a file...
	}
	SetupUtils::builddir($sTmpDir);
	return $sTmpDir;
	}

[jf-cbd]

This will make the tests pass and warn the user in case of wrong config

[Hipska]

It doesn't return a unique temporary directory in some (default) cases.
It also removes to option to use PHP's temporary directory without needing to specify it also in the iTop config file.