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.

Sign up for GitHub

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

Jump to bottom

Always show applied migration configuration in VMI status #7267

Merged
merged 5 commits into from Mar 24, 2022
Merged

Always show applied migration configuration in VMI status #7267

merged 5 commits into from Mar 24, 2022

Conversation

iholder101
Copy link
Contributor

What this PR does / why we need it:
In this PR #6399 that introduced Migration Policies, new fields were introduced to VirtualMachineInstanceMigrationState struct which is a property of the VMI status. One of those fields is named MigrationConfiguration which lists the actual migration configurations that were applied.

Before this PR this information was part of a VMI status only if the migration was matched to a migration policy.
After this PR this information would be part of VMI status for every migration - not only migrations that were matched to a policy.

Usually, when a VMI migrates without matching to a policy (which means the configs are taken from Kubevirt CR) the applied configurations can be found in Kubevirt CR. This is not guaranteed, however, since Kubevirt CR can be changed at any moment. In such circumstances we wouldn't know the actual applied migration configs.

This information is valuable for debugging or inspecting the actual migration configs.

Release note:

Applied migration configurations can now be found in VMI's status

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Feb 23, 2022
@iholder101
Copy link
Contributor Author

/cc @vladikr @dshchedr

@iholder101
Copy link
Contributor Author

/retest

1 similar comment
@iholder101
Copy link
Contributor Author

/retest

@iholder101 iholder101 force-pushed the feature/VmiMigrationConfigAlwaysUpdate branch from cb6bde5 to 7878145 Compare February 23, 2022 15:45
@iholder101
Copy link
Contributor Author

Tests pass locally...
trying again

/retest

@iholder101
Copy link
Contributor Author

/retest

@Barakmor1
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 3, 2022
@iholder101 iholder101 force-pushed the feature/VmiMigrationConfigAlwaysUpdate branch from a47008e to 45e89a4 Compare March 13, 2022 10:53
@kubevirt-bot kubevirt-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 13, 2022
@iholder101
Copy link
Contributor Author

/retest

@Barakmor1
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2022
@iholder101
Copy link
Contributor Author

/retest

@vladikr
Copy link
Member

vladikr commented Mar 21, 2022

@iholder-redhat This PR looks great already!
I also think that it will beneficial to log the migrationOptions that are being applied before the migration starts, here: https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-handler/vm.go#L2341
Would it be possible to add it to this PR?

@iholder101
Update VMI status' MigrationConfiguration on every migration
6df8485 
Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101
Add functional tests
ee9c010 
Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101
Adjust unit tests
ea70f87 
Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101
getMigrationConfigPatch to support getting custom configs
a8c601a 
Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101 iholder101 force-pushed the feature/VmiMigrationConfigAlwaysUpdate branch from 45e89a4 to 87251d2 Compare March 22, 2022 09:04
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 22, 2022
@iholder101
Copy link
Contributor Author

@iholder-redhat This PR looks great already! I also think that it will beneficial to log the migrationOptions that are being applied before the migration starts, here: https://github.com/kubevirt/kubevirt/blob/main/pkg/virt-handler/vm.go#L2341 Would it be possible to add it to this PR?

Great idea! Thanks! Added.

BTW - the way it is now the log entry would just show pointers (as that's what %+v shows). According to a quick google search I see that there is no standard way of solving this problem - only by either reflection, serialization or dedicated function.

Do you have any suggestions?

@iholder101
Copy link
Contributor Author

/retest

1 similar comment
@iholder101
Copy link
Contributor Author

/retest

@iholder101
Use DeepCopy() to copy migration configs
86a7e6a 
- Also add log entry for matched configs

Signed-off-by: Itamar Holder <iholder@redhat.com>
@iholder101 iholder101 force-pushed the feature/VmiMigrationConfigAlwaysUpdate branch from 87251d2 to 86a7e6a Compare March 22, 2022 14:55
@iholder101
Copy link
Contributor Author

/retest

1 similar comment
@iholder101
Copy link
Contributor Author

/retest

@Barakmor1
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Mar 23, 2022
stu-gott
stu-gott reviewed Mar 23, 2022
View reviewed changes
Copy link
Member

@stu-gott stu-gott left a comment

Choose a reason for hiding this comment

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

/approve
/cc @vladikr

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: stu-gott

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 23, 2022
@iholder101
Copy link
Contributor Author

/retest

@kubevirt-commenter-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs.
Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@kubevirt-bot kubevirt-bot merged commit cd23934 into kubevirt:main Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stu-gott stu-gott stu-gott left review comments

@enp0s3 enp0s3 Awaiting requested review from enp0s3

@ILpinto ILpinto Awaiting requested review from ILpinto

@dshchedr dshchedr Awaiting requested review from dshchedr

@vladikr vladikr Awaiting requested review from vladikr

Assignees

@Barakmor1 Barakmor1

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@iholder101 @Barakmor1 @vladikr @kubevirt-bot @kubevirt-commenter-bot @stu-gott