Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core: temporary disable exporter service #12118

Merged
merged 1 commit into from
Apr 21, 2023

Conversation

avanthakkar
Copy link
Member

@avanthakkar avanthakkar commented Apr 20, 2023

The exporter service should only be created if exporter pods are created. Recently the creation was disabled temporarily due to some regression detected in Ceph v17.2.6 #12077
Signed-off-by: avanthakkar avanjohn@gmail.com

Description of your changes:
Create the exporter service only if exporter pods are created.

Which issue is resolved by this Pull Request:
Resolves #

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.

Copy link
Member

@galexrt galexrt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one small nit

if _, err := k8sutil.CreateOrUpdateService(r.opManagerContext, r.context.Clientset, cephCluster.Namespace, service); err != nil {
return errors.Wrap(err, "failed to create ceph-exporter metrics service")
}
if cephVersion.IsAtLeast(cephver.CephVersion{Major: 18, Minor: 0, Extra: 0}) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a short comment on "why" Ceph version 18 is required here

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this version is used in two places, let's create a package var for it, and then add the comment there about the reason.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a MinVersionForCephExporter var

if cephVersion.IsAtLeast(cephver.CephVersion{Major: 18, Minor: 0, Extra: 0}) {
logger.Debugf("ceph exporter successfully reconciled for node %q. operation: %q", node.Name, op)
// create the metrics service
service, err := MakeCephExporterMetricsService(cephCluster, exporterServiceMetricName, r.scheme)
Copy link
Member

@parth-gr parth-gr Apr 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PLease update the PR description,

And also the PR tracker link where you disabled exporter pods.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Create the exporter service only if exporter pods are created.
Signed-off-by: avanthakkar <avanjohn@gmail.com>
@subhamkrai
Copy link
Contributor

I tested these changes on ocp and it is working

kubectl get cephcluster -oyaml | grep monitoring -A 2
      monitoring:
        rook.io/managedBy: my-storagecluster
    logCollector:
--
    monitoring:
      enabled: true
    network:

Screenshot from 2023-04-21 16-06-17

 ceph-nodedaemon-controller: Skipping exporter reconcile on ceph version "17.2.6-0 quincy"
srai@fedora ~ $ kc get svc | grep rook-ceph-exporter
~/Documents/odf-cluster
srai@fedora ~ $ 

we can see no svc was created and all the ceph daemons are created successfully.

Copy link
Contributor

@subhamkrai subhamkrai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@subhamkrai subhamkrai changed the title core: disable exporter service core: temporary disable exporter service Apr 21, 2023
@travisn travisn merged commit 7d890ae into rook:master Apr 21, 2023
49 of 50 checks passed
travisn added a commit that referenced this pull request Apr 24, 2023
core: temporary disable exporter service (backport #12118)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants