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

backup: init migrate controller #408

Merged
merged 2 commits into from
Oct 24, 2023
Merged

Conversation

Xieql
Copy link
Contributor

@Xieql Xieql commented Oct 21, 2023

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

init the migrate controller.

part of #404

this PR's CI checks is rely on #407

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

backup: init migrate controller

@netlify
Copy link

netlify bot commented Oct 21, 2023

Deploy Preview for kurator-dev canceled.

Name Link
🔨 Latest commit 1ac96a0
🔍 Latest deploy log https://app.netlify.com/sites/kurator-dev/deploys/65376dd0ef0afe00081376c5

@hzxuzhonghu
Copy link
Member

please rebase

@Xieql
Copy link
Contributor Author

Xieql commented Oct 23, 2023

please rebase

done

Signed-off-by: Xieql <xieqianglong@huawei.com>
migrate.Status.Phase = backupapi.MigratePhaseInProgress
log.Info("Migrate Phase changes", "phase", backupapi.MigratePhaseInProgress)
// Ensure the restore point is created after the backup.
time.Sleep(MigrationDelay)
Copy link
Member

Choose a reason for hiding this comment

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

can you explain why we need sleep, because i see in above L173, we guaranteed backup is complete

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This variable is no longer needed.

The previous version of the code, written last month, lacked the condition, leading to the current redundant use of time.Sleep(MigrationDelay).

conditions.MarkTrue(migrate, backupapi.SourceReadyCondition)
}

return ctrl.Result{}, nil
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't requeue after if veleroBackup.Status.Phase != velerov1.BackupPhaseCompleted

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this check alreay in here:

// reconcileMigrateRestore handles the restore stage of the migration process.
func (m *MigrateManager) reconcileMigrateRestore(ctx context.Context, migrate *backupapi.Migrate) (ctrl.Result, error) {
	log := ctrl.LoggerFrom(ctx)

	// If source cluster's backup resource is not ready, return directly.
	if !isMigrateSourceReady(migrate) {
		return ctrl.Result{RequeueAfter: RequeueAfter}, nil
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think your idea is better

}

if migrate.Status.Phase != backupapi.MigratePhaseInProgress {
migrate.Status.Phase = backupapi.MigratePhaseInProgress
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 confusing, InProgress should be the interval between backup begin and restore complete.

if err = getResourceFromClusterClient(ctx, referredBackupName, VeleroNamespace, *clusterAccess, referredVeleroBackup); err != nil {
if apierrors.IsNotFound(err) {
// if not found, requeue with `RequeueAfter`
return ctrl.Result{RequeueAfter: RequeueAfter}, nil
Copy link
Member

Choose a reason for hiding this comment

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

In theory this could not happen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the sync of velero backup may take time, so it could happen. the velero backup need syn from source cluster to target cluster

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the velero backup is create in source clusters, so we need ensure that this can be found in each target clusters

Signed-off-by: Xieql <xieqianglong@huawei.com>
@Xieql
Copy link
Contributor Author

Xieql commented Oct 24, 2023

/label tide/merge-method-squash

@Xieql
Copy link
Contributor Author

Xieql commented Oct 24, 2023

@hzxuzhonghu PTAL

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

/lgtm

@kurator-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kurator-bot kurator-bot merged commit a5a20e2 into kurator-dev:main Oct 24, 2023
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants