Skip to content

Add some tests for fleet oldest checking utilities #48

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

Merged
UnfamousThomas merged 2 commits into from
May 13, 2025
Merged

Add some tests for fleet oldest checking utilities #48

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0960cf9
Add some tests for fleet oldest checking utilities
UnfamousThomas May 13, 2025
b53015a
Merge branch 'master' into add-fleet-oldest-tests
UnfamousThomas May 13, 2025
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
7 changes: 4 additions & 3 deletions operator/cmd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,9 +172,10 @@ func main() {
}
}
if err = (&controller.FleetReconciler{
Client: mgr.GetClient(),
Recorder: mgr.GetEventRecorderFor("fleet"),
Scheme: mgr.GetScheme(),
Client: mgr.GetClient(),
Recorder: mgr.GetEventRecorderFor("fleet"),
DeletionChecker: prodChecker,
Scheme: mgr.GetScheme(),
}).SetupWithManager(mgr); err != nil {
setupLog.Error(err, "unable to create controller", "controller", "Fleet")
os.Exit(1)
Expand Down
7 changes: 4 additions & 3 deletions operator/internal/controller/fleet_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,9 @@ const FLEET_FINALIZER = "fleets.unfamousthomas.me/finalizer"
// FleetReconciler reconciles a Fleet object
type FleetReconciler struct {
client.Client
Scheme *runtime.Scheme
Recorder record.EventRecorder
Scheme *runtime.Scheme
Recorder record.EventRecorder
DeletionChecker utils.FleetDeletionChecker
}

// +kubebuilder:rbac:groups=network.unfamousthomas.me,resources=fleets,verbs=get;list;watch;create;update;patch;delete
Expand Down Expand Up @@ -128,7 +129,7 @@ func (r *FleetReconciler) scaleServerCount(ctx context.Context, fleet *networkv1
if err != nil {
return err
}
server, err := utils.FindDeleteServer(ctx, fleet, servers, r.Client)
server, err := utils.FindDeleteServer(ctx, fleet, servers, r.Client, r.DeletionChecker)
if err != nil {
return err
}
Expand Down
58 changes: 34 additions & 24 deletions operator/internal/controller/fleet_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
networkv1alpha1 "github.com/unfamousthomas/thesis-operator/api/v1alpha1"
"github.com/unfamousthomas/thesis-operator/internal/utils"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
Expand All @@ -30,6 +31,7 @@ import (
"time"
)

var prodChecker = utils.ProdDeletionChecker{}
var basicFleetSpec = networkv1alpha1.FleetSpec{
Scaling: networkv1alpha1.FleetScaling{
Replicas: 2,
Expand Down Expand Up @@ -79,9 +81,10 @@ var _ = Describe("Fleet Controller", func() {
Expect(k8sClient.Delete(ctx, fleet)).To(Succeed())
recorder := NewFakeRecorder()
reconciler := &FleetReconciler{
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: recorder,
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: recorder,
DeletionChecker: prodChecker,
}
_, err = reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName})
Expect(err).ToNot(HaveOccurred())
Expand All @@ -105,9 +108,10 @@ var _ = Describe("Fleet Controller", func() {
By("Setting up reconciler")
recorder := NewFakeRecorder()
reconciler := &FleetReconciler{
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: recorder,
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: recorder,
DeletionChecker: prodChecker,
}

By("Initial reconcile")
Expand Down Expand Up @@ -161,9 +165,10 @@ var _ = Describe("Fleet Controller", func() {
})
It("should delete all servers and remove the finalizer on fleet deletion", func() {
reconciler := &FleetReconciler{
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
DeletionChecker: prodChecker,
}

// Initial reconciles to create servers
Expand Down Expand Up @@ -204,9 +209,10 @@ var _ = Describe("Fleet Controller", func() {

It("should add a finalizer if not present", func() {
reconciler := &FleetReconciler{
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
DeletionChecker: prodChecker,
}

_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName})
Expand All @@ -219,9 +225,10 @@ var _ = Describe("Fleet Controller", func() {

It("should scale up servers to match the desired replicas", func() {
reconciler := &FleetReconciler{
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
DeletionChecker: prodChecker,
}

_, err := reconciler.Reconcile(ctx, reconcile.Request{NamespacedName: namespacedName})
Expand Down Expand Up @@ -262,9 +269,10 @@ var _ = Describe("Fleet Controller", func() {

It("should delete all servers when fleet is deleted", func() {
reconciler := &FleetReconciler{
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
Client: k8sClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
DeletionChecker: prodChecker,
}

// Ensure the fleet is scaled first
Expand Down Expand Up @@ -307,9 +315,10 @@ var _ = Describe("Fleet Controller", func() {
FailPatch: false,
}
reconciler := &FleetReconciler{
Client: fakeClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
Client: fakeClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
DeletionChecker: prodChecker,
}

By("Fail Get")
Expand Down Expand Up @@ -347,9 +356,10 @@ var _ = Describe("Fleet Controller", func() {
FailPatch: false,
}
reconciler := &FleetReconciler{
Client: fakeClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
Client: fakeClient,
Scheme: k8sClient.Scheme(),
Recorder: NewFakeRecorder(),
DeletionChecker: prodChecker,
}

By("Fail Create")
Expand Down
82 changes: 44 additions & 38 deletions operator/internal/utils/fleet_deletion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,22 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
)

type FleetDeletionChecker interface {
isDeleteAllowed(ctx context.Context, server *networkv1alpha1.Server, c *client.Client) (bool, error)
}

// FindDeleteServer is used to find the server that should be deleted.
// It is based on the specs agepriority field.
func FindDeleteServer(ctx context.Context, fleet *networkv1alpha1.Fleet, servers *networkv1alpha1.ServerList, client client.Client) (*networkv1alpha1.Server, error) {
func FindDeleteServer(ctx context.Context, fleet *networkv1alpha1.Fleet, servers *networkv1alpha1.ServerList, client client.Client, checker FleetDeletionChecker) (*networkv1alpha1.Server, error) {
strategy := fleet.Spec.Scaling.AgePriority
deleteFirst := fleet.Spec.Scaling.PrioritizeAllowed

if strategy == networkv1alpha1.OldestFirst {
return getOldestServer(ctx, servers, deleteFirst, client)
return getOldestServer(ctx, servers, deleteFirst, &client, checker)
}

if strategy == networkv1alpha1.NewestFirst {
return getNewestServer(ctx, servers, deleteFirst, client)
return getNewestServer(ctx, servers, deleteFirst, &client, checker)

}
return nil, fmt.Errorf("invalid scaling strategy: %s", strategy)
Expand All @@ -31,33 +35,34 @@ func FindDeleteServer(ctx context.Context, fleet *networkv1alpha1.Fleet, servers
// getOldestServer gets the server of the fleet, that is the oldest
// If deleteFirst is enabled, then it tries to get the server that can be deleted, but that is the oldest out of those.
// If it cannot find any where deletion is allowed, it returns the oldest server.
func getOldestServer(ctx context.Context, servers *networkv1alpha1.ServerList, deleteFirst bool, client client.Client) (*networkv1alpha1.Server, error) {
func getOldestServer(ctx context.Context, servers *networkv1alpha1.ServerList, deleteFirst bool, client *client.Client, checker FleetDeletionChecker) (*networkv1alpha1.Server, error) {
var oldestServer *networkv1alpha1.Server
var oldestTime *metav1.Time
var oldestAllowedServer *networkv1alpha1.Server
var oldestAllowTime *metav1.Time

//Go over all servers
for _, server := range servers.Items {
for i := range servers.Items {
server := &servers.Items[i]
//If time is null, or this server was created before current oldest
if oldestTime == nil || server.CreationTimestamp.Before(oldestTime) {
//Set new oldest
oldestTime = &server.CreationTimestamp
oldestServer = &server
// Check if we want to prioritize allowed
if deleteFirst {
//Check if this is allowed
allowed, err := isDeleteAllowed(ctx, &server, client)
if err != nil {
return nil, err
}
if allowed {
//If it is allowed, check current oldest allowed
if oldestAllowTime == nil || server.CreationTimestamp.Before(oldestAllowTime) {
//Update to new oldest allowed
oldestAllowTime = &server.CreationTimestamp
oldestAllowedServer = &server
}
oldestServer = server
}
// Check if we want to prioritize allowed
if deleteFirst {
//Check if this is allowed
allowed, err := checker.isDeleteAllowed(ctx, server, client)
if err != nil {
return nil, err
}
if allowed {
//If it is allowed, check current oldest allowed
if oldestAllowTime == nil || server.CreationTimestamp.Before(oldestAllowTime) {
//Update to new oldest allowed
oldestAllowTime = &server.CreationTimestamp
oldestAllowedServer = server
}
}
}
Expand All @@ -79,33 +84,34 @@ func getOldestServer(ctx context.Context, servers *networkv1alpha1.ServerList, d
// getNewestServer gets the server of the fleet, that is the newest
// If deleteFirst is enabled, then it tries to get the server that can be deleted, but that is the youngest out of those.
// If it cannot find any where deletion is allowed, it returns the youngest server.
func getNewestServer(ctx context.Context, servers *networkv1alpha1.ServerList, deleteFirst bool, client client.Client) (*networkv1alpha1.Server, error) {
func getNewestServer(ctx context.Context, servers *networkv1alpha1.ServerList, deleteFirst bool, client *client.Client, checker FleetDeletionChecker) (*networkv1alpha1.Server, error) {

var newestServer *networkv1alpha1.Server
var newestTime *metav1.Time
var newestAllowedServer *networkv1alpha1.Server
var newestAllowTime *metav1.Time

// Go over all of the servers
for _, server := range servers.Items {
for i := range servers.Items {
server := &servers.Items[i]
//If time is null, or this server was created after current youngest
if newestTime == nil || server.CreationTimestamp.After(newestTime.Time) {
// Update with new youngest
newestTime = &server.CreationTimestamp
newestServer = &server
// Check if we want to prioritize allowed
if deleteFirst {
//Check if this is allowed
allowed, err := isDeleteAllowed(ctx, &server, client)
if err != nil {
return nil, err
}
if allowed {
// If it is, we want to check if there already is a prioritized time, and is it after the current iterations one
if newestAllowTime == nil || server.CreationTimestamp.After(newestAllowTime.Time) {
newestAllowTime = &server.CreationTimestamp
newestAllowedServer = &server
}
newestServer = server
}
// Check if we want to prioritize allowed
if deleteFirst {
//Check if this is allowed
allowed, err := checker.isDeleteAllowed(ctx, server, client)
if err != nil {
return nil, err
}
if allowed {
// If it is, we want to check if there already is a prioritized time, and is it after the current iterations one
if newestAllowTime == nil || server.CreationTimestamp.After(newestAllowTime.Time) {
newestAllowTime = &server.CreationTimestamp
newestAllowedServer = server
}
}
}
Expand All @@ -125,10 +131,10 @@ func getNewestServer(ctx context.Context, servers *networkv1alpha1.ServerList, d
}

// isDeleteAllowed is a utility for a server object, to communicate with the sidecar to see if deletion is allowed
func isDeleteAllowed(ctx context.Context, server *networkv1alpha1.Server, c client.Client) (bool, error) {
func (ProdDeletionChecker) isDeleteAllowed(ctx context.Context, server *networkv1alpha1.Server, c *client.Client) (bool, error) {
podName := server.Name + "-pod"
pod := &v1.Pod{}
err := c.Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: podName}, pod)
err := (*c).Get(ctx, types.NamespacedName{Namespace: server.Namespace, Name: podName}, pod)
if err != nil {
return false, err
}
Expand Down
Loading