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

Rework Ginkgo usage #9522

Merged
merged 5 commits into from
Feb 16, 2023
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Makefile
Original file line number Diff line number Diff line change
@@ -31,7 +31,7 @@ TAG ?= $(shell cat TAG)

# e2e settings
# Allow limiting the scope of the e2e tests. By default run everything
FOCUS ?= .*
FOCUS ?=
# number of parallel test
E2E_NODES ?= 7
# run e2e test suite with tests that check for memory leaks? (default is false)
1 change: 0 additions & 1 deletion internal/ingress/controller/store/store.go
Original file line number Diff line number Diff line change
@@ -702,7 +702,6 @@ func New(
},
}

// TODO: add e2e test to verify that changes to one or more configmap trigger an update
changeTriggerUpdate := func(name string) bool {
return name == configmap || name == tcp || name == udp
}
2 changes: 1 addition & 1 deletion internal/k8s/main.go
Original file line number Diff line number Diff line change
@@ -78,7 +78,7 @@ func GetNodeIPOrName(kubeClient clientset.Interface, name string, useInternalIP
var (
// IngressPodDetails hold information about the ingress-nginx pod
IngressPodDetails *PodInfo
// IngressNodeDetails old information about the node running ingress-nginx pod
// IngressNodeDetails hold information about the node running ingress-nginx pod
IngressNodeDetails *NodeInfo
)

73 changes: 21 additions & 52 deletions test/e2e-image/e2e.sh
Original file line number Diff line number Diff line change
@@ -14,70 +14,39 @@
# See the License for the specific language governing permissions and
# limitations under the License.

set -e
set -eu

NC='\e[0m'
BGREEN='\e[32m'

#SLOW_E2E_THRESHOLD=${SLOW_E2E_THRESHOLD:-"5s"}
FOCUS=${FOCUS:-.*}
E2E_NODES=${E2E_NODES:-5}
E2E_CHECK_LEAKS=${E2E_CHECK_LEAKS:-""}

reportFile="report-e2e-test-suite.xml"
ginkgo_args=(
"-randomize-all"
"-flake-attempts=2"
"-fail-fast"
"--show-node-events"
"--fail-fast"
"--flake-attempts=2"
"--junit-report=${reportFile}"
"--nodes=${E2E_NODES}"
"--poll-progress-after=180s"
# "-slow-spec-threshold=${SLOW_E2E_THRESHOLD}"
"-succinct"
"-timeout=75m"
"--randomize-all"
"--show-node-events"
"--succinct"
"--timeout=75m"
)

# Variable for the prefix of report filenames
reportFileNamePrefix="report-e2e-test-suite"

echo -e "${BGREEN}Running e2e test suite (FOCUS=${FOCUS})...${NC}"
ginkgo "${ginkgo_args[@]}" \
-focus="${FOCUS}" \
-skip="\[Serial\]|\[MemoryLeak\]|\[TopologyHints\]" \
-nodes="${E2E_NODES}" \
--junit-report=$reportFileNamePrefix.xml \
/e2e.test
# Create configMap out of a compressed report file for extraction later
if [ -n "${FOCUS}" ]; then
ginkgo_args+=("--focus=${FOCUS}")
fi

# Must be isolated, there is a collision if multiple helms tries to install same clusterRole at same time
echo -e "${BGREEN}Running e2e test for topology aware hints...${NC}"
ginkgo "${ginkgo_args[@]}" \
-focus="\[TopologyHints\]" \
-skip="\[Serial\]|\[MemoryLeak\]]" \
-nodes="${E2E_NODES}" \
--junit-report=$reportFileNamePrefix-topology.xml \
/e2e.test
# Create configMap out of a compressed report file for extraction later
if [ -z "${E2E_CHECK_LEAKS}" ]; then
ginkgo_args+=("--skip=\[Memory Leak\]")
fi

echo -e "${BGREEN}Running e2e test suite with tests that require serial execution...${NC}"
ginkgo "${ginkgo_args[@]}" \
-focus="\[Serial\]" \
-skip="\[MemoryLeak\]" \
--junit-report=$reportFileNamePrefix-serial.xml \
/e2e.test
# Create configMap out of a compressed report file for extraction later
echo -e "${BGREEN}Running e2e test suite...${NC}"
(set -x; ginkgo "${ginkgo_args[@]}" /e2e.test)

if [[ ${E2E_CHECK_LEAKS} != "" ]]; then
echo -e "${BGREEN}Running e2e test suite with tests that check for memory leaks...${NC}"
ginkgo "${ginkgo_args[@]}" \
-focus="\[MemoryLeak\]" \
-skip="\[Serial\]" \
--junit-report=$reportFileNamePrefix-memleak.xml \
/e2e.test
# Create configMap out of a compressed report file for extraction later
fi

for rFile in `ls $reportFileNamePrefix*`
do
gzip -k $rFile
kubectl create cm $rFile.gz --from-file $rFile.gz
kubectl label cm $rFile.gz junitreport=true
done
gzip -k ${reportFile}
kubectl create cm ${reportFile}.gz --from-file ${reportFile}.gz
kubectl label cm ${reportFile}.gz junitreport=true
19 changes: 1 addition & 18 deletions test/e2e-image/namespace-overlays/topology/values.yaml
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@ controller:
digest:
digestChroot:
scope:
# Necessary to allow the ingress controller to get the topology information from the nodes
enabled: false
config:
worker-processes: "1"
@@ -19,12 +20,7 @@ controller:
periodSeconds: 1
service:
type: NodePort
electionID: ingress-controller-leader
ingressClassResource:
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason to remove this? IIRC on tests this may generate some conflict as multiple ingresses may try to reconcile it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok I see you remove the charts after it. Maybe we should always remove charts and add this as part of AfterEach :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

any reason to remove this?

as this chart is expected to run alone (Serial), there is no need to disable the non-namespaced objects

Maybe we should always remove charts and add this as part of AfterEach :)

