Skip to content

fix(ndm-exporters): changed exporter service common label#647

Merged
kmova merged 6 commits intoopenebs-archive:developfrom
Ab-hishek:fix-ndm-exporter-service-labels
Nov 26, 2021
Merged

fix(ndm-exporters): changed exporter service common label#647
kmova merged 6 commits intoopenebs-archive:developfrom
Ab-hishek:fix-ndm-exporter-service-labels

Conversation

@Ab-hishek
Copy link
Copy Markdown

Signed-off-by: Abhishek Agarwal abhishek.agarwal@mayadata.io

Why is this PR required? What issue does it fix?:
openebs/monitoring#79

What this PR does?:
This PR removes:

  1. Removes the name and component label from the ndm operator's exporter service yamls.
  2. Add a new label(common for both the cluster as well as node exporters) in the ndm exporter services that will be used for monitoring. Label to added be added is- app: openebs-ndm-exporter in both the operator as well as helm deployment.

Does this PR require any upgrade changes?:
No

If the changes in this PR are manually verified, list down the scenarios covered::

Any additional information for your reviewer? :
Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

Abhishek Agarwal added 2 commits September 20, 2021 15:04
Signed-off-by: Abhishek Agarwal <abhishek.agarwal@mayadata.io>
Signed-off-by: Abhishek Agarwal <abhishek.agarwal@mayadata.io>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 20, 2021

Codecov Report

Merging #647 (b3791e3) into develop (8472a70) will decrease coverage by 0.47%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #647      +/-   ##
===========================================
- Coverage    46.62%   46.15%   -0.48%     
===========================================
  Files           78       78              
  Lines         3820     3820              
===========================================
- Hits          1781     1763      -18     
- Misses        1880     1900      +20     
+ Partials       159      157       -2     
Impacted Files Coverage Δ
cmd/ndm_daemonset/probe/eventhandler.go 29.33% <0.00%> (-8.00%) ⬇️
cmd/ndm_daemonset/probe/addhandler.go 71.33% <0.00%> (-3.08%) ⬇️
cmd/ndm_daemonset/probe/udevprobe.go 49.01% <0.00%> (-1.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8472a70...b3791e3. Read the comment docs.

Comment thread deploy/helm/charts/templates/_helpers.tpl
Comment thread deploy/helm/charts/templates/cluster-exporter.yaml
avishnu
avishnu previously approved these changes Sep 20, 2021
Copy link
Copy Markdown

@avishnu avishnu left a comment

Choose a reason for hiding this comment

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

One minor comment on indentation, else lgtm. Thanks.

@kmova kmova added the pr/hold-merge The PR should not be merged now label Sep 21, 2021
avishnu
avishnu previously approved these changes Sep 22, 2021
Copy link
Copy Markdown

@avishnu avishnu left a comment

Choose a reason for hiding this comment

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

Lgtm

@akhilerm
Copy link
Copy Markdown
Contributor

Why is this PR in hold-merge ?

@Ab-hishek
Copy link
Copy Markdown
Author

It shouldn't be. Removing it.

@Ab-hishek Ab-hishek removed the pr/hold-merge The PR should not be merged now label Oct 12, 2021
@Ab-hishek
Copy link
Copy Markdown
Author

Ab-hishek commented Oct 12, 2021

Figured out an issue. My understandings:

  1. we are able to deploy the chart with a manual Helm install/upgrade command
  2. But when using some automated tools for helm chart installation/upgrade, the indentation may cause problem( The indentation problem is seen at 2 places- ndm-node-exporter service and ndm-cluster-exporter service). Or it will also occur due to additional line getting added in the ndm-operator deployment, ndm daemonset and ndm exporters pod templates. For eg:
  template:
    metadata:
      labels:
        chart: openebs-ndm-1.7.2
        heritage: Helm
        openebs.io/version: "1.7.0"
        app: openebs-ndm-operator
        release: RELEASE-NAME
        component: openebs-ndm-operator
        openebs.io/component-name: openebs-ndm-operator
        
        name: openebs-ndm-operator

The additional line present above has 8 spaces of indentation also present which might cause the install/upgrade problems.
Solutions:

  1. Remove the unnecessary new line with indentation from the respective templates.
  2. Remove the unwanted indentation from the exporter service templates.

The last 2 commit contains the above changes.

@kmova kmova merged commit 669c5f6 into openebs-archive:develop Nov 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants