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

Jump to bottom

Simplify instance controller reconciliation #1551

Merged
merged 7 commits into from
Jun 8, 2020
Merged

Simplify instance controller reconciliation #1551

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b217482
Simplify instance controller reconciliation
zen-dog Jun 4, 2020
09583c9
making linter happy
zen-dog Jun 4, 2020
10fdf9a
review feedback with logging and naming improvements
zen-dog Jun 5, 2020
649fec9
fixed some typos
zen-dog Jun 5, 2020
cd69d8d
initializing plan status only when the plan is new or upgrade
zen-dog Jun 5, 2020
b032fbb
simplified `ensurePlanStatusInitialized` method and added a test for it
zen-dog Jun 5, 2020
a3e6a98
making linter happy
zen-dog Jun 5, 2020
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
61 changes: 7 additions & 54 deletions pkg/apis/kudo/v1beta1/instance_types_helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,20 +2,17 @@ package v1beta1

import (
"context"
"encoding/json"
"fmt"
"log"

"github.com/thoas/go-funk"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/uuid"
"sigs.k8s.io/controller-runtime/pkg/client"
)

const (
instanceCleanupFinalizerName = "kudo.dev.instance.cleanup"
snapshotAnnotation = "kudo.dev/last-applied-instance-state"
)

// ScheduledPlan returns plan status of currently active plan or nil if no plan is running. In most cases this method
Expand Down Expand Up @@ -88,34 +85,19 @@ func (i *Instance) UpdateInstanceStatus(planStatus *PlanStatus, updatedTimestamp
}
}

