Improve server profile idempotency

[fgbulsoni]

Scenario/Intent

On some usage scenarios, when running with the 'present' ensurable, the server profile does not behave idempotently as expected and makes calls when it shouldn't.

Environment Details

Puppet module for HPE OneView Version: 2.0.0 OneView SDK Version: 3.1.0 OneView Appliance Version: 3.0 OneView Client API Version: 300 Puppet Version: 4.8 Ruby Version: 2.2.6 Platform: Windows

Steps to Reproduce

Run complete scenarios with server profile, specifying as many options as possible.

Expected Result

If there are no changes between manifest runs, or if the change is something which should not matter, like order on a hash, then no create/update/patch calls should be made to OneView since it should be able to notice nothing has changed.

Actual Result

On some scenarios it actually does not act as expected and runs update calls.

[marikrg]

Reopening this issue, the module doesn't seem idempotent.

I have a manifest to create a server profile with connections and deployment settings:

oneview_server_profile{'server_profile_present':
  ensure  => 'present',
  data    =>
  {
    name                  => 'Server Profile with DP',
    serverHardwareTypeUri => 'SY 480 Gen9 1',
    enclosureGroupUri     => 'DCS-i3s-EG',
    osDeploymentSettings  => {
      osDeploymentPlanUri => '/rest/os-deployment-plans/18d42e6d-f64e-422b-affd-7185603ab9c4'
    },
    bootMode              => {
      manageMode    => true,
      mode          => 'UEFIOptimized',
      pxeBootPolicy => 'Auto'
    },
    connections           => [{
      id            => 1,
      portId        => 'Mezz 3:1-a',
      name          => 'Deployment Network A',
      functionType  => 'Ethernet',
      networkUri    => 'Deployment Network',
      requestedMbps => 2500,
      requestedVFs  => 'Auto',
      boot          => {
        priority            => 'Primary',
        initiatorNameSource => 'ProfileInitiatorName'
      }
    }, {
      id            => 2,
      portId        => 'Mezz 3:2-a',
      name          => 'Deployment Network B',
      functionType  => 'Ethernet',
      networkUri    => 'Deployment Network',
      requestedMbps => 2500,
      requestedVFs  => 'Auto',
      boot          => {
        priority            => 'Secondary',
        initiatorNameSource => 'ProfileInitiatorName'
      }
    }]
  }
}

Every time I run this manifest (with no changes), the resource is being updated due to the differences found at:

{
  "connections": [
    {
      "id": 1,
      "portId": "Mezz 3:1-a",
      "name": "Deployment Network A",
      "functionType": "Ethernet",
      "networkUri": "/rest/ethernet-networks/36b9d48f-853c-4485-bdc9-4e57f92339c7",
      "requestedMbps": 2500,
      "requestedVFs": "Auto",
      "boot": {
        "priority": "Primary",
        "initiatorNameSource": "ProfileInitiatorName"
      }
    },
    {
      "id": 2,
      "portId": "Mezz 3:2-a",
      "name": "Deployment Network B",
      "functionType": "Ethernet",
      "networkUri": "/rest/ethernet-networks/36b9d48f-853c-4485-bdc9-4e57f92339c7",
      "requestedMbps": 2500,
      "requestedVFs": "Auto",
      "boot": {
        "priority": "Secondary",
        "initiatorNameSource": "ProfileInitiatorName"
      }
    }
  ],
  "osDeploymentSettings": {
    "osDeploymentPlanUri": "/rest/os-deployment-plans/18d42e6d-f64e-422b-affd-7185603ab9c4"
  }
}

More details about the issue:

• Appliance: OneView 3.0
• API Version: 300
• Hardware Variant: Synergy

I guess the Server Profile Template (#101) would have the same behaviour with similar data.

[tmiotto]

@marikrg

Could you highlight exactly what points do you think the module is classifying as changes?

It could be by comments like:

{
  a: 1,
  b: 2,  # Considering this line as a change, but it is not since it's the same value for 'b' in Integer
  c: 3
}

[marikrg]

@tmiotto Yes, I'm going to investigate it later. At this point, I just realised that the difference is located at the connections list and osDeploymentSettings.

[marikrg]

The current merge/diff works well only for the string/integer/boolean values in the first depth.

In case of hashes (e.g. connections, bios/boot settting, osDeploymentSettings, etc), the value must be exactly the same from the OneView appliance, otherwise the resource will be updated.

Often, we are not able to provide all values, because some of them will be set automatically, or have default values. For example, if I have a manifest with the osDeploymentSettings:

  "osDeploymentSettings": {
    "osDeploymentPlanUri": "/rest/os-deployment-plans/18d42e6d-f64e-422b-affd-7185603ab9c4"
  }

it doesn't work because of these auto-assigned values:

    "osVolumeUri": null,
    "osCustomAttributes": [

    ]

[fgbulsoni]

Suggestion: Currently, the oneview_server_profile provider's exists? method uses find_by passing in the whole @data. How about using the like? comparisson from the SDK instead? I understand that find_by should look for exact matches while our like? method should handle cases such as those defaults or other stuff. This is the expected behavior, right @tmiotto ? @marikrg, can we verify if the issue still persists even using like? P.S. This would be related to #66 if swapping out for the like? does not work, and it's something we'd likely want fixed in the ruby sdk.

[tmiotto]

Yep, the like? will only look for the attributes declared you want to change (A Hash or an Incomplete resource) that you need to test against a remote resource (retrieved by retrieve! or find_by). The find_by will only gather the resources that match exactly with the passed attributes.

[marikrg]

@fgbulsoni @tmiotto Yes, I will check.

[abiliogp]

The 'like?' has the same problem of 'find_by' function. Actually, I had investigated this problem to, and I think that the problem is the 'merge' function. Maybe a deep merge should be used, instead of default merge. In:

def resource_update
  ...   
  raw_merged_data = current_attributes.merge(data)
  ...

Because the default merge ignores deep values, such as 'connections'. @marikrg @tmiotto @fgbulsoni

[fgbulsoni]

Maybe a deep merge should be used,

This is looking like two issues instead of one if this is the case.

1. If you pass in a value (such as uri_field: null), that is passed in by default and gets changed after creation, that should not be counted as a difference.
2. If you pass a field that contains multiple levels of hashes or arrays and that is actually not changed, should not be updated, but is being updated somehow because of order issues.

For 1, the like? should handle that. And we should not even get to call the resource_update then. If like? does not correctly handle it, an issue should be open against the ruby sdk and explicitly show the case where it fails. For 2, if we're actually updating something with its exact value just because of ordering, then a deep merge could/should be implemented in the resource_update to fix it. Let me just remind you that deep_merge is actually a rails method and will have to be implemented manually since we do not use rails.

We should be sure if we're failing in one or both of these scenarios and fix them accordingly.