Create a new version of odf-operator #498

iamniting · 2024-10-11T08:30:48Z

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

operator-sdk init --domain openshift.io \ --repo github.com/red-hat-storage/odf-operator \ --license apache2 --owner "Red Hat OpenShift Data Foundation" Signed-off-by: Nitin Goyal <[email protected]>

operator-sdk create api --group odf \ --version v1alpha1 --kind StorageSystem --resource --controller Signed-off-by: Nitin Goyal <[email protected]>

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

openshift-ci · 2024-10-11T08:30:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: iamniting

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

Needs approval from an approver in each of these files:

OWNERS

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

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

openshift-ci · 2024-10-22T07:42:39Z

@iamniting: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/images	`f467467`	link	true	`/test images`
ci/prow/odf-operator-e2e-aws	`f467467`	link	true	`/test odf-operator-e2e-aws`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

leelavg · 2024-11-05T11:22:13Z

cmd/main.go

-		Scheme: mgr.GetScheme(),
+		Client:            mgr.GetClient(),
+		Scheme:            mgr.GetScheme(),
+		OperatorNamespace: "openshift-storage",


pls get the namespace via downward-api.

leelavg · 2024-11-05T11:22:24Z

cmd/main.go

+	if err = (&odfwebhook.ClusterServiceVersionDefaulter{
+		Client:            mgr.GetClient(),
+		Decoder:           admission.NewDecoder(mgr.GetScheme()),
+		OperatorNamespace: "openshift-storage",


pls get the namespace via downward-api.

leelavg · 2024-11-05T11:23:37Z

internal/controller/storagesystem_controller.go

 }

 //+kubebuilder:rbac:groups=odf.openshift.io,resources=storagesystems,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=odf.openshift.io,resources=storagesystems/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=odf.openshift.io,resources=storagesystems/finalizers,verbs=update
+//+kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;create;update;patch;delete


I don't think patch verb for svc is required here.

leelavg · 2024-11-05T11:26:54Z

internal/controller/storagesystem_controller.go

 }

 //+kubebuilder:rbac:groups=odf.openshift.io,resources=storagesystems,verbs=get;list;watch;create;update;patch;delete
 //+kubebuilder:rbac:groups=odf.openshift.io,resources=storagesystems/status,verbs=get;update;patch
 //+kubebuilder:rbac:groups=odf.openshift.io,resources=storagesystems/finalizers,verbs=update
+//+kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;create;update;patch;delete
+//+kubebuilder:rbac:groups=admissionregistration.k8s.io,resources=mutatingwebhookconfigurations,verbs=get;list;watch;create;update;patch;delete


I don't think patch verb for webhook is required here.

leelavg · 2024-11-05T11:27:40Z

internal/controller/storagesystem_controller.go

