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
Add Windows VM tests #809
Add Windows VM tests #809
Conversation
d102c4b
to
30ad695
Compare
Start and stop windows VM tests passed, working on other tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far!
tests/windows_test.go
Outdated
import ( | ||
"flag" | ||
"fmt" | ||
//"time" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove commented module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure will remove it
tests/windows_test.go
Outdated
) | ||
}) | ||
|
||
XIt("should have correct UUID", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The X means "skip", right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I still trying to make work exec command via API, so I disable this tests ro run locally until it will work.
d67bde9
to
0454222
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would have preferred the possibility to directly talk to windows from within our code with winrm, but it is a good start to create this extra container.
images/winrmcli/Dockerfile
Outdated
# Copyright 2018 Red Hat, Inc. | ||
# | ||
|
||
FROM golang:1.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder how we can provide winrm-cli without having to pull in anoter image which is not fedora:27.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check if we have the possibility to bring fedora 27 with go inside
tests/windows_test.go
Outdated
tests.BeforeAll(func() { | ||
windowsPv, err := virtClient.CoreV1().PersistentVolumes().Get(tests.DiskWindows, metav1.GetOptions{}) | ||
if err != nil { | ||
Skip(fmt.Sprintf("Skip Windows tests that requires PVC %s", tests.DiskWindows)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could create a helper which is called something like SkipIfNoWindowsImage()
and isolate the logic in utils.go
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't we call this in a BeforeEach block? The BeforeAll is a not so well done extension for Ginkgo by us.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- will create a separate helper
- yes I know, I just wanted to avoid calling to Kube API for each test, it just relevant for all windows tests
@rmohr Do you have some objections vsBeforeAll
, I checkedginkgo
issue and the only objections vs it, was the possibility to run tests in parallel, but for windows testing, we can't run in parallel anyway(without test code changes)
tests/windows_test.go
Outdated
if err != nil { | ||
Skip(fmt.Sprintf("Skip Windows tests that requires PVC %s", tests.DiskWindows)) | ||
} | ||
windowsPv.Spec.ClaimRef = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that before testing we already added windows PV to the cluster, and maybe it already used by some PVC(currently not), so the PV will have the state Released
, but new PVC still cannot use it(https://kubernetes.io/docs/concepts/storage/persistent-volumes/#retain), so I remove ClaimRef
to move it to Available
state(I checked via google, looks it acceptable action that does not do any harm to the cluster).
I think I will check on PV state, if it has Released
state I will remove ClaimRef
, otherwise will skip windows tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I also more like API usage instead of calling to pod exec command, but currently we run tests outside of the cluster and we do not have the possibility to reach VM
images/winrmcli/Dockerfile
Outdated
# Copyright 2018 Red Hat, Inc. | ||
# | ||
|
||
FROM golang:1.6 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check if we have the possibility to bring fedora 27 with go inside
tests/windows_test.go
Outdated
tests.BeforeAll(func() { | ||
windowsPv, err := virtClient.CoreV1().PersistentVolumes().Get(tests.DiskWindows, metav1.GetOptions{}) | ||
if err != nil { | ||
Skip(fmt.Sprintf("Skip Windows tests that requires PVC %s", tests.DiskWindows)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- will create a separate helper
- yes I know, I just wanted to avoid calling to Kube API for each test, it just relevant for all windows tests
@rmohr Do you have some objections vsBeforeAll
, I checkedginkgo
issue and the only objections vs it, was the possibility to run tests in parallel, but for windows testing, we can't run in parallel anyway(without test code changes)
tests/windows_test.go
Outdated
if err != nil { | ||
Skip(fmt.Sprintf("Skip Windows tests that requires PVC %s", tests.DiskWindows)) | ||
} | ||
windowsPv.Spec.ClaimRef = nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume that before testing we already added windows PV to the cluster, and maybe it already used by some PVC(currently not), so the PV will have the state Released
, but new PVC still cannot use it(https://kubernetes.io/docs/concepts/storage/persistent-volumes/#retain), so I remove ClaimRef
to move it to Available
state(I checked via google, looks it acceptable action that does not do any harm to the cluster).
I think I will check on PV state, if it has Released
state I will remove ClaimRef
, otherwise will skip windows tests.
0454222
to
1107854
Compare
Passed fine on my environment 😄
|
1107854
to
7c2a559
Compare
7c2a559
to
e3482cc
Compare
I ran it on nested environment and it looks pretty good 😄
Will see how does it will work on CI. |
@rmohr I ran it on CI host and unexpectedly it all passed with pretty good times(almost the same as on my environment) 😄 |
retest this please |
6219815
to
39e342e
Compare
39e342e
to
7a751a2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor changes requests.
Expect(output).Should(ContainSubstring(strings.ToUpper(windowsFirmware))) | ||
}, 360) | ||
|
||
It("should have pod IP", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of fetching the pod, you can simply wait for the vm to run and look up the ip from its status.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tests/windows_test.go
Outdated
tests.WaitForSuccessfulVMStartWithTimeout(vm, 180) | ||
}, 300) | ||
|
||
It("should success to stop", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It("should succeed to stop a running vm"), By("Strating the vm"), By ("Stopping the vm")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tests/windows_test.go
Outdated
windowsVm.Spec = windowsVmSpec | ||
}) | ||
|
||
It("should success to start", func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It("should succeed to start a vm")
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
tests/windows_test.go
Outdated
var cli []string | ||
var output string | ||
|
||
BeforeEach(func() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok I am convinced now that the BeforeAll which we have should also be good enough in this case.
7a751a2
to
a6bfeb8
Compare
To test Windows VM we need to create additional pod that will have winrm-cli tool. Tests: - Start Windows VM - Stop Windows VM - Verify that Windows VM has correct UUID - Verify that Windows VM has pod IP Signed-off-by: Lukianov Artyom <alukiano@redhat.com>
a6bfeb8
to
9692f91
Compare
Signed-off-by: Lukianov Artyom <alukiano@redhat.com>
9692f91
to
428b051
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good now. Once the windows image is in place again we can rerun a last time and then merge.
Adding code to not allow importing from registry when contenType is not kubevirt
Requirments for Windows VM testing:
disk-windows
PVC that bounded to some PVwinrm-cli
toolTests:
Signed-off-by: Lukianov Artyom alukiano@redhat.com