RDR: ODF info CM logic #2440

raaizik · 2024-02-04T16:06:02Z

Changes

Introduces a new CM that replaces ClusterClaim's functionalities that expose relevant data to ACM, as well as populates its data upon each ~~onboarded~~ consumer's status refresh triggered by heartbeat signal.
*️⃣ Removes isDROptimized feature that used to be part of the now deprecated ClusterClaims.

TODOs

Adjust unit tests
- *️⃣ Bug detected in the initialization function due to a yet-to-be-filled-in status of CephClusterwhich is being used by the isDROptimized function. It's been suggested to consider this feature's removal for upcoming version (4.17).
Deprecate ClusterClaims.go
Add unit tests for OdfInfoConfig

openshift-ci · 2024-02-04T16:06:07Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

raaizik · 2024-03-04T09:32:46Z

/cc @umangachapagain

nb-ohad

@raaizik This PR is trying to achieve two separate and distinct goals:

Implement a reconciler for the ODF info config map
Extend the provider onboarding request to handle additional data

Please separate each concern into its own PR

controllers/storagecluster/clusterclaims.go

controllers/storagecluster/odfinfoconfig.go

services/provider/proto/provider.proto

services/provider/server/server.go

raaizik · 2024-05-08T12:02:59Z

/retest

Signed-off-by: raaizik <[email protected]>

Introduces new CM that replaces ClusterClaims + tests Signed-off-by: raaizik <[email protected]>

nb-ohad · 2024-05-19T11:05:15Z

/lgtm

openshift-ci · 2024-05-19T11:05:25Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: nb-ohad, raaizik

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~~ [nb-ohad]

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

Nikhil-Ladha · 2024-05-20T05:55:30Z

controllers/storagecluster/odfinfoconfig_test.go

+				Kind:       "StorageSystem",
+				APIVersion: "v1",
+			},
+			ocsVersion:                   "3.16.0",


@raaizik shouldn't this be 4.16.0?
And, with the CSV version changed to 4.17 now, it would be 4.17.0, isn't it?

I have opened a PR to fix the failing test case, because of CSV version change. But, if this change is unintentional I can revert it in my open PR.

Also, I don't think we should have a dependency of version check like this in the tests, this would mean that we would need to update the version in the test after each release, which is an added task.

This test case isn't failing, @Nikhil-Ladha. Plus it's just a dummy for unit test purposes, not sure why you think it must store an up-to-date value.

The ocsversion is fetched from the storagecluster CR and compared to the one hardcoded in the test case.
And, the storagecluster version is updated in each release.
Also, the test is failing on master branch now, because the CSV is updated to 4.17.0 so, the assertion is failing at 3.17.0 != 3.16.0.
Ex: https://github.com/red-hat-storage/ocs-operator/actions/runs/9147599372/job/25149216914

@raaizik @Nikhil-Ladha In a unit test you should assume the desired value, not read it from program logic. If you do so you will get into a situation where the test does not test anything as a bug occurs in the underlying logic.

For example, assume a bug in version.Version, if both the Program code and the test read this value then even if we have a bug in the code that fills in this value we will still get the test passing.

