Skip to content
Draft
Show file tree
Hide file tree
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
1 change: 1 addition & 0 deletions .github/workflows/e2e-ginkgo.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -257,6 +257,7 @@ jobs:
ginkgo-label: "${{ needs.parse_label-filter.outputs.label-filter }} || pr"
additional-args: "--vcluster-image=${{ env.REPOSITORY_NAME }}:${{ env.TAG_NAME }} --teardown=false"
additional-ginkgo-flags: "-v --show-node-events"
timeout: 90m
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The matching timeout: 90m was not mirrored in .github/workflows/e2e-ginkgo-nightly.yaml:117, which still passes timeout: "60m" to run-ginkgo. If the snapshot-large-restore label is ever included in a nightly run (e.g. via workflow_dispatch or once added to a default label set), Ginkgo will abort after 60 minutes — before the three 90-minute Eventually/waitForRequestToFinish calls in test_snapshot.go (lines 894, 916, 938) can complete. Consider bumping the nightly workflow's timeout too, or documenting why it intentionally stays at 60m.

continue-on-error: true

- name: Upload Ginkgo JSON report
Expand Down
1 change: 1 addition & 0 deletions e2e-next/constants/timeouts.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,5 @@ const (
PollingTimeout = time.Second * 60
PollingTimeoutLong = time.Second * 120
PollingTimeoutVeryLong = time.Second * 300
PollingTimeoutExtraLong = time.Minute * 90
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

PollingTimeoutExtraLong (90m) is only used by the new large-restore spec, yet it lives in the shared constants package and jumps 18x from PollingTimeoutVeryLong (5m). Once a generic 90m option exists, future flaky tests are tempted to reach for it rather than diagnose. Consider either keeping it scoped (e.g. a package-local const largeRestoreTimeout = 90 * time.Minute inside e2e-next/test_storage/snapshot/), or renaming to convey scope so reuse outside this scenario is discouraged:

Suggested change
PollingTimeoutExtraLong = time.Minute * 90
SnapshotLargeRestoreTimeout = time.Minute * 90

)
27 changes: 14 additions & 13 deletions e2e-next/labels/labels.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,20 @@ var (
Security = Label("security")

// Resource-specific labels for targeted filtering inside sync tests.
PriorityClasses = Label("priorityclasses")
RuntimeClasses = Label("runtimeclasses")
StorageClasses = Label("storageclasses")
IngressClasses = Label("ingressclasses")
ConfigMaps = Label("configmaps")
Secrets = Label("secrets")
NetworkPolicies = Label("networkpolicies")
Pods = Label("pods")
PVCs = Label("pvcs")
Events = Label("events")
CoreDNS = Label("coredns")
Webhooks = Label("webhooks")
Snapshots = Label("snapshots")
PriorityClasses = Label("priorityclasses")
RuntimeClasses = Label("runtimeclasses")
StorageClasses = Label("storageclasses")
IngressClasses = Label("ingressclasses")
ConfigMaps = Label("configmaps")
Secrets = Label("secrets")
NetworkPolicies = Label("networkpolicies")
Pods = Label("pods")
PVCs = Label("pvcs")
Events = Label("events")
CoreDNS = Label("coredns")
Webhooks = Label("webhooks")
Snapshots = Label("snapshots")
SnapshotLargeRestore = Label("snapshot-large-restore")

// Suite-primary labels (one per opt-in suite).
Scheduler = Label("scheduler")
Expand Down
35 changes: 35 additions & 0 deletions e2e-next/suite_snapshot_large_restore_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
package e2e_next

import (
"context"
_ "embed"

"github.com/loft-sh/e2e-framework/pkg/setup/cluster"
"github.com/loft-sh/vcluster/e2e-next/clusters"
"github.com/loft-sh/vcluster/e2e-next/labels"
"github.com/loft-sh/vcluster/e2e-next/setup"
"github.com/loft-sh/vcluster/e2e-next/setup/lazyvcluster"
"github.com/loft-sh/vcluster/e2e-next/test_storage/snapshot"
. "github.com/onsi/ginkgo/v2"
)

const snapshotLargeRestoreVClusterName = "large-restore-vcluster"

func init() { suiteSnapshotLargeRestore() }

func suiteSnapshotLargeRestore() {
Describe("large-restore-vcluster", labels.SnapshotLargeRestore, Ordered,
cluster.Use(clusters.HostCluster),
func() {
BeforeAll(func(ctx context.Context) context.Context {
return lazyvcluster.LazyVCluster(ctx,
snapshotLargeRestoreVClusterName,
snapshotVClusterYAML,
lazyvcluster.WithPreSetup(setup.SnapshotPreSetup(snapshotLargeRestoreVClusterName)),
)
})

snapshot.SnapshotLargeRestoreSpec()
},
)
}
23 changes: 23 additions & 0 deletions e2e-next/test_storage/snapshot/helpers.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,3 +169,26 @@ func toJSON(obj any) string {
b, _ := json.Marshal(obj)
return string(b)
}

// countConfigMapsByLabel counts all ConfigMaps matching the given label selector in namespace,
// paginating through the full result set to handle large counts.
func countConfigMapsByLabel(ctx context.Context, client kubernetes.Interface, namespace, labelSelector string) (int, error) {
var count int
var continueToken string
for {
list, err := client.CoreV1().ConfigMaps(namespace).List(ctx, metav1.ListOptions{
LabelSelector: labelSelector,
Limit: 1000,
Continue: continueToken,
})
if err != nil {
return 0, err
}
count += len(list.Items)
if list.Continue == "" {
break
}
continueToken = list.Continue
}
return count, nil
}
160 changes: 160 additions & 0 deletions e2e-next/test_storage/snapshot/test_snapshot.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@ import (
"strings"
"time"

"golang.org/x/sync/errgroup"

"github.com/ghodss/yaml"
snapshotsv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/clientset/versioned"
"github.com/loft-sh/e2e-framework/pkg/setup/cluster"
Expand Down Expand Up @@ -800,6 +802,164 @@ func deletePVC(ctx context.Context, vClusterClient, _ kubernetes.Interface, _, _
}).WithPolling(constants.PollingInterval).WithTimeout(constants.PollingTimeout).Should(Succeed())
}

