fix: handle array as comma separated value in zapi #1810

Hardikl · 2023-03-10T11:08:59Z

Tested with Volume and NetworkInterface object.

For Volume, with changes:

hardikl@hardikl-mac-2 ~ % curl -s 'http://localhost:13001/metrics'  | grep volume_labels | grep aggr | wc -l
     164

hardikl@hardikl-mac-2 ~ % curl -s 'http://localhost:13001/metrics'  | grep volume_labels | grep flexvol | grep aggr | wc -l
     163
hardikl@hardikl-mac-2 ~ % curl -s 'http://localhost:13001/metrics'  |grep volume_labels | grep flexvol | grep aggr     
volume_labels{aggr="umeng_aff300_aggr2",cluster="umeng-aff300-01-02",datacenter="u2",isEncrypted="false",isHardwareEncrypted="false",is_sis_volume="true",node="umeng-aff300-01",node_root="false",root_volume="No",snapshot_policy="none",state="online",style="flexvol",svm="astra_300",svm_root="false",type="rw",volume="trident_pvc_4e7e77ff_39a3_43e2_b4c8_69e31ce90b93"} 1.0
volume_labels{aggr="umeng_aff300_aggr2",cluster="umeng-aff300-01-02",datacenter="u2",isEncrypted="false",isHardwareEncrypted="false",is_sis_volume="true",node="umeng-aff300-01",node_root="false",root_volume="No",snapshot_policy="none",state="online",style="flexvol",svm="astra_300",svm_root="false",type="rw",volume="trident_pvc_9b4f1397_106d_4359_8076_2f3d89ba447f"} 1.0
volume_labels{aggr="umeng_aff300_aggr2",cluster="umeng-aff300-01-02",datacenter="u2",isEncrypted="false",isHardwareEncrypted="false",is_sis_volume="true",node="umeng-aff300-01",node_root="false",root_volume="No",snapshot_policy="none",state="online",style="flexvol",svm="astra_300",svm_root="false",type="rw",volume="trident_pvc_c37ce9f9_2b8d_406a_8610_2f511868882c"} 1.0
volume_labels{aggr="test",cluster="umeng-aff300-01-02",datacenter="u2",isEncrypted="false",isHardwareEncrypted="false",is_sis_volume="true",node="umeng-aff300-02",node_root="false",root_volume="No",snapshot_policy="default",state="online",style="flexvol",svm="astra_304",svm_root="false",type="rw",volume="dummy"} 1.0
volume_labels{aggr="test",cluster="umeng-aff300-01-02",datacenter="u2",isEncrypted="false",isHardwareEncrypted="false",is_sis_volume="true",node="umeng-aff300-02",node_root="false",root_volume="No",snapshot_policy="none",state="online",style="flexvol",svm="astra_304",svm_root="false",type="rw",volume="trident_pvc_ce04e59a_9073_407c_bbce_c149b0e6a32c"} 1.0
volume_labels{aggr="test",cluster="umeng-aff300-01-02",datacenter="u2",isEncrypted="false",isHardwareEncrypted="false",is_sis_volume="false",node="umeng-aff300-02",node_root="false",root_volume="Yes",snapshot_policy="default",state="online",style="flexvol",svm="test_banner_154_1",svm_root="true",type="rw",volume="test_banner_154_1_root"} 1.0
...

hardikl@hardikl-mac-2 ~ % curl -s 'http://localhost:13001/metrics'  | grep volume_labels | grep flexgroup | grep aggr | wc -l
       1
hardikl@hardikl-mac-2 ~ % curl -s 'http://localhost:13001/metrics'  |grep volume_labels | grep flexgroup | grep aggr        
volume_labels{aggr="test,umeng_aff300_aggr2",cluster="umeng-aff300-01-02",datacenter="u2",isEncrypted="false",isHardwareEncrypted="false",is_sis_volume="true",node="",node_root="false",root_volume="No",snapshot_policy="none",state="online",style="flexgroup",svm="osc",svm_root="false",type="rw",volume="RahulTest"} 1.0

For NetworkInterface, custom Rest:

name:               networkinterface
query:              api/private/cli/network/interface
object:             netint

counters:
  - ^^data_protocol => data_protocol
  - ^address        => address
  - ^^lif           => interface

plugins:
  LabelAgent:
    value_to_num:
      - test_status status ok ok 1

export_options:
  include_all_labels: true

For NetworkInterface, custom zapi, with changes,

name: networkinterface
query: net-interface-get-iter
object: netint

collect_only_labels: true

counters:
  net-interface-info:
    - ^address
    - ^^interface-name => interface
    - data-protocols:
      - ^data-protocol => data_protocol

plugins:
  LabelAgent:
    value_to_num:
      - test_status status ok ok 1

export_options:
  include_all_labels: true

code changes
unit test case for array

