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

🐛 Fix unit tests, improve debuggability #3126

Merged
merged 1 commit into from
Jul 30, 2024
Merged
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
3 changes: 2 additions & 1 deletion controllers/controllers_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ func TestControllers(t *testing.T) {

var (
testEnv *helpers.TestEnvironment
tracker *remote.ClusterCacheTracker
ctx = ctrl.SetupSignalHandler()
)

Expand Down Expand Up @@ -80,7 +81,7 @@ func setup() {
panic("unable to create secret caching client")
}

tracker, err := remote.NewClusterCacheTracker(
tracker, err = remote.NewClusterCacheTracker(
testEnv.Manager,
remote.ClusterCacheTrackerOptions{
SecretCachingClient: secretCachingClient,
Expand Down
7 changes: 7 additions & 0 deletions controllers/serviceaccount_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"fmt"
"reflect"
"strings"
"time"

"github.com/pkg/errors"
Expand Down Expand Up @@ -242,6 +243,12 @@ func (r *ServiceAccountReconciler) reconcileNormal(ctx context.Context, guestClu
func (r *ServiceAccountReconciler) ensureProviderServiceAccounts(ctx context.Context, guestClusterCtx *vmwarecontext.GuestClusterContext, pSvcAccounts []vmwarev1.ProviderServiceAccount) error {
log := ctrl.LoggerFrom(ctx)

pSvcAccountNames := []string{}
for _, pSvcAccount := range pSvcAccounts {
pSvcAccountNames = append(pSvcAccountNames, pSvcAccount.Name)
}
log.V(5).Info(fmt.Sprintf("Reconcile ProviderServiceAccounts: %v", strings.Join(pSvcAccountNames, ",")))
Copy link
Member Author

Choose a reason for hiding this comment

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

Just printing the ProviderServiceAccounts, this should make it easier to catch race conditions where there might not be any ProviderServiceAccounts


for i, pSvcAccount := range pSvcAccounts {
// Note: We have to use := here to not overwrite log & ctx outside the for loop.
log := log.WithValues("ProviderServiceAccount", klog.KRef(pSvcAccount.Namespace, pSvcAccount.Name))
Expand Down
11 changes: 9 additions & 2 deletions controllers/serviceaccount_controller_intg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,14 +59,21 @@ var _ = Describe("ProviderServiceAccount controller integration tests", func() {
helpers.CreateAndWait(ctx, intCtx.Client, intCtx.KubeconfigSecret)
})

By("Verifying that the guest cluster client works")
guestClient, err := tracker.GetClient(ctx, client.ObjectKeyFromObject(intCtx.Cluster))
Expect(err).ToNot(HaveOccurred())
// Note: Create a Service informer, so the test later doesn't fail if this doesn't work.
Expect(guestClient.List(ctx, &corev1.ServiceList{}, client.InNamespace(metav1.NamespaceDefault))).To(Succeed())
Copy link
Member Author

Choose a reason for hiding this comment

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

Saw some errors in the logs. Not sure if the test failures are due to envtest not coming up (probalby not, but shouldn't hurt to have this explicit check here)


pSvcAccount = getTestProviderServiceAccount(intCtx.Namespace, intCtx.VSphereCluster)
createTestResource(ctx, intCtx.Client, pSvcAccount)
assertEventuallyExistsInNamespace(ctx, intCtx.Client, intCtx.Namespace, pSvcAccount.GetName(), pSvcAccount)
})
AfterEach(func() {
// Deleting the provider service account is not strictly required as the context itself
// gets teared down but keeping it for clarity.
deleteTestResource(ctx, intCtx.Client, pSvcAccount)
deleteTestResource(ctx, intCtx.Client, intCtx.VSphereCluster)
Copy link
Member Author

Choose a reason for hiding this comment

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

Just some explicit cleanup. Deleting a namespace in testenv doesn't lead to all object in the namespace being deleted (there is no kube-controller-manager)

deleteTestResource(ctx, intCtx.Client, intCtx.Cluster)
deleteTestResource(ctx, intCtx.Client, intCtx.KubeconfigSecret)
})

Context("When serviceaccount secret is created", func() {
Expand Down
10 changes: 10 additions & 0 deletions controllers/servicediscovery_controller_intg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"sigs.k8s.io/controller-runtime/pkg/client"

helpers "sigs.k8s.io/cluster-api-provider-vsphere/internal/test/helpers/vmware"
Expand All @@ -37,8 +38,17 @@ var _ = Describe("Service Discovery controller integration tests", func() {
helpers.CreateAndWait(ctx, intCtx.Client, intCtx.VSphereCluster)
helpers.CreateAndWait(ctx, intCtx.Client, intCtx.KubeconfigSecret)
})

By("Verifying that the guest cluster client works")
guestClient, err := tracker.GetClient(ctx, client.ObjectKeyFromObject(intCtx.Cluster))
Expect(err).ToNot(HaveOccurred())
// Note: Create a Service informer, so the test later doesn't fail if this doesn't work.
Expect(guestClient.List(ctx, &corev1.ServiceList{}, client.InNamespace(metav1.NamespaceDefault))).To(Succeed())
})
AfterEach(func() {
deleteTestResource(ctx, intCtx.Client, intCtx.VSphereCluster)
deleteTestResource(ctx, intCtx.Client, intCtx.Cluster)
deleteTestResource(ctx, intCtx.Client, intCtx.KubeconfigSecret)
intCtx.AfterEach()
})

Expand Down
6 changes: 5 additions & 1 deletion controllers/vspherecluster_reconciler_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,11 @@ var _ = Describe("VIM based VSphere ClusterReconciler", func() {
})

AfterEach(func() {
Expect(testEnv.CleanupAndWait(ctx, instance, zoneOne, capiCluster, namespace)).To(Succeed())
// Note: Make sure VSphereCluster is deleted before the Cluster is deleted.
// Otherwise reconcileDelete in VSphereCluster reconciler will fail because the Cluster cannot be found.
Expect(testEnv.CleanupAndWait(ctx, instance, zoneOne)).To(Succeed())
Expect(testEnv.CleanupAndWait(ctx, capiCluster)).To(Succeed())
Expect(testEnv.CleanupAndWait(ctx, namespace)).To(Succeed())
})

It("should reconcile a cluster", func() {
Expand Down
9 changes: 9 additions & 0 deletions internal/test/helpers/envtest.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ import (
"k8s.io/apimachinery/pkg/util/wait"
clientgoscheme "k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"k8s.io/component-base/logs"
logsv1 "k8s.io/component-base/logs/api/v1"
"k8s.io/klog/v2"
clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
"sigs.k8s.io/cluster-api/util/kubeconfig"
Expand All @@ -60,7 +62,14 @@ import (
)

func init() {
// Set log level 5 as default for testing (default for prod is 2).
Copy link
Member Author

Choose a reason for hiding this comment

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

Log level 5 gives us more verbose logs of our controllers, but also a lot of useful logs from CR around Reconciles

logOptions := logs.NewOptions()
logOptions.Verbosity = 5
if err := logsv1.ValidateAndApply(logOptions, nil); err != nil {
panic(err)
}
ctrl.SetLogger(klog.Background())

// add logger for ginkgo
klog.SetOutput(ginkgo.GinkgoWriter)
}
Expand Down