TBH I was thinking about it but finally forgo because that would slightly increase the execution time of the general case where the removal of the ns is enough.
But I'm still leaning towards this idea as this would help to not fall into the trap when the removal of the chart deployment is necessary. Let me know what you think.

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 just read your other comment where you approve the idea ;)

# We will create and remove each IC/ClusterRole/ClusterRoleBinding per test so there's no conflict
enabled: false
extraArgs:
tcp-services-configmap: $NAMESPACE/tcp-services
# e2e tests do not require information about ingress status
update-status: "false"
terminationGracePeriodSeconds: 1
@@ -33,19 +29,6 @@ controller:

enableTopologyAwareRouting: true

# ulimit -c unlimited
# mkdir -p /tmp/coredump
# chmod a+rwx /tmp/coredump
# echo "/tmp/coredump/core.%e.%p.%h.%t" > /proc/sys/kernel/core_pattern
extraVolumeMounts:
- name: coredump
mountPath: /tmp/coredump

extraVolumes:
- name: coredump
hostPath:
path: /tmp/coredump

rbac:
create: true
scope: false
17 changes: 1 addition & 16 deletions test/e2e/admission/admission.go
Original file line number Diff line number Diff line change
@@ -40,19 +40,14 @@ var (
pathImplSpecific = networkingv1.PathTypeImplementationSpecific
)

