Skip to content

⚠ minor API improvements #1250

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
Sep 12, 2024
Merged

⚠ minor API improvements #1250

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
34 changes: 15 additions & 19 deletions api/v1alpha1/clusterextension_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,8 +23,7 @@ import (
)

var (
ClusterExtensionGVK = SchemeBuilder.GroupVersion.WithKind("ClusterExtension")
Copy link
Member Author

Choose a reason for hiding this comment

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

ClusterExtensionGVK is unused, and is not really necessary for inclusion as an exported global variable in the API.

Callers can construct themselves like this:

gvk := ocv1alpha1.GroupVersion.WithKind(v1alpha1.ClusterExtensionKind)

ClusterExtensionKind = ClusterExtensionGVK.Kind
ClusterExtensionKind = "ClusterExtension"
)

type UpgradeConstraintPolicy string
Expand Down Expand Up @@ -386,15 +385,15 @@ type PreflightConfig struct {
// The CRD Upgrade Safety pre-flight check safeguards from unintended
// consequences of upgrading a CRD, such as data loss.
//
// This field is required if the spec.preflight field is specified.
// This field is required if the spec.install.preflight field is specified.
CRDUpgradeSafety *CRDUpgradeSafetyPreflightConfig `json:"crdUpgradeSafety"`
}

