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

PWX-27385: Honor IncludeResources flag when entering Application stage. #1226

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

adityadani
Copy link
Contributor

What type of PR is this?

Uncomment only one and also add the corresponding label in the PR:
bug

What this PR does / why we need it:

  • The migration controller was not honoring the IncludeResources flag and was
    trying to migrate all the resources even if IncludeResources was set to false.

  • Add comments to the MigrationSpec.

Does this PR change a user-facing CRD or CLI?:
No

Is a release note needed?:
Yes

Will handle release note for this bug PWX-27385 in a follow up PR which involves more changes to reduce the overall time for migration. 

Does this change need to be cherry-picked to a release branch?:
Yes - 2.12

- The migration controller was not honoring the IncludeResources flag and was
  trying to migrate all the resources even if IncludeResources was set to false.

- Add comments to the MigrationSpec.
@@ -2131,30 +2137,28 @@ func (m *MigrationController) getMigrationSummary(migration *stork_api.Migration
migrationSummary.ElapsedTimeForVolumeMigration = elapsedTimeVolume
}

if migration.Spec.IncludeResources == nil || *migration.Spec.IncludeResources {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should calculate the MigrationSummary and populate the time taken for migration k8s resources, even if IncludeResources is set to false, since we would be migrating the PV and PVC objects.

@cnbu-jenkins
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Contributor

@pp511 pp511 left a comment

Choose a reason for hiding this comment

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

lgtm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants