controllers: Ensure console plugin creation after CSV verification #511

iamniting · 2024-11-21T11:36:29Z

Delay the creation of the console plugin until after verifying that the required CSVs are present. This ensures the necessary CRDs are present in the cluster, preventing UI errors like "Model does not exist."

Signed-off-by: Nitin Goyal [email protected]

SanjalKatiyar · 2024-11-21T12:36:27Z

controllers/clusterversion_controller.go

-	err := mgr.Add(manager.RunnableFunc(func(context.Context) error {
-		clusterVersion, err := util.DetermineOpenShiftVersion(r.Client)
-		if err != nil {
-			return err
-		}
-
-		return r.ensureConsolePlugin(clusterVersion)
-	}))
-	if err != nil {
-		return err
-	}
-


@bipuladh u remember why this custom runnable was added ?? Removing it seems reasonable, but double checking.

leelavg · 2024-11-28T03:37:03Z

pkg/util/readiness.go

pls recheck if exporting all methods is not necessary.

leelavg · 2024-11-28T03:40:12Z

pkg/util/readiness.go

+	for _, csv := range csvList.Items {
+		if csv.Name == name {
+			return &csv, true
+		}
+	}


pls query by index, here there is a creation of new var and it creates many not required values escaping the function scope.

leelavg · 2024-11-28T03:46:52Z

controllers/clusterversion_controller.go

