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

[release-0.51] Fixing host model support heterogeneus cluster #7950

Conversation

Barakmor1
Copy link
Member

@Barakmor1 Barakmor1 commented Jun 20, 2022

What this PR does / why we need it:
Calculate selectors only for the initial Node to remeber the required features,
allow vmi with host-model to migrate when target node does't have the same host-model cpuModel.
manual backporting #7672 ,#7770 from main
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Release note:

NONE

bmordeha added 2 commits June 20, 2022 12:26
… features,

Allow vmi with host-model to migrate when target node does't have the same host-model cpuModel.
Manual back port kubevirt#7672,kubevirt#7770 from main

Signed-off-by: bmordeha <bmodeha@redhat.com>
Modify existing tests to consider the new changes of the back port.

Signed-off-by: bmordeha <bmodeha@redhat.com>
@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/L labels Jun 20, 2022
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign vladikr after the PR has been reviewed.
You can assign the PR to them by writing /assign @vladikr in a comment when ready.

The full list of commands accepted by this bot can be found 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

@Barakmor1
Copy link
Member Author

/retest

3 similar comments
@Barakmor1
Copy link
Member Author

/retest

@Barakmor1
Copy link
Member Author

/retest

@Barakmor1
Copy link
Member Author

/retest

@jean-edouard
Copy link
Contributor

The original PR has 5 commits, I would expect this one to have 5 as well (unless most of the code had to be rewritten, but that doesn't seem to be the case).
Please use git cherry-pick -x to keep a reference to the original commit IDs.
This PR seems to not only backport #7672 but also parts of #7770, namely this commit: 47794de
I would suggest cherry-picking that commit too and explicitly stating it in the PR description.
Finally, in the PR title, please move [release-0.51] to the beginning of the line.

@Barakmor1
Copy link
Member Author

Barakmor1 commented Jun 21, 2022

The original PR has 5 commits, I would expect this one to have 5 as well (unless most of the code had to be rewritten, but that doesn't seem to be the case). Please use git cherry-pick -x to keep a reference to the original commit IDs. This PR seems to not only backport #7672 but also parts of #7770, namely this commit: 47794de I would suggest cherry-picking that commit too and explicitly stating it in the PR description. Finally, in the PR title, please move [release-0.51] to the beginning of the line.

Hey @jean-edouard ,
You are right this PR also back port #7770 And some of the code has been rewritten so i took the main branch as reference.

Please note that some of the original commits are using functions of libnode,libvmi and alertIfHostModelIsUnschedulable function that weren't exist in older versions which make it very difficult using git cherry-pick with the original commits.

Also i had to modify some of the additional tests(second commit) because we discovered that they are flacky ( #7752 ).

Because of the complexity of maintaining the order of the commits and the amount of versions that we need to be back port I decided to simplify it to 2 commits (i think that the commits are cohesive) .

@Barakmor1 Barakmor1 changed the title Fixing host model support heterogeneus cluster [release-0.51] [release-0.51] Fixing host model support heterogeneus cluster Jun 21, 2022
@jean-edouard
Copy link
Contributor

We should be really careful with backports. Including various bits from various PRs can make subsequent backports exponentially harder.
Since this is effectively backporting 3 PRs (#7672 #7752 #7770), can one or two of those be backported cleanly? (i.e. using cherry-picks).
Thanks for the improved PR description and commit messages, that's already a big improvement to help future backporters.

@Barakmor1
Copy link
Member Author

Barakmor1 commented Jun 21, 2022

@jean-edouard , @vladikr This is the history of these PRs:

  1. Adding and fixing support for host-model migration in heterogeneous cluster #7672 got merged
  2. lot of tests started to fail because of some things that were missing in Adding and fixing support for host-model migration in heterogeneous cluster #7672
  3. we fixed this bug in The host-model cpuModel isn't usually fully supported in the nodes and we should take it into account #7770 and modifed ALL the functions that were modified in Adding and fixing support for host-model migration in heterogeneous cluster #7672 (prepareNodeSelectorForHostCpuModel , alertIfHostModelIsUnschedulable , loadDomCapabilities )
  4. we discovered that tests that update the nodes are flaky because of the node's heartbeat and Itamar created this PR Optimize libnode's annotation/label modification functions + stabilize & fix migration functests #7752 to fix this
  5. i couldn't back port Optimize libnode's annotation/label modification functions + stabilize & fix migration functests #7752 because it has many dependencies that didn't exist in previous versions so i just added eventually when updating nodes instead of patching them like Itamar did in Optimize libnode's annotation/label modification functions + stabilize & fix migration functests #7752

beside that the original tests are using functions that are in libnode and libvmi so i couldn't backport the commits of the tests as well.
and also in the original commits we modify alertIfHostModelIsUnschedulable that didn't exist in previous versions

I know that it is much harder to review this PR without keeping the original commits and the order but i couldn't find a better way.

@Barakmor1
Copy link
Member Author

Barakmor1 commented Jun 21, 2022

in other versions i used git cherry pick locally to get commits from this and other versions when i could:

#7957
#7956
#7955
#7954
#7953
#7952
#7950
#7949
MERGED:
#7870
#7857

@Barakmor1
Copy link
Member Author

/close

@kubevirt-bot
Copy link
Contributor

@Barakmor1: Closed this PR.

In response to this:

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dco-signoff: yes Indicates the PR's author has DCO signed all their commits. release-note-none Denotes a PR that doesn't merit a release note. size/L
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants