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

Add vlan/macvlan/ipvlan incontainer master tests #27700

Merged
merged 1 commit into from
Mar 1, 2023

Conversation

mmirecki
Copy link

@mmirecki mmirecki commented Feb 1, 2023

test/extended/networking/vlan.go Outdated Show resolved Hide resolved
@dougbtv
Copy link
Member

dougbtv commented Feb 15, 2023

/retest-required

test/extended/networking/vlan.go Outdated Show resolved Hide resolved
test/extended/networking/vlan.go Show resolved Hide resolved
test/extended/networking/vlan.go Show resolved Hide resolved
@mmirecki mmirecki force-pushed the incontainer_vlan branch 2 times, most recently from dc34afc to a928a3e Compare February 20, 2023 11:35
pod.Spec.Containers[0].Command = []string{"/bin/bash", "-c", fmt.Sprintf("ping -c 1 %s", ip1)}
})

g.By("having another pod on different vlan unanble to ping the first pod")

Choose a reason for hiding this comment

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

Suggested change
g.By("having another pod on different vlan unanble to ping the first pod")
g.By("having another pod on different vlan unable to ping the first pod")

Copy link
Contributor

Choose a reason for hiding this comment

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

@mmirecki looks like this nit got missed.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @bparees, I changed it. (@mmirecki is leaving RH and I'm taking over this PR).

@mmirecki
Copy link
Author

/retest-required

if err != nil {
return false, err
}
return retrievedPod.Status.ContainerStatuses[0].Ready == false, nil
Copy link
Member

Choose a reason for hiding this comment

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

there is a panic here

Copy link
Member

Choose a reason for hiding this comment

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

STEP: having another pod on different vlan unanble to ping the first pod 02/20/23 22:04:26.144
E0220 22:04:26.297917 58812 runtime.go:79] Observed a panic: runtime.boundsError{x:0, y:0, signed:true, code:0x0} (runtime error: index out of range [0] with length 0)
goroutine 766 [running]:
k8s.io/apimachinery/pkg/util/runtime.logPanic({0x825abe0?, 0xc0048cf980})
k8s.io/apimachinery@v0.25.0/pkg/util/runtime/runtime.go:75 +0x99
k8s.io/apimachinery/pkg/util/runtime.HandleCrash({0x0, 0x0, 0x0?})
k8s.io/apimachinery@v0.25.0/pkg/util/runtime/runtime.go:49 +0x75
panic({0x825abe0, 0xc0048cf980})
runtime/panic.go:884 +0x212
github.com/openshift/origin/test/extended/networking.glob..func14.1.3()
github.com/openshift/origin/test/extended/networking/vlan.go:129 +0xd5
k8s.io/apimachinery/pkg/util/wait.ConditionFunc.WithContext.func1({0x18, 0xc0000d6c00})
k8s.io/apimachinery@v0.25.0/pkg/util/wait/wait.go:222 +0x1b
k8s.io/apimachinery/pkg/util/wait.runConditionWithCrashProtectionWithContext({0x9462ed8?, 0xc00007e130?}, 0xc004803c28?)
k8s.io/apimachinery@v0.25.0/pkg/util/wait/wait.go:235 +0x57
k8s.io/apimachinery/pkg/util/wait.poll({0x9462ed8, 0xc00007e130}, 0x68?,

Copy link
Author

Choose a reason for hiding this comment

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

The test probably executes before the container is created.
Adding a check for len of containers.

@mlguerrero12
Copy link
Member

@deads2k, @bparees, could you please review/approve this pr?

Signed-off-by: mmirecki <mmirecki@redhat.com>
@bparees
Copy link
Contributor

bparees commented Feb 28, 2023

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2023
@mlguerrero12
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 1, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 1, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees, cgoncalves, mlguerrero12, mmirecki

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Mar 1, 2023

@mmirecki: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-metal-ipi-sdn 5c0c9a4 link false /test e2e-metal-ipi-sdn
ci/prow/e2e-aws-ovn-single-node-upgrade 5c0c9a4 link false /test e2e-aws-ovn-single-node-upgrade
ci/prow/e2e-aws-ovn-single-node-serial 5c0c9a4 link false /test e2e-aws-ovn-single-node-serial
ci/prow/e2e-aws-ovn-single-node 5c0c9a4 link false /test e2e-aws-ovn-single-node
ci/prow/e2e-aws-ovn-upgrade 5c0c9a4 link false /test e2e-aws-ovn-upgrade
ci/prow/e2e-azure-ovn-etcd-scaling 5c0c9a4 link false /test e2e-azure-ovn-etcd-scaling
ci/prow/e2e-vsphere-ovn-etcd-scaling 5c0c9a4 link false /test e2e-vsphere-ovn-etcd-scaling

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

var _ = g.Describe("[sig-network][Feature:vlan]", func() {
oc := exutil.NewCLI("vlan")
f := oc.KubeFramework()
for _, pluginType := range []string{"vlan", "ipvlan", "macvlan"} {
Copy link
Contributor

Choose a reason for hiding this comment

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

@mmirecki You can still use a for loop for the acutal testcase logic, but don't for-loop the setup. Do that with BeforeEach()/AfterEach() so that things get cleaned up correctly too.

Anything that's common between the tests can go into the BeforeEach() and be a variable declared inside the top-level Describe(). BeforeEach() should also generate a unique bridge name to pass to the CNI plugin. Be a bit careful here of parallel tests since each spec can run in parallel and variables sometimes get evaluated before the test is actually run; we can handle that in the resubmit PR review though.

AfterEach() needs to clean up anything the test creates in the kube API, like all the attachment definitions if they exist, so that errors that stop the test in the middle don't leave them around.

podName1, podName2, podName3 := "pod1", "pod2", "pod3"
vlan1, vlan2, otherVlan := "vlan01", "vlan02", "vlan03"
ip1, ip2, otherIp := "10.10.0.2", "10.10.0.3", "10.10.0.4"
bridge := "br0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Per above, this needs to be auto-generated as a unique name in the BeforeEach() so that each test uses a different name.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants