✨ make extension and clusterextension api consistent #842

ankitathomas · 2024-05-09T04:10:51Z

This PR makes the clusterextension api more consistent with the extension api by:

copying over missing fields to the clusterextension api
adding the Progressing condition to indicate a progressing installation
updating the Installed condition to match the extension controller's behavior.

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

netlify · 2024-05-09T04:11:07Z

✅ Deploy Preview for olmv1 ready!

Name	Link
🔨 Latest commit	`a934c27`
🔍 Latest deploy log	https://app.netlify.com/sites/olmv1/deploys/663c4ccee03d1000087a76a9
😎 Deploy Preview	https://deploy-preview-842--olmv1.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

codecov · 2024-05-09T04:17:03Z

Codecov Report

Attention: Patch coverage is 70.47619% with 31 lines in your changes are missing coverage. Please review.

Project coverage is 71.70%. Comparing base (6d73b73) to head (a934c27).
Report is 10 commits behind head on main.

Files	Patch %	Lines
api/v1alpha1/zz_generated.deepcopy.go	34.78%	15 Missing ⚠️
...nternal/controllers/clusterextension_controller.go	70.00%	9 Missing and 3 partials ⚠️
internal/controllers/extension_controller.go	76.47%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #842      +/-   ##
==========================================
+ Coverage   71.02%   71.70%   +0.68%     
==========================================
  Files          17       18       +1     
  Lines        1301     1329      +28     
==========================================
+ Hits          924      953      +29     
+ Misses        304      300       -4     
- Partials       73       76       +3

Flag	Coverage Δ
e2e	`44.77% <55.23%> (+2.03%)`	⬆️
unit	`63.76% <61.90%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

everettraven · 2024-05-10T14:14:52Z

api/v1alpha1/clusterextension_types.go

+	// skipCRDUpgradeSafetyCheck specifies whether the CRD upgrade safety checks should be skipped when attempting to install the ClusterExtension
+	SkipCRDUpgradeSafetyCheck bool `json:"skipCRDUpgradeSafetyCheck,omitempty"`


I think @rashmigottipati is working on some changes that are going to be more extensible for future preflight checks. I think it would be okay to keep this field out of these changes

Oh, I see - this PR still has the extension type. I forgot #820 has not merged yet :)

Ignore the previous comment. I accidentally commented this on the wrong review thread.

api/v1alpha1/common_types.go

everettraven · 2024-05-13T15:31:41Z

config/admission/admission.yaml

Just a heads up, I think this is being removed in #850 due to the findings in #819

everettraven · 2024-05-13T15:36:58Z

internal/controllers/clusterextension_controller.go

+	// Don't do anything if Paused
+	ext.Status.Paused = ext.Spec.Paused
+	if ext.Spec.Paused {
+		l.Info("resource is paused", "name", ext.GetName(), "namespace", ext.GetNamespace())
+		return ctrl.Result{}, nil
+	}


I know that in theory we shouldn't do anything, but from a UX perspective I think it might be worth setting some form of status indicating that "no work was done" because it is paused.

What do you think of something like setting Progressing to False with a reason Paused?

I'm also OK with this being done in a follow-up since the scope of this PR is to make the two APIs consistent

everettraven

A couple comments relating to existing or future PRs that are modifying some of the same things as a heads up and a suggestion for UX on paused state so users don't have to check the logs to verify that a resource is in the paused state.

Since the scope of this PR is to bring the Extension and ClusterExtension APIs to parity I think that it is OK to merge this without addressing any of my comments and implementing my pause suggestion as a follow-up.

Great work @ankitathomas !

varshaprasad96 · 2024-05-13T16:36:00Z

api/v1alpha1/clusterextension_types.go

+type ClusterExtensionSource struct {
+	//+kubebuilder:validation:Enum:=package
+	//+kubebuilder:validation:Required
+	// SourceType is the discriminator for the source type
+	SourceType string `json:"sourceType"`
+
+	// Package defines a reference for a bundle in a catalog defined by a name and a version and/or channel
+	Package *SourcePackage `json:"package,omitempty"`
+}


Heads up - this change is going have us modify the helm-poc branch.
cc: @bentito @dtfranz @tmshort

varshaprasad96 · 2024-05-13T16:42:28Z

internal/controllers/clusterextension_controller.go

 	// 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())
+		if c := apimeta.FindStatusCondition(ext.Status.Conditions, ocv1alpha1.TypeInstalled); c == nil {


Do we need this additional check for finding status condition? IIRC Installed status is added by default. I see that we are not doing this for bundle validation or unpack, so to maintain consistency, it's better to rather remove this check here, and explicitly set installed to nil regardless.

We only need the status check if we want to preserve the installed condition from the currently installed thing in case of a resolution failure to indicate that the installed version is healthy even if there's an error on the Extension/ClusterExtension. We probably don't need this check once we have a separate healthy condition the users could look at instead.

varshaprasad96 · 2024-05-13T16:50:56Z

internal/controllers/clusterextension_controller.go

@@ -155,6 +167,7 @@ func (r *ClusterExtensionReconciler) reconcile(ctx context.Context, ext *ocv1alp
 		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())
+		setProgressingStatusConditionProgressing(&ext.Status.Conditions, "installation failed", ext.GetGeneration())


If installed is explicitly set to failed, and we are not able to find if the bundle deployment exists on cluster or not, I think this should be set to False instead, since there is no other state of improvement after this. Was there a specific reason to make it true instead?

I thought of the progressing condition as more of an indication of 'an install/upgrade is in progress, the currently installed bundle may be unstable'. Since it's stuck progressing here, it should be an indicator that the current installation is possibly incomplete/broken.

varshaprasad96 · 2024-05-13T16:51:54Z

internal/controllers/clusterextension_controller.go

 		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
+		setProgressingStatusConditionProgressing(&ext.Status.Conditions, "installation failed", ext.GetGeneration())


The same comment as above, why set Progressing as true and not as false when we are returning an error here and know that we cannot proceed with install?

Thinking about it, my previous comment may with respect to rukpak API may not be relevant when we get Helm stuff in, but this particular case would still hold true.

openshift-merge-robot · 2024-05-14T09:22:24Z

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

varshaprasad96 · 2024-05-15T15:52:23Z

Please hold this from merging till #846 gets in. Thanks!

ankitathomas · 2024-05-15T17:57:08Z

/hold

make extension and clusterextension api consistent

a934c27

Signed-off-by: Ankita Thomas <ankithom@redhat.com>

ankitathomas requested a review from a team as a code owner May 9, 2024 04:10

gallettilance mentioned this pull request May 9, 2024

[epic] Port relevant Extension reconciler logic over to ClusterExtension #747

Open

everettraven reviewed May 10, 2024

View reviewed changes

api/v1alpha1/common_types.go Show resolved Hide resolved

everettraven mentioned this pull request May 10, 2024

⚠ REMOVE extension api from main branch #820

Merged

4 tasks

everettraven reviewed May 13, 2024

View reviewed changes

config/admission/admission.yaml

Copy link

Contributor

everettraven May 13, 2024

Choose a reason for hiding this comment

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

Just a heads up, I think this is being removed in #850 due to the findings in #819

everettraven reviewed May 13, 2024

View reviewed changes

everettraven approved these changes May 13, 2024

View reviewed changes

varshaprasad96 reviewed May 13, 2024

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 14, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 15, 2024

varshaprasad96 mentioned this pull request May 17, 2024

🌱 Helm POC e2e fixes #870

Merged

This was referenced May 21, 2024

Review Reason and Type strings in ClusterExtension API. #880

Open

✨ Modify ClusterExtension to use Helm under the hood to apply contents into the cluster #846

Merged

varshaprasad96 mentioned this pull request May 28, 2024

🌱 Proposed way to unskip some tests #874

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ make extension and clusterextension api consistent #842

✨ make extension and clusterextension api consistent #842

ankitathomas commented May 9, 2024

netlify bot commented May 9, 2024 •

edited

codecov bot commented May 9, 2024 •

edited

everettraven May 10, 2024

everettraven May 10, 2024

everettraven May 13, 2024

everettraven May 13, 2024

everettraven May 13, 2024

everettraven May 13, 2024

everettraven left a comment

varshaprasad96 May 13, 2024

varshaprasad96 May 13, 2024

ankitathomas May 13, 2024

varshaprasad96 May 13, 2024

ankitathomas May 13, 2024

varshaprasad96 May 13, 2024

varshaprasad96 May 13, 2024

openshift-merge-robot commented May 14, 2024

varshaprasad96 commented May 15, 2024

ankitathomas commented May 15, 2024

		// skipCRDUpgradeSafetyCheck specifies whether the CRD upgrade safety checks should be skipped when attempting to install the ClusterExtension
		SkipCRDUpgradeSafetyCheck bool `json:"skipCRDUpgradeSafetyCheck,omitempty"`

		setDeprecationStatusesUnknown(&ext.Status.Conditions, "deprecation checks have not been attempted as installation has failed", ext.GetGeneration())
		setProgressingStatusConditionProgressing(&ext.Status.Conditions, "installation failed", ext.GetGeneration())

✨ make extension and clusterextension api consistent #842

Are you sure you want to change the base?

✨ make extension and clusterextension api consistent #842

Conversation

ankitathomas commented May 9, 2024

netlify bot commented May 9, 2024 • edited

✅ Deploy Preview for olmv1 ready!

codecov bot commented May 9, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

everettraven left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-merge-robot commented May 14, 2024

varshaprasad96 commented May 15, 2024

ankitathomas commented May 15, 2024

netlify bot commented May 9, 2024 •

edited

codecov bot commented May 9, 2024 •

edited