// ResetPlanStatus method resets a PlanStatus for a passed plan name and instance. Plan/phase/step statuses
// are set to ExecutionPending meaning that the controller will restart plan execution.
nfnt marked this conversation as resolved.
Show resolved Hide resolved
func (i *Instance) ResetPlanStatus(plan string, updatedTimestamp *metav1.Time) error {
planStatus := i.PlanStatus(plan)
if planStatus == nil {
return fmt.Errorf("failed to find planStatus for the plan '%s'", plan)
}

// reset plan's phases and steps by setting them to ExecutionPending
planStatus.Set(ExecutionPending)
// when using webhooks, instance admission webhook already generates an UID for current plan, otherwise, we generate a new one.
if i.Spec.PlanExecution.UID != "" {
planStatus.UID = i.Spec.PlanExecution.UID
} else {
planStatus.UID = uuid.NewUUID()
}

for i, ph := range planStatus.Phases {
planStatus.Phases[i].Set(ExecutionPending)
func (i *Instance) ResetPlanStatus(ps *PlanStatus, uid types.UID, updatedTimestamp *metav1.Time) {
zen-dog marked this conversation as resolved.
Show resolved Hide resolved
ps.UID = uid
ps.Status = ExecutionPending
for i, ph := range ps.Phases {
ps.Phases[i].Set(ExecutionPending)

for j := range ph.Steps {
zen-dog marked this conversation as resolved.
Show resolved Hide resolved
planStatus.Phases[i].Steps[j].Set(ExecutionPending)
ps.Phases[i].Steps[j].Set(ExecutionPending)
}
}

// update plan status and instance aggregated status
i.UpdateInstanceStatus(planStatus, updatedTimestamp)
return nil
i.UpdateInstanceStatus(ps, updatedTimestamp)
}

// IsDeleting returns true is the instance is being deleted.
Expand Down Expand Up @@ -145,35 +127,6 @@ func (i *Instance) PlanStatus(plan string) *PlanStatus {
return nil
}

// annotateSnapshot stores the current spec of Instance into the snapshot annotation
// this information is used when executing update/upgrade plans, this overrides any snapshot that existed before
func (i *Instance) AnnotateSnapshot() error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fun fact: we don't need to save instance spec in the annotation anymore

Copy link
Member

Choose a reason for hiding this comment

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

Fun fact 2: It's probably coming back for correct patching from the client side ;) But that doesn't concern the instance controller anymore, and it's good that it's gone here, having two versions in the annotations would be quite confusing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fun fact 3: I was hoping that we can avoid that and use patchStrategy: replace on the corresponding instance spec fields 😉 But yeah, having two of these would be very confusing.

jsonBytes, err := json.Marshal(i.Spec)
if err != nil {
return err
}
if i.Annotations == nil {
i.Annotations = make(map[string]string)
}
i.Annotations[snapshotAnnotation] = string(jsonBytes)
return nil
}

func (i *Instance) SnapshotSpec() (*InstanceSpec, error) {
if i.Annotations != nil {
snapshot, ok := i.Annotations[snapshotAnnotation]
if ok {
var spec *InstanceSpec
err := json.Unmarshal([]byte(snapshot), &spec)
if err != nil {
return nil, err
}
return spec, nil
}
}
return nil, nil
}

func (i *Instance) HasCleanupFinalizer() bool {
return funk.ContainsString(i.ObjectMeta.Finalizers, instanceCleanupFinalizerName)
}
Expand Down
77 changes: 0 additions & 77 deletions pkg/apis/kudo/v1beta1/instance_types_helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,11 @@ import (
"time"

"github.com/stretchr/testify/assert"
v1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/uuid"
)

var (
testTime = time.Date(2019, 10, 17, 1, 1, 1, 1, time.UTC)
testUUID = uuid.NewUUID()
)

func TestGetLastExecutedPlanStatus(t *testing.T) {
Expand Down Expand Up @@ -81,80 +78,6 @@ func TestGetLastExecutedPlanStatus(t *testing.T) {
}
}

func TestInstance_ResetPlanStatus(t *testing.T) {
// a test instance with 'deploy' plan IN_PROGRESS and 'update' that NEVER_RUN
instance := &Instance{
TypeMeta: metav1.TypeMeta{
APIVersion: "kudo.dev/v1beta1",
Kind: "Instance",
},
ObjectMeta: metav1.ObjectMeta{
Name: "test",
Namespace: "test",
},
Spec: InstanceSpec{
OperatorVersion: v1.ObjectReference{
Name: "foo-operator",
},
Parameters: map[string]string{
"foo": "foo",
},
},
Status: InstanceStatus{
PlanStatus: map[string]PlanStatus{
"deploy": {
Status: ExecutionInProgress,
Name: "deploy",
LastUpdatedTimestamp: &metav1.Time{Time: testTime},
UID: testUUID,
Phases: []PhaseStatus{{Name: "phase", Status: ExecutionInProgress, Steps: []StepStatus{{Status: ExecutionInProgress, Name: "step"}}}},
},
"update": {
Status: ExecutionNeverRun,
Name: "update",
Phases: []PhaseStatus{{Name: "phase", Status: ExecutionNeverRun, Steps: []StepStatus{{Status: ExecutionNeverRun, Name: "step"}}}},
},
},
},
}

oldUID := instance.Status.PlanStatus["deploy"].UID

err := instance.ResetPlanStatus("deploy", &metav1.Time{Time: time.Now()})
assert.NoError(t, err)

// we test that UID has changed. afterwards, we replace it with the old one and compare new
// plan status with the desired state
assert.NotEqual(t, instance.Status.PlanStatus["deploy"].UID, oldUID)
assert.Equal(t, instance.Spec.PlanExecution.Status, ExecutionPending)

oldPlanStatus := instance.Status.PlanStatus["deploy"]
statusCopy := oldPlanStatus.DeepCopy()
statusCopy.UID = testUUID
statusCopy.LastUpdatedTimestamp = &metav1.Time{Time: testTime}
instance.Status.PlanStatus["deploy"] = *statusCopy

// Expected:
// - the status of the 'deploy' plan to be reset: all phases and steps should be PENDING, new UID should be assigned
// - 'update' plan status should be unchanged
assert.Equal(t, InstanceStatus{
PlanStatus: map[string]PlanStatus{
"deploy": {
Status: ExecutionPending,
Name: "deploy",
LastUpdatedTimestamp: &metav1.Time{Time: testTime},
UID: testUUID,
Phases: []PhaseStatus{{Name: "phase", Status: ExecutionPending, Steps: []StepStatus{{Status: ExecutionPending, Name: "step"}}}},
},
"update": {
Status: ExecutionNeverRun,
Name: "update",
Phases: []PhaseStatus{{Name: "phase", Status: ExecutionNeverRun, Steps: []StepStatus{{Status: ExecutionNeverRun, Name: "step"}}}},
},
},
}, instance.Status)
}

func TestGetParamDefinitions(t *testing.T) {
ov := &OperatorVersion{
ObjectMeta: metav1.ObjectMeta{Name: "foo-operator", Namespace: "default"},
Expand Down
Loading