// SnapshotLargeRestoreSpec registers the large-object snapshot and restore test.
// Call this from a dedicated suite that provisions its own vCluster.
func SnapshotLargeRestoreSpec() {
var s snapshotCtx
BeforeAll(func(ctx context.Context) {
s = *newSnapshotCtx(ctx)
})
describeSnapshotLargeObjectRestore(&s)
}

func describeSnapshotLargeObjectRestore(s *snapshotCtx) {
Comment thread
jjaferson marked this conversation as resolved.
const (
objectCount = 100_000
nsCount = 100
objectsPerNS = objectCount / nsCount // 1_000 per namespace
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

objectsPerNS = objectCount / nsCount carries an implicit invariant that objectCount is divisible by nsCount. If a future maintainer tweaks objectCount to e.g. 100_001, the channel-fed loop below still publishes objectCount indices and i/objectsPerNS will exceed nsCount-1, producing creates into a non-existent namespace. Either assert the invariant (Expect(objectCount % nsCount).To(Equal(0))) or iterate for ns := range nsCount { for j := range objectsPerNS { ... } } so the loop bound matches the namespace topology.

deleteNSCount = 4
createWorkers = 50
labelKey = "large-restore"
)

// Ordered: write objects → take snapshot → delete objects → restore → verify.
Describe("snapshot and restore with 100,000 objects", Ordered, func() {
var (
nsPrefix string
snapshotPath string
labelVal string
)

BeforeAll(func(ctx context.Context) {
nsPrefix = "large-restore-" + random.String(6)
snapshotPath = "container:///snapshot-data/" + nsPrefix + ".tar.gz"
labelVal = nsPrefix
cleanupAllSnapshotArtifacts(ctx, s.hostClient, s.vClusterNS)

for i := range nsCount {
Comment thread
jjaferson marked this conversation as resolved.
ns := fmt.Sprintf("%s-%d", nsPrefix, i)
_, err := s.vClusterClient.CoreV1().Namespaces().Create(ctx, &corev1.Namespace{
ObjectMeta: metav1.ObjectMeta{Name: ns},
}, metav1.CreateOptions{})
Expect(err).To(Or(Succeed(), Satisfy(kerrors.IsAlreadyExists)))
}
})

It("Writes 100,000 configmaps into the tenant cluster", func(ctx context.Context) {
By("Creating 1,000 configmaps in each of 100 namespaces concurrently", func() {
// The default client-go rate limiter (5 QPS, burst 10) would make 100k creates
// take ~20,000 seconds. Use a high-QPS client so the goroutines actually run in
// parallel and we hit the server's throughput ceiling instead.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Do you roughly know how this performs on GH runners? I'm a little concerned about it taking forever there or just straight up failing

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've tested it locally and to run the requests in parallel without being blocked by the rate limiter in client-go, so using the default, the first 10 requests go through immediately then after that the next one goes every ~200 ms

currentClusterName := cluster.CurrentClusterNameFrom(ctx)
bulkRestConfig := *cluster.From(ctx, currentClusterName).KubernetesRestConfig()
bulkRestConfig.QPS = 500
bulkRestConfig.Burst = 1000
bulkClient, err := kubernetes.NewForConfig(&bulkRestConfig)
Expect(err).To(Succeed())

eg, egCtx := errgroup.WithContext(ctx)
ch := make(chan int, objectCount)
for i := range objectCount {
ch <- i
}
close(ch)

for range createWorkers {
eg.Go(func() error {
for i := range ch {
// To avoid creating a channel for each namespace, we calculate the namespace and object name from the index.
// eg: object 0 goes to ns-0, object 1 to ns-0, ..., object 999 to ns-0, object 1000 to ns-1, etc.
ns := fmt.Sprintf("%s-%d", nsPrefix, i/objectsPerNS)
name := fmt.Sprintf("obj-%d", i%objectsPerNS)
_, createErr := bulkClient.CoreV1().ConfigMaps(ns).Create(egCtx, &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: name,
Namespace: ns,
Labels: map[string]string{labelKey: labelVal},
},
}, metav1.CreateOptions{})
if createErr != nil {
return fmt.Errorf("create configmap %s/%s: %w", ns, name, createErr)
}
}
return nil
})
}
Expect(eg.Wait()).To(Succeed())
})
})

It("Takes a snapshot of the tenant cluster", func(ctx context.Context) {
createSnapshot(s.vClusterName, s.vClusterNS, true, snapshotPath, false)
waitForRequestToFinish(ctx, s.hostClient, s.vClusterNS,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This open-codes a waitForRequestToFinish(...) call with a custom timeout instead of using waitForSnapshotToBeCreated, leaking helper-internal knowledge (label key + unmarshal fn) into the spec. Consider extending the helper to accept a timeout — e.g. waitForSnapshotToBeCreated(ctx, host, ns, timeout) with existing callers passing PollingTimeoutVeryLong and this one passing the extra-long value — so all snapshot specs share the same abstraction.

pkgconstants.SnapshotRequestLabel, snapshot.UnmarshalSnapshotRequest,
constants.PollingTimeoutExtraLong)
})

It("Deletes the first 4 namespaces and waits for them to be removed", func(ctx context.Context) {
By("Deleting namespaces 0 through 3", func() {
for i := range deleteNSCount {
ns := fmt.Sprintf("%s-%d", nsPrefix, i)
err := s.vClusterClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
Expect(err).To(Or(Succeed(), Satisfy(kerrors.IsNotFound)))
}
})

By("Waiting for the 4 namespaces to be fully terminated", func() {
Eventually(func(g Gomega, ctx context.Context) {
for i := range deleteNSCount {
ns := fmt.Sprintf("%s-%d", nsPrefix, i)
_, err := s.vClusterClient.CoreV1().Namespaces().Get(ctx, ns, metav1.GetOptions{})
g.Expect(kerrors.IsNotFound(err)).To(BeTrue(),
"namespace %s should be deleted but still exists", ns)
}
}).WithContext(ctx).WithPolling(constants.PollingInterval).WithTimeout(constants.PollingTimeoutExtraLong).Should(Succeed())
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Using a 90-minute polling timeout for namespace deletion conflates two concerns: the overall test budget and the expected operation latency. A real 90m wait on namespace termination usually masks a stuck finalizer rather than legitimately taking that long. Consider scoping each Eventually to its expected upper bound (a few minutes for ns deletion of 1000 cms) so failures fail fast with the right signal, and reserving the extra-long timeout only for the snapshot/restore RPC waits where 90m is plausible.

})
})

It("Restores snapshot and verifies the deleted namespaces are back", func(ctx context.Context) {
By("Restoring the tenant cluster from snapshot", func() {
restoreVCluster(ctx, s.hostClient, s.vClusterName, s.vClusterNS, snapshotPath, true, false)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The large-restore suite uses PollingTimeoutExtraLong (90m) elsewhere, but restoreVCluster(...) still goes through a helper that hardcodes PollingTimeoutVeryLong (5m) for the post-restore pod-ready wait (helpers.go:74). With a 100k-object restore the pods may take longer to become ready, and the suite will fail here long before the later 90m assertions run. Consider threading the timeout into restoreVCluster.

s.refreshClient(ctx)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

s.refreshClient(ctx) hardcodes a PollingTimeoutLong (2m) wait for the restored API server to become usable (test_snapshot.go:112). After restoring 100k objects the pod may be Running while the tenant API is still replaying state, so this reconnect can time out even though the restore would eventually succeed. Worth letting the large-restore suite use a longer reconnect timeout.

})

By("Verifying the deleted namespaces and their configmaps are restored", func() {
Eventually(func(g Gomega, ctx context.Context) {
for i := range deleteNSCount {
Comment thread
jjaferson marked this conversation as resolved.
ns := fmt.Sprintf("%s-%d", nsPrefix, i)
_, err := s.vClusterClient.CoreV1().Namespaces().Get(ctx, ns, metav1.GetOptions{})
g.Expect(err).To(Succeed(), "namespace %s should exist after restore", ns)

count, err := countConfigMapsByLabel(ctx, s.vClusterClient, ns, labelKey+"="+labelVal)
g.Expect(err).To(Succeed())
g.Expect(count).To(Equal(objectsPerNS),
"namespace %s should have %d configmaps after restore, got %d", ns, objectsPerNS, count)
}
}).WithContext(ctx).WithPolling(constants.PollingInterval).WithTimeout(constants.PollingTimeoutExtraLong).Should(Succeed())
})

By("Spot-checking non-deleted namespaces survived restore", func() {
for _, i := range []int{nsCount / 4, nsCount / 2, nsCount - 1} {
ns := fmt.Sprintf("%s-%d", nsPrefix, i)
count, err := countConfigMapsByLabel(ctx, s.vClusterClient, ns, labelKey+"="+labelVal)
Expect(err).To(Succeed())
Expect(count).To(Equal(objectsPerNS),
"non-deleted namespace %s should have %d configmaps after restore, got %d", ns, objectsPerNS, count)
}
})
})

AfterAll(func(ctx context.Context) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could this go into a DeferCleanup block instead or does AfterAll also fire appropriately (I don't know)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good question, I am using AfterAll as it's used in other parts of the e2e snapshot test suite, @adriankabala do you know which one should we use here?

AfterAll: splits creation and cleanup across two functions.
DeferCleanup: runs inside BeforeAll

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In this case DeferCleanup and AfterAll should work in the same way, both should get executed when the Ordered container completes (after all It specs).

using AfterAll as it's used in other parts of the e2e snapshot test suite

➕ here, for the sake of consistency, it's probably better to use AfterAll in this PR.

The benefit of having DeferCleanup in BeforeAll is that it would place the cleanup code next to the setup code, which would be easier and more reliable to maintain in tests, e.g. when you setup something new in the test, the cleanup code is right next to the setup to remind you (or the agent) that your new resource should be also cleaned up.

All this being said, we could refactor cleanup code in tests and use DeferCleanup instead of AfterAll where it brings some value, but that's probably and idea for the @loft-sh/eng-qa team :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the whole AfterAll is not needed at all. Why would we want to delete all these namespaces right before the tenant cluster is dropped anyway?

for i := range nsCount {
ns := fmt.Sprintf("%s-%d", nsPrefix, i)
err := s.vClusterClient.CoreV1().Namespaces().Delete(ctx, ns, metav1.DeleteOptions{})
Expect(err).To(Or(Succeed(), Satisfy(kerrors.IsNotFound)))
}
deleteSnapshotRequestConfigMaps(ctx, s.hostClient, s.vClusterNS)
})
})
}

func getTwoSnapshotRequests(g Gomega, ctx context.Context, hostClient kubernetes.Interface, vClusterNamespace string) (*snapshot.Request, *snapshot.Request) {
configMaps, err := hostClient.CoreV1().ConfigMaps(vClusterNamespace).List(ctx, metav1.ListOptions{
LabelSelector: pkgconstants.SnapshotRequestLabel,
Expand Down
5 changes: 5 additions & 0 deletions e2e-next/vcluster-snapshot.yaml
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
controlPlane:
distro:
k8s:
apiServer:
extraArgs:
- "--etcd-compaction-interval=1h"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Adding --etcd-compaction-interval=1h here changes the API server config for all snapshot suites that embed this YAML (both suite_snapshot_test.go and the new suite_snapshot_large_restore_test.go). The existing snapshot suite did not need this tuning. To avoid coupling unrelated suites to a perf knob added for one of them, consider either (a) moving this to a dedicated vcluster-snapshot-large-restore.yaml embedded only from the new suite, or (b) parameterizing via lazyvcluster.WithExtraTemplateVars so the existing suite keeps the default.

statefulSet:
image:
registry: ""
Expand Down
Loading