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.

Sign up for GitHub

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

Refactor Controller Unit Tests #118

Merged
merged 5 commits into from
Feb 19, 2024
Merged

Refactor Controller Unit Tests #118

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8c20657
Refactor nmo controller unit test and suite
razo7 Feb 7, 2024
e4527ee
Refactor unit test for initMaintenanceStatus and exclude remediation …
razo7 Feb 7, 2024
c068b59
Refactor unit tests for reconciliation behavior
razo7 Feb 7, 2024
199b494
Fixing remarks for clearer test readability
razo7 Feb 8, 2024
b3ced39
Pass context in tests
razo7 Feb 18, 2024
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
67 changes: 34 additions & 33 deletions controllers/suite_test.go → controllers/controller_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,18 @@ import (
"path/filepath"
"testing"

"github.com/medik8s/common/pkg/lease"
Copy link
Member

Choose a reason for hiding this comment

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

about the file name: eiher just keep suite_test.go (good enough IMHO), or use <package_name>_suite_test.go please

Copy link
Member Author

Choose a reason for hiding this comment

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

I wanted to go with <package_name>_suite_test.go, and I forgot the suffix s 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

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

BTW should nodemaintenance_controller.go and nodemaintenance_controller_test follow the same pattern? Since they both use "controller" and not "controllers"

Copy link
Member

Choose a reason for hiding this comment

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

no, each file is about a single controller, not?

Copy link
Member

Choose a reason for hiding this comment

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

but the package can contain multiple

Copy link
Member Author

Choose a reason for hiding this comment

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

but the package can contain multiple

Correct, as SNR has multiple controllers

Copy link
Member

Choose a reason for hiding this comment

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

filename still is wrong

Copy link
Member Author

Choose a reason for hiding this comment

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

done

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
"k8s.io/client-go/tools/record"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/envtest"
logf "sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/log/zap"
metricsServer "sigs.k8s.io/controller-runtime/pkg/metrics/server"

nodemaintenancev1beta1 "github.com/medik8s/node-maintenance-operator/api/v1beta1"
//+kubebuilder:scaffold:imports
Expand All @@ -40,10 +41,15 @@ import (
// These tests use Ginkgo (BDD-style Go testing framework). Refer to
// http://onsi.github.io/ginkgo/ to learn more about Ginkgo.

var cfg *rest.Config
var k8sClient client.Client
var testEnv *envtest.Environment
var ctxFromSignalHandler context.Context
var (
cfg *rest.Config
k8sClient client.Client
testEnv *envtest.Environment
ctx context.Context
cancel context.CancelFunc
fakeRecorder *record.FakeRecorder
r *NodeMaintenanceReconciler
)

func TestControllers(t *testing.T) {
RegisterFailHandler(Fail)
Expand All @@ -55,9 +61,6 @@ var _ = BeforeSuite(func() {

// call start or refactor when moving to "normal" testEnv test

})

func startTestEnv() {
By("bootstrapping test environment")
testEnv = &envtest.Environment{
CRDDirectoryPaths: []string{filepath.Join("..", "config", "crd", "bases")},
Expand All @@ -74,40 +77,38 @@ func startTestEnv() {
Expect(err).NotTo(HaveOccurred())

//+kubebuilder:scaffold:scheme
k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme.Scheme})
Expect(err).NotTo(HaveOccurred())
Expect(k8sManager).NotTo(BeNil())

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

k8sManager, err := ctrl.NewManager(cfg, ctrl.Options{
Scheme: scheme.Scheme,
Metrics: metricsServer.Options{BindAddress: "0"},
})
Expect(err).ToNot(HaveOccurred())
mockManager, _ := lease.NewManager(k8sClient, "")
Copy link
Member

Choose a reason for hiding this comment

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

Nit: there are a lot of managers involved, maybe mockLeaseManager is better name ?

Copy link
Member Author

Choose a reason for hiding this comment

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

mockLeaseManager is already used in nodemaintenance_controller_test.go.
Maybe tempMockManager?

// Create a ReconcileNodeMaintenance object with the scheme and fake client
r = &NodeMaintenanceReconciler{
Client: k8sClient,
Scheme: scheme.Scheme,
LeaseManager: &mockLeaseManager{mockManager},
logger: ctrl.Log.WithName("unit test"),
}
Expect(initDrainer(r, cfg)).To(Succeed())
// in test pods are not evicted, so don't wait forever for them
r.drainer.SkipWaitForDeleteTimeoutSeconds = 0

// comment in when moving to "normal" testEnv test
// this isn't needed atm, because the controller tests call relevant funcs of the controller themself
//err = (&NodeMaintenanceReconciler{
// Client: k8sManager.GetClient(),
// Scheme: k8sManager.GetScheme(),
//}).SetupWithManager(k8sManager)
//Expect(err).ToNot(HaveOccurred())
err = (r).SetupWithManager(k8sManager)
Expect(err).NotTo(HaveOccurred())

go func() {
if ctxFromSignalHandler == nil {
ctxFromSignalHandler = ctrl.SetupSignalHandler()
}
err = k8sManager.Start(ctxFromSignalHandler)
Expect(err).ToNot(HaveOccurred())
// https://github.com/kubernetes-sigs/controller-runtime/issues/1571
ctx, cancel = context.WithCancel(ctrl.SetupSignalHandler())
Expect(k8sManager.Start(ctx)).To(Succeed())
}()
}

var _ = AfterSuite(func() {
// call stop or refactor when moving to "normal" testEnv test
})

func stopTestEnv() {
var _ = AfterSuite(func() {
By("tearing down the test environment")
err := testEnv.Stop()
Expect(err).NotTo(HaveOccurred())
}
cancel()
Expect(testEnv.Stop()).To(Succeed())
})
36 changes: 18 additions & 18 deletions controllers/nodemaintenance_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func (r *NodeMaintenanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
return reconcile.Result{}, nil
}

err = r.initMaintenanceStatus(instance)
err = initMaintenanceStatus(instance, r.drainer, r.Client)
if err != nil {
r.logger.Error(err, "Failed to update NodeMaintenance with \"Running\" status")
return r.onReconcileError(instance, err)
Expand All @@ -152,7 +152,7 @@ func (r *NodeMaintenanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
return r.onReconcileError(instance, err)
}

r.setOwnerRefToNode(instance, node)
setOwnerRefToNode(instance, node, r.logger)

updateOwnedLeaseFailed, err := r.obtainLease(node)
if err != nil && updateOwnedLeaseFailed {
Expand Down Expand Up @@ -180,7 +180,7 @@ func (r *NodeMaintenanceReconciler) Reconcile(ctx context.Context, req ctrl.Requ
}
}
//Add exclude from remediation label
if err := r.addExcludeRemediationLabel(ctx, node); err != nil {
if err := addExcludeRemediationLabel(ctx, node, r.Client, r.logger); err != nil {
return r.onReconcileError(instance, err)
}

Expand Down Expand Up @@ -285,15 +285,15 @@ func initDrainer(r *NodeMaintenanceReconciler, config *rest.Config) error {
return nil
}

func (r *NodeMaintenanceReconciler) setOwnerRefToNode(instance *nodemaintenancev1beta1.NodeMaintenance, node *corev1.Node) {
func setOwnerRefToNode(nm *nodemaintenancev1beta1.NodeMaintenance, node *corev1.Node, log logr.Logger) {

for _, ref := range instance.ObjectMeta.GetOwnerReferences() {
for _, ref := range nm.ObjectMeta.GetOwnerReferences() {
if ref.APIVersion == node.TypeMeta.APIVersion && ref.Kind == node.TypeMeta.Kind && ref.Name == node.ObjectMeta.GetName() && ref.UID == node.ObjectMeta.GetUID() {
return
}
}

r.logger.Info("setting owner ref to node")
log.Info("setting owner ref to node")

nodeMeta := node.TypeMeta
ref := metav1.OwnerReference{
Expand All @@ -305,7 +305,7 @@ func (r *NodeMaintenanceReconciler) setOwnerRefToNode(instance *nodemaintenancev
Controller: pointer.Bool(false),
}

instance.ObjectMeta.SetOwnerReferences(append(instance.ObjectMeta.GetOwnerReferences(), ref))
nm.ObjectMeta.SetOwnerReferences(append(nm.ObjectMeta.GetOwnerReferences(), ref))
}

func (r *NodeMaintenanceReconciler) obtainLease(node *corev1.Node) (bool, error) {
Expand All @@ -320,28 +320,28 @@ func (r *NodeMaintenanceReconciler) obtainLease(node *corev1.Node) (bool, error)
return false, nil
}

func (r *NodeMaintenanceReconciler) addExcludeRemediationLabel(ctx context.Context, node *corev1.Node) error {
func addExcludeRemediationLabel(ctx context.Context, node *corev1.Node, r client.Client, log logr.Logger) error {
if node.Labels[labels.ExcludeFromRemediation] != "true" {
patch := client.MergeFrom(node.DeepCopy())
if node.Labels == nil {
node.Labels = map[string]string{labels.ExcludeFromRemediation: "true"}
} else if node.Labels[labels.ExcludeFromRemediation] != "true" {
node.Labels[labels.ExcludeFromRemediation] = "true"
}
if err := r.Client.Patch(ctx, node, patch); err != nil {
r.logger.Error(err, "Failed to add exclude from remediation label from the node", "node name", node.Name)
if err := r.Patch(ctx, node, patch); err != nil {
log.Error(err, "Failed to add exclude from remediation label from the node", "node name", node.Name)
return err
}
}
return nil
}

func (r *NodeMaintenanceReconciler) removeExcludeRemediationLabel(ctx context.Context, node *corev1.Node) error {
func removeExcludeRemediationLabel(ctx context.Context, node *corev1.Node, r client.Client, log logr.Logger) error {
if node.Labels[labels.ExcludeFromRemediation] == "true" {
patch := client.MergeFrom(node.DeepCopy())
delete(node.Labels, labels.ExcludeFromRemediation)
if err := r.Client.Patch(ctx, node, patch); err != nil {
r.logger.Error(err, "Failed to remove exclude from remediation label from the node", "node name", node.Name)
if err := r.Patch(ctx, node, patch); err != nil {
log.Error(err, "Failed to remove exclude from remediation label from the node", "node name", node.Name)
return err
}
}
Expand All @@ -362,7 +362,7 @@ func (r *NodeMaintenanceReconciler) stopNodeMaintenanceImp(ctx context.Context,
if err := r.LeaseManager.InvalidateLease(ctx, node); err != nil {
return err
}
return r.removeExcludeRemediationLabel(ctx, node)
return removeExcludeRemediationLabel(ctx, node, r.Client, r.logger)
}

func (r *NodeMaintenanceReconciler) stopNodeMaintenanceOnDeletion(ctx context.Context, nodeName string) error {
Expand Down Expand Up @@ -392,11 +392,11 @@ func (r *NodeMaintenanceReconciler) fetchNode(nodeName string) (*corev1.Node, er
return node, nil
}

func (r *NodeMaintenanceReconciler) initMaintenanceStatus(nm *nodemaintenancev1beta1.NodeMaintenance) error {
func initMaintenanceStatus(nm *nodemaintenancev1beta1.NodeMaintenance, drainer *drain.Helper, r client.Client) error {
if nm.Status.Phase == "" {
nm.Status.Phase = nodemaintenancev1beta1.MaintenanceRunning
setLastUpdate(nm)
pendingList, errlist := r.drainer.GetPodsForDeletion(nm.Spec.NodeName)
pendingList, errlist := drainer.GetPodsForDeletion(nm.Spec.NodeName)
if errlist != nil {
return fmt.Errorf("failed to get pods for eviction while initializing status")
}
Expand All @@ -405,7 +405,7 @@ func (r *NodeMaintenanceReconciler) initMaintenanceStatus(nm *nodemaintenancev1b
}
nm.Status.EvictionPods = len(nm.Status.PendingPods)

podlist, err := r.drainer.Client.CoreV1().Pods(metav1.NamespaceAll).List(
podlist, err := drainer.Client.CoreV1().Pods(metav1.NamespaceAll).List(
context.Background(),
metav1.ListOptions{
FieldSelector: fields.SelectorFromSet(fields.Set{"spec.nodeName": nm.Spec.NodeName}).String(),
Expand All @@ -414,7 +414,7 @@ func (r *NodeMaintenanceReconciler) initMaintenanceStatus(nm *nodemaintenancev1b
return err
}
nm.Status.TotalPods = len(podlist.Items)
err = r.Client.Status().Update(context.TODO(), nm)
err = r.Status().Update(context.TODO(), nm)
return err
}
return nil
Expand Down
Loading
Loading