Hardikl · 2023-03-10T11:11:03Z

Fix #1360

Hardikl · 2023-03-13T12:55:53Z

As no extra symbol for array handling in Zapi, we don't need any documentation change for this.

Hardikl · 2023-03-13T13:03:19Z

For Array case, response would be something like this:

printed as {{node name}} - {{node value}} : {{child names list}}

2023-03-13T16:03:09+05:30 INF collector/zapi.go:306 > volume-id-attributes - <aggr-list><aggr-name>Ak_Aggregate</aggr-name><aggr-name>Ak_Aggregate_HE</aggr-name><aggr-name>puk_aggr2_on_154</aggr-name><aggr-name>puk_aggr3_on_154</aggr-name></aggr-list><instance-uuid>ce6d03e9-be1c-11eb-b50d-00a098d7b0b9</instance-uuid><name>test_flexgroup_dont_delete_1</name><owning-vserver-name>svm_workload_dont_delete_1622024777713</owning-vserver-name><style-extended>flexgroup</style-extended><type>rw</type> : [aggr-list instance-uuid name owning-vserver-name style-extended type] Poller=cluster-01 collector=Zapi:Volume

2023-03-13T16:03:09+05:30 INF collector/zapi.go:306 > aggr-list - <aggr-name>Ak_Aggregate</aggr-name><aggr-name>Ak_Aggregate_HE</aggr-name><aggr-name>puk_aggr2_on_154</aggr-name><aggr-name>puk_aggr3_on_154</aggr-name> : [aggr-name aggr-name aggr-name aggr-name] Poller=cluster-01 collector=Zapi:Volume

cgrinds · 2023-03-14T14:54:06Z

cmd/collectors/zapi/collector/zapi.go

