pool: initialize only rbd application pools #10963

Rakshith-R · 2022-09-13T09:23:31Z

rbd pool init cmd initializes pool for rbd images. This commit makes modification to init only
rbd application pools, since it is not required
by other pools like ".nfs",".mgr" & "mgr_devicehealth".

Signed-off-by: Rakshith R rar@redhat.com

refer: #8923 (comment)

This maybe the reason cephnfs with multus is failing too.
@parth-gr ^

@idryomov can you please take a look too.

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide).
Skip Tests for Docs: If this is only a documentation change, add the label skip-ci on the PR.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

idryomov

Ack from the RBD perspective

idryomov · 2022-09-13T09:34:21Z

pkg/operator/ceph/pool/controller.go

-		return errors.Wrapf(err, "failed to initialize pool %q. %s", p.Name, string(output))
+	if appName == poolApplicationNameRBD {
+		// initialize only rbd pools.
+		logger.Infof("initializing pool %q", p.Name)


Suggested change

logger.Infof("initializing pool %q", p.Name)

logger.Infof("initializing pool %q for RBD use", p.Name)

idryomov · 2022-09-13T09:35:25Z

pkg/operator/ceph/pool/controller.go

+		if err != nil {
+			return errors.Wrapf(err, "failed to initialize pool %q. %s", p.Name, string(output))
+		}
+		logger.Infof("successfully initialized pool %q", p.Name)


Suggested change

logger.Infof("successfully initialized pool %q", p.Name)

logger.Infof("successfully initialized pool %q for RBD use", p.Name)

idryomov · 2022-09-13T09:35:43Z

pkg/operator/ceph/pool/controller.go

-	if err != nil {
-		return errors.Wrapf(err, "failed to initialize pool %q. %s", p.Name, string(output))
+	if appName == poolApplicationNameRBD {
+		// initialize only rbd pools.


Suggested change

// initialize only rbd pools.

parth-gr · 2022-09-13T09:37:40Z

pkg/operator/ceph/pool/controller.go

-	output, err := cephclient.NewRBDCommand(context, clusterInfo, args).Run()
-	if err != nil {
-		return errors.Wrapf(err, "failed to initialize pool %q. %s", p.Name, string(output))
+	if appName == poolApplicationNameRBD {


Why this function is even called if it only needs to be run for poolApplicationNameRBD

idryomov · 2022-09-13T09:40:24Z

This maybe the reason cephnfs with multus is failing too. @parth-gr ^

Whatever the failure is, I doubt that because rbd pool init is a no-op for pools with application set to something other than rbd.

Rakshith-R · 2022-09-13T09:45:58Z

Ack from the RBD perspective

Thanks, made requested changes 👍

Madhu-1 · 2022-09-13T12:14:02Z

pkg/operator/ceph/pool/controller.go

-	logger.Infof("initializing pool %q", p.Name)
-	args := []string{"pool", "init", p.Name}
-	output, err := cephclient.NewRBDCommand(context, clusterInfo, args).Run()
-	if err != nil {
-		return errors.Wrapf(err, "failed to initialize pool %q. %s", p.Name, string(output))
+	if appName == poolApplicationNameRBD {
+		logger.Infof("initializing pool %q for RBD use", p.Name)
+		args := []string{"pool", "init", p.Name}
+		output, err := cephclient.NewRBDCommand(context, clusterInfo, args).Run()
+		if err != nil {
+			return errors.Wrapf(err, "failed to initialize pool %q for RBD use. %s", p.Name, string(output))
+		}
+		logger.Infof("successfully initialized pool %q for RBD use", p.Name)
 	}
-	logger.Infof("successfully initialized pool %q", p.Name)


Suggested change

logger.Infof("initializing pool %q", p.Name)

args := []string{"pool", "init", p.Name}

output, err := cephclient.NewRBDCommand(context, clusterInfo, args).Run()

if err != nil {

return errors.Wrapf(err, "failed to initialize pool %q. %s", p.Name, string(output))

if appName == poolApplicationNameRBD {

logger.Infof("initializing pool %q for RBD use", p.Name)

args := []string{"pool", "init", p.Name}

output, err := cephclient.NewRBDCommand(context, clusterInfo, args).Run()

if err != nil {

return errors.Wrapf(err, "failed to initialize pool %q for RBD use. %s", p.Name, string(output))

}

logger.Infof("successfully initialized pool %q for RBD use", p.Name)

}

logger.Infof("successfully initialized pool %q", p.Name)

if appName != poolApplicationNameRBD{

return nil

}

// create the pool

logger.Infof("creating pool %q in namespace %q", p.Name, clusterInfo.Namespace)

if err := cephclient.CreatePool(context, clusterInfo, clusterSpec, *p, appName); err != nil {

return errors.Wrapf(err, "failed to create pool %q", p.Name)

}

logger.Infof("initializing pool %q", p.Name)

args := []string{"pool", "init", p.Name}

output, err := cephclient.NewRBDCommand(context, clusterInfo, args).Run()

if err != nil {

return errors.Wrapf(err, "failed to initialize pool %q. %s", p.Name, string(output))

}

logger.Infof("successfully initialized pool %q", p.Name)

return nil

`rbd pool init` cmd initializes pool for rbd images. This commit makes modification to init only rbd application pools, since it is not required by other pools like ".nfs",".mgr" & "mgr_devicehealth". Signed-off-by: Rakshith R <rar@redhat.com>

travisn · 2022-09-13T13:41:36Z

rbd pool init is a no-op for pools with application set to something other than rbd.

Just to be clear, no behavior change is expected with this PR, except that the operator avoids a no-op call for non-rbd pools, right?

Rakshith-R · 2022-09-13T13:47:47Z

rbd pool init is a no-op for pools with application set to something other than rbd.

Just to be clear, no behavior change is expected with this PR, except that the operator avoids a no-op call for non-rbd pools, right?

Yes,

pool: initialize only rbd application pools (backport #10963)

BlaineEXE · 2022-09-13T15:37:09Z

pkg/operator/ceph/pool/controller.go

+	if appName != poolApplicationNameRBD {
+		return nil
+	}


Is this the right behavior? Should we instead return a request to requeue the reconcile in 5-15 seconds or something instead so we are sure to configure the pool once it is initialized? @travisn ?

For non-rbd pools, the pool configuration is done at this point. For these pools there is no need to run rbd pool init, so we can just return.

Rakshith-R added block backport-release-1.10 labels Sep 13, 2022

Rakshith-R requested review from travisn and Madhu-1 September 13, 2022 09:23

idryomov approved these changes Sep 13, 2022

View reviewed changes

parth-gr reviewed Sep 13, 2022

View reviewed changes

Rakshith-R force-pushed the init-only-rbd-pool branch 2 times, most recently from bfdcf0a to 1115bba Compare September 13, 2022 09:43

Rakshith-R requested a review from idryomov September 13, 2022 09:46

idryomov approved these changes Sep 13, 2022

View reviewed changes

Madhu-1 reviewed Sep 13, 2022

View reviewed changes

Rakshith-R requested a review from Madhu-1 September 13, 2022 12:48

Rakshith-R force-pushed the init-only-rbd-pool branch from 1115bba to c02f319 Compare September 13, 2022 12:48

Madhu-1 approved these changes Sep 13, 2022

View reviewed changes

parth-gr approved these changes Sep 13, 2022

View reviewed changes

travisn merged commit c057c41 into rook:master Sep 13, 2022

mergify bot mentioned this pull request Sep 13, 2022

pool: initialize only rbd application pools (backport #10963) #10970

Merged

travisn added a commit that referenced this pull request Sep 13, 2022

Merge pull request #10970 from rook/mergify/bp/release-1.10/pr-10963

2c9751e

pool: initialize only rbd application pools (backport #10963)

BlaineEXE reviewed Sep 13, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pool: initialize only rbd application pools #10963

pool: initialize only rbd application pools #10963

Rakshith-R commented Sep 13, 2022

idryomov left a comment

idryomov Sep 13, 2022

idryomov Sep 13, 2022

idryomov Sep 13, 2022

parth-gr Sep 13, 2022

idryomov commented Sep 13, 2022

Rakshith-R commented Sep 13, 2022

Madhu-1 Sep 13, 2022 •

edited

travisn commented Sep 13, 2022

Rakshith-R commented Sep 13, 2022

BlaineEXE Sep 13, 2022

travisn Sep 13, 2022

	logger.Infof("initializing pool %q", p.Name)
	logger.Infof("initializing pool %q for RBD use", p.Name)

	logger.Infof("successfully initialized pool %q", p.Name)
	logger.Infof("successfully initialized pool %q for RBD use", p.Name)

pool: initialize only rbd application pools #10963

pool: initialize only rbd application pools #10963

Conversation

Rakshith-R commented Sep 13, 2022

idryomov left a comment

Choose a reason for hiding this comment

idryomov Sep 13, 2022

Choose a reason for hiding this comment

idryomov Sep 13, 2022

Choose a reason for hiding this comment

idryomov Sep 13, 2022

Choose a reason for hiding this comment

parth-gr Sep 13, 2022

Choose a reason for hiding this comment

idryomov commented Sep 13, 2022

Rakshith-R commented Sep 13, 2022

Madhu-1 Sep 13, 2022 • edited

Choose a reason for hiding this comment

travisn commented Sep 13, 2022

Rakshith-R commented Sep 13, 2022

BlaineEXE Sep 13, 2022

Choose a reason for hiding this comment

travisn Sep 13, 2022

Choose a reason for hiding this comment

Madhu-1 Sep 13, 2022 •

edited