var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
var _ = framework.IngressNginxDescribeSerial("[Admission] admission controller", func() {
f := framework.NewDefaultFramework("admission")

ginkgo.BeforeEach(func() {
f.NewEchoDeployment()
f.NewSlowEchoDeployment()
})

ginkgo.AfterEach(func() {
err := uninstallChart(f)
assert.Nil(ginkgo.GinkgoT(), err, "uninstalling helm chart")
})

ginkgo.It("reject ingress with global-rate-limit annotations when memcached is not configured", func() {
host := "admission-test"

@@ -252,16 +247,6 @@ var _ = framework.IngressNginxDescribe("[Serial] admission controller", func() {
})
})

func uninstallChart(f *framework.Framework) error {
cmd := exec.Command("helm", "uninstall", "--namespace", f.Namespace, "nginx-ingress")
_, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("unexpected error uninstalling ingress-nginx release: %v", err)
}

return nil
}

const (
validV1Ingress = `
apiVersion: networking.k8s.io/v1
5 changes: 1 addition & 4 deletions test/e2e/annotations/fastcgi.go
Original file line number Diff line number Diff line change
@@ -21,7 +21,6 @@ import (
"strings"

"github.com/onsi/ginkgo/v2"
"github.com/stretchr/testify/assert"
corev1 "k8s.io/api/core/v1"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -81,9 +80,7 @@ var _ = framework.DescribeAnnotation("backend-protocol - FastCGI", func() {
},
}

cm, err := f.EnsureConfigMap(configuration)
assert.Nil(ginkgo.GinkgoT(), err, "creating configmap")
assert.NotNil(ginkgo.GinkgoT(), cm, "expected a configmap but none returned")
f.EnsureConfigMap(configuration)

host := "fastcgi-params-configmap"

19 changes: 1 addition & 18 deletions test/e2e/endpointslices/topology.go
Original file line number Diff line number Diff line change
@@ -21,7 +21,6 @@ import (
"encoding/json"
"fmt"
"net/http"
"os/exec"
"strings"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -33,20 +32,14 @@ import (
"k8s.io/ingress-nginx/test/e2e/framework"
)

var _ = framework.IngressNginxDescribe("[TopologyHints] topology aware routing", func() {
var _ = framework.IngressNginxDescribeSerial("[TopologyHints] topology aware routing", func() {
f := framework.NewDefaultFramework("topology")
host := "topology-svc.foo.com"

ginkgo.BeforeEach(func() {
f.NewEchoDeployment(framework.WithDeploymentReplicas(2), framework.WithSvcTopologyAnnotations())
})

ginkgo.AfterEach(func() {
// we need to uninstall chart because of clusterRole which is not destroyed with namespace
err := uninstallChart(f)
assert.Nil(ginkgo.GinkgoT(), err, "uninstalling helm chart")
})

ginkgo.It("should return 200 when service has topology hints", func() {

annotations := make(map[string]string)
@@ -100,13 +93,3 @@ var _ = framework.IngressNginxDescribe("[TopologyHints] topology aware routing",
}
})
})

func uninstallChart(f *framework.Framework) error {
cmd := exec.Command("helm", "uninstall", "--namespace", f.Namespace, "nginx-ingress")
_, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("unexpected error uninstalling ingress-nginx release: %v", err)
}

return nil
}
10 changes: 10 additions & 0 deletions test/e2e/framework/exec.go
Original file line number Diff line number Diff line change
@@ -152,6 +152,16 @@ func (f *Framework) KubectlProxy(port int) (int, *exec.Cmd, error) {
return -1, cmd, fmt.Errorf("failed to parse port from proxy stdout: %s", output)
}

func (f *Framework) UninstallChart() error {
cmd := exec.Command("helm", "uninstall", "--namespace", f.Namespace, "nginx-ingress")
_, err := cmd.CombinedOutput()
if err != nil {
return fmt.Errorf("unexpected error uninstalling ingress-nginx release: %v", err)
}

return nil
}

func startCmdAndStreamOutput(cmd *exec.Cmd) (stdout, stderr io.ReadCloser, err error) {
stdout, err = cmd.StdoutPipe()
if err != nil {
18 changes: 11 additions & 7 deletions test/e2e/framework/framework.go
Original file line number Diff line number Diff line change
@@ -150,7 +150,11 @@ func (f *Framework) AfterEach() {

defer func(kubeClient kubernetes.Interface, ingressclass string) {
defer ginkgo.GinkgoRecover()
err := deleteIngressClass(kubeClient, ingressclass)

err := f.UninstallChart()
assert.Nil(ginkgo.GinkgoT(), err, "uninstalling helm chart")

err = deleteIngressClass(kubeClient, ingressclass)
assert.Nil(ginkgo.GinkgoT(), err, "deleting IngressClass")
}(f.KubeClientSet, f.IngressClass)

@@ -192,6 +196,11 @@ func IngressNginxDescribe(text string, body func()) bool {
return ginkgo.Describe(text, body)
}

// IngressNginxDescribeSerial wrapper function for ginkgo describe. Adds namespacing.
func IngressNginxDescribeSerial(text string, body func()) bool {
return ginkgo.Describe(text, ginkgo.Serial, body)
}

// DescribeAnnotation wrapper function for ginkgo describe. Adds namespacing.
func DescribeAnnotation(text string, body func()) bool {
return ginkgo.Describe("[Annotations] "+text, body)
@@ -202,11 +211,6 @@ func DescribeSetting(text string, body func()) bool {
return ginkgo.Describe("[Setting] "+text, body)
}

// MemoryLeakIt is wrapper function for ginkgo It. Adds "[MemoryLeak]" tag and makes static analysis easier.
func MemoryLeakIt(text string, body interface{}) bool {
return ginkgo.It(text+" [MemoryLeak]", body)
}

// GetNginxIP returns the number of TCP port where NGINX is running
func (f *Framework) GetNginxIP() string {
s, err := f.KubeClientSet.
@@ -387,7 +391,7 @@ func (f *Framework) UpdateNginxConfigMapData(key string, value string) {
}

// WaitForReload calls the passed function and
// asser it has caused at least 1 reload.
// asserts it has caused at least 1 reload.
func (f *Framework) WaitForReload(fn func()) {
initialReloadCount := getReloadCount(f.pod, f.Namespace, f.KubeClientSet)

6 changes: 2 additions & 4 deletions test/e2e/framework/influxdb.go
Original file line number Diff line number Diff line change
@@ -68,9 +68,7 @@ func (f *Framework) NewInfluxDBDeployment() {
},
}

cm, err := f.EnsureConfigMap(configuration)
assert.Nil(ginkgo.GinkgoT(), err, "creating an Influxdb deployment")
assert.NotNil(ginkgo.GinkgoT(), cm, "expected a configmap but none returned")
f.EnsureConfigMap(configuration)

deployment := &appsv1.Deployment{
ObjectMeta: metav1.ObjectMeta{
@@ -136,7 +134,7 @@ func (f *Framework) NewInfluxDBDeployment() {

d := f.EnsureDeployment(deployment)

err = waitForPodsReady(f.KubeClientSet, DefaultTimeout, 1, f.Namespace, metav1.ListOptions{
err := waitForPodsReady(f.KubeClientSet, DefaultTimeout, 1, f.Namespace, metav1.ListOptions{
LabelSelector: fields.SelectorFromSet(fields.Set(d.Spec.Template.ObjectMeta.Labels)).String(),
})
assert.Nil(ginkgo.GinkgoT(), err, "waiting for influxdb pod to become ready")
Loading