+		return ctrl.Result{}, err
+	}
+
+	if err := util.EnsureCSVsPresent(ctx, r.Client, r.OperatorNamespace, EssentialCSVs...); err != nil {


if this is all we want, why to continue to use readiness probe involving a hack?

We need both of these. The readiness probe will stop the customer from creating the storage cluster. Creating the console plugin later will show the popup once everything is ready.

from creating the storage cluster.

from CLI? ack.

It will stop on the UI. We can not stop on the CLI and we are not concerned about the CLI.

leelavg · 2024-11-28T03:48:25Z

controllers/clusterversion_controller.go

+
+	if err := util.EnsureCSVsPresent(ctx, r.Client, r.OperatorNamespace, EssentialCSVs...); err != nil {
+		logger.Error(err, "Could not ensure CSVs in Succeeded state")
+		return ctrl.Result{Requeue: true, RequeueAfter: time.Second * 2}, nil


2 sec is demanding, the observed time was 60s in total and 10s interval will be relaxing. (or) remove the time and add a csv predicate creation event w/ essential CSVs.

I don't want to add long intervals, Adding a longer interval can delay the creation of the web console, which means a delay in the installation. Also, this will happen only when there is an installation or upgrade. It reminds me to handle the upgrade case.

I don't want to add long intervals,

not sure for what qualifies as longer or shorter interval in a reconciliation and so dropping my comment.

leelavg · 2024-11-28T03:52:39Z

controllers/subscription_controller.go

+
+		odfDepsSub := GetStorageClusterSubscriptions()[0]
+
+		if odfDepsSub.Spec.Package != OdfDepsSubscriptionPackage {
+			panic("odfDepsSub variable is expected to contain the 'odf-dependencies' subscription. " +
+				"Ensure the 'odf-dependencies' subscription indexed at 0.")
+		}
+
+		return EnsureDesiredSubscription(r.Client, odfDepsSub)
+


pls remove extra lines, all actions seems to be a single unit.

leelavg · 2024-11-28T03:53:30Z

controllers/subscription_controller.go

@@ -290,6 +291,22 @@ func (r *SubscriptionReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		},
 	)

+	err := mgr.Add(manager.RunnableFunc(func(ctx context.Context) error {


may I know what's the expectation w/ runnablefunc?

This will invoke the function as soon as the manager starts. It means it will create this sub even before reconciles are started.

leelavg · 2024-11-28T03:53:45Z

controllers/subscription_controller.go

+			panic("odfDepsSub variable is expected to contain the 'odf-dependencies' subscription. " +
+				"Ensure the 'odf-dependencies' subscription indexed at 0.")


who needs to ensure this? if this is static at build time why not move this to main? may I know what's the requirement? why can't we make an arrangement for caller to explicitly get deps subscription and then the remaining?

We developer needs to ensure this while coding. I added a panic so that we can catch this while testing. Maybe I can create a unit test to ensure this. This is not a build time. It is introduced at a coding time.

coding time.

I do mean the same, static at/by build time. Unit test would suffice, ack.

Delay the creation of the console plugin until after verifying that the required CSVs are present. This ensures the necessary CRDs are present in the cluster, preventing UI errors like "Model does not exist." Signed-off-by: Nitin Goyal <[email protected]>

creating odf-deps subscription while starting manager will speed up the process. Signed-off-by: Nitin Goyal <[email protected]>

leelavg

/hold
Unhold if no one else is going to review. Thanks.

bipuladh

Looks good to me.

openshift-ci · 2024-12-09T06:38:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bipuladh, iamniting, leelavg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [iamniting]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

SanjalKatiyar · 2024-12-09T07:19:31Z

controllers/clusterversion_controller.go

+	// Check if it is upgrade from 4.17 to 4.18
+	// The new CSVs won't exists while upgrading
+	// They will exists only after new operator has created a new subscription
+	if !util.AreMultipleOdfOperatorCsvsPresent(csvList) {


just a general comment/doubt (no need to change the current implementation):

this controller will only run when either OCP is upgraded (i.e., For(&configv1.ClusterVersion{})) or when controller will restart after ODF is upgraded (in which case new CSVs will be existing soon)... we don't really need !util.AreMultipleOdfOperatorCsvsPresent(csvList) check, right ?? or am I missing any case ??

This check is to make sure that we don't block the reconcile in case of upgrades. For example assume customers has messed up the catalog for some reason in that case only odf will be upgraded and other operators wont be upgraded. So we should not block the console updation in that case right?

So we should not block the console updation in that case right?

make sense.

For example assume customers has messed up the catalog for some reason in that case only odf will be upgraded and other operators wont be upgraded.

If ODF is upgraded and others won't, then we are still blocked right ?? because !util.AreMultipleOdfOperatorCsvsPresent(csvList) will be true in that case.

Yes, that's right, It will skip if and only if the odf-operator upgrade is in progress.

actually that was my initial doubt, why would this controller run at all when ODF upgrade is in progress: #511 (comment)

Once the manager is started, The controllers will also get triggered.

I have added this check to keep the behavior as before and that is once the odf-operator is upgraded, I also upgrade the console plugin.

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 21, 2024

SanjalKatiyar reviewed Nov 21, 2024

View reviewed changes

iamniting force-pushed the ui branch 3 times, most recently from 53d1354 to 38ad0b1 Compare November 27, 2024 14:20

iamniting requested review from leelavg and bipuladh November 27, 2024 15:03

leelavg reviewed Nov 28, 2024

View reviewed changes

iamniting added 2 commits December 4, 2024 16:05

controllers: create odf-deps sub while starting manager

3649806

creating odf-deps subscription while starting manager will speed up the process. Signed-off-by: Nitin Goyal <[email protected]>

iamniting force-pushed the ui branch from 38ad0b1 to 3649806 Compare December 4, 2024 10:35

iamniting requested a review from leelavg December 4, 2024 10:36

leelavg approved these changes Dec 5, 2024

View reviewed changes

openshift-ci bot added the do-not-merge/hold label Dec 5, 2024

openshift-ci bot assigned leelavg Dec 5, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 5, 2024

bipuladh approved these changes Dec 9, 2024

View reviewed changes

openshift-ci bot assigned bipuladh Dec 9, 2024

SanjalKatiyar reviewed Dec 9, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

controllers: Ensure console plugin creation after CSV verification #511

controllers: Ensure console plugin creation after CSV verification #511

iamniting commented Nov 21, 2024 •

edited

Loading

SanjalKatiyar Nov 21, 2024

leelavg Nov 28, 2024

iamniting Dec 4, 2024

leelavg Nov 28, 2024

iamniting Dec 4, 2024

leelavg Nov 28, 2024

iamniting Nov 28, 2024

leelavg Nov 28, 2024

iamniting Dec 4, 2024 •

edited

Loading

leelavg Nov 28, 2024

iamniting Nov 28, 2024

leelavg Nov 28, 2024

leelavg Nov 28, 2024

iamniting Dec 4, 2024

leelavg Nov 28, 2024

iamniting Nov 28, 2024

leelavg Nov 28, 2024

iamniting Nov 28, 2024 •

edited

Loading

leelavg Nov 28, 2024 •

edited

Loading

iamniting Dec 4, 2024

leelavg left a comment •

edited

Loading

bipuladh left a comment

openshift-ci bot commented Dec 9, 2024

SanjalKatiyar Dec 9, 2024

iamniting Dec 9, 2024

SanjalKatiyar Dec 9, 2024

iamniting Dec 9, 2024 •

edited

Loading

SanjalKatiyar Dec 9, 2024 •

edited

Loading

iamniting Dec 9, 2024

		panic("odfDepsSub variable is expected to contain the 'odf-dependencies' subscription. " +
		"Ensure the 'odf-dependencies' subscription indexed at 0.")

controllers: Ensure console plugin creation after CSV verification #511

Are you sure you want to change the base?

controllers: Ensure console plugin creation after CSV verification #511

Conversation

iamniting commented Nov 21, 2024 • edited Loading

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

iamniting Dec 4, 2024 • edited Loading

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

Choose a reason for hiding this comment

iamniting Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

leelavg Nov 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leelavg left a comment • edited Loading

Choose a reason for hiding this comment

bipuladh left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Dec 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamniting Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

SanjalKatiyar Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamniting commented Nov 21, 2024 •

edited

Loading

iamniting Dec 4, 2024 •

edited

Loading

iamniting Nov 28, 2024 •

edited

Loading

leelavg Nov 28, 2024 •

edited

Loading

leelavg left a comment •

edited

Loading

iamniting Dec 9, 2024 •

edited

Loading

SanjalKatiyar Dec 9, 2024 •

edited

Loading