Now check your code on my lab. Get idempotent result.

I'm not sure I following what you mean - the first task in your screenshot Create VM portgroup (always causes a change) is marked as changed in the log - which is the same behaviour I'm seeing.

Perhaps this is a more understandable test case, though less minimal as it includes a loop which isn't required to demonstrate the issue.

- name: Create VM portgroup (always causes a change)
  community.vmware.vmware_dvs_portgroup:
    portgroup_name: portgroup1
    switch_name: dvswitch1
    vlan_id: 999
    port_binding: static
    port_allocation: elastic
    mac_learning:
      allow_unicast_flooding: true
      enabled: true
      limit: 4096
      limit_policy: allow
    state: present
    hostname: "{{ vcenter_url }}"
    username: "{{ vsphere_username }}"
    password: "{{ vsphere_password }}"
    validate_certs: false
  with_sequence: count=5

- name: Create VM portgroup (no change)
  community.vmware.vmware_dvs_portgroup:
    portgroup_name: portgroup1
    switch_name: dvswitch1
    vlan_id: 999
    port_binding: static
    port_allocation: elastic
    state: present
    hostname: "{{ vcenter_url }}"
    username: "{{ vsphere_username }}"
    password: "{{ vsphere_password }}"
    validate_certs: false
  with_sequence: count=5

@TheMysteriousX Now I run playbook again (portgroup1 was created yesterday) and both play get same result. Module is idempotent. What do you think is wrong?

I did some further testing and it looks like there's an additional element. I'd overlooked that in the task prior to setting mac_learning, we were setting network_policy.

It doesn't appear to matter if network_policy is set at the same time, or prior to setting mac_learning so I've adapted the test case to highlight both.

- name: Set network_policy then mac_learning, alternating - incorrect behaviour
  community.vmware.vmware_dvs_portgroup:
    portgroup_name: portgroup1
    switch_name: dvswitch1
    vlan_id: 999
    port_binding: static
    port_allocation: elastic
    network_policy: "{{ network_policy_dict if ansible_loop.index0 is even else omit }}"
    mac_learning: "{{ mac_learn_dict if ansible_loop.index0 is odd else omit }}"
    state: present
    hostname: "{{ vcenter_url }}"
    username: "{{ vsphere_username }}"
    password: "{{ vsphere_password }}"
    validate_certs: false
  with_sequence: count=6
  loop_control:
    extended: true
  vars:
    mac_learn_dict:
      allow_unicast_flooding: true
      enabled: true
      limit: 4096
      limit_policy: allow
    network_policy_dict:
      inherited: true

- name: Always causes a change - incorrect behaviour
  community.vmware.vmware_dvs_portgroup:
    portgroup_name: portgroup1
    switch_name: dvswitch1
    vlan_id: 999
    port_binding: static
    port_allocation: elastic
    mac_learning:
      allow_unicast_flooding: true
      enabled: true
      limit: 4096
      limit_policy: allow
    network_policy:
      inherited: true
    state: present
    hostname: "{{ vcenter_url }}"
    username: "{{ vsphere_username }}"
    password: "{{ vsphere_password }}"
    validate_certs: false
  with_sequence: count=5

- name: Does not cause a change - correct behaviour
  community.vmware.vmware_dvs_portgroup:
    portgroup_name: portgroup1
    switch_name: dvswitch1
    vlan_id: 999
    port_binding: static
    port_allocation: elastic
    network_policy:
      inherited: true
    state: present
    hostname: "{{ vcenter_url }}"
    username: "{{ vsphere_username }}"
    password: "{{ vsphere_password }}"
    validate_certs: false
  with_sequence: count=5

- name: Causes a change the first iteration - incorrect behaviour
  community.vmware.vmware_dvs_portgroup:
    portgroup_name: portgroup1
    switch_name: dvswitch1
    vlan_id: 999
    port_binding: static
    port_allocation: elastic
    mac_learning:
      allow_unicast_flooding: true
      enabled: true
      limit: 4096
      limit_policy: allow
    state: present
    hostname: "{{ vcenter_url }}"
    username: "{{ vsphere_username }}"
    password: "{{ vsphere_password }}"
    validate_certs: false
  with_sequence: count=5

TASK [vsphere : Set network_policy then mac_learning, alternating - incorrect behaviour, mac_learning is not controlled by policy/inheritable] **********************************************************
changed: [host] => (item=1)
changed: [host] => (item=2)
changed: [host] => (item=3)
changed: [host] => (item=4)
changed: [host] => (item=5)
changed: [host] => (item=6)

TASK [vsphere : Always causes a change - incorrect behaviour] ******************************************************************************************************************************************
changed: [host] => (item=1)
changed: [host] => (item=2)
changed: [host] => (item=3)
changed: [host] => (item=4)
changed: [host] => (item=5)

TASK [vsphere : Does not cause a change - correct behaviour] ******************************************************************************************************************************************
ok: [host] => (item=1)
ok: [host] => (item=2)
ok: [host] => (item=3)
ok: [host] => (item=4)
ok: [host] => (item=5)

TASK [vsphere : Causes a change the first iteration - incorrect behaviour] *****************************************************************************************************************************
changed: [host] => (item=1)
ok: [host] => (item=2)
ok: [host] => (item=3)
ok: [host] => (item=4)
ok: [host] => (item=5)

I run your code in my lab and I see that every second reconfigure action change config.defaultPortConfig.securityPolicy.inherited, config.defaultPortConfig.macManagementPolicy.inherited and config.defaultPortConfig.macManagementPolicy.macLearningPolicy.* config parts (from true to false or vice versa, and another changes in config.defaultPortConfig).

Need to dive more thoughtfully into the module code to understand why it behaves this way.

@mariolenz Mario, this module is terrible.

    def update_port_group(self):
        config = self.build_config()
        config.configVersion = self.dvs_portgroup.config.configVersion
        task = self.dvs_portgroup.ReconfigureDVPortgroup_Task(config)
        changed, result = wait_for_task(task)
        return changed, result

    def create_port_group(self):
        config = self.build_config()
        task = self.dv_switch.AddDVPortgroup_Task([config])
        changed, result = wait_for_task(task)
        return changed, result

It no change any difference in current config and desired config. Just create new config (without some parts of config). This make module not idempotent. =(

Is this still an issue?

I haven't tested it with 5.x.x but it's still present on 4.x.x - if there's something that you think will have fixed it in the 5.x.x releases, I can try updating our plays.

I had another look and I think this should also happen in 5.x. I guess we have to have a closer look at this part of the code:

BTW what version of the collection are you using? Is it the current 4.x one, that is 4.8.2?

Note to myself: DVSMacLearningPolicy(vim.dvs.VmwareDistributedVirtualSwitch.MacLearningPolicy)

I can't reproduce this. We have actually an integration test to make sure that mac_learning is idempotent. Only without allow_unicast_flooding, limit and limit_policy.

I've added them, just to make sure. But the CI still succeeds: See #2360 for 5.x and #2361 for 4.x.

I've even checked the logs to make sure that the tests were really executed. They were, and didn't report any issues with idempotency when mac_learning is set.

I just updated to 4.8.3 to confirm and it's definitely still present.

In your test case, I don't see network_policy being set - we discovered this was also required to expose the issue: #1873 (comment)

Yes, this might be an explanation. I'll have another look.

Out of curiosity, what's the setting of your dvswitch for network_policy.mac_changes? Is it true or false? At the end of the day, how does the config really look like? No matter if you do this via this collection or not.

Why I want to know this: Maybe the API call to allow MAC address learning silently overrides this. After all, it doesn't make much sense to have MAC address learning if MAC addresses can't change... does it?

So my idea at the moment is:

1. mac_learning.enabled: true overrides network_policy.inherited to false and allows MAC address changes
2. network_policy.inherited: true is set if you run the playbook again
3. start again at 1

An infinite loop. I'm not 100% sure, just guessing. Although (I hope!) it's an educated guess :-)

Anyway, if you don't want to allow MAC address changes, you don't need MAC learning... or do you?

BTW #2360 and #2361 fail with network_policy.inherited: true.

On the dvSwitch, we have the policy defined as:

  forged_transmits: false
  mac_changes: false
  promiscuous: false

On most of the port groups, we just want to pull that through with inheritance, except for a few port groups where layer 2 trickery is used for HA, and a few more where multiple MAC's sit on the port.

It shouldn't be a conflict, as the use case for mac learning is to avoid having to set the "allow bad behaviour" policy flags, but I haven't explictly checked - I've just been assuming that as all our VM's are up and reachable, the config gets set correctly in spite of the 'changed' notice.

I can give it a poke with powercli and see what the actual state of each port is, probably tomorrow.

I think I understand now. There's no toggling between two configs, MAC address learning ist simply ignored.

Technically, the configuration for MAC address learning is in the same place like promiscuous, forged transmits and MAC address changes: DVSMacManagementPolicy(vim.dvs.VmwareDistributedVirtualSwitch.MacManagementPolicy).

This policy can be set on the portgroup or inherited from the switch. But inheriting means the whole set of policies, including the stuff for MAC address learning. You can't inherit some of them and override others. It's all or nothing. And this isn't something we have under control or can change in the module, it's how the vSphere API works.

I guess we should document this somewhere 🤔

That makes sense, I hadn't realised the mac learning setting is also inherited.

I guess a warning or error if mac_learning is set when network_policy: inherit is too would serve as documentation.

It's unfortunate that mac_learning isn't a sub-option of network_policy. If it was, it would be more clear to users. In the long run, I guess it would make sense to move mac_learning to network_policy. But I'm not sure what's the best way to do this.

I guess a warning or error if mac_learning is set when network_policy: inherit is too would serve as documentation.

ATM I'm working on mentioning this at least in the documentation.

@ihumster What do you think about the module issuing a warning in this case? But then, we should also have a warning for the other sub-options of network_policy, too. I prefer to mention it just in the documentation only. What's your opinion?