@@ -279,7 +287,13 @@ func (z *Zapi) PollData() (map[string]*matrix.Matrix, error) {
 		}

 		for _, child := range node.GetChildren() {
-			fetch(instance, child, newpath)
+			uniqueNames := util.RemoveDuplicateStr(child.GetAllChildNamesS())
+			if len(child.GetAllChildNamesS()) > 1 && len(uniqueNames) < len(child.GetAllChildNamesS()) && len(uniqueNames) == 1 {


since child.GetAllChildNamesS() does work and allocations, better to calculate once

also not sure I completely understand this logic - an array is defined as:

multiple child elements (makes sense)

where those children have duplicates (why can an array have duplicates?)

where those duplicates unique to a single item (why?)

This is one of the response o/p:

<volume-id-attributes> <aggr-list> <aggr-name>Ak_Aggregate</aggr-name> <aggr-name>Ak_Aggregate_HE</aggr-name> <aggr-name>puk_aggr2_on_154</aggr-name> <aggr-name>puk_aggr3_on_154</aggr-name> </aggr-list> <instance-uuid>ce6d03e9-be1c-11eb-b50d-00a098d7b0b9</instance-uuid> <name>test_flexgroup_dont_delete_1</name> <owning-vserver-name>svm_workload_dont_delete_1622024777713</owning-vserver-name> <style-extended>flexgroup</style-extended> <type>rw</type> </volume-id-attributes>

For node volume-id-attributes, child elements are : [aggr-list instance-uuid name owning-vserver-name style-extended type]
Whereas for node aggr-list, child elements are: [aggr-name aggr-name aggr-name aggr-name]

From Zapidoc,

So,
Case 1: Parent node have multiple different child elements Ex. volume-id-attributes
Case 2: Parent node may have few duplicates and few unique child elements (I am not much sure about this case)
Case 3: Parent node have multiple same child elements Ex. aggr-list, which eventually only 1 child with array case.

cmd/collectors/zapi/collector/zapi.go

pkg/util/util.go

conf/zapi/cdot/9.8.0/volume.yaml

cmd/collectors/zapi/collector/zapi.go

rahulguptajss · 2023-03-16T10:05:35Z

cmd/collectors/zapi/collector/zapi.go

+				// Handling array with comma separated values
+				previousValue := instance.GetLabel(label)
+				if isAppend && previousValue != "" {
+					instance.SetLabel(label, previousValue+","+value)


In past we have been setting empty for array elements. That needs to be fixed as well.

Out of 4 volume plugin(zapi, zapiperf, rest, restperf),

zapiperf and restperf are exact same and handling aggr, node as perf call don't have this array in response. --> no changes required.

zapi don't have any handling for flexgroup.

rest has this below handling for aggruuid and it still holds true as zapi don't handle aggruuid for flexgroup.

How is isHardwareEncrypted handled for flexgroup if aggruuid is always empty for it? Also comment about aggrUUid in code doesn't match if Zapi doesn't do it this way.

So, Zapi would handle this way,
First populate aggrsDisksMap and then populate aggrsMap.
For flexgroup, which span across multiple aggregates, it can't be part of any map.
So for them label isHardwareEncrypted alway be false.

for _, aggrDiskData := range result { aggrUUID := aggrDiskData.GetChildContentS("aggregate-uuid") aggrName := aggrDiskData.GetChildContentS("aggregate") aggrDiskList := aggrDiskData.GetChildS("aggr-plex-list").GetChildS("aggr-plex-info").GetChildS("aggr-raidgroup-list").GetChildS("aggr-raidgroup-info").GetChildS("aggr-disk-list").GetChildren() for _, aggrDisk := range aggrDiskList { diskName = aggrDisk.GetChildContentS("disk") aggrsDisksMap[diskName] = aggrData{aggrUUID: aggrUUID, aggrName: aggrName} } } return aggrsDisksMap, nil

func (my *Volume) updateAggrMap(disks []string, aggrDiskMap map[string]aggrData) { if disks != nil && aggrDiskMap != nil { // Clean aggrsMap map my.aggrsMap = make(map[string]string) for _, disk := range disks { aggr := aggrDiskMap[disk] my.aggrsMap[aggr.aggrUUID] = aggr.aggrName } } }

rahulguptajss · 2023-03-16T10:07:02Z

conf/zapi/cdot/9.8.0/volume.yaml

      - ^owning-vserver-name        => svm
-      - ^containing-aggregate-name  => aggr
+      - aggr-list:
+          - ^aggr-name              => aggr


We should revisit added metrics volume_aggr_labels which were added last release. Let's discuss if it is safe to remove them or we want to keep publishing them.

for now, I don't change it and based on our discussion we will take call on volume_aggr_labels.

rahulguptajss · 2023-03-17T06:26:08Z

cmd/collectors/zapi/collector/zapi.go

+				// Handling array with comma separated values
+				previousValue := instance.GetLabel(label)
+				if isAppend && previousValue != "" {
+					instance.SetLabel(label, previousValue+","+value)


How is isHardwareEncrypted handled for flexgroup if aggruuid is always empty for it? Also comment about aggrUUid in code doesn't match if Zapi doesn't do it this way.

pkg/util/util.go

rahulguptajss · 2023-03-20T14:13:21Z

looks good. Completes Zapi gap with Rest 💯

fix: handle array as comma separated value in zapi

de2ad20

Hardikl requested review from cgrinds and rahulguptajss as code owners March 10, 2023 11:09

cla-bot bot added the cla-signed label Mar 10, 2023

Hardikl added 4 commits March 10, 2023 18:50

Merge remote-tracking branch 'origin/main' into zapi_array_handle

e4e3b84

fix: handle array in zapi

d8949b1

Merge remote-tracking branch 'origin/main' into zapi_array_handle

ab7b4d2

fix: remove unused var

64505a7

cgrinds reviewed Mar 14, 2023

View reviewed changes

fix: handle review comments

e3e8c8d

cgrinds previously approved these changes Mar 15, 2023

View reviewed changes

rahulguptajss reviewed Mar 16, 2023

View reviewed changes

fix: handle review comments

78c45a0

Hardikl dismissed cgrinds’s stale review via 78c45a0 March 16, 2023 13:39

Merge remote-tracking branch 'origin/main' into zapi_array_handle

e4cfb3c

rahulguptajss reviewed Mar 17, 2023

View reviewed changes

Hardikl added 2 commits March 20, 2023 17:16

fix: handle review comments

7ce9863

Merge remote-tracking branch 'origin/main' into zapi_array_handle

445186b

rahulguptajss approved these changes Mar 20, 2023

View reviewed changes

cgrinds merged commit 372a8b1 into main Mar 20, 2023

cgrinds deleted the zapi_array_handle branch March 20, 2023 14:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: handle array as comma separated value in zapi #1810

fix: handle array as comma separated value in zapi #1810

Hardikl commented Mar 10, 2023 •

edited

Loading

Hardikl commented Mar 10, 2023

Hardikl commented Mar 13, 2023

Hardikl commented Mar 13, 2023

cgrinds Mar 14, 2023

Hardikl Mar 15, 2023

rahulguptajss Mar 16, 2023

Hardikl Mar 16, 2023

rahulguptajss Mar 17, 2023

Hardikl Mar 20, 2023

rahulguptajss Mar 16, 2023

Hardikl Mar 16, 2023

rahulguptajss Mar 17, 2023

rahulguptajss commented Mar 20, 2023

fix: handle array as comma separated value in zapi #1810

fix: handle array as comma separated value in zapi #1810

Conversation

Hardikl commented Mar 10, 2023 • edited Loading

Hardikl commented Mar 10, 2023

Hardikl commented Mar 13, 2023

Hardikl commented Mar 13, 2023

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

rahulguptajss commented Mar 20, 2023

Hardikl commented Mar 10, 2023 •

edited

Loading