Rework Ginkgo usage #9522

dud225 · 2023-01-18T14:54:50Z

Currently Ginkgo is launched multiple times with different options to
accomodate various use-cases. In particular, some specs needs to be run
sequentially because non-namespaced objects are created that conflicts with
concurent Helm deployments. However Ginkgo is able to handle such cases
natively, in particular specs that needs to be run sequentially are
supported (Serial spec).

This change marks the specs that needs to be run sequentially as Serial
specs and runs the whole test suite from a single Ginkgo invocation. As a
result, a single JUnit report is now generated.

As part of this change, the following controller error in the TopologyHints
test is fixed:

Error getting ConfigMap "$NAMESPACE/tcp-services": no object matching key
"$NAMESPACE/tcp-services" in local store

In addition, "go get" invocations are being replaced by "go install" because
the former performs changes in the go.sum and go.mod files.

k8s-ci-robot · 2023-01-18T14:54:58Z

@dud225: This issue is currently awaiting triage.

If Ingress contributors determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

k8s-ci-robot · 2023-01-18T14:54:59Z

Hi @dud225. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

tao12345666333

Thanks, I'm going to trigger the test, but I haven't looked at the specific changes yet.

/ok-to-test

rikatz · 2023-01-19T14:38:39Z

test/e2e-image/namespace-overlays/topology/values.yaml

@@ -19,12 +20,7 @@ controller:
    periodSeconds: 1
  service:
    type: NodePort
-  electionID: ingress-controller-leader
-  ingressClassResource:


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

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

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.

I just read your other comment where you approve the idea ;)

rikatz · 2023-01-19T14:41:41Z

@dud225 this is an amazing change, thank you very much!! We really need a bunch on improvements on how we use ginkgo.

This one is approved to me, as soon as you can just answer about the uninstallChart as part of the AfterEach, if it does make sense, and if it does, push a new commit we can merge it.

/approve

dud225 · 2023-01-19T16:24:46Z

@dud225 this is an amazing change, thank you very much!! We really need a bunch on improvements on how we use ginkgo.

This one is approved to me, as soon as you can just answer about the uninstallChart as part of the AfterEach, if it does make sense, and if it does, push a new commit we can merge it.

Thanks for the compliment.
I've updated the test suite to clean out the Helm deployment systematically.

longwuyuan · 2023-01-20T02:08:35Z

@dud225 Can you please check the failed CI
@rikatz hopefully we can merge this before the break

dud225 · 2023-01-20T07:53:53Z

Hello
to me it's a transient error with no relation whatsoever with the change:

Run actions/download-artifact@9782bd6a9848b53b110e712e20e42d89988822b7
Starting download for docker.tar.gz
Error: Unable to find any artifacts for the associated workflow

In addition there are different flavour of this particular job targetting the last Kubernetes versions which all running fine.
In any case I don't have the permission to launch again the failed job.

So I think that this PR can be merged.

Currently Ginkgo is launched multiple times with different options to accomodate various use-cases. In particular, some specs needs to be run sequentially because non-namespaced objects are created that conflicts with concurent Helm deployments. However Ginkgo is able to handle such cases natively, in particular specs that needs to be run sequentially are supported (Serial spec). This commit marks the specs that needs to be run sequentially as Serial specs and runs the whole test suite from a single Ginkgo invocation. As a result, a single JUnit report is now generated. Signed-off-by: Hervé Werner <[email protected]>

Error getting ConfigMap "$NAMESPACE/tcp-services": no object matching key "$NAMESPACE/tcp-services" in local store Signed-off-by: Hervé Werner <[email protected]>

Executing "go get" changes the go.mod & go.sum files which is not the case of "go install". Signed-off-by: Hervé Werner <[email protected]>

Signed-off-by: Hervé Werner <[email protected]>

…an update Signed-off-by: Hervé Werner <[email protected]>

rikatz · 2023-02-16T14:10:17Z

/lgtm
/approve
Thank you

k8s-ci-robot · 2023-02-16T14:10:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dud225, rikatz

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:

~~OWNERS~~ [rikatz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 18, 2023

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 18, 2023

k8s-ci-robot added needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority labels Jan 18, 2023

k8s-ci-robot requested review from ElvinEfendi and rikatz January 18, 2023 14:55

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 18, 2023

tao12345666333 reviewed Jan 18, 2023

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 18, 2023

rikatz reviewed Jan 19, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 19, 2023

dud225 requested review from rikatz and removed request for ElvinEfendi January 30, 2023 08:09

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2023

dud225 added 4 commits February 1, 2023 09:48

Fix controller error in test

be9fb6b

Error getting ConfigMap "$NAMESPACE/tcp-services": no object matching key "$NAMESPACE/tcp-services" in local store Signed-off-by: Hervé Werner <[email protected]>

Replace "go get" invocations by "go install"

3805e09

Executing "go get" changes the go.mod & go.sum files which is not the case of "go install". Signed-off-by: Hervé Werner <[email protected]>

Always clean out the Helm deployment

ad81bf5

Signed-off-by: Hervé Werner <[email protected]>

dud225 force-pushed the ginkgo branch from 7a3427c to 36e7354 Compare February 1, 2023 08:50

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 1, 2023

Add E2E test to verify that changes to one or more configmap trigger …

74d73ff

…an update Signed-off-by: Hervé Werner <[email protected]>

dud225 force-pushed the ginkgo branch from 36e7354 to 74d73ff Compare February 1, 2023 10:39

k8s-ci-robot assigned rikatz Feb 16, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 16, 2023

k8s-ci-robot merged commit d6bba85 into kubernetes:main Feb 16, 2023

dud225 mentioned this pull request Apr 25, 2023

Add E2E test to verify that changes to one or more configmap trigger an update #9530

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Ginkgo usage #9522

Rework Ginkgo usage #9522

dud225 commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

tao12345666333 left a comment

rikatz Jan 19, 2023

rikatz Jan 19, 2023

dud225 Jan 19, 2023

dud225 Jan 19, 2023

rikatz commented Jan 19, 2023

dud225 commented Jan 19, 2023

longwuyuan commented Jan 20, 2023

dud225 commented Jan 20, 2023 •

edited

Loading

rikatz commented Feb 16, 2023

k8s-ci-robot commented Feb 16, 2023

Rework Ginkgo usage #9522

Rework Ginkgo usage #9522

Conversation

dud225 commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

k8s-ci-robot commented Jan 18, 2023

tao12345666333 left a comment

Choose a reason for hiding this comment

rikatz Jan 19, 2023

Choose a reason for hiding this comment

rikatz Jan 19, 2023

Choose a reason for hiding this comment

dud225 Jan 19, 2023

Choose a reason for hiding this comment

dud225 Jan 19, 2023

Choose a reason for hiding this comment

rikatz commented Jan 19, 2023

dud225 commented Jan 19, 2023

longwuyuan commented Jan 20, 2023

dud225 commented Jan 20, 2023 • edited Loading

rikatz commented Feb 16, 2023

k8s-ci-robot commented Feb 16, 2023

dud225 commented Jan 20, 2023 •

edited

Loading