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

Windows VMI - adding bridge binding test to validate ip address #6833

Merged
merged 1 commit into from
Dec 27, 2021
Merged

Windows VMI - adding bridge binding test to validate ip address #6833

merged 1 commit into from
Dec 27, 2021

Conversation

dteplits
Copy link
Contributor

@dteplits dteplits commented Nov 23, 2021 •
edited
Loading

What this PR does / why we need it:
tests, windows: Add bridge binding test to validate IP address

In case of bridge binding on Windows VM, DHCP server must provide
IPv4 address in proper v4 format, otherwise it will be parsed
incorrectly by the VM and as a result Windows VM will become
invisible on cluster network
This problem was fixed by issue#6448
Current test verifies that Windows VMI can be recognized by other components
in cluster

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 #
https://issues.redhat.com/browse/CNV-14301
See also: #6462, #6448
Special notes for your reviewer:

Release note:

NONE

@kubevirt-bot kubevirt-bot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: no Indicates the PR's author has not DCO signed all their commits. size/M needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 23, 2021
@kubevirt-bot
Copy link
Contributor

Hi @dteplits. Thanks for your PR.

I'm waiting for a kubevirt member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@kubevirt-bot kubevirt-bot added dco-signoff: yes Indicates the PR's author has DCO signed all their commits. and removed dco-signoff: no Indicates the PR's author has not DCO signed all their commits. labels Nov 23, 2021
@EdDev
Copy link
Member

EdDev commented Nov 23, 2021

/ok-to-test

@kubevirt-bot kubevirt-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Nov 23, 2021
@dteplits
Copy link
Contributor Author

/retest

1 similar comment
@EdDev
Copy link
Member

EdDev commented Nov 25, 2021

/retest

@dteplits
Copy link
Contributor Author

/retest

@dteplits
Copy link
Contributor Author

/retest

@dteplits
Copy link
Contributor Author

/retest

@dteplits
Copy link
Contributor Author

/retest

1 similar comment
@dteplits
Copy link
Contributor Author

/retest

@dteplits
Copy link
Contributor Author

/retest

@dteplits
Copy link
Contributor Author

/test ?

@dteplits
Copy link
Contributor Author

done

@dteplits dteplits closed this Dec 21, 2021
@dteplits dteplits reopened this Dec 21, 2021
@dteplits
Copy link
Contributor Author

/retest

@dteplits
Copy link
Contributor Author

/retest

1 similar comment
@dteplits
Copy link
Contributor Author

/retest

EdDev
EdDev suggested changes Dec 23, 2021
View reviewed changes
tests/windows_test.go
Comment on lines +278 to +287
getVirtHandlerPod := func() (*k8sv1.Pod, error) {
winVmiPod := tests.GetRunningPodByVirtualMachineInstance(windowsVMI, windowsVMI.Namespace)
nodeName := winVmiPod.Spec.NodeName

pod, err := kubecli.NewVirtHandlerClient(virtClient).Namespace(flags.KubeVirtInstallNamespace).ForNode(nodeName).Pod()
if err != nil {
return nil, fmt.Errorf("failed to get virt-handler pod on node %s: %v", nodeName, err)
}
return pod, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Looks like we have multiple ways to get the same thing. For example: getPodByKubeVirtRole.
But even with this existing method, there is a lot of code duplication as this one is re-defined all over.

Can you please follow up on this and extract it to some reasonable place and update other usages to use it? (in a following PR)

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 will do it in a separate PR

tests/windows_test.go Outdated
command,
)
return err
}, time.Minute*1, time.Second*15).ShouldNot(HaveOccurred())
Copy link
Member

Choose a reason for hiding this comment

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

Should(Succeed())

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

tests/windows_test.go Outdated

By(fmt.Sprintf("Running \"%s\" command via winrm-cli", command))
Eventually(func() error {
output, err = tests.ExecuteCommandOnPod(
Copy link
Member

Choose a reason for hiding this comment

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

If you do not use the output variable, the name it _ to express it is not used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@dteplits
Copy link
Contributor Author

/retest

EdDev
EdDev approved these changes Dec 26, 2021
View reviewed changes
Copy link
Member

@EdDev EdDev left a comment

Choose a reason for hiding this comment

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

Thank you!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 26, 2021
@AlonaKaplan
Copy link
Member

Great work! Please remove from the PR and the commit messages the following "Current test verifies that IP address on Windows VM matches".
Seems that the test is not testing it.

@dteplits
tests, windows: Add bridge binding test to validate IP address
c2ca0af 
In case of bridge binding on Windows VM, DHCP server must provide
IPv4 address in proper v4 format, otherwise it will be parsed
incorrectly by the VM and as a result Windows VM will become
invisible on cluster network
This problem was fixed by issue#6448
Current test verifies that Windows VMI can be recognized by other
components in cluster

Test for: kubevirt#6462
See also: kubevirt#6448

Signed-off-by: Diana Teplits <dteplits@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Dec 27, 2021
@AlonaKaplan
Copy link
Member

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AlonaKaplan

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 Dec 27, 2021
@AlonaKaplan
Copy link
Member

/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Dec 27, 2021
@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 8e95f18 into kubevirt:main Dec 27, 2021
@maiqueb maiqueb mentioned this pull request Jan 5, 2022
Closed
@dteplits dteplits mentioned this pull request Feb 16, 2022
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlonaKaplan AlonaKaplan AlonaKaplan left review comments

@EdDev EdDev EdDev approved these changes

@ILpinto ILpinto Awaiting requested review from ILpinto

@rnetser rnetser Awaiting requested review from rnetser

Assignees

@EdDev EdDev

@AlonaKaplan AlonaKaplan

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. release-note-none Denotes a PR that doesn't merit a release note. size/M
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dteplits @kubevirt-bot @EdDev @AlonaKaplan @kubevirt-commenter-bot