-// the user.
-//
-// For more details, check Reconcile and its Result here:
-// - https://pkg.go.dev/sigs.k8s.io/[email protected]/pkg/reconcile
 func (r *StorageSystemReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {


will the reconcile for odf-console be added in another PR?

We need to copy-paste the whole reconcile, I will do that as part of the other PR.

leelavg · 2024-11-05T12:25:56Z

internal/webhook/csv.go

+)
+
+const (
+	WebhookPath = "/mutate-operators-coreos-com-v1alpha1-clusterserviceversion"


this doesn't need to be GVK identifiable by any means, not user facing, longer names are prone to typos and short name would suffice, how about '/mutate-csv' or can expand csv.

leelavg · 2024-11-05T12:26:41Z

internal/webhook/csv.go

+)
+
+var (
+	ManagedPkgNames = []string{


is this required outside of the package?

leelavg · 2024-11-05T12:28:35Z

internal/webhook/csv.go

+		// base csvs
+		"ocs-operator",
+		"rook-ceph-operator",
+		"ocs-client-operator",
+		"mcg-operator",
+		"noobaa-operator",
+		"odf-csi-addons-operator",
+		"csi-addons",
+		"cephcsi-operator",
+		"odf-prometheus-operator",
+		"recipe",
+
+		// extended csvs
+		"ibm-storage-odf-operator",


why do we need it here? you can scale down any csv which isn't odf-operator (can be queried from OLM set env easily) in the namespace since we are managing the ns

the edge case is only operators installed in all namespaces and maybe we need to look at how to filter those out.

(or)

we might want to get this info during build time about all the csvs that we going to manage, wdyt?

leelavg · 2024-11-05T12:30:55Z

internal/webhook/csv.go

+	r.logger = log.FromContext(r.context)
+	r.logger.Info("Request received for ClusterServiceVersion review")
+
+	instance := &opv1a1.ClusterServiceVersion{}


pls use the shortname, instance doesn't say anything. same in remaining places.

Suggested change

instance := &opv1a1.ClusterServiceVersion{}

csv := &opv1a1.ClusterServiceVersion{}

leelavg · 2024-11-05T12:42:29Z

internal/controller/storagesystem_controller.go

+	if err := r.reconcileWebhookService(); err != nil {
+		r.logger.Error(err, "unable to reconcile webhook service")
+		return ctrl.Result{}, err
+	}
+
+	if err := r.reconcileSubscriptionValidatingWebhook(); err != nil {
+		r.logger.Error(err, "unable to register subscription validating webhook")
+		return ctrl.Result{}, err
+	}


may I know what else be part of another PR? to name a few as below seems to be related to this PR only

watches on webhook and service is missing

addition and removal of finalizer on storagesystem is missing

removal of webhook and service during uninstallation is missing

iamniting added 3 commits September 9, 2024 13:26

Initialize domain openshift.io scaffold

96577f3

operator-sdk init --domain openshift.io \ --repo github.com/red-hat-storage/odf-operator \ --license apache2 --owner "Red Hat OpenShift Data Foundation" Signed-off-by: Nitin Goyal <[email protected]>

Create StorageSystem api scaffold

d26884a

operator-sdk create api --group odf \ --version v1alpha1 --kind StorageSystem --resource --controller Signed-off-by: Nitin Goyal <[email protected]>

Define StorageSystem api

e475f64

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

iamniting force-pushed the new1 branch 7 times, most recently from ce88e2d to 6d69725 Compare October 12, 2024 09:32

iamniting force-pushed the new1 branch from 6d69725 to 4a28ea7 Compare October 22, 2024 06:32

makefile: Update image variables

99eb888

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

iamniting force-pushed the new1 branch 2 times, most recently from fea52eb to 3636060 Compare October 22, 2024 07:05

iamniting added 9 commits October 22, 2024 12:39

bundle: add generated changes

308ca50

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

webhook: create csv webhook

5e5c631

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

config: add webhook config requirements

df87784

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

bundle: add generated changes

936d819

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

godeps: update go deps

316d270

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

dockerfile: update go version to 1.22

5c09737

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

dockerfile: add webhook dir in dockerfile

e26861a

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

makefile: create seperate dependencies bundle

a7861f6

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

bundle: add generated changes

f467467

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

iamniting force-pushed the new1 branch from 3636060 to f467467 Compare October 22, 2024 07:09

leelavg reviewed Nov 5, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a new version of odf-operator #498

Create a new version of odf-operator #498

iamniting commented Oct 11, 2024 •

edited

Loading

openshift-ci bot commented Oct 11, 2024

openshift-ci bot commented Oct 22, 2024

leelavg Nov 5, 2024

leelavg Nov 5, 2024

leelavg Nov 5, 2024

leelavg Nov 5, 2024

leelavg Nov 5, 2024

iamniting Nov 11, 2024

leelavg Nov 5, 2024

leelavg Nov 5, 2024

leelavg Nov 5, 2024

leelavg Nov 5, 2024

leelavg Nov 5, 2024

	instance := &opv1a1.ClusterServiceVersion{}
	csv := &opv1a1.ClusterServiceVersion{}

Create a new version of odf-operator #498

Are you sure you want to change the base?

Create a new version of odf-operator #498

Conversation

iamniting commented Oct 11, 2024 • edited Loading

openshift-ci bot commented Oct 11, 2024

openshift-ci bot commented Oct 22, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iamniting commented Oct 11, 2024 •

edited

Loading