Conformance headless #12990
Conformance headless #12990
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
kubevirt-bot
added
the
release-note-none
kubevirt-bot
added
the
dco-signoff: yes
| It("[Serial] multiple HTTP calls should re-use connections and not grow the number of open connections", Serial, func() { | ||
|
|
||
| getHandlerConnectionCount := func(nodeName string) int { | ||
| cmd := []string{"bash", "-c", fmt.Sprintf("ss -ntlap | grep %d | wc -l", virt_api.DefaultConsoleServerPort)} |
There was a problem hiding this comment.
nit: this would fail if an unrelated process has 8186 in its name or listens to 18186 or if virt-handler is configured to use non-default port.
There was a problem hiding this comment.
Right and is why I don't include it in Conformance set. Regarding the other process listening, I doubt as we should have our own net ns. But the non-default port is true case.
| * | ||
| */ | ||
|
|
||
| package tests_test |
There was a problem hiding this comment.
This PR is only moving an existing test, but still:
virt-handler is not part of the KubeVirt API, so testing that it does not leak a specific kind of TCP connections does not really belong in an end-to-end test.
Can this be generalized to check that kubevirt components do not leak sockets/memory/files/whatnot during the entire suite?
If you decide to keep it here, can you place it under its own package? tests_test is a generic and misleading name.
There was a problem hiding this comment.
This test was added as part of found regression and I marked it as such. I believe it is useful for the project but I agree it is not really e2e. I think we need to bring this up this discussion on how we want to group tests... but I like to keep it out of this PR
There was a problem hiding this comment.
The test may be useful to protect against a recurrence of a specific regression, but it is not a good test. It has a very strong dependency on implementation details and leaves out other leaks untested.
Splitting tests to sensible packages and owning teams urgently needed - but it can wait form another PR.
There was a problem hiding this comment.
but it can wait form another PR.
Thanks
kubevirt-bot
added
the
needs-rebase
Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>
Unit test "and not add the graphics and video device if it is set to false on amd64" in converter_test.go is covering this scenario. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>
These tests don't relate to Headless VMs Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>
1. The test name is missleading as Pod should have more memory if it has graphic device, not the other way around. 2. The test is covered by unit test in template_test.go "should check autoattachGraphicsDevicse and consider graphics overhead if it is not set on amd64" and other variants. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>
The test showcase expected behavior for user therefore let's cover it in Conformance. As part of this commit we also merge the console test as these tests are not cheap. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>
This e2e is only counting number of vncs. This can be tested by unit test. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>
The test "multiple HTTP calls should re-use connections and not grow the number of open connections" doesn't have correlation with headless VMs. Therefore move it to its own file. Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>
Signed-off-by: Luboslav Pivarc <lpivarc@redhat.com>
xpivarc
force-pushed
the
conformance-headless
branch
from
f4b9eb0
to
57fde4d
Compare
kubevirt-bot
removed
the
needs-rebase
|
@xpivarc: The following test failed, say
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-sigs/prow repository. I understand the commands that are listed here. |
|
|
||
| memDiff := normalComputeContainer.Resources.Requests.Memory() | ||
| memDiff.Sub(*computeContainer.Resources.Requests.Memory()) | ||
| It("[test_id:738][posneg:negative]should not connect to VNC but still be able to connect to console", decorators.Conformance, func() { |
There was a problem hiding this comment.
sorry for not thinking of this in my first review, but I don't think this merits inclusion in decorators.Conformance. We already unittest that no vnc device is passed to libvirt. I'm not sure we must test libvirt not to invent a graphic device, or test that Alpine boots without a graphic device. But I am certain it does not belong in our slim, do-or-die, conformance suite.
I believe that every test that is added to Conformance should have a strong reason described in the commit message. The current explanation
The test showcase expected behavior for
user therefore let's cover it in Conformance.
can be applied to every proper e2e test.
There was a problem hiding this comment.
Hmm, I must confess I haven't think about this in this way. I was more focused on the fact that we would cover the expected behavior when user defines VMI with
...
autoattachGraphicsDevice : false
...
so at least I should change the reasoning...
I believe that every test that is added to Conformance should have a strong reason described in the commit message.
Agree, I think generally the Conformance test should cover the expected behavior of the API (this test would therefore fit), thinks I don't see as such are:
- Every permutation
- Edge cases
- Negative tests or more what the API doesn't mean, in other words we should define the API in terms of what it does and not what it doesn't do...
Would you keep it with the reason of covering the expected behavior of the API field?
There was a problem hiding this comment.
Would you keep it with the reason of covering the expected behavior of the API field?
I really don't like the fact that our API has defaults and the flags to disable the defaults, and defaults for the flags. It's two complex for humans to follow.
IMO autoattachGraphicsDevice=false is an edge case, that is properly tested by unit tests. The code here tests the the API does not create a device. I see no urgent need to pay time, electricity and labor to test it in every time we run Compliance.
What this PR does
1 .Covers headless tests in Conformance tests
2. Removes tests that don't bring value
3. Converts e2e tests to unit tests
4. Make sure that only relevant tests are in the vmi_headless_test.go
Fixes #
Special notes for your reviewer
Release note