Ceph: Ability to enable pg_autoscaler mgr module via CRD #3769

leseb · 2019-09-04T15:06:50Z

Description of your changes:
We can now enable via the CRD any manager module, to do this simply do
the following:

mgr:
  modules:
  - name: pg_autoscaler
    enabled: true

Which issue is resolved by this Pull Request:
Resolves #560

Checklist:

Reviewed the developer guide on Submitting a Pull Request
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.
Pending release notes updated with breaking and/or notable changes, if necessary.
Upgrade from previous release is tested and upgrade user guide is updated, if necessary.
Code generation (make codegen) has been run to update object specifications, if necessary.
Comments have been added or updated based on the standards set in CONTRIBUTING.md
Add the flag for skipping the CI if this PR does not require a build. See here for more details.

// known CI issue
[skip ci]

cluster/examples/kubernetes/ceph/cluster.yaml

pkg/operator/ceph/cluster/mgr/mgr.go

cluster/examples/kubernetes/ceph/cluster.yaml

travisn

LGTM, assuming the final test change and then we need the CI back up again...

tests/framework/installer/ceph_manifests_v1.0.go

BlaineEXE · 2019-09-04T20:21:23Z

pkg/operator/ceph/cluster/mgr/mgr.go

+		// Ceph Octopus will have that option enabled
+		if module.Name == "pg_autoscaler" && c.clusterInfo.CephVersion.IsAtLeastNautilus() {
+			monStore := config.GetMonStore(context, clusterName)
+			err := monStore.Set("global", "osd_pool_default_pg_autoscale_mode", "on")


This will re-enable the autoscale module with every orchestration, even if the user has configured this to be off. I thought we talked about how this would be a bad idea.

I thought we discussed it would be a bad idea if we were always doing this. In this PR we are only doing this when pg_autoscaler is in the list of modules to enable in the CR, the user should expect autoscaling to be enabled, right? If they don't want the autoscaler enabled, then they wouldn't add it to the CR.

BlaineEXE · 2019-09-04T20:21:59Z

This seems to be feature creep in our v1.1 release. We will struggle with our releases continuing to be delayed if we aren't resolute about refusing new features for a release after the feature freeze date.

travisn

This seems to be feature creep in our v1.1 release.

Agreed that it is a very late feature. Enabling the autoscaler by default in the examples could be a risky change as well. How about if we comment out the pg_autoscaler in the example cluster.yaml by default? This way the new feature won't be a risk, while also making it simpler for the user to enable the autoscaler if they choose to do that instead of manage PGs.

travisn · 2019-09-04T20:29:04Z

pkg/operator/ceph/cluster/mgr/mgr.go

+		// Ceph Octopus will have that option enabled
+		if module.Name == "pg_autoscaler" && c.clusterInfo.CephVersion.IsAtLeastNautilus() {
+			monStore := config.GetMonStore(context, clusterName)
+			err := monStore.Set("global", "osd_pool_default_pg_autoscale_mode", "on")


I thought we discussed it would be a bad idea if we were always doing this. In this PR we are only doing this when pg_autoscaler is in the list of modules to enable in the CR, the user should expect autoscaling to be enabled, right? If they don't want the autoscaler enabled, then they wouldn't add it to the CR.

Documentation/ceph-cluster-crd.md

BlaineEXE · 2019-09-06T14:53:57Z

cluster/examples/kubernetes/ceph/cluster.yaml

+    modules:
+    # Several modules should not need to be included in this list. The "dashboard" and "monitoring" modules
+    # are already enabled by other settings in the cluster CR and the "rook" module is always enabled.
+    #- name: pg_autoscaler


need space between # and - and the below line should be spaced one more also to keep with the format used by most editors where commenting a block puts # before lines as has been done for the description above.

cluster/examples/kubernetes/ceph/cluster-test.yaml

BlaineEXE · 2019-09-06T14:55:48Z

pkg/operator/ceph/cluster/mgr/mgr.go

+	// Enable mgr modules from the spec
+	for _, module := range c.mgrSpec.Modules {
+		if module.Name == "" {
+			return fmt.Errorf("Name not specified for the mgr module configuration")


Errors should not start with capitals.

BlaineEXE · 2019-09-06T14:55:53Z

pkg/operator/ceph/cluster/mgr/mgr.go

+			return fmt.Errorf("Name not specified for the mgr module configuration")
+		}
+		if wellKnownModule(module.Name) {
+			return fmt.Errorf("Cannot configure mgr module %s that is configured with other cluster settings", module.Name)


Errors should not start with capitals.

BlaineEXE · 2019-09-06T14:55:58Z

pkg/operator/ceph/cluster/mgr/mgr.go

+		}
+		minVersion, versionOK := c.moduleMeetsMinVersion(module.Name)
+		if !versionOK {
+			return fmt.Errorf("Module %s cannot be configured because it requires at least Ceph version %v", module.Name, minVersion)


Errors should not start with capitals.

pkg/operator/ceph/cluster/mgr/mgr.go

BlaineEXE · 2019-09-06T15:01:03Z

pkg/operator/ceph/cluster/mgr/mgr_test.go

+	assert.Equal(t, 0, len(configSettings))
+}
+
+func TestWellKnownModule(t *testing.T) {


I don't think this test really adds value. It is effectively just testing the contents of the knownModules slice in the wellKnownModule function, and when well known modules are added in the future, it will be likely someone will miss that this test exists, bringing theout of sync with the function under test.

tests/framework/installer/ceph_manifests.go

We can now enable via the cluster CR any manager module with a setting under the new mgr element mgr: modules: - name: pg_autoscaler enabled: true Co-authored-by: Sébastien Han <seb@redhat.com> Co-authored-by: travisn <tnielsen@redhat.com> Signed-off-by: travisn <tnielsen@redhat.com>

leseb requested a review from travisn September 4, 2019 15:06

leseb force-pushed the mgr-modules branch 2 times, most recently from dc596b9 to 1e37209 Compare September 4, 2019 16:07

travisn reviewed Sep 4, 2019

View reviewed changes

cluster/examples/kubernetes/ceph/cluster.yaml Outdated Show resolved Hide resolved

pkg/operator/ceph/cluster/mgr/mgr.go Outdated Show resolved Hide resolved

leseb force-pushed the mgr-modules branch from 1e37209 to 80346d5 Compare September 4, 2019 16:38

leseb requested a review from travisn September 4, 2019 16:38

travisn reviewed Sep 4, 2019

View reviewed changes

cluster/examples/kubernetes/ceph/cluster.yaml Outdated Show resolved Hide resolved

leseb force-pushed the mgr-modules branch from 80346d5 to d8b1f27 Compare September 4, 2019 19:43

leseb requested a review from travisn September 4, 2019 19:44

travisn mentioned this pull request Sep 4, 2019

Ceph: remove configOverrides from CRD #3767

Merged

9 tasks

travisn reviewed Sep 4, 2019

View reviewed changes

tests/framework/installer/ceph_manifests_v1.0.go Outdated Show resolved Hide resolved

leseb force-pushed the mgr-modules branch from d8b1f27 to 38fa4e7 Compare September 4, 2019 19:56

BlaineEXE requested changes Sep 4, 2019

View reviewed changes

BlaineEXE added ceph main ceph tag configuration crd Changes related to a custom resource definition ceph-mgr Relating to the Ceph mgr or mgr modules labels Sep 4, 2019

travisn reviewed Sep 4, 2019

View reviewed changes

petr-balogh mentioned this pull request Sep 4, 2019

Workaround for PG bug red-hat-storage/ocs-ci#688

Merged

leseb force-pushed the mgr-modules branch from 38fa4e7 to 81b9314 Compare September 4, 2019 21:20

travisn changed the title ~~ceph: ability to enable mgr module via CRD~~ Ceph: Ability to enable pg_autoscaler mgr module via CRD Sep 5, 2019

mbukatov mentioned this pull request Sep 5, 2019

WA for BZ 1747388 with WARN too few PGs red-hat-storage/ocs-ci#687

Closed

BlaineEXE added this to the 1.2 milestone Sep 5, 2019

travisn force-pushed the mgr-modules branch from 81b9314 to 7cd1fc8 Compare September 5, 2019 23:24

travisn requested a review from BlaineEXE September 5, 2019 23:26

travisn force-pushed the mgr-modules branch 2 times, most recently from 432ec82 to 9478bad Compare September 5, 2019 23:33

BlaineEXE reviewed Sep 6, 2019

View reviewed changes

Documentation/ceph-cluster-crd.md Show resolved Hide resolved

BlaineEXE requested changes Sep 6, 2019

View reviewed changes

travisn force-pushed the mgr-modules branch from 9478bad to 8fccf71 Compare September 6, 2019 16:46

travisn requested a review from BlaineEXE September 6, 2019 16:47

travisn force-pushed the mgr-modules branch from 8fccf71 to 0808728 Compare September 6, 2019 17:31

travisn force-pushed the mgr-modules branch from 0808728 to c30cc69 Compare September 6, 2019 17:33

BlaineEXE approved these changes Sep 6, 2019

View reviewed changes

travisn merged commit fe4874d into rook:master Sep 6, 2019

rohantmp mentioned this pull request Sep 9, 2019

Rook v1.1.0-beta.2 requirement tracker. red-hat-storage/ocs-operator#105

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ceph: Ability to enable pg_autoscaler mgr module via CRD #3769

Ceph: Ability to enable pg_autoscaler mgr module via CRD #3769

leseb commented Sep 4, 2019 •

edited by travisn

travisn left a comment

BlaineEXE Sep 4, 2019

travisn Sep 4, 2019

BlaineEXE commented Sep 4, 2019 •

edited

travisn left a comment

travisn Sep 4, 2019

BlaineEXE Sep 6, 2019

BlaineEXE Sep 6, 2019

BlaineEXE Sep 6, 2019

BlaineEXE Sep 6, 2019

BlaineEXE Sep 6, 2019

Ceph: Ability to enable pg_autoscaler mgr module via CRD #3769

Ceph: Ability to enable pg_autoscaler mgr module via CRD #3769

Conversation

leseb commented Sep 4, 2019 • edited by travisn

travisn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE commented Sep 4, 2019 • edited

travisn 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

leseb commented Sep 4, 2019 •

edited by travisn

BlaineEXE commented Sep 4, 2019 •

edited