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

Timeout on setup nodes in ginkgo v2 #882

Closed
mgoerens opened this issue Jan 13, 2022 · 7 comments
Closed

Timeout on setup nodes in ginkgo v2 #882

mgoerens opened this issue Jan 13, 2022 · 7 comments

Comments

@mgoerens
Copy link

I've upgraded my project to ginkgo v2 today.

With ginkgo v1.x, I could add a 60s timeout to BeforeSuite (among other setup nodes) like this:

var _ = BeforeSuite(func() {
    # PrepareSpec
}, 60)

Is there a way to achieve the same behavior in ginkgo v2 ?

@blgm
Copy link
Collaborator

blgm commented Jan 13, 2022

Hi @mgoerens. My guess (which may be corrected by @onsi later) is that this relates to the removal of async tests, with the recommendation to use async assertions instead: https://onsi.github.io/ginkgo/MIGRATING_TO_V2#removed-async-testing

@onsi
Copy link
Owner

onsi commented Jan 13, 2022

That's right @blgm - @mgoerens please take a look at the section of the migration doc George linked to. There's also a deeper dive into async testing patterns in the documentation here.

If you run into issues let us know and we can help further. In fact if you can point me at your code (if it's open source) i can make some concrete suggestions on how to migrate things beyond the simple first-pass shared in the migration doc.

@mgoerens
Copy link
Author

Thanks a lot for your answers. I had another look at the docs.

Of course, the reality is a bit more complex than my very simple code snippet :) I'm using operator-sdk (https://github.com/operator-framework/operator-sdk/) to build a go-based Kubernetes Operator. operator-sdk scaffolds a base project, including a minimal suite_test.go, using ginkgo 1.x, and using the deprecated timeout feature on BeforeSuite. Here is an example of a scaffolded project without any further customization: https://github.com/opdev/synapse-operator/blob/89f06097cffe2f3feb710891118663eed4aa1ed5/controllers/synapse/suite_test.go

Note that this is for integration tests, and it starts a test environment using envTest.

I'm not a developer of operator-sdk, so I can only guess the logic behind the 60s timeout. Most likely the intention is to make sure that tests setup (the whole BeforeSuite node), doesn't take more than 60s. Hence my original question.

The actual line which is likely to hang is probably cfg, err := testEnv.Start() so I could add a timeout for this line only if that's easier. Right now I simply fixed the error by removing the timeout altogether.

@onsi
Copy link
Owner

onsi commented Jan 17, 2022

hey @mgoerens will reply more when i'm at my computer. for now just wanted to let you know that github link 404s. any chance you have a public repo i can look at?

@mgoerens
Copy link
Author

Right, forgot this was still private. I set the visibility of the repository to public now, as it should be.

@onsi
Copy link
Owner

onsi commented Jan 17, 2022

Thanks for opening the repo up. It looks like the original code you've linked to wasn't doing what folks expect. In Ginkgo V1 the timeout only applies if the function passed to BeforeSuite accepts a Done channel argument. The code in the repo you linked to does not - which basically means it was running synchronously and the time out wasn't being enforced.

So, you can just drop the 60 and move on. In fact, I dug in a bit deeper and it looks like testEnv.Start() includes some internal timeouts to guard against things taking too long. I believe it'll return an error if the timeout elapses.

If you do want to wrap your own timeout around testEnv.Start() you can:

var _ = BeforeSuite(func() {
	logf.SetLogger(zap.New(zap.WriteTo(GinkgoWriter), zap.UseDevMode(true)))

	By("bootstrapping test environment")
	testEnv = &envtest.Environment{
		CRDDirectoryPaths:     []string{filepath.Join("..", "..", "config", "crd", "bases")},
		ErrorIfCRDPathMissing: true,
	}

        var cfg *rest.Config
        var err error
        done := make(chan interface{}, 0)
        go func() {
            defer GinkgoRecover()
	    cfg, err = testEnv.Start()
            close(done)
        }()
        Eventually(done).WithTimeout(time.Minute).Should(BeClosed())
	Expect(err).NotTo(HaveOccurred())
	Expect(cfg).NotTo(BeNil())
       
	err = synapsev1alpha1.AddToScheme(scheme.Scheme)
	Expect(err).NotTo(HaveOccurred())

	//+kubebuilder:scaffold:scheme

	k8sClient, err = client.New(cfg, client.Options{Scheme: scheme.Scheme})
	Expect(err).NotTo(HaveOccurred())
	Expect(k8sClient).NotTo(BeNil())

})

@mgoerens
Copy link
Author

Great infos, thanks a lot ! There's nothing else to add I think :)

maxdrib added a commit to kubernetes-sigs/cluster-api-provider-cloudstack that referenced this issue May 11, 2022
aws-paris-bot pushed a commit to kubernetes-sigs/cluster-api-provider-cloudstack that referenced this issue May 11, 2022
* Bumping ginkgo to v2 and adding codecov github action

* Generating coverage in e2e test

* Fixing test module go.mod and go.sum

* Merging new library

* Removing junit reporter

* Removing deprecated ginkgo argument

* Splitting tests to use ginkgo v2 for unit tests and v1 for e2e tests

* Minor fixups and linting

* more linting

* more linting

* Fixing spacing

* Correcting timeout to align with ginkgo v2 as discussed in onsi/ginkgo#882
liornoy added a commit to liornoy/metallb-operator that referenced this issue Nov 22, 2022
Bump Ginkgo's import and refactor the test suite.
GinkgoV2 donesn't have built-in timeout param in the BeforeSuite,
so to keep having a timeout limit, using a "done" channel as
advised here: onsi/ginkgo#882 (comment)
liornoy added a commit to liornoy/metallb-operator that referenced this issue Nov 22, 2022
Bump Ginkgo's import and refactor the test suite.
GinkgoV2 donesn't have built-in timeout param in the BeforeSuite,
so to keep having a timeout limit, using a "done" channel as
advised here: onsi/ginkgo#882 (comment)
ShyamsundarR added a commit to ShyamsundarR/ramen that referenced this issue Apr 3, 2023
Required a small change in BeforeSuite to remove
the timeout, and use a channel instead.

As discussed here: onsi/ginkgo#882

Signed-off-by: Shyamsundar Ranganathan <srangana@redhat.com>
ShyamsundarR added a commit to ShyamsundarR/ramen that referenced this issue Apr 14, 2023
Required a small change in BeforeSuite to remove
the timeout, and use a channel instead.

As discussed here: onsi/ginkgo#882

Signed-off-by: Shyamsundar Ranganathan <srangana@redhat.com>
ShyamsundarR added a commit to RamenDR/ramen that referenced this issue Apr 17, 2023
Required a small change in BeforeSuite to remove
the timeout, and use a channel instead.

As discussed here: onsi/ginkgo#882

Signed-off-by: Shyamsundar Ranganathan <srangana@redhat.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 5, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 16, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 16, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 16, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 16, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 16, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 17, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 18, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
heanlan added a commit to heanlan/antrea that referenced this issue May 18, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
tnqn pushed a commit to antrea-io/antrea that referenced this issue May 19, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext
* make codegen and manifests
* allow access from container users to git directories

Related to #4901

Signed-off-by: heanlan <hanlan@vmware.com>
ceclinux pushed a commit to ceclinux/antrea that referenced this issue Jun 5, 2023
* k8s.io/apimachinery/pkg/util/sets.String was deprecated, and replaced with
  sets.Set[string]. sets.String.List() was replaced with sets.List(sets.Set[string]).
  Similar changes happened on sets.Int32, sets.Int64
* k8s.io/apiserver/pkg/registry/rest.Storage interface added requirement on
  implementation of Destroy() method
* k8s.io/component-base/metrics/prometheus/ratelimiter was removed as it was
  not doing anything. The call in third_party/ipam/nodeipam was removed
  accordingly. Refer to PR: kubernetes/kubernetes#113054
* upgraded ginkgo to v2 as required by sigs.k8s.io/controller-runtime@v0.14.6.
  timeout was removed from BeforeSuite, and was refactored referencing to
  onsi/ginkgo#882
* k8s.io/client-go/tools/remotecommand.Executor.Stream was deprecated, and
  replaced with StreamWithContext
* make codegen and manifests
* allow access from container users to git directories

Related to antrea-io#4901

Signed-off-by: heanlan <hanlan@vmware.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants