test(vdsnapshot): disable filesystem frozen check#981
Conversation
eofff
force-pushed
the
test/vdsnapshot/disable-fs-frozen-check
branch
2 times, most recently
from
9aefeb1 to
b320205
Compare
eofff
force-pushed
the
test/vdsnapshot/disable-fs-frozen-check
branch
from
b320205 to
b6ced4e
Compare
| watcher, err := virtClient.VirtualMachines(conf.Namespace).Watch(context.Background(), v1.ListOptions{TimeoutSeconds: ptr.To(int64(filesystemReadyTimeout.Seconds()))}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| var hasFrozen bool | ||
| for { |
There was a problem hiding this comment.
for {
select {
case <-time.After(5 * time.Minute):
panic ....
}
}There was a problem hiding this comment.
We already have timeout in ListOptions
|
|
||
| watcher, err := virtClient.VirtualMachines(conf.Namespace).Watch(context.Background(), v1.ListOptions{TimeoutSeconds: ptr.To(int64(filesystemReadyTimeout.Seconds()))}) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| var hasFrozen bool |
There was a problem hiding this comment.
After fix problem, we will use it state outer for
| break | ||
| } | ||
|
|
||
| vm, ok := event.Object.(*virtv2.VirtualMachine) |
There was a problem hiding this comment.
Is it old or new object?
There was a problem hiding this comment.
eofff
force-pushed
the
test/vdsnapshot/disable-fs-frozen-check
branch
from
464536b to
deaa7aa
Compare
Signed-off-by: Valeriy Khorunzhin <valeriy.khorunzhin@flant.com>
hardcoretime
left a comment
hardcoretime
left a comment
There was a problem hiding this comment.
I think we should uncomment this check because the test does not check that the snapshot becomes ready.
virtualization/tests/e2e/vd_snapshots_test.go
Line 545 in 48bdf48
Signed-off-by: Valeriy Khorunzhin <valeriy.khorunzhin@flant.com>
|
Workflow has started. The target step completed with status: success. |
| for { | ||
| hasFrozen = false | ||
| for _, frozen := range vmFSFrozenByName { | ||
| hasFrozen = hasFrozen || frozen |
There was a problem hiding this comment.
hasFrozen is always false here; you unconditionally set it to false above
There was a problem hiding this comment.
Why do you need to iterate all the virtual machines if you've already found the frozen one? Why not just use a break for the for loop at line 249?
|
|
||
| vm, ok := event.Object.(*virtv2.VirtualMachine) | ||
| if !ok { | ||
| continue |
There was a problem hiding this comment.
This will result in an infinite loop in case of an error. If something goes wrong, throw an error in the test
| } | ||
| _, err := WaitForVMsUnfrozen(int64(filesystemReadyTimeout.Seconds())) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| // TODO: It is a known issue that VM filesystems after snapshot not unfreezing. To prevent this error from causing noise during testing, we disabled this check. It will need to be re-enabled once this issue is fixed. |
There was a problem hiding this comment.
t seems like the issue has been fixed, it doesn't throw anymore. Let's restore the check.
| filesystemReadyPollingInterval, | ||
| ).Should(Succeed()) | ||
| } | ||
| _, err := WaitForVMsUnfrozen(int64(filesystemReadyTimeout.Seconds())) |
There was a problem hiding this comment.
In fact, it's much more important for us to check the freezing event during the snapshot process rather than the unfreezing at the end of the test through Watch.
7 participants
Description
It is a known issue that filesystem not always freeze while vd snapshot tests.
To prevent this error from causing noise during testing, we disabled this check.
It will need to be re-enabled once this issue is fixed.
Why do we need it, and what problem does it solve?
What is the expected result?
Checklist
Changelog entries