Skip to content

⚠ use bundle name and version instead of image in (cluster)extension status #679

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

Merged
merged 1 commit into from
Mar 27, 2024
Merged

⚠ use bundle name and version instead of image in (cluster)extension status #679

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
4 changes: 2 additions & 2 deletions api/v1alpha1/clusterextension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -122,9 +122,9 @@ func init() {
// ClusterExtensionStatus defines the observed state of ClusterExtension
type ClusterExtensionStatus struct {
// +optional
InstalledBundleResource string `json:"installedBundleResource,omitempty"`
InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"`
// +optional
ResolvedBundleResource string `json:"resolvedBundleResource,omitempty"`
ResolvedBundle *BundleMetadata `json:"resolvedBundle,omitempty"`
Comment on lines +125 to +127
Copy link
Contributor

@everettraven everettraven Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Can we make this change on the ClusterExtension API without it being considered a breaking change to the resultant CRD?

I can see an argument for not being concerned about this, but I know in the past we've talked about trying to adhere to CRD versioning standards even though we are in a v0/alpha state with this project where by semver breaking changes are allowed to happen. Is adhering to the idea of not making breaking changes to a CRD still something we want to focus on moving forward?

As an aside, I don't see this as a problem for the Extension API as I don't think we have cut a release with that API available yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could if we had to. We could add this field and deprecate the old one. I don't think it's worth it to go to the trouble in an alpha API that we'll likely drop in the next couple of months.

I suppose you could argue, "why make the change at all if we're planning to remove the API?". I'd say that:

  1. ClusterExtension is still more mature, so making this change there is essentially a preview for existing users of ClusterExtension
  2. It's easier, at least in this case, to keep ClusterExtension and Extension functionality aligned in the areas where there is no fundamental difference between the two.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think it's worth it to go to the trouble in an alpha API that we'll likely drop in the next couple of months

Totally fair. I just wanted to explicitly call this out to make sure that we were making this breaking change as a conscious decision since we have intentionally tried to avoid those in the past, even on our alpha APIs.

I do think in general, we should err on the side of not making breaking changes due to potential side effect to the cluster/clients but I do understand that it should be okay since we are in an alpha state both as a project and for the APIs.


// +patchMergeKey=type
// +patchStrategy=merge
Expand Down
9 changes: 7 additions & 2 deletions api/v1alpha1/extension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,9 +95,9 @@ type ExtensionStatus struct {
Paused bool `json:"paused"`

// +optional
InstalledBundleResource string `json:"installedBundleResource,omitempty"`
InstalledBundle *BundleMetadata `json:"installedBundle,omitempty"`
// +optional
ResolvedBundleResource string `json:"resolvedBundleResource,omitempty"`
ResolvedBundle *BundleMetadata `json:"resolvedBundle,omitempty"`

// +patchMergeKey=type
// +patchStrategy=merge
Expand All @@ -106,6 +106,11 @@ type ExtensionStatus struct {
Conditions []metav1.Condition `json:"conditions,omitempty" patchStrategy:"merge" patchMergeKey:"type" protobuf:"bytes,1,rep,name=conditions"`
}

type BundleMetadata struct {
Name string `json:"name"`
Version string `json:"version"`
}

//+kubebuilder:object:root=true
//+kubebuilder:subresource:status
//+kubebuilder:printcolumn:name="Paused",type=string,JSONPath=`.status.paused`,description="The current reconciliation state of this extension"
Expand Down
35 changes: 35 additions & 0 deletions api/v1alpha1/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

24 changes: 20 additions & 4 deletions config/crd/bases/olm.operatorframework.io_clusterextensions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,10 +144,26 @@ spec:
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
installedBundleResource:
type: string
resolvedBundleResource:
type: string
installedBundle:
properties:
name:
type: string
version:
type: string
required:
- name
- version
type: object
resolvedBundle:
properties:
name:
type: string
version:
type: string
required:
- name
- version
type: object
type: object
type: object
served: true
Expand Down
24 changes: 20 additions & 4 deletions config/crd/bases/olm.operatorframework.io_extensions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,14 +185,30 @@ spec:
x-kubernetes-list-map-keys:
- type
x-kubernetes-list-type: map
installedBundleResource:
type: string
installedBundle:
properties:
name:
type: string
version:
type: string
required:
- name
- version
type: object
paused:
description: paused indicates the current reconciliation state of
this extension
type: boolean
resolvedBundleResource:
type: string
resolvedBundle:
properties:
name:
type: string
version:
type: string
required:
- name
- version
type: object
required:
- paused
type: object
Expand Down
34 changes: 17 additions & 17 deletions internal/controllers/clusterextension_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,9 +118,9 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
// gather vars for resolution
vars, err := r.variables(ctx)
if err != nil {
ext.Status.InstalledBundleResource = ""
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.ResolvedBundleResource = ""
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())
Expand All @@ -130,9 +130,9 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
// run resolution
selection, err := r.Resolver.Solve(vars)
if err != nil {
ext.Status.InstalledBundleResource = ""
ext.Status.InstalledBundle = nil
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration())
ext.Status.ResolvedBundleResource = ""
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())
Expand All @@ -143,17 +143,17 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
// ClusterExtension's desired package name.
bundle, err := r.bundleFromSolution(selection, ext.Spec.PackageName)
if err != nil {
ext.Status.InstalledBundleResource = ""
ext.Status.InstalledBundle = nil
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "installation has not been attempted as resolution failed", ext.GetGeneration())
ext.Status.ResolvedBundleResource = ""
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
}

// Now we can set the Resolved Condition, and the resolvedBundleSource field to the bundle.Image value.
ext.Status.ResolvedBundleResource = bundle.Image
ext.Status.ResolvedBundle = bundleMetadataFor(bundle)
setResolvedStatusConditionSuccess(&ext.Status.Conditions, fmt.Sprintf("resolved to %q", bundle.Image), ext.GetGeneration())

// TODO: Question - Should we set the deprecation statuses after we have successfully resolved instead of after a successful installation?
Expand All @@ -175,7 +175,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
dep := r.GenerateExpectedBundleDeployment(*ext, bundle.Image, bundleProvisioner)
if err := r.ensureBundleDeployment(ctx, dep); err != nil {
// originally Reason: ocv1alpha1.ReasonInstallationFailed
ext.Status.InstalledBundleResource = ""
ext.Status.InstalledBundle = nil
setInstalledStatusConditionFailed(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
return ctrl.Result{}, err
Expand All @@ -185,15 +185,15 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
existingTypedBundleDeployment := &rukpakv1alpha2.BundleDeployment{}
if err := runtime.DefaultUnstructuredConverter.FromUnstructured(dep.UnstructuredContent(), existingTypedBundleDeployment); err != nil {
// originally Reason: ocv1alpha1.ReasonInstallationStatusUnknown
ext.Status.InstalledBundleResource = ""
ext.Status.InstalledBundle = nil
setInstalledStatusConditionUnknown(&ext.Status.Conditions, err.Error(), ext.GetGeneration())
setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
return ctrl.Result{}, err
}

// Let's set the proper Installed condition and InstalledBundleResource field based on the
// Let's set the proper Installed condition and InstalledBundle field based on the
// existing BundleDeployment object status.
mapBDStatusToInstalledCondition(existingTypedBundleDeployment, ext)
mapBDStatusToInstalledCondition(existingTypedBundleDeployment, ext, bundle)

SetDeprecationStatus(ext, bundle)

Expand All @@ -218,16 +218,16 @@ func (r *ClusterExtensionReconciler) variables(ctx context.Context) ([]deppy.Var
return GenerateVariables(allBundles, clusterExtensionList.Items, bundleDeploymentList.Items)
}

func mapBDStatusToInstalledCondition(existingTypedBundleDeployment *rukpakv1alpha2.BundleDeployment, ext *ocv1alpha1.ClusterExtension) {
func mapBDStatusToInstalledCondition(existingTypedBundleDeployment *rukpakv1alpha2.BundleDeployment, ext *ocv1alpha1.ClusterExtension, bundle *catalogmetadata.Bundle) {
bundleDeploymentReady := apimeta.FindStatusCondition(existingTypedBundleDeployment.Status.Conditions, rukpakv1alpha2.TypeInstalled)
if bundleDeploymentReady == nil {
ext.Status.InstalledBundleResource = ""
ext.Status.InstalledBundle = nil
setInstalledStatusConditionUnknown(&ext.Status.Conditions, "bundledeployment status is unknown", ext.GetGeneration())
return
}

if bundleDeploymentReady.Status != metav1.ConditionTrue {
ext.Status.InstalledBundleResource = ""
ext.Status.InstalledBundle = nil
setInstalledStatusConditionFailed(
&ext.Status.Conditions,
fmt.Sprintf("bundledeployment not ready: %s", bundleDeploymentReady.Message),
Expand All @@ -236,25 +236,25 @@ func mapBDStatusToInstalledCondition(existingTypedBundleDeployment *rukpakv1alph
return
}

installedBundle := bundleMetadataFor(bundle)
bundleDeploymentSource := existingTypedBundleDeployment.Spec.Source
switch bundleDeploymentSource.Type {
case rukpakv1alpha2.SourceTypeImage:
ext.Status.InstalledBundleResource = bundleDeploymentSource.Image.Ref
ext.Status.InstalledBundle = installedBundle
setInstalledStatusConditionSuccess(
&ext.Status.Conditions,
fmt.Sprintf("installed from %q", bundleDeploymentSource.Image.Ref),
ext.GetGeneration(),
)
case rukpakv1alpha2.SourceTypeGit:
ext.Status.InstalledBundle = installedBundle
resource := bundleDeploymentSource.Git.Repository + "@" + bundleDeploymentSource.Git.Ref.Commit
ext.Status.InstalledBundleResource = resource
setInstalledStatusConditionSuccess(
&ext.Status.Conditions,
fmt.Sprintf("installed from %q", resource),
ext.GetGeneration(),
)
default:
ext.Status.InstalledBundleResource = ""
setInstalledStatusConditionUnknown(
&ext.Status.Conditions,
fmt.Sprintf("unknown bundledeployment source type %q", bundleDeploymentSource.Type),
Expand Down
Loading