Skip to content

minor cleanup and refactoring for consistency and simplicity #40

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

Closed
wants to merge 2 commits into from
Closed

minor cleanup and refactoring for consistency and simplicity #40

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e12c4ec
minor cleanup and refactoring for consistency and simplicity
joelanford Sep 7, 2022
e080b5e
address PR feedback
joelanford Sep 27, 2022
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
45 changes: 32 additions & 13 deletions controllers/aggregated_clusteroperator_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"context"

configv1 "github.com/openshift/api/config/v1"
rukpakv1alpha1 "github.com/operator-framework/rukpak/api/v1alpha1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -62,40 +64,43 @@ func (r *AggregatedClusterOperatorReconciler) Reconcile(ctx context.Context, req
}
defer func() {
if err := coWriter.UpdateStatus(ctx, aggregatedCO, coBuilder.GetStatus()); err != nil {
log.Error(err, "error updating CO status")
log.Error(err, "error updating cluster operator status")
Copy link
Contributor

@tylerslaton tylerslaton Sep 27, 2022

Choose a reason for hiding this comment

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

nit:

Suggested change
log.Error(err, "error updating cluster operator status")
log.Error(err, "error updating ClusterOperator status")

All other instances apply if we want to change this.

}
}()

// Set the default CO status conditions: Progressing=True, Degraded=False, Available=False
coBuilder.WithProgressing(configv1.ConditionTrue, "")
coBuilder.WithDegraded(configv1.ConditionFalse)
coBuilder.WithAvailable(configv1.ConditionFalse, "", "")
// TODO: always set a reason (message is optional, but desirable)
coBuilder.WithProgressing(metav1.ConditionTrue, "", "")
coBuilder.WithDegraded(metav1.ConditionFalse, "", "")
coBuilder.WithAvailable(metav1.ConditionFalse, "", "")
coBuilder.WithVersion("operator", r.ReleaseVersion)
coBuilder.WithRelatedObject("", "namespaces", "", r.SystemNamespace)
coBuilder.WithRelatedObject("platform.openshift.io", "platformoperators", "", "")
coBuilder.WithRelatedObject("core.rukpak.io", "bundles", "", "")
coBuilder.WithRelatedObject("core.rukpak.io", "bundledeployments", "", "")

// Set the static set of related objects
setStaticRelatedObjects(coBuilder, r.SystemNamespace)

poList := &platformv1alpha1.PlatformOperatorList{}
if err := r.List(ctx, poList); err != nil {
return ctrl.Result{}, err
}
if len(poList.Items) == 0 {
// No POs on cluster, everything is fine
coBuilder.WithAvailable(configv1.ConditionTrue, "No POs are present in the cluster", "NoPOsFound")
coBuilder.WithProgressing(configv1.ConditionFalse, "No POs are present in the cluster")
coBuilder.WithAvailable(metav1.ConditionTrue, clusteroperator.ReasonAsExpected, "No platform operators are present in the cluster")
coBuilder.WithProgressing(metav1.ConditionFalse, clusteroperator.ReasonAsExpected, "No platform operators are present in the cluster")
return ctrl.Result{}, nil
}

// check whether any of the underlying PO resources are reporting
// any failing status states, and update the aggregate CO resource
// to reflect those failing PO resources.

// TODO: consider something more fine-grained than a catch-all "PlatformOperatorError" reason.
// There's a non-negligible difference between "PO is explicitly failing installation" and "PO is not yet installed"
Comment on lines +96 to +97
Copy link
Contributor

@tylerslaton tylerslaton Sep 27, 2022

Choose a reason for hiding this comment

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

Maybe we could define these two conditions as reasons and then bubble them up via util.InspectPlatformOperators() return? That way we can set that reason inside this WithAvailable() call and
have a less generic "PlatformOperatorError" reason.

if statusErrorCheck := util.InspectPlatformOperators(poList); statusErrorCheck != nil {
coBuilder.WithAvailable(configv1.ConditionFalse, statusErrorCheck.Error(), "POError")
coBuilder.WithAvailable(metav1.ConditionFalse, clusteroperator.ReasonPlatformOperatorError, statusErrorCheck.Error())
return ctrl.Result{}, nil
}
coBuilder.WithAvailable(configv1.ConditionTrue, "All POs in a successful state", "POsHealthy")
coBuilder.WithProgressing(configv1.ConditionFalse, "All POs in a successful state")
coBuilder.WithAvailable(metav1.ConditionTrue, clusteroperator.ReasonAsExpected, "All platform operators are in a successful state")
Copy link
Contributor

@tylerslaton tylerslaton Sep 27, 2022

Choose a reason for hiding this comment

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

nit:

Suggested change
coBuilder.WithAvailable(metav1.ConditionTrue, clusteroperator.ReasonAsExpected, "All platform operators are in a successful state")
coBuilder.WithAvailable(metav1.ConditionTrue, clusteroperator.ReasonAsExpected, "All PlatformOperators are in a successful state")

Applies to all other instances if we want to fix this.

coBuilder.WithProgressing(metav1.ConditionFalse, clusteroperator.ReasonAsExpected, "All platform operators are in a successful state")

return ctrl.Result{}, nil
}
Expand All @@ -109,3 +114,17 @@ func (r *AggregatedClusterOperatorReconciler) SetupWithManager(mgr ctrl.Manager)
Watches(&source.Kind{Type: &platformv1alpha1.PlatformOperator{}}, handler.EnqueueRequestsFromMapFunc(util.RequeueClusterOperator(mgr.GetClient(), clusteroperator.AggregateResourceName))).
Complete(r)
}

func setStaticRelatedObjects(coBuilder *clusteroperator.Builder, systemNamespace string) {
coBuilder.
WithRelatedObject(configv1.ObjectReference{Group: "", Resource: "namespaces", Name: systemNamespace}).

// NOTE: Group and resource can be referenced without name/namespace set, which is a signal
// that _ALL_ objects of that group/resource are related objects. This is useful for
// must-gather automation.
WithRelatedObject(configv1.ObjectReference{Group: platformv1alpha1.GroupName, Resource: "platformoperators"}).

// TODO: move platform operator ownership of rukpak objects out prior to rukpak or PO GA.
WithRelatedObject(configv1.ObjectReference{Group: rukpakv1alpha1.GroupVersion.Group, Resource: "bundles"}).
WithRelatedObject(configv1.ObjectReference{Group: rukpakv1alpha1.GroupVersion.Group, Resource: "bundledeployments"})
}
Loading