// CRDUpgradeSafetyPreflightConfig is the configuration for CRD upgrade safety preflight check.
type CRDUpgradeSafetyPreflightConfig struct {
// policy is used to configure the state of the CRD Upgrade Safety pre-flight check.
//
// This field is required when the spec.preflight.crdUpgradeSafety field is
// This field is required when the spec.install.preflight.crdUpgradeSafety field is
// specified.
//
// Allowed values are ["Enabled", "Disabled"]. The default value is "Enabled".
Expand Down Expand Up @@ -485,15 +484,17 @@ type ClusterExtensionStatus struct {
// - Reason: a string representation of the reason for the current state of the condition. Typically useful for building automation around particular Type+Reason combinations.
// - Message: a human readable message that further elaborates on the state of the condition
//
// The current set of condition types are:
// - "Installed", represents whether or not the package referenced in the spec.packageName field has been installed
// The global set of condition types are:
// - "Installed", represents whether or not the a bundle has been installed for this ClusterExtension
// - "Resolved", represents whether or not a bundle was found that satisfies the selection criteria outlined in the spec
// - "Deprecated", represents an aggregation of the PackageDeprecated, ChannelDeprecated, and BundleDeprecated condition types.
// - "PackageDeprecated", represents whether or not the package specified in the spec.packageName field has been deprecated
// - "ChannelDeprecated", represents whether or not the channel specified in spec.channel has been deprecated
// - "BundleDeprecated", represents whether or not the bundle installed is deprecated
// - "Unpacked", represents whether or not the bundle contents have been successfully unpacked
//
// When the ClusterExtension is sourced from a catalog, the following conditions are also possible:
Copy link
Member Author

Choose a reason for hiding this comment

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

I grouped the Deprecated* conditions into their own section for two reasons:

  1. I think it is helpful to see the other "main" conditions without the "noise" of these being intermingled.
  2. Deprecation metadata comes from catalogs, and while we currently only support sourcing from catalogs, I thought it might be good to call out the fact that deprecations will only ever show up if the ClusterExtension is sourced from a catalog.

// - "Deprecated", represents an aggregation of the PackageDeprecated, ChannelDeprecated, and BundleDeprecated condition types
// - "PackageDeprecated", represents whether or not the package specified in the spec.source.catalog.packageName field has been deprecated
// - "ChannelDeprecated", represents whether or not any channel specified in spec.source.catalog.channels has been deprecated
// - "BundleDeprecated", represents whether or not the installed bundle is deprecated
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm pretty sure this line is not true, but want to double check. A quick glance at the code makes me think we should make the following edit as well:

Suggested change
// - "BundleDeprecated", represents whether or not the installed bundle is deprecated
// - "BundleDeprecated", represents whether or not the resolved bundle is deprecated

However, I think the original sentiment of having this condition be based on the installed bundle is what we actually want. That might require a bit more conversation and consideration though, as it isn't clear to me how exactly we would assess the deprecation status of an installed bundle during subsequent reconciles after it is installed.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we'd have two possibilities:

  1. You want to install a bundle from the catalog, and it's deprecated - which is straightforward
  2. You install a bundle that isn't deprecated, and in the next catalog version it is (I guess this is only meaningful for version ranges that don't let the bundle upgrade to the next (non-deprecated) version.

In the 2nd case, shouldn't it pick up the condition during reconcile? Maybe it's OK that we only express deprecated conditions for resolved bundles (and not worry about the the installed bundles) since either the CE will get upgraded, or if it can't/won't, we'd anyway resolve to the installed bundle?

Copy link
Member Author

Choose a reason for hiding this comment

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

My initial thought is that we only touch the BundleDeprecated condition when:

  1. We successfully install a bundle. In this case the resolved bundle and the installed bundle are the same, so the resolved deprecation is applicable to the installed bundle.
  2. We detect that a bundle is somehow no longer installed, in which case we unset (or set to Unknown) the BundleDeprecated condition.

//
// The current set of reasons are:
// - "Success", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when unpacking a bundle's content, resolution and installation/upgrading is successful
// - "Failed", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when an error has occurred while unpacking the contents of a bundle, during resolution or installation.
Expand All @@ -511,22 +512,17 @@ type ClusterExtensionInstallStatus struct {
//
// A "bundle" is a versioned set of content that represents the resources that
// need to be applied to a cluster to install a package.
//
// This field is only updated once a bundle has been successfully installed and
// once set will only be updated when a new version of the bundle has
// successfully replaced the currently installed version.
//
//+optional
Bundle *BundleMetadata `json:"bundle,omitempty"`
Bundle BundleMetadata `json:"bundle"`
Copy link
Member Author

@joelanford joelanford Sep 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I removed the pointer here because the parent struct is a pointer. If the parent struct is non-nil, this field is always expected to be set.

}

type ClusterExtensionResolutionStatus struct {
// bundle is a representation of the bundle that was identified during
// resolution to meet all installation/upgrade constraints and is slated to be
// installed or upgraded to.
//
//+optional
Bundle *BundleMetadata `json:"bundle,omitempty"`
// A "bundle" is a versioned set of content that represents the resources that
// need to be applied to a cluster to install a package.
Bundle BundleMetadata `json:"bundle"`
Copy link
Member Author

Choose a reason for hiding this comment

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

See: https://github.com/operator-framework/operator-controller/pull/1250/files#r1754304430

}

//+kubebuilder:object:root=true
Expand Down
16 changes: 4 additions & 12 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.

29 changes: 17 additions & 12 deletions config/base/crd/bases/olm.operatorframework.io_clusterextensions.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,14 +100,14 @@ spec:
The CRD Upgrade Safety pre-flight check safeguards from unintended
consequences of upgrading a CRD, such as data loss.

This field is required if the spec.preflight field is specified.
This field is required if the spec.install.preflight field is specified.
properties:
policy:
default: Enabled
description: |-
policy is used to configure the state of the CRD Upgrade Safety pre-flight check.

This field is required when the spec.preflight.crdUpgradeSafety field is
This field is required when the spec.install.preflight.crdUpgradeSafety field is
specified.

Allowed values are ["Enabled", "Disabled"]. The default value is "Enabled".
Expand Down Expand Up @@ -474,15 +474,17 @@ spec:
- Reason: a string representation of the reason for the current state of the condition. Typically useful for building automation around particular Type+Reason combinations.
- Message: a human readable message that further elaborates on the state of the condition

The current set of condition types are:
- "Installed", represents whether or not the package referenced in the spec.packageName field has been installed
The global set of condition types are:
- "Installed", represents whether or not the a bundle has been installed for this ClusterExtension
- "Resolved", represents whether or not a bundle was found that satisfies the selection criteria outlined in the spec
- "Deprecated", represents an aggregation of the PackageDeprecated, ChannelDeprecated, and BundleDeprecated condition types.
- "PackageDeprecated", represents whether or not the package specified in the spec.packageName field has been deprecated
- "ChannelDeprecated", represents whether or not the channel specified in spec.channel has been deprecated
- "BundleDeprecated", represents whether or not the bundle installed is deprecated
- "Unpacked", represents whether or not the bundle contents have been successfully unpacked

When the ClusterExtension is sourced from a catalog, the following conditions are also possible:
- "Deprecated", represents an aggregation of the PackageDeprecated, ChannelDeprecated, and BundleDeprecated condition types
- "PackageDeprecated", represents whether or not the package specified in the spec.source.catalog.packageName field has been deprecated
- "ChannelDeprecated", represents whether or not any channel specified in spec.source.catalog.channels has been deprecated
- "BundleDeprecated", represents whether or not the installed bundle is deprecated

The current set of reasons are:
- "Success", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when unpacking a bundle's content, resolution and installation/upgrading is successful
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to call out the reasons for the deprecated conditions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. The "Reason" for these conditions are basically 1-to-1 with the Status. "The reason this is deprecated is because the author provided a deprecation"

- "Failed", this reason is set on the "Unpacked", "Resolved" and "Installed" conditions when an error has occurred while unpacking the contents of a bundle, during resolution or installation.
Expand Down Expand Up @@ -552,10 +554,6 @@ spec:

A "bundle" is a versioned set of content that represents the resources that
need to be applied to a cluster to install a package.

This field is only updated once a bundle has been successfully installed and
once set will only be updated when a new version of the bundle has
successfully replaced the currently installed version.
properties:
name:
description: |-
Expand All @@ -571,6 +569,8 @@ spec:
- name
- version
type: object
required:
- bundle
type: object
resolution:
properties:
Expand All @@ -579,6 +579,9 @@ spec:
bundle is a representation of the bundle that was identified during
resolution to meet all installation/upgrade constraints and is slated to be
installed or upgraded to.

A "bundle" is a versioned set of content that represents the resources that
need to be applied to a cluster to install a package.
properties:
name:
description: |-
Expand All @@ -594,6 +597,8 @@ spec:
- name
- version
type: object
required:
- bundle
type: object
type: object
type: object
Expand Down
Loading
Loading