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

Do not requeue for running Job #237

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Do not requeue for running Job #237

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 11 additions & 12 deletions modules/common/job/job.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,15 @@ func NewJob(
job *batchv1.Job,
jobType string,
preserve bool,
timeout time.Duration,
timeout time.Duration, // unused
Copy link
Contributor

Choose a reason for hiding this comment

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

wondering if we should still support this, like when 0 passed it is not used and something higher to consider requeue ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My goal is to get rid of the requeue within this helper. I can even drop the field from the Job struct as I anyhow need to do adaptation PRs in other operators so I can do the signature change as well at the same time.

If we return the status of the job to the caller then the caller can decide to requeue or just return. Still I believe that we should never need to explicitly requeue due to a status of a k8s managed resource as we can watch that instead. (The situation can be different for non k8s resource statuses like when we depend on something to happen with openstack or galera itself)

Copy link
Contributor

Choose a reason for hiding this comment

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

should we proceed with this? looks like a valid and nice to have fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we should. If you have time feel free to pick it up. This needs adaptation in all our service-operators.

beforeHash string,
) *Job {

return &Job{
job: job,
jobType: jobType,
preserve: preserve,
timeout: timeout,
timeout: timeout, // unused
beforeHash: beforeHash,
changed: false,
}
Expand All @@ -68,14 +68,14 @@ func (j *Job) createJob(
})
if err != nil {
if k8s_errors.IsNotFound(err) {
h.GetLogger().Info(fmt.Sprintf("Job %s not found, reconcile in %s", j.job.Name, j.timeout))
return ctrl.Result{RequeueAfter: j.timeout}, nil
h.GetLogger().Info(fmt.Sprintf("Job %s not found", j.job.Name))
return ctrl.Result{}, err
}
return ctrl.Result{}, err
}
if op != controllerutil.OperationResultNone {
h.GetLogger().Info(fmt.Sprintf("Job %s %s - %s", j.jobType, j.job.Name, op))
return ctrl.Result{RequeueAfter: j.timeout}, nil
return ctrl.Result{}, nil
}

return ctrl.Result{}, nil
Expand Down Expand Up @@ -158,7 +158,7 @@ func (j *Job) DoJob(
}

if wait {
ctrlResult, err := waitOnJob(ctx, h, j.job.Name, j.job.Namespace, j.timeout)
ctrlResult, err := waitOnJob(ctx, h, j.job.Name, j.job.Namespace)
if (ctrlResult != ctrl.Result{}) {
return ctrlResult, nil
}
Expand Down Expand Up @@ -225,31 +225,30 @@ func waitOnJob(
h *helper.Helper,
name string,
namespace string,
timeout time.Duration,
) (ctrl.Result, error) {
// Check if this Job already exists
job, err := GetJobWithName(ctx, h, name, namespace)
if err != nil {
if k8s_errors.IsNotFound(err) {
h.GetLogger().Info("Job was not found.")
return ctrl.Result{RequeueAfter: timeout}, nil
return ctrl.Result{}, err
}
h.GetLogger().Info("WaitOnJob err")
return ctrl.Result{}, err
}

if job.Status.Active > 0 {
h.GetLogger().Info("Job Status Active... requeuing")
return ctrl.Result{RequeueAfter: timeout}, nil
h.GetLogger().Info("Job Status Active... returning")
return ctrl.Result{}, nil
} else if job.Status.Succeeded > 0 {
h.GetLogger().Info("Job Status Successful")
return ctrl.Result{}, nil
} else if job.Status.Failed > 0 {
h.GetLogger().Info("Job Status Failed")
return ctrl.Result{}, k8s_errors.NewInternalError(errors.New("Job Failed. Check job logs"))
}
h.GetLogger().Info("Job Status incomplete... requeuing")
return ctrl.Result{RequeueAfter: timeout}, nil
h.GetLogger().Info("Job Status incomplete... returning")
return ctrl.Result{}, nil
}

// GetJobWithName func
Expand Down