Skip to content

Commit

Permalink
chore(operator): refactor KeptnTask controller logic (keptn#1536)
Browse files Browse the repository at this point in the history 
Signed-off-by: odubajDT <[email protected]>
Signed-off-by: odubajDT <[email protected]>
Co-authored-by: Florian Bacher <[email protected]>
  • Loading branch information
@odubajDT @bacherfl @StackScribe
2 people authored and StackScribe committed Jun 22, 2023
1 parent 7289828 commit dd59c52
Show file tree
Hide file tree
Showing 10 changed files with 175 additions and 164 deletions.
26 changes: 14 additions & 12 deletions operator/controllers/lifecycle/keptntask/container_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,14 +13,18 @@ type ContainerBuilder struct {
spec *klcv1alpha3.ContainerSpec
}

func NewContainerBuilder(spec *klcv1alpha3.ContainerSpec) *ContainerBuilder {
func NewContainerBuilder(options BuilderOptions) *ContainerBuilder {
return &ContainerBuilder{
spec: spec,
spec: options.containerSpec,
}
}

func (c *ContainerBuilder) CreateContainerWithVolumes(ctx context.Context) (*corev1.Container, []corev1.Volume, error) {
return c.spec.Container, c.generateVolumes(), nil
func (c *ContainerBuilder) CreateContainer(ctx context.Context) (*corev1.Container, error) {
return c.spec.Container, nil
}

func (c *ContainerBuilder) CreateVolume(ctx context.Context) (*corev1.Volume, error) {
return c.generateVolume(), nil
}

func (c *ContainerBuilder) getVolumeSource() *corev1.EmptyDirVolumeSource {
Expand All @@ -39,16 +43,14 @@ func (c *ContainerBuilder) getVolumeSource() *corev1.EmptyDirVolumeSource {
}
}

func (c *ContainerBuilder) generateVolumes() []corev1.Volume {
func (c *ContainerBuilder) generateVolume() *corev1.Volume {
if !common.IsVolumeMountPresent(c.spec) {
return []corev1.Volume{}
return nil
}
return []corev1.Volume{
{
Name: c.spec.VolumeMounts[0].Name,
VolumeSource: corev1.VolumeSource{
EmptyDir: c.getVolumeSource(),
},
return &corev1.Volume{
Name: c.spec.VolumeMounts[0].Name,
VolumeSource: corev1.VolumeSource{
EmptyDir: c.getVolumeSource(),
},
}
}
116 changes: 59 additions & 57 deletions operator/controllers/lifecycle/keptntask/container_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,9 @@ func TestContainerBuilder_CreateContainerWithVolumes(t *testing.T) {
name string
builder ContainerBuilder
wantContainer *v1.Container
wantVolumes []v1.Volume
}{
{
name: "defined without volumes",
name: "defined",
builder: ContainerBuilder{
spec: &v1alpha3.ContainerSpec{
Container: &v1.Container{
Expand All @@ -29,7 +28,41 @@ func TestContainerBuilder_CreateContainerWithVolumes(t *testing.T) {
wantContainer: &v1.Container{
Image: "image",
},
wantVolumes: []v1.Volume{},
},
{
name: "nil",
builder: ContainerBuilder{
spec: &v1alpha3.ContainerSpec{
Container: nil,
},
},
wantContainer: nil,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
container, _ := tt.builder.CreateContainer(context.TODO())
require.Equal(t, tt.wantContainer, container)
})
}
}

func TestContainerBuilder_CreateVolume(t *testing.T) {
tests := []struct {
name string
builder ContainerBuilder
wantVolume *v1.Volume
}{
{
name: "defined without volume",
builder: ContainerBuilder{
spec: &v1alpha3.ContainerSpec{
Container: &v1.Container{
Image: "image",
},
},
},
wantVolume: nil,
},
{
name: "defined with volume",
Expand All @@ -46,23 +79,12 @@ func TestContainerBuilder_CreateContainerWithVolumes(t *testing.T) {
},
},
},
wantContainer: &v1.Container{
Image: "image",
VolumeMounts: []v1.VolumeMount{
{
Name: "test-volume",
MountPath: "path",
},
},
},
wantVolumes: []v1.Volume{
{
Name: "test-volume",
VolumeSource: v1.VolumeSource{
EmptyDir: &v1.EmptyDirVolumeSource{
SizeLimit: resource.NewQuantity(1, resource.Format("Gi")),
Medium: v1.StorageMedium("Memory"),
},
wantVolume: &v1.Volume{
Name: "test-volume",
VolumeSource: v1.VolumeSource{
EmptyDir: &v1.EmptyDirVolumeSource{
SizeLimit: resource.NewQuantity(1, resource.Format("Gi")),
Medium: v1.StorageMedium("Memory"),
},
},
},
Expand All @@ -87,39 +109,21 @@ func TestContainerBuilder_CreateContainerWithVolumes(t *testing.T) {
},
},
},

wantContainer: &v1.Container{
Image: "image",
VolumeMounts: []v1.VolumeMount{
{
Name: "test-volume",
MountPath: "path",
},
},
Resources: v1.ResourceRequirements{
Limits: v1.ResourceList{
"memory": *resource.NewQuantity(100, resource.Format("Mi")),
},
},
},
wantVolumes: []v1.Volume{
{
Name: "test-volume",
VolumeSource: v1.VolumeSource{
EmptyDir: &v1.EmptyDirVolumeSource{
SizeLimit: resource.NewQuantity(100, resource.Format("Mi")),
Medium: v1.StorageMedium("Memory"),
},
wantVolume: &v1.Volume{
Name: "test-volume",
VolumeSource: v1.VolumeSource{
EmptyDir: &v1.EmptyDirVolumeSource{
SizeLimit: resource.NewQuantity(100, resource.Format("Mi")),
Medium: v1.StorageMedium("Memory"),
},
},
},
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
container, volumes, _ := tt.builder.CreateContainerWithVolumes(context.TODO())
require.Equal(t, tt.wantContainer, container)
require.Equal(t, tt.wantVolumes, volumes)
volume, _ := tt.builder.CreateVolume(context.TODO())
require.Equal(t, tt.wantVolume, volume)
})
}
}
Expand All @@ -128,7 +132,7 @@ func Test_GenerateVolumes(t *testing.T) {
tests := []struct {
name string
spec *v1alpha3.ContainerSpec
want []v1.Volume
want *v1.Volume
}{
{
name: "defined",
Expand All @@ -143,30 +147,28 @@ func Test_GenerateVolumes(t *testing.T) {
},
},
},
want: []v1.Volume{
{
Name: "name",
VolumeSource: v1.VolumeSource{
EmptyDir: &v1.EmptyDirVolumeSource{
SizeLimit: resource.NewQuantity(1, resource.Format("Gi")),
Medium: v1.StorageMedium("Memory"),
},
want: &v1.Volume{
Name: "name",
VolumeSource: v1.VolumeSource{
EmptyDir: &v1.EmptyDirVolumeSource{
SizeLimit: resource.NewQuantity(1, resource.Format("Gi")),
Medium: v1.StorageMedium("Memory"),
},
},
},
},
{
name: "empty",
spec: &v1alpha3.ContainerSpec{},
want: []v1.Volume{},
want: nil,
},
}
for _, tt := range tests {
builder := ContainerBuilder{
spec: tt.spec,
}
t.Run(tt.name, func(t *testing.T) {
require.Equal(t, tt.want, builder.generateVolumes())
require.Equal(t, tt.want, builder.generateVolume())
})
}
}
Expand Down
8 changes: 2 additions & 6 deletions operator/controllers/lifecycle/keptntask/controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func (r *KeptnTaskReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
defer func() {
err := r.Client.Status().Update(ctx, task)
if err != nil {
r.Log.Error(err, "could not update status")
r.Log.Error(err, "could not update KeptnTask status reference for: "+task.Name)
}
}()

Expand All @@ -108,11 +108,7 @@ func (r *KeptnTaskReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
}

if !task.Status.Status.IsCompleted() {
err := r.updateJob(ctx, req, task)
if err != nil {
span.SetStatus(codes.Error, err.Error())
return ctrl.Result{Requeue: true, RequeueAfter: 10 * time.Second}, err
}
r.updateTaskStatus(job, task)
return ctrl.Result{Requeue: true, RequeueAfter: 10 * time.Second}, nil
}

Expand Down
9 changes: 5 additions & 4 deletions operator/controllers/lifecycle/keptntask/job_runner_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import (
// JobRunnerBuilder is the interface that describes the operations needed to help build job specs of a task
type JobRunnerBuilder interface {
// CreateContainerWithVolumes returns a job container and volumes based on the task definition spec
CreateContainerWithVolumes(ctx context.Context) (*corev1.Container, []corev1.Volume, error)
CreateContainer(ctx context.Context) (*corev1.Container, error)
CreateVolume(ctx context.Context) (*corev1.Volume, error)
}

// BuilderOptions contains everything needed to build the current job
Expand All @@ -30,12 +31,12 @@ type BuilderOptions struct {
ConfigMap string
}

func getJobRunnerBuilder(options BuilderOptions) JobRunnerBuilder {
func NewJobRunnerBuilder(options BuilderOptions) JobRunnerBuilder {
if options.funcSpec != nil {
return NewFunctionBuilder(options)
return NewRuntimeBuilder(options)
}
if options.containerSpec != nil {
return NewContainerBuilder(options.containerSpec)
return NewContainerBuilder(options)
}
return nil
}
10 changes: 5 additions & 5 deletions operator/controllers/lifecycle/keptntask/job_runner_builder_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import (
)

func Test_getJobRunnerBuilder(t *testing.T) {
functionBuilderOptions := BuilderOptions{
runtimeBuilderOptions := BuilderOptions{
funcSpec: &v1alpha3.RuntimeSpec{
Inline: v1alpha3.Inline{
Code: "some code",
Expand All @@ -30,13 +30,13 @@ func Test_getJobRunnerBuilder(t *testing.T) {
}{
{
name: "js builder",
options: functionBuilderOptions,
want: NewFunctionBuilder(functionBuilderOptions),
options: runtimeBuilderOptions,
want: NewRuntimeBuilder(runtimeBuilderOptions),
},
{
name: "container builder",
options: containerBuilderOptions,
want: NewContainerBuilder(containerBuilderOptions.containerSpec),
want: NewContainerBuilder(containerBuilderOptions),
},
{
name: "invalid builder",
Expand All @@ -46,7 +46,7 @@ func Test_getJobRunnerBuilder(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
require.Equal(t, tt.want, getJobRunnerBuilder(tt.options))
require.Equal(t, tt.want, NewJobRunnerBuilder(tt.options))
})
}
}
39 changes: 17 additions & 22 deletions operator/controllers/lifecycle/keptntask/job_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,11 +33,7 @@ func (r *KeptnTaskReconciler) createJob(ctx context.Context, req ctrl.Request, t

task.Status.JobName = jobName
task.Status.Status = apicommon.StatePending
err = r.Client.Status().Update(ctx, task)
if err != nil {
r.Log.Error(err, "could not update KeptnTask status reference for: "+task.Name)
}
r.Log.Info("updated configmap status reference for: " + definition.Name)

return nil
}

Expand All @@ -58,17 +54,7 @@ func (r *KeptnTaskReconciler) createFunctionJob(ctx context.Context, req ctrl.Re
return job.Name, nil
}

func (r *KeptnTaskReconciler) updateJob(ctx context.Context, req ctrl.Request, task *klcv1alpha3.KeptnTask) error {
job, err := r.getJob(ctx, task.Status.JobName, req.Namespace)
if err != nil {
task.Status.JobName = ""
controllercommon.RecordEvent(r.Recorder, apicommon.PhaseReconcileTask, "Warning", task, "JobReferenceRemoved", "removed Job Reference as Job could not be found", "")
err = r.Client.Status().Update(ctx, task)
if err != nil {
r.Log.Error(err, "could not remove job reference for: "+task.Name)
}
return err
}
func (r *KeptnTaskReconciler) updateTaskStatus(job *batchv1.Job, task *klcv1alpha3.KeptnTask) {
if len(job.Status.Conditions) > 0 {
if job.Status.Conditions[0].Type == batchv1.JobComplete {
task.Status.Status = apicommon.StateSucceeded
Expand All @@ -78,8 +64,8 @@ func (r *KeptnTaskReconciler) updateJob(ctx context.Context, req ctrl.Request, t
task.Status.Reason = job.Status.Conditions[0].Reason
}
}
return nil
}

func (r *KeptnTaskReconciler) getJob(ctx context.Context, jobName string, namespace string) (*batchv1.Job, error) {
job := &batchv1.Job{}
err := r.Client.Get(ctx, types.NamespacedName{Name: jobName, Namespace: namespace}, job)
Expand All @@ -101,6 +87,7 @@ func setupTaskContext(task *klcv1alpha3.KeptnTask) klcv1alpha3.TaskContext {
taskContext.ObjectType = "Application"
taskContext.AppVersion = task.Spec.AppVersion
}
taskContext.TaskType = string(task.Spec.Type)
taskContext.AppName = task.Spec.AppName

return taskContext
Expand Down Expand Up @@ -146,18 +133,26 @@ func (r *KeptnTaskReconciler) generateJob(ctx context.Context, task *klcv1alpha3
ConfigMap: definition.Status.Function.ConfigMap,
}

builder := getJobRunnerBuilder(builderOpt)
builder := NewJobRunnerBuilder(builderOpt)
if builder == nil {
return nil, controllererrors.ErrNoTaskDefinitionSpec
}

container, volumes, err := builder.CreateContainerWithVolumes(ctx)
container, err := builder.CreateContainer(ctx)
if err != nil {
return nil, fmt.Errorf("could not create container for Job: %w", err)
}

volume, err := builder.CreateVolume(ctx)
if err != nil {
r.Log.Error(err, "could not create Job")
return nil, controllererrors.ErrCannotMarshalParams
return nil, fmt.Errorf("could not create volume for Job: %w", err)
}

if volume != nil {
job.Spec.Template.Spec.Volumes = []corev1.Volume{*volume}
}

job.Spec.Template.Spec.Containers = []corev1.Container{*container}
job.Spec.Template.Spec.Volumes = volumes

return job, nil
}
Loading

0 comments on commit dd59c52

Please sign in to comment.