-
Notifications
You must be signed in to change notification settings - Fork 104
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.
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
KEP-29: Introducing operator dependency cycle detection to the instance controller #1559
Changes from 4 commits
226ac95
73fa0c6
1d938e1
2809619
6dc4e9e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,18 +15,13 @@ const ( | |
instanceCleanupFinalizerName = "kudo.dev.instance.cleanup" | ||
) | ||
|
||
// ScheduledPlan returns plan status of currently active plan or nil if no plan is running. In most cases this method | ||
// will return the same plan status as the [GetPlanInProgress](pkg/apis/kudo/v1beta1/instance_types_helpers.go:25) below. | ||
// However, there is a small window where both might return different results: | ||
// 1. GetScheduledPlan reads the plan from i.Spec.PlanExecution.PlanName which is set and reset by the instance admission | ||
// webhook | ||
// 2. GetPlanInProgress goes through i.Spec.PlanStatus map and returns the first found plan that is running | ||
// | ||
// In (1), i.Spec.PlanExecution.PlanName is set directly when the user updates the instance and reset **after** the plan | ||
// is terminal | ||
// In (2) i.Spec.PlanStatus is updated **AFTER** the instance controller is done with the reconciliation call | ||
func (i *Instance) GetScheduledPlan() *PlanStatus { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not used |
||
return i.PlanStatus(i.Spec.PlanExecution.PlanName) | ||
func GetInstance(namespacedName types.NamespacedName, c client.Client) (i *Instance, err error) { | ||
i = &Instance{} | ||
err = c.Get(context.TODO(), namespacedName, i) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return i, nil | ||
} | ||
|
||
// GetPlanInProgress returns plan status of currently active plan or nil if no plan is running | ||
|
@@ -186,18 +181,18 @@ func (i *Instance) GetOperatorVersion(c client.Reader) (ov *OperatorVersion, err | |
return GetOperatorVersionByName(i.Spec.OperatorVersion.Name, i.OperatorVersionNamespace(), c) | ||
} | ||
|
||
func GetOperatorVersionByName(ovn, ns string, c client.Reader) (ov *OperatorVersion, err error) { | ||
ov = &OperatorVersion{} | ||
err = c.Get(context.TODO(), | ||
types.NamespacedName{ | ||
Name: ovn, | ||
Namespace: ns, | ||
}, | ||
ov) | ||
if err != nil { | ||
return nil, err | ||
// IsChildInstance method return true if this instance is owned by another instance (as a dependency) and false otherwise | ||
func (i *Instance) IsChildInstance() bool { | ||
for _, or := range i.GetOwnerReferences() { | ||
if or.Kind == i.Kind { | ||
nfnt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return true | ||
} | ||
} | ||
return ov, nil | ||
return false | ||
} | ||
|
||
func (i *Instance) IsTopLevelInstance() bool { | ||
return !i.IsChildInstance() | ||
} | ||
|
||
// wasRunAfter returns true if p1 was run after p2 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
package v1beta1 | ||
|
||
import ( | ||
"context" | ||
|
||
"k8s.io/apimachinery/pkg/types" | ||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
) | ||
|
||
func GetOperator(name, ns string, c client.Client) (*Operator, error) { | ||
o := &Operator{} | ||
err := c.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: ns}, o) | ||
if err != nil { | ||
return nil, err | ||
} | ||
return o, nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,6 +50,8 @@ import ( | |
"github.com/kudobuilder/kudo/pkg/engine/renderer" | ||
"github.com/kudobuilder/kudo/pkg/engine/task" | ||
"github.com/kudobuilder/kudo/pkg/engine/workflow" | ||
"github.com/kudobuilder/kudo/pkg/kudoctl/packages" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With package handling no longer exclusive to the CLI, let's move the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mind but let's do it in a separate PR |
||
"github.com/kudobuilder/kudo/pkg/kudoctl/packages/install" | ||
"github.com/kudobuilder/kudo/pkg/util/convert" | ||
) | ||
|
||
|
@@ -197,6 +199,17 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) { | |
r.Recorder.Event(instance, "Normal", "PlanStarted", fmt.Sprintf("Execution of plan %s started", plan)) | ||
} | ||
|
||
// if this is a top-level instance, check for dependency cycles | ||
if instance.IsTopLevelInstance() { | ||
err := r.resolveDependencies(instance, ov) | ||
if err != nil { | ||
planStatus.SetWithMessage(kudov1beta1.ExecutionFatalError, err.Error()) | ||
instance.UpdateInstanceStatus(planStatus, &metav1.Time{Time: time.Now()}) | ||
err = r.handleError(err, instance, oldInstance) | ||
return reconcile.Result{}, err | ||
} | ||
} | ||
|
||
// ---------- 3. Execute the scheduled plan ---------- | ||
|
||
metadata := &engine.Metadata{ | ||
|
@@ -241,8 +254,18 @@ func (r *Reconciler) Reconcile(request ctrl.Request) (ctrl.Result, error) { | |
return reconcile.Result{}, nil | ||
} | ||
|
||
func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Instance, client client.Client) error { | ||
func (r *Reconciler) resolveDependencies(i *kudov1beta1.Instance, ov *kudov1beta1.OperatorVersion) error { | ||
resolver := &InClusterResolver{ns: i.Namespace, c: r.Client} | ||
root := packages.Resources{OperatorVersion: ov} | ||
|
||
_, err := install.ResolveDependencies(root, resolver) | ||
if err != nil { | ||
return engine.ExecutionError{Err: fmt.Errorf("%w%v", engine.ErrFatalExecution, err), EventName: "CircularDependency"} | ||
} | ||
return nil | ||
} | ||
|
||
func updateInstance(instance *kudov1beta1.Instance, oldInstance *kudov1beta1.Instance, client client.Client) error { | ||
// The order of both updates below is important: *first* the instance Spec and Metadata and *then* the Status. | ||
// If Status is updated first, a new reconcile request will be scheduled and might fetch the *WRONG* instance | ||
// Spec.PlanExecution. This request will then try to execute an already finished plan (again). | ||
|
@@ -331,22 +354,13 @@ func (r *Reconciler) handleError(err error, instance *kudov1beta1.Instance, oldI | |
} | ||
} | ||
|
||
// for code being processed on instance, we need to handle these errors as well | ||
var iError *kudov1beta1.InstanceError | ||
if errors.As(err, &iError) { | ||
if iError.EventName != nil { | ||
r.Recorder.Event(instance, "Warning", convert.StringValue(iError.EventName), err.Error()) | ||
} | ||
} | ||
return err | ||
} | ||
|
||
// getInstance retrieves the instance by namespaced name | ||
func (r *Reconciler) getInstance(request ctrl.Request) (instance *kudov1beta1.Instance, err error) { | ||
instance = &kudov1beta1.Instance{} | ||
err = r.Get(context.TODO(), request.NamespacedName, instance) | ||
instance, err = kudov1beta1.GetInstance(request.NamespacedName, r.Client) | ||
if err != nil { | ||
// Error reading the object - requeue the request. | ||
log.Printf("InstanceController: Error getting instance %v: %v", | ||
request.NamespacedName, | ||
err) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
package instance | ||
|
||
import ( | ||
"fmt" | ||
|
||
"sigs.k8s.io/controller-runtime/pkg/client" | ||
|
||
"github.com/kudobuilder/kudo/pkg/apis/kudo/v1beta1" | ||
"github.com/kudobuilder/kudo/pkg/kudoctl/packages" | ||
) | ||
|
||
// InClusterResolver resolves packages that are already installed in the cluster. Note, that unlike other resolvers, the | ||
// resulting 'packages.Package' struct does not contain package 'packages.Files' (we don't have the original files) and | ||
// doesn't have an Instance resource because multiple Instances of the same Operator/OperatorVersion can exist | ||
type InClusterResolver struct { | ||
c client.Client | ||
ns string | ||
} | ||
|
||
func (r InClusterResolver) Resolve(name string, appVersion string, operatorVersion string) (*packages.Package, error) { | ||
ovn := v1beta1.OperatorVersionName(name, operatorVersion) | ||
|
||
ov, err := v1beta1.GetOperatorVersionByName(ovn, r.ns, r.c) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to resolve operator version %s/%s:%s", r.ns, ovn, appVersion) | ||
} | ||
|
||
// sanity check, as there is an explicit 1:1 relationship between an operator and app version | ||
if ov.Spec.AppVersion != appVersion { | ||
return nil, fmt.Errorf("found operator version %s/%s but found appVersion %s is not equal to the requested %s", r.ns, ovn, ov.Spec.AppVersion, appVersion) | ||
} | ||
|
||
o, err := v1beta1.GetOperator(name, r.ns, r.c) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to resolve operator %s/%s", r.ns, name) | ||
} | ||
|
||
return &packages.Package{ | ||
Resources: &packages.Resources{ | ||
Operator: o, | ||
OperatorVersion: ov, | ||
Instance: nil, | ||
}, | ||
Files: nil, | ||
}, nil | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not used