A test must make assumptions. For this test case, I would overwrite the version.Version value with mock value (Let's say 9.9.9) and they will try to verify that the CM has the 9.9.9 by comparing to a hardcoded value

Well, that is something that should be caught while reviewing the PR.
The current code creates a new dependency of updating the hardcoded value in the test for each release, which is not something we would want to do I assume?
cc @iamniting

I think @nb-ohad's suggestion is to resolve that so called dependency by coupling the mock with some hard-coded value check in the test env. Meaning treat it as a mock in the first place as opposed to its actual real value.

I would not prefer to have hardcoded values which we will need to change every release. I agree with @Nikhil-Ladha here.

SanjalKatiyar · 2024-07-11T10:06:34Z

controllers/storagecluster/odfinfoconfig.go

+
+func getOcsVersion(r *StorageClusterReconciler, storageCluster *ocsv1.StorageCluster) (string, error) {
+	var csvs operatorsv1alpha1.ClusterServiceVersionList
+	err := r.Client.List(r.ctx, &csvs, client.InNamespace(storageCluster.Namespace))


@raaizik shouldn't this be operatorNamespace instead of storageCluster.Namespace ??
In case of multiple StorageSystems/StorageClusters, each system/cluster will be in different namespace, but CSV will always be in operator installation namespace. Am I missing something ??

I think you're right. I guess I didn't take multi SCs into account here.

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 4, 2024

raaizik force-pushed the rdr-odfinfo branch 5 times, most recently from fc09552 to 588dc5a Compare February 11, 2024 15:59

raaizik force-pushed the rdr-odfinfo branch 5 times, most recently from bd9a4fe to 5e189d6 Compare February 12, 2024 18:28

raaizik force-pushed the rdr-odfinfo branch 4 times, most recently from 16106fd to 0cbbe5a Compare February 20, 2024 14:10

raaizik force-pushed the rdr-odfinfo branch 7 times, most recently from ded7a1f to 97e2bed Compare March 3, 2024 19:21

openshift-ci bot requested a review from umangachapagain March 4, 2024 09:32

nb-ohad requested changes Mar 4, 2024

View reviewed changes

openshift-ci bot assigned nb-ohad Mar 4, 2024

raaizik force-pushed the rdr-odfinfo branch 2 times, most recently from c5226c1 to f7c208e Compare March 10, 2024 11:21

raaizik changed the title ~~RDR: odf-info map logic~~ RDR: ODF info CM logic Mar 11, 2024

raaizik force-pushed the rdr-odfinfo branch 7 times, most recently from a667762 to 4040225 Compare May 15, 2024 12:16

Update client bundle image

5b94962

Signed-off-by: raaizik <[email protected]>

raaizik force-pushed the rdr-odfinfo branch 5 times, most recently from d5c5636 to c808435 Compare May 15, 2024 13:23

ODF info CM: reconciler

8d54518

Introduces new CM that replaces ClusterClaims + tests Signed-off-by: raaizik <[email protected]>

raaizik force-pushed the rdr-odfinfo branch from 1d7dc25 to 8d54518 Compare May 15, 2024 15:04

raaizik changed the title ~~[WIP] RDR: ODF info CM logic~~ RDR: ODF info CM logic May 15, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 15, 2024

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

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

openshift-merge-bot bot merged commit ec44394 into red-hat-storage:main May 19, 2024
11 checks passed

Nikhil-Ladha reviewed May 20, 2024

View reviewed changes

raaizik mentioned this pull request May 20, 2024

Fix storagecluster version in odfinfoconfig unit test #2619

Merged

raaizik mentioned this pull request Jul 4, 2024

Reverts deprecated ClusterClaims for odf-info #2686

Merged

2 tasks

SanjalKatiyar reviewed Jul 11, 2024

View reviewed changes

SanjalKatiyar mentioned this pull request Jul 15, 2024

Bug 2297447: Use ManagedClusterView for ODF info - DRPolicy creation red-hat-storage/odf-console#1470

Merged

This was referenced Jul 16, 2024

Migrate odf-info structs to API folder #2698

Merged

Fix fetching CSV from right namespace #2699

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RDR: ODF info CM logic #2440

RDR: ODF info CM logic #2440

raaizik commented Feb 4, 2024 •

edited

Loading

openshift-ci bot commented Feb 4, 2024

raaizik commented Mar 4, 2024

nb-ohad left a comment

raaizik commented May 8, 2024

nb-ohad commented May 19, 2024

openshift-ci bot commented May 19, 2024

Nikhil-Ladha May 20, 2024

Nikhil-Ladha May 20, 2024 •

edited

Loading

Nikhil-Ladha May 20, 2024

raaizik May 20, 2024 •

edited

Loading

Nikhil-Ladha May 20, 2024

raaizik May 20, 2024

nb-ohad May 20, 2024 •

edited

Loading

Nikhil-Ladha May 20, 2024

raaizik May 20, 2024 •

edited

Loading

iamniting May 21, 2024

SanjalKatiyar Jul 11, 2024

raaizik Jul 11, 2024

RDR: ODF info CM logic #2440

RDR: ODF info CM logic #2440

Conversation

raaizik commented Feb 4, 2024 • edited Loading

Changes

TODOs

openshift-ci bot commented Feb 4, 2024

raaizik commented Mar 4, 2024

nb-ohad left a comment

Choose a reason for hiding this comment

raaizik commented May 8, 2024

nb-ohad commented May 19, 2024

openshift-ci bot commented May 19, 2024

Choose a reason for hiding this comment

Nikhil-Ladha May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raaizik May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nb-ohad May 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raaizik May 20, 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

raaizik commented Feb 4, 2024 •

edited

Loading

Nikhil-Ladha May 20, 2024 •

edited

Loading

raaizik May 20, 2024 •

edited

Loading

nb-ohad May 20, 2024 •

edited

Loading

raaizik May 20, 2024 •

edited

Loading