-
Notifications
You must be signed in to change notification settings - Fork 64
⚠️ Remove Deppy solver #758
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
Changes from all commits
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 |
---|---|---|
|
@@ -19,8 +19,10 @@ package controllers | |
import ( | ||
"context" | ||
"fmt" | ||
"sort" | ||
"strings" | ||
|
||
mmsemver "github.com/Masterminds/semver/v3" | ||
"github.com/go-logr/logr" | ||
"k8s.io/apimachinery/pkg/api/equality" | ||
apimeta "k8s.io/apimachinery/pkg/api/meta" | ||
|
@@ -37,22 +39,20 @@ import ( | |
"sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
|
||
catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" | ||
"github.com/operator-framework/deppy/pkg/deppy" | ||
"github.com/operator-framework/deppy/pkg/deppy/solver" | ||
"github.com/operator-framework/operator-registry/alpha/declcfg" | ||
rukpakv1alpha2 "github.com/operator-framework/rukpak/api/v1alpha2" | ||
|
||
ocv1alpha1 "github.com/operator-framework/operator-controller/api/v1alpha1" | ||
"github.com/operator-framework/operator-controller/internal/catalogmetadata" | ||
olmvariables "github.com/operator-framework/operator-controller/internal/resolution/variables" | ||
catalogfilter "github.com/operator-framework/operator-controller/internal/catalogmetadata/filter" | ||
catalogsort "github.com/operator-framework/operator-controller/internal/catalogmetadata/sort" | ||
) | ||
|
||
// ClusterExtensionReconciler reconciles a ClusterExtension object | ||
type ClusterExtensionReconciler struct { | ||
client.Client | ||
BundleProvider BundleProvider | ||
Scheme *runtime.Scheme | ||
Resolver *solver.Solver | ||
} | ||
|
||
//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clusterextensions,verbs=get;list;watch | ||
|
@@ -116,33 +116,8 @@ func checkForUnexpectedFieldChange(a, b ocv1alpha1.ClusterExtension) bool { | |
// | ||
//nolint:unparam | ||
func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (ctrl.Result, error) { | ||
// gather vars for resolution | ||
vars, err := r.variables(ctx) | ||
if err != nil { | ||
ext.Status.InstalledBundle = nil | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted due to failure to gather data for resolution", ext.GetGeneration()) | ||
ext.Status.ResolvedBundle = nil | ||
setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
|
||
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted due to failure to gather data for resolution", ext.GetGeneration()) | ||
return ctrl.Result{}, err | ||
} | ||
|
||
// run resolution | ||
selection, err := r.Resolver.Solve(vars) | ||
if err != nil { | ||
ext.Status.InstalledBundle = nil | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration()) | ||
ext.Status.ResolvedBundle = nil | ||
setResolvedStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration()) | ||
|
||
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as resolution failed", ext.GetGeneration()) | ||
return ctrl.Result{}, err | ||
} | ||
|
||
// lookup the bundle in the solution that corresponds to the | ||
// ClusterExtension's desired package name. | ||
bundle, err := r.bundleFromSolution(selection, ext.Spec.PackageName) | ||
// Lookup the bundle that corresponds to the ClusterExtension's desired package. | ||
bundle, err := r.resolve(ctx, ext) | ||
if err != nil { | ||
ext.Status.InstalledBundle = nil | ||
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration()) | ||
|
@@ -202,21 +177,105 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp | |
return ctrl.Result{}, nil | ||
} | ||
|
||
func (r *ClusterExtensionReconciler) variables(ctx context.Context) ([]deppy.Variable, error) { | ||
func (r *ClusterExtensionReconciler) resolve(ctx context.Context, ext *ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) { | ||
allBundles, err := r.BundleProvider.Bundles(ctx) | ||
if err != nil { | ||
return nil, err | ||
} | ||
clusterExtensionList := ocv1alpha1.ClusterExtensionList{} | ||
if err := r.Client.List(ctx, &clusterExtensionList); err != nil { | ||
|
||
installedBundle, err := r.installedBundle(ctx, allBundles, ext) | ||
if err != nil { | ||
return nil, err | ||
} | ||
bundleDeploymentList := rukpakv1alpha2.BundleDeploymentList{} | ||
if err := r.Client.List(ctx, &bundleDeploymentList); err != nil { | ||
|
||
packageName := ext.Spec.PackageName | ||
channelName := ext.Spec.Channel | ||
versionRange := ext.Spec.Version | ||
|
||
predicates := []catalogfilter.Predicate[catalogmetadata.Bundle]{ | ||
catalogfilter.WithPackageName(packageName), | ||
} | ||
|
||
if channelName != "" { | ||
predicates = append(predicates, catalogfilter.InChannel(channelName)) | ||
} | ||
|
||
if versionRange != "" { | ||
vr, err := mmsemver.NewConstraint(versionRange) | ||
if err != nil { | ||
return nil, fmt.Errorf("invalid version range %q: %w", versionRange, err) | ||
} | ||
predicates = append(predicates, catalogfilter.InMastermindsSemverRange(vr)) | ||
} | ||
|
||
if ext.Spec.UpgradeConstraintPolicy != ocv1alpha1.UpgradeConstraintPolicyIgnore && installedBundle != nil { | ||
upgradePredicate, err := SuccessorsPredicate(installedBundle) | ||
if err != nil { | ||
return nil, err | ||
} | ||
|
||
predicates = append(predicates, upgradePredicate) | ||
} | ||
|
||
resultSet := catalogfilter.Filter(allBundles, catalogfilter.And(predicates...)) | ||
|
||
var upgradeErrorPrefix string | ||
if installedBundle != nil { | ||
installedBundleVersion, err := installedBundle.Version() | ||
if err != nil { | ||
return nil, err | ||
} | ||
upgradeErrorPrefix = fmt.Sprintf("error upgrading from currently installed version %q: ", installedBundleVersion.String()) | ||
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. Nit: 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. Wrapping errors makes them part of the API and I believe we currently do not have a use case for type checking. I suggest to leave errors opaque and add types, if/when we need them. |
||
} | ||
if len(resultSet) == 0 { | ||
if versionRange != "" && channelName != "" { | ||
return nil, fmt.Errorf("%sno package %q matching version %q found in channel %q", upgradeErrorPrefix, packageName, versionRange, channelName) | ||
} | ||
if versionRange != "" { | ||
return nil, fmt.Errorf("%sno package %q matching version %q found", upgradeErrorPrefix, packageName, versionRange) | ||
} | ||
if channelName != "" { | ||
return nil, fmt.Errorf("%sno package %q found in channel %q", upgradeErrorPrefix, packageName, channelName) | ||
} | ||
return nil, fmt.Errorf("%sno package %q found", upgradeErrorPrefix, packageName) | ||
} | ||
sort.SliceStable(resultSet, func(i, j int) bool { | ||
return catalogsort.ByVersion(resultSet[i], resultSet[j]) | ||
}) | ||
sort.SliceStable(resultSet, func(i, j int) bool { | ||
return catalogsort.ByDeprecated(resultSet[i], resultSet[j]) | ||
}) | ||
|
||
return resultSet[0], nil | ||
} | ||
|
||
func (r *ClusterExtensionReconciler) installedBundle(ctx context.Context, allBundles []*catalogmetadata.Bundle, ext *ocv1alpha1.ClusterExtension) (*catalogmetadata.Bundle, error) { | ||
bd := &rukpakv1alpha2.BundleDeployment{} | ||
err := r.Client.Get(ctx, types.NamespacedName{Name: ext.GetName()}, bd) | ||
if client.IgnoreNotFound(err) != nil { | ||
return nil, err | ||
} | ||
|
||
return GenerateVariables(allBundles, clusterExtensionList.Items, bundleDeploymentList.Items) | ||
if bd.Spec.Source.Image == nil || bd.Spec.Source.Image.Ref == "" { | ||
// Bundle not yet installed | ||
return nil, nil | ||
} | ||
|
||
bundleImage := bd.Spec.Source.Image.Ref | ||
// find corresponding bundle for the installed content | ||
resultSet := catalogfilter.Filter(allBundles, catalogfilter.And( | ||
catalogfilter.WithPackageName(ext.Spec.PackageName), | ||
catalogfilter.WithBundleImage(bundleImage), | ||
)) | ||
if len(resultSet) == 0 { | ||
return nil, fmt.Errorf("bundle with image %q for package %q not found in available catalogs but is currently installed via BundleDeployment %q", bundleImage, ext.Spec.PackageName, bd.Name) | ||
} | ||
|
||
sort.SliceStable(resultSet, func(i, j int) bool { | ||
return catalogsort.ByVersion(resultSet[i], resultSet[j]) | ||
}) | ||
|
||
return resultSet[0], nil | ||
} | ||
|
||
func mapBDStatusToInstalledCondition(existingTypedBundleDeployment *rukpakv1alpha2.BundleDeployment, ext *ocv1alpha1.ClusterExtension, bundle *catalogmetadata.Bundle) { | ||
|
@@ -346,19 +405,6 @@ func SetDeprecationStatus(ext *ocv1alpha1.ClusterExtension, bundle *catalogmetad | |
} | ||
} | ||
|
||
func (r *ClusterExtensionReconciler) bundleFromSolution(selection []deppy.Variable, packageName string) (*catalogmetadata.Bundle, error) { | ||
for _, variable := range selection { | ||
switch v := variable.(type) { | ||
case *olmvariables.BundleVariable: | ||
bundlePkgName := v.Bundle().Package | ||
if packageName == bundlePkgName { | ||
return v.Bundle(), nil | ||
} | ||
} | ||
} | ||
return nil, fmt.Errorf("bundle for package %q not found in solution", packageName) | ||
} | ||
|
||
func (r *ClusterExtensionReconciler) GenerateExpectedBundleDeployment(o ocv1alpha1.ClusterExtension, bundlePath string, bundleProvisioner string) *unstructured.Unstructured { | ||
// We use unstructured here to avoid problems of serializing default values when sending patches to the apiserver. | ||
// If you use a typed object, any default values from that struct get serialized into the JSON patch, which could | ||
|
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.
At some point, I know we talked about adding some labels or annotations to the BD for the installed package name and version and using that in order to derive successors.
Looking on the removed code in the installed_package variable source, I see that we never made that change. So I think it's fine to stick with that same logic like we are in this PR. But I also think we should capture a follow-up issue (if we don't already have one), to implement the installed bundle detection that way.
Somewhat related: I don't think we should have to find the existing installed bundle in the catalog in order to be able to upgrade from it.
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.
@joelanford I can't find the thread in upstream slack, but I think we ended up deciding against using labels and in favour of digests which were supposed to support non-image sources. We have this issue for non-image bundle types support and the idea with digests captured here.
I might be missing something, but in order to support both server and legacy semantics we seem to need the following data:
Now that we have #679 we might be able to get this info from
ClusterExtensions
's status, but we probably don't want to to read fromClusterExtensions
's status while reconcilingClusterExtensions
.Even if we store this data in labels and use on upgrades - it will be an equivalent of looking at
ClusterExtensions
's status.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.
Agreed we should not read from
ClusterExtension
status. I think @varshaprasad96 is planning to include that information as labels on the Helm release secret. Prior to the helm integration landing, we could include that information as BD labels or annotations.I could have sworn we added BD annotations for that info already, but maybe not.
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.
@joelanford I think you did it in your PoC a while ago and I had a draft PR which we then closed in favour of the digest idea, but we didn't get to implement the the digest idea.