Skip to content

Commit

Permalink
No Notification for "Completed" and without controller Pods (#485)
Browse files Browse the repository at this point in the history 
* No Notification for "Completed" and without controller Pods

Signed-off-by: Feny Mehta <[email protected]>

* addressing the review comments

Signed-off-by: Feny Mehta <[email protected]>

* Adding a unit test case

Signed-off-by: Feny Mehta <[email protected]>

* Fixing the duplication

Signed-off-by: Feny Mehta <[email protected]>

* Adding testcases for controlled and completed pod status notify

Signed-off-by: Feny Mehta <[email protected]>

* Removing duplication in test cases

Signed-off-by: Feny Mehta <[email protected]>

* addresing review comments for test cases

Signed-off-by: Feny Mehta <[email protected]>

* Adding multiple conditions type for test

Signed-off-by: Feny Mehta <[email protected]>

---------

Signed-off-by: Feny Mehta <[email protected]>
  • Loading branch information
@fbm3307
fbm3307 authored Oct 27, 2023
1 parent 68b24f2 commit 6669ed5
Show file tree
Hide file tree
Showing 2 changed files with 122 additions and 47 deletions.
24 changes: 17 additions & 7 deletions controllers/idler/idler_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,13 @@ func (r *Reconciler) ensureIdling(logger logr.Logger, idler *toolchainv1alpha1.I
// Already tracking this pod. Check the timeout.
if time.Now().After(trackedPod.StartTime.Add(time.Duration(idler.Spec.TimeoutSeconds) * time.Second)) {
podLogger.Info("Pod running for too long. Killing the pod.", "start_time", trackedPod.StartTime.Format("2006-01-02T15:04:05Z"), "timeout_seconds", idler.Spec.TimeoutSeconds)
var podreason string
podcondition := pod.Status.Conditions
for _, podtype := range podcondition {
if podtype.Type == "Ready" {
podreason = podtype.Reason
}
}

// Check if it belongs to a controller (Deployment, DeploymentConfig, etc) and scale it down to zero.
appType, appName, deletedByController, err := r.scaleControllerToZero(podLogger, pod.ObjectMeta)
Expand All @@ -150,13 +157,16 @@ func (r *Reconciler) ensureIdling(logger logr.Logger, idler *toolchainv1alpha1.I
appName = pod.Name
appType = "Pod"
}

// By now either a pod has been deleted or scaled to zero by controller, idler Triggered notification should be sent
if err := r.createNotification(logger, idler, appName, appType); err != nil {
logger.Error(err, "failed to create Notification")
if err = r.setStatusIdlerNotificationCreationFailed(idler, err.Error()); err != nil {
logger.Error(err, "failed to set status IdlerNotificationCreationFailed")
} // not returning error to continue tracking remaining pods
// Send notification if the deleted pod was managed by a controller or was a standalone pod that was not completed
// eg. If a build pod is in "PodCompleted" status then it was not running so there's no reason to send an idler notification
if podreason != "PodCompleted" || deletedByController {
// By now either a pod has been deleted or scaled to zero by controller, idler Triggered notification should be sent
if err := r.createNotification(logger, idler, appName, appType); err != nil {
logger.Error(err, "failed to create Notification")
if err = r.setStatusIdlerNotificationCreationFailed(idler, err.Error()); err != nil {
logger.Error(err, "failed to set status IdlerNotificationCreationFailed")
} // not returning error to continue tracking remaining pods
}
}

} else {
Expand Down
145 changes: 105 additions & 40 deletions controllers/idler/idler_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -682,51 +682,116 @@ func TestAppNameTypeForControllers(t *testing.T) {
}
}

func TestAppNameTypeForInidividualPods(t *testing.T) {
func TestNotificationAppNameTypeForPods(t *testing.T) {
//given
idler := &toolchainv1alpha1.Idler{
ObjectMeta: metav1.ObjectMeta{
Name: "alex-stage",
Name: "feny-stage",
Labels: map[string]string{
toolchainv1alpha1.SpaceLabelKey: "alex",
toolchainv1alpha1.SpaceLabelKey: "feny",
},
},
Spec: toolchainv1alpha1.IdlerSpec{TimeoutSeconds: 60},
}
namespaces := []string{"dev", "stage"}
usernames := []string{"feny"}
nsTmplSet := newNSTmplSet(test.MemberOperatorNs, "feny", "advanced", "abcde11", namespaces, usernames)
mur := newMUR("feny")
var pname string
testpod := map[string]struct {
pcond []corev1.PodCondition
expectedAppType string
expectedNotificationCreated bool
controllerOwned bool
}{
"Individual-Completed-Pod": {
pcond: []corev1.PodCondition{{Type: "Ready", Reason: "PodCompleted"}},
expectedAppType: "Pod",
expectedNotificationCreated: false,
controllerOwned: false,
},
"Individual-NonCompleted-Pod": {
pcond: []corev1.PodCondition{{Type: "Ready", Reason: ""}},
expectedAppType: "Pod",
expectedNotificationCreated: true,
controllerOwned: false,
},
"Controlled-Completed-Pod": {
pcond: []corev1.PodCondition{{Type: "Ready", Reason: "PodCompleted"}},
expectedAppType: "Deployment",
expectedNotificationCreated: true,
controllerOwned: true,
},
"Controlled-NonCompleted-Pod": {
pcond: []corev1.PodCondition{{Type: "Ready", Reason: ""}},
expectedAppType: "Deployment",
expectedNotificationCreated: true,
controllerOwned: true,
},
"Controlled-Pod-nocondition": {
expectedAppType: "Deployment",
expectedNotificationCreated: true,
controllerOwned: true,
},
"Controlled-Pod-multiplecondition": {
pcond: []corev1.PodCondition{{Type: "Ready", Reason: ""},
{Type: "Initiated"},
{Type: "ContainersReady"}},
expectedAppType: "Deployment",
expectedNotificationCreated: true,
controllerOwned: true,
},
}

t.Run("Test AppName/Type in notification", func(t *testing.T) {
namespaces := []string{"dev", "stage"}
usernames := []string{"alex"}
nsTmplSet := newNSTmplSet(test.MemberOperatorNs, "alex", "advanced", "abcde11", namespaces, usernames)
mur := newMUR("alex")
reconciler, req, cl, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur)
idlerTimeoutPlusOneSecondAgo := time.Now().Add(-time.Duration(idler.Spec.TimeoutSeconds+1) * time.Second)
p := preparePayloadsSinglePod(t, reconciler, idler.Name, "todelete-", idlerTimeoutPlusOneSecondAgo).standalonePods[0]
// first reconcile to track pods
res, err := reconciler.Reconcile(context.TODO(), req)
assert.NoError(t, err)
assert.True(t, res.Requeue)
for pt, tcs := range testpod {
t.Run(pt, func(t *testing.T) {
reconciler, req, cl, _ := prepareReconcile(t, idler.Name, getHostCluster, idler, nsTmplSet, mur)
idlerTimeoutPlusOneSecondAgo := time.Now().Add(-time.Duration(idler.Spec.TimeoutSeconds+1) * time.Second)

// second reconcile should delete pods and create notification
res, err = reconciler.Reconcile(context.TODO(), req)
//then
assert.NoError(t, err)
memberoperatortest.AssertThatIdler(t, idler.Name, cl).
HasConditions(memberoperatortest.Running(), memberoperatortest.IdlerNotificationCreated())
//check the notification is actually created
hostCl, _ := reconciler.GetHostCluster()
notification := &toolchainv1alpha1.Notification{}
err = hostCl.Client.Get(context.TODO(), types.NamespacedName{
Namespace: test.HostOperatorNs,
Name: "alex-stage-idled",
}, notification)
require.NoError(t, err)
require.Equal(t, "[email protected]", notification.Spec.Recipient)
require.Equal(t, "idled", notification.Labels[toolchainv1alpha1.NotificationTypeLabelKey])
require.Equal(t, "Pod", notification.Spec.Context["AppType"])
require.Equal(t, p.Name, notification.Spec.Context["AppName"])
if tcs.controllerOwned {
preparePayloads(t, reconciler, idler.Name, "todelete-", idlerTimeoutPlusOneSecondAgo, tcs.pcond...)
} else {
p := preparePayloadsSinglePod(t, reconciler, idler.Name, "todelete-", idlerTimeoutPlusOneSecondAgo, tcs.pcond...).standalonePods[0]
pname = p.Name
}

})
// first reconcile to track pods
res, err := reconciler.Reconcile(context.TODO(), req)
assert.NoError(t, err)
assert.True(t, res.Requeue)

// second reconcile should delete pods and create notification
res, err = reconciler.Reconcile(context.TODO(), req)
//then
assert.NoError(t, err)

if tcs.expectedNotificationCreated {
memberoperatortest.AssertThatIdler(t, idler.Name, cl).
HasConditions(memberoperatortest.Running(), memberoperatortest.IdlerNotificationCreated())
} else {
memberoperatortest.AssertThatIdler(t, idler.Name, cl).
HasConditions(memberoperatortest.Running())
}
//check the notification is actually created
hostCl, _ := reconciler.GetHostCluster()
notification := &toolchainv1alpha1.Notification{}
err = hostCl.Client.Get(context.TODO(), types.NamespacedName{
Namespace: test.HostOperatorNs,
Name: "feny-stage-idled",
}, notification)
if tcs.expectedNotificationCreated {
require.NoError(t, err)
require.Equal(t, "[email protected]", notification.Spec.Recipient)
require.Equal(t, "idled", notification.Labels[toolchainv1alpha1.NotificationTypeLabelKey])
require.Equal(t, tcs.expectedAppType, notification.Spec.Context["AppType"])
if !tcs.controllerOwned {
require.Equal(t, pname, notification.Spec.Context["AppName"])
}
} else {
require.EqualError(t, err, "notifications.toolchain.dev.openshift.com \"feny-stage-idled\" not found")
}
})
}

}
func TestCreateNotification(t *testing.T) {
Expand Down Expand Up @@ -960,7 +1025,7 @@ type payloads struct {
job *batchv1.Job
}

func preparePayloads(t *testing.T, r *Reconciler, namespace, namePrefix string, startTime time.Time) payloads {
func preparePayloads(t *testing.T, r *Reconciler, namespace, namePrefix string, startTime time.Time, conditions ...corev1.PodCondition) payloads {
sTime := metav1.NewTime(startTime)
replicas := int32(3)

Expand All @@ -979,7 +1044,7 @@ func preparePayloads(t *testing.T, r *Reconciler, namespace, namePrefix string,
require.NoError(t, err)
err = r.AllNamespacesClient.Create(context.TODO(), rs)
require.NoError(t, err)
controlledPods := createPods(t, r, rs, sTime, make([]*corev1.Pod, 0, 3))
controlledPods := createPods(t, r, rs, sTime, make([]*corev1.Pod, 0, 3), conditions...)

// Deployment with Camel K integration as an owner reference and a scale sub resource
integration := &appsv1.Deployment{
Expand Down Expand Up @@ -1135,15 +1200,15 @@ func preparePayloads(t *testing.T, r *Reconciler, namespace, namePrefix string,
}
}

func preparePayloadsSinglePod(t *testing.T, r *Reconciler, namespace, namePrefix string, startTime time.Time) payloads {
func preparePayloadsSinglePod(t *testing.T, r *Reconciler, namespace, namePrefix string, startTime time.Time, conditions ...corev1.PodCondition) payloads {
sTime := metav1.NewTime(startTime)

// Pods with no owner.
standalonePods := make([]*corev1.Pod, 0, 1)
for i := 0; i < 1; i++ {
pod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("%s%s-pod-%d", namePrefix, namespace, i), Namespace: namespace},
Status: corev1.PodStatus{StartTime: &sTime},
Status: corev1.PodStatus{StartTime: &sTime, Conditions: conditions},
}
standalonePods = append(standalonePods, pod)
err := r.AllNamespacesClient.Create(context.TODO(), pod)
Expand All @@ -1154,11 +1219,11 @@ func preparePayloadsSinglePod(t *testing.T, r *Reconciler, namespace, namePrefix
}
}

func createPods(t *testing.T, r *Reconciler, owner metav1.Object, startTime metav1.Time, podsToTrack []*corev1.Pod) []*corev1.Pod {
func createPods(t *testing.T, r *Reconciler, owner metav1.Object, startTime metav1.Time, podsToTrack []*corev1.Pod, conditions ...corev1.PodCondition) []*corev1.Pod {
for i := 0; i < 3; i++ {
pod := &corev1.Pod{
ObjectMeta: metav1.ObjectMeta{Name: fmt.Sprintf("%s-pod-%d", owner.GetName(), i), Namespace: owner.GetNamespace()},
Status: corev1.PodStatus{StartTime: &startTime},
Status: corev1.PodStatus{StartTime: &startTime, Conditions: conditions},
}
err := controllerutil.SetControllerReference(owner, pod, r.Scheme)
require.NoError(t, err)
Expand Down

0 comments on commit 6669ed5

Please sign in to comment.