Skip to content

Commit dc70b74

Browse files
camilamacedo86camilamacedo86
committed
wip: review
1 parent a085534 commit dc70b74

File tree

10 files changed

+164
-41
lines changed

10 files changed

+164
-41
lines changed

‎api/v1/clusterextension_types.go‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -485,10 +485,13 @@ type ClusterExtensionStatus struct {
485485
//
486486
// When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.
487487
// These are indications from a package owner to guide users away from a particular package, channel, or bundle.
488-
// PackageDeprecated becomes True when the catalog marks the requested package deprecated; otherwise it stays False.
489-
// ChannelDeprecated becomes True when any requested channel is marked deprecated; otherwise it stays False.
490-
// BundleDeprecated reports the catalog status of the installed bundle: it remains Unknown until a bundle installs, then becomes True or False depending on whether the catalog marks that bundle deprecated.
491-
// Deprecated is a rollup that mirrors True whenever any of the specific deprecation conditions are True.
488+
// PackageDeprecated reports the requested package as deprecated when the catalog says so. When no catalog talks
489+
// about the package, the condition stays Unknown instead of claiming False.
490+
// ChannelDeprecated follows the same rules for each requested channel.
491+
// BundleDeprecated describes the installed bundle once one exists. Until a bundle installs, or when no catalog
492+
// data is available, it remains Unknown.
493+
// Deprecated is still the rollup: it goes True when any individual deprecation condition is True, and Unknown
494+
// when we have no catalog information to report.
492495
//
493496
// +listType=map
494497
// +listMapKey=type

‎docs/api-reference/olmv1-api-reference.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -359,7 +359,7 @@ _Appears in:_
359359

360360
| Field | Description | Default | Validation |
361361
| --- | --- | --- | --- |
362-
| `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#condition-v1-meta) array_ | The set of condition types which apply to all spec.source variations are Installed and Progressing.<br />The Installed condition represents whether or not the bundle has been installed for this ClusterExtension.<br />When Installed is True and the Reason is Succeeded, the bundle has been successfully installed.<br />When Installed is False and the Reason is Failed, the bundle has failed to install.<br />The Progressing condition represents whether or not the ClusterExtension is advancing towards a new state.<br />When Progressing is True and the Reason is Succeeded, the ClusterExtension is making progress towards a new state.<br />When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.<br />When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.<br />When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.<br />These are indications from a package owner to guide users away from a particular package, channel, or bundle.<br />PackageDeprecated becomes True when the catalog marks the requested package deprecated; otherwise it stays False.<br />ChannelDeprecated becomes True when any requested channel is marked deprecated; otherwise it stays False.<br />BundleDeprecated reports the catalog status of the installed bundle: it remains Unknown until a bundle installs, then becomes True or False depending on whether the catalog marks that bundle deprecated.<br />Deprecated is a rollup that mirrors True whenever any of the specific deprecation conditions are True. | | |
362+
| `conditions` _[Condition](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.31/#condition-v1-meta) array_ | The set of condition types which apply to all spec.source variations are Installed and Progressing.<br />The Installed condition represents whether or not the bundle has been installed for this ClusterExtension.<br />When Installed is True and the Reason is Succeeded, the bundle has been successfully installed.<br />When Installed is False and the Reason is Failed, the bundle has failed to install.<br />The Progressing condition represents whether or not the ClusterExtension is advancing towards a new state.<br />When Progressing is True and the Reason is Succeeded, the ClusterExtension is making progress towards a new state.<br />When Progressing is True and the Reason is Retrying, the ClusterExtension has encountered an error that could be resolved on subsequent reconciliation attempts.<br />When Progressing is False and the Reason is Blocked, the ClusterExtension has encountered an error that requires manual intervention for recovery.<br />When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.<br />These are indications from a package owner to guide users away from a particular package, channel, or bundle.<br />PackageDeprecated reports the requested package as deprecated when the catalog says so. When no catalog talks<br />about the package, the condition stays Unknown instead of claiming False.<br />ChannelDeprecated follows the same rules for each requested channel.<br />BundleDeprecated describes the installed bundle once one exists. Until a bundle installs, or when no catalog<br />data is available, it remains Unknown.<br />Deprecated is still the rollup: it goes True when any individual deprecation condition is True, and Unknown<br />when we have no catalog information to report. | | |
363363
| `install` _[ClusterExtensionInstallStatus](#clusterextensioninstallstatus)_ | install is a representation of the current installation status for this ClusterExtension. | | |
364364

365365

‎helm/olmv1/base/operator-controller/crd/experimental/olm.operatorframework.io_clusterextensions.yaml‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -520,10 +520,13 @@ spec:
520520
521521
When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.
522522
These are indications from a package owner to guide users away from a particular package, channel, or bundle.
523-
PackageDeprecated becomes True when the catalog marks the requested package deprecated; otherwise it stays False.
524-
ChannelDeprecated becomes True when any requested channel is marked deprecated; otherwise it stays False.
525-
BundleDeprecated reports the catalog status of the installed bundle: it remains Unknown until a bundle installs, then becomes True or False depending on whether the catalog marks that bundle deprecated.
526-
Deprecated is a rollup that mirrors True whenever any of the specific deprecation conditions are True.
523+
PackageDeprecated reports the requested package as deprecated when the catalog says so. When no catalog talks
524+
about the package, the condition stays Unknown instead of claiming False.
525+
ChannelDeprecated follows the same rules for each requested channel.
526+
BundleDeprecated describes the installed bundle once one exists. Until a bundle installs, or when no catalog
527+
data is available, it remains Unknown.
528+
Deprecated is still the rollup: it goes True when any individual deprecation condition is True, and Unknown
529+
when we have no catalog information to report.
527530
items:
528531
description: Condition contains details for one aspect of the current
529532
state of this API Resource.

‎helm/olmv1/base/operator-controller/crd/standard/olm.operatorframework.io_clusterextensions.yaml‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -481,10 +481,13 @@ spec:
481481
482482
When the ClusterExtension is sourced from a catalog, it may surface deprecation conditions based on catalog metadata.
483483
These are indications from a package owner to guide users away from a particular package, channel, or bundle.
484-
PackageDeprecated becomes True when the catalog marks the requested package deprecated; otherwise it stays False.
485-
ChannelDeprecated becomes True when any requested channel is marked deprecated; otherwise it stays False.
486-
BundleDeprecated reports the catalog status of the installed bundle: it remains Unknown until a bundle installs, then becomes True or False depending on whether the catalog marks that bundle deprecated.
487-
Deprecated is a rollup that mirrors True whenever any of the specific deprecation conditions are True.
484+
PackageDeprecated reports the requested package as deprecated when the catalog says so. When no catalog talks
485+
about the package, the condition stays Unknown instead of claiming False.
486+
ChannelDeprecated follows the same rules for each requested channel.
487+
BundleDeprecated describes the installed bundle once one exists. Until a bundle installs, or when no catalog
488+
data is available, it remains Unknown.
489+
Deprecated is still the rollup: it goes True when any individual deprecation condition is True, and Unknown
490+
when we have no catalog information to report.
488491
items:
489492
description: Condition contains details for one aspect of the current
490493
state of this API Resource.

‎internal/operator-controller/controllers/clusterextension_controller.go‎

Lines changed: 53 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -252,18 +252,27 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
252252
// * if nothing installs, BundleDeprecated stays Unknown/Absent
253253
// * if a bundle installs, we report its real deprecation status
254254
// * install errors never leak into the deprecation conditions
255+
// We pass pointers into the defer because the interesting values show up after this point:
256+
// * When resolution succeeds we overwrite the pointers with the resolved bundle and catalog warnings. If we
257+
// captured the zero values instead, SetDeprecationStatus would later claim everything is Unknown.
258+
// * When resolution fails but still returns deprecation entries, we update the pointer so the warning reaches
259+
// the user. Without the pointer, the message would disappear.
260+
// * When a rollout is already in progress (Boxcutter keeps a revision in RollingOut) we reuse that revision
261+
// after this defer is registered. Capturing by value would drop the installed bundle name and confuse the
262+
// bundle condition.
255263
var resolvedDeprecation *declcfg.Deprecation
256-
defer func(resolvedDeprecationPtr **declcfg.Deprecation, revisionStatesPtr **RevisionStates) {
264+
var hadCatalogDeprecationData bool
265+
defer func(resolvedDeprecationPtr **declcfg.Deprecation, revisionStatesPtr **RevisionStates, hasDataPtr *bool) {
257266
installedBundleName := ""
258-
if revisionStatesPtr != nil && *revisionStatesPtr != nil && (*revisionStatesPtr).Installed != nil {
267+
if *revisionStatesPtr != nil && (*revisionStatesPtr).Installed != nil {
259268
installedBundleName = (*revisionStatesPtr).Installed.Name
260269
}
261270
var resolvedDeprecationVal *declcfg.Deprecation
262271
if resolvedDeprecationPtr != nil {
263272
resolvedDeprecationVal = *resolvedDeprecationPtr
264273
}
265-
SetDeprecationStatus(ext, installedBundleName, resolvedDeprecationVal)
266-
}(&resolvedDeprecation, &revisionStates)
274+
SetDeprecationStatus(ext, installedBundleName, resolvedDeprecationVal, hasDataPtr != nil && *hasDataPtr)
275+
}(&resolvedDeprecation, &revisionStates, &hadCatalogDeprecationData)
267276

268277
var resolvedRevisionMetadata *RevisionMetadata
269278
if len(revisionStates.RollingOut) == 0 {
@@ -275,6 +284,9 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
275284
var resolvedBundle *declcfg.Bundle
276285
var resolvedBundleVersion *bsemver.Version
277286
resolvedBundle, resolvedBundleVersion, resolvedDeprecation, err = r.Resolver.Resolve(ctx, ext, bm)
287+
if err == nil || resolvedDeprecation != nil {
288+
hadCatalogDeprecationData = true
289+
}
278290
// Keep any deprecation data the resolver returned. The deferred update will use it
279291
// even if installation later fails or never begins.
280292
if err != nil {
@@ -293,6 +305,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1.Cl
293305
BundleMetadata: bundleutil.MetadataFor(resolvedBundle.Name, *resolvedBundleVersion),
294306
}
295307
} else {
308+
hadCatalogDeprecationData = true
296309
resolvedRevisionMetadata = revisionStates.RollingOut[0]
297310
}
298311

@@ -376,23 +389,55 @@ type DeprecationInfo struct {
376389
// Install or validation errors never appear here because they belong on the
377390
// Progressing/Installed conditions instead. Callers should invoke this after reconcile
378391
// finishes (for example via a defer) so catalog data replaces any transient error messages.
379-
func SetDeprecationStatus(ext *ocv1.ClusterExtension, installedBundleName string, deprecation *declcfg.Deprecation) {
392+
func SetDeprecationStatus(ext *ocv1.ClusterExtension, installedBundleName string, deprecation *declcfg.Deprecation, hasCatalogData bool) {
380393
info := buildDeprecationInfo(ext, installedBundleName, deprecation)
381394

382395
packageMessages := collectDeprecationMessages(info.PackageEntries)
383396
channelMessages := collectDeprecationMessages(info.ChannelEntries)
384397
bundleMessages := collectDeprecationMessages(info.BundleEntries)
385398

399+
if !hasCatalogData {
400+
SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
401+
Type: ocv1.TypeDeprecated,
402+
Status: metav1.ConditionUnknown,
403+
Reason: ocv1.ReasonDeprecated,
404+
Message: "",
405+
ObservedGeneration: ext.GetGeneration(),
406+
})
407+
SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
408+
Type: ocv1.TypePackageDeprecated,
409+
Status: metav1.ConditionUnknown,
410+
Reason: ocv1.ReasonDeprecated,
411+
Message: "",
412+
ObservedGeneration: ext.GetGeneration(),
413+
})
414+
SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
415+
Type: ocv1.TypeChannelDeprecated,
416+
Status: metav1.ConditionUnknown,
417+
Reason: ocv1.ReasonDeprecated,
418+
Message: "",
419+
ObservedGeneration: ext.GetGeneration(),
420+
})
421+
SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
422+
Type: ocv1.TypeBundleDeprecated,
423+
Status: metav1.ConditionUnknown,
424+
Reason: ocv1.ReasonAbsent,
425+
Message: "",
426+
ObservedGeneration: ext.GetGeneration(),
427+
})
428+
return
429+
}
430+
386431
messages := slices.Concat(packageMessages, channelMessages, bundleMessages)
387432

388-
status := metav1.ConditionFalse
433+
deprecatedStatus := metav1.ConditionFalse
389434
if len(messages) > 0 {
390-
status = metav1.ConditionTrue
435+
deprecatedStatus = metav1.ConditionTrue
391436
}
392437

393438
SetStatusCondition(&ext.Status.Conditions, metav1.Condition{
394439
Type: ocv1.TypeDeprecated,
395-
Status: status,
440+
Status: deprecatedStatus,
396441
Reason: ocv1.ReasonDeprecated,
397442
Message: strings.Join(messages, "\n"),
398443
ObservedGeneration: ext.GetGeneration(),

‎internal/operator-controller/controllers/clusterextension_controller_test.go‎

Lines changed: 61 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1033,9 +1033,57 @@ func TestSetDeprecationStatus(t *testing.T) {
10331033
expectedClusterExtension *ocv1.ClusterExtension
10341034
bundle *declcfg.Bundle
10351035
deprecation *declcfg.Deprecation
1036+
hasCatalogData bool
10361037
}{
10371038
{
1038-
name: "no deprecations, all deprecation statuses set to False",
1039+
name: "no catalog data, all deprecation statuses set to Unknown",
1040+
clusterExtension: &ocv1.ClusterExtension{
1041+
ObjectMeta: metav1.ObjectMeta{
1042+
Generation: 1,
1043+
},
1044+
Status: ocv1.ClusterExtensionStatus{
1045+
Conditions: []metav1.Condition{},
1046+
},
1047+
},
1048+
expectedClusterExtension: &ocv1.ClusterExtension{
1049+
ObjectMeta: metav1.ObjectMeta{
1050+
Generation: 1,
1051+
},
1052+
Status: ocv1.ClusterExtensionStatus{
1053+
Conditions: []metav1.Condition{
1054+
{
1055+
Type: ocv1.TypeDeprecated,
1056+
Reason: ocv1.ReasonDeprecated,
1057+
Status: metav1.ConditionUnknown,
1058+
ObservedGeneration: 1,
1059+
},
1060+
{
1061+
Type: ocv1.TypePackageDeprecated,
1062+
Reason: ocv1.ReasonDeprecated,
1063+
Status: metav1.ConditionUnknown,
1064+
ObservedGeneration: 1,
1065+
},
1066+
{
1067+
Type: ocv1.TypeChannelDeprecated,
1068+
Reason: ocv1.ReasonDeprecated,
1069+
Status: metav1.ConditionUnknown,
1070+
ObservedGeneration: 1,
1071+
},
1072+
{
1073+
Type: ocv1.TypeBundleDeprecated,
1074+
Reason: ocv1.ReasonAbsent,
1075+
Status: metav1.ConditionUnknown,
1076+
ObservedGeneration: 1,
1077+
},
1078+
},
1079+
},
1080+
},
1081+
bundle: &declcfg.Bundle{},
1082+
deprecation: nil,
1083+
hasCatalogData: false,
1084+
},
1085+
{
1086+
name: "catalog consulted but no deprecations, statuses set to False",
10391087
clusterExtension: &ocv1.ClusterExtension{
10401088
ObjectMeta: metav1.ObjectMeta{
10411089
Generation: 1,
@@ -1077,8 +1125,9 @@ func TestSetDeprecationStatus(t *testing.T) {
10771125
},
10781126
},
10791127
},
1080-
bundle: &declcfg.Bundle{},
1081-
deprecation: nil,
1128+
bundle: &declcfg.Bundle{},
1129+
deprecation: nil,
1130+
hasCatalogData: true,
10821131
},
10831132
{
10841133
name: "deprecated channel, but no channel specified, all deprecation statuses set to False",
@@ -1144,6 +1193,7 @@ func TestSetDeprecationStatus(t *testing.T) {
11441193
},
11451194
}},
11461195
},
1196+
hasCatalogData: true,
11471197
},
11481198
{
11491199
name: "deprecated channel, but a non-deprecated channel specified, all deprecation statuses set to False",
@@ -1215,6 +1265,7 @@ func TestSetDeprecationStatus(t *testing.T) {
12151265
},
12161266
},
12171267
},
1268+
hasCatalogData: true,
12181269
},
12191270
{
12201271
name: "deprecated channel specified, ChannelDeprecated and Deprecated status set to true, others set to false",
@@ -1287,6 +1338,7 @@ func TestSetDeprecationStatus(t *testing.T) {
12871338
},
12881339
},
12891340
},
1341+
hasCatalogData: true,
12901342
},
12911343
{
12921344
name: "deprecated package and channel specified, deprecated bundle, all deprecation statuses set to true",
@@ -1372,6 +1424,7 @@ func TestSetDeprecationStatus(t *testing.T) {
13721424
},
13731425
},
13741426
},
1427+
hasCatalogData: true,
13751428
},
13761429
{
13771430
name: "deprecated channel specified, deprecated bundle, all deprecation statuses set to true, all deprecation statuses set to true except PackageDeprecated",
@@ -1611,7 +1664,11 @@ func TestSetDeprecationStatus(t *testing.T) {
16111664
},
16121665
} {
16131666
t.Run(tc.name, func(t *testing.T) {
1614-
controllers.SetDeprecationStatus(tc.clusterExtension, tc.bundle.Name, tc.deprecation)
1667+
hasCatalogData := tc.hasCatalogData
1668+
if tc.deprecation != nil && !hasCatalogData {
1669+
hasCatalogData = true
1670+
}
1671+
controllers.SetDeprecationStatus(tc.clusterExtension, tc.bundle.Name, tc.deprecation, hasCatalogData)
16151672
// TODO: we should test for unexpected changes to lastTransitionTime. We only expect
16161673
// lastTransitionTime to change when the status of the condition changes.
16171674
assert.Empty(t, cmp.Diff(tc.expectedClusterExtension, tc.clusterExtension, cmpopts.IgnoreFields(metav1.Condition{}, "Message", "LastTransitionTime")))

0 commit comments

Comments
 (0)