Skip to content

Conversation

@alexbleotu
Copy link
Contributor

What does this PR do?

Implements new states and required execution and utils functions to manage distributed virtual switches (DVS) and distributed virtual portgroups (DVportgroup).

A new function file was added salt.states.dvs; in it's sysdoc there are examples of expected input.

State functions:

  • salt.states.dvs_configured state that configures/adds a DVS
  • salt.states.portgroups_configured state that configures/adds/removes DVPortgroups
  • salt.states.uplink_portgroup_configured state that configures the uplink portgroup of a DVS

Execution functions:

  • salt.modules.vsphere.list_dvss to list dict representations of a DVS
  • salt.modules.vsphere.create_dvs to create a DVS based on a dict representations
  • salt.modules.vsphere.update_dvs to update a DVS based on a dict representations
  • salt.modules.vsphere.list_dvportgroups to list dict representations of a DVPortgroups
  • salt.modules.vsphere.list_uplink_dvportgroup to list the dict representation of the uplink portgroup of a DVS
  • salt.modules.vsphere.create_dvportgroup to create a DVPortgroup based on a dict representations
  • salt.modules.vsphere.update_dvportgroup to update a DVPortgroup based on a dict representations
  • salt.modules.vsphere.remove_dvportgroup to remove a DVPortgroup

Utils functions:

  • salt.utils.vmware.get_dvss that retrieves DVSs in a datacenter
  • salt.utils.vmware.get_network_folder that retrieves the network folder
  • salt.utils.vmware.create_dvs
  • salt.utils.vmware.update_dvs
  • salt.utils.vmware.set_dvs_network_resource_management_enabled
  • salt.utils.vmware.get_dvportgroups to retrieve distributed virtual portgroups
  • salt.utils.vmware.get_uplink_dvportgroup to retrieve the uplink distributed virtual portgroup
  • salt.utils.vmware.create_dvportgroup to create a distributed virtual portgroup
  • salt.utils.vmware.update_dvportgroup to update a distributed virtual portgroup
  • salt.utils.vmware.remove_dvportgroup

Tests written?

Yes (utils functions only)

Please review Salt's Contributing Guide for best practices.

@ghost
Copy link

ghost commented Sep 19, 2017

@alexbleotu, thanks for your PR! By analyzing the history of the files in this pull request, we identified @rallytime, @cro and @skazi0 to be potential reviewers.

@cachedout
Copy link
Contributor

@alexbleotu Wow this is really great! While we get started on the reviews, could you please clean up the lint errors? They are here:

https://jenkins.saltstack.com/job/PR/job/salt-pr-lint-n/14747/violations/

This is going to be a fantastic addition!

@alexbleotu
Copy link
Contributor Author

@cachedout had to rebase, there were some exceptions added in the PR you merged yesterday I needed in this one

@alexbleotu
Copy link
Contributor Author

Don't know how to fix the remaining pylint error. Anyone know?

@cachedout
Copy link
Contributor

Hey @rallytime I don't have my new laptop set up with pylint yet. Could you check on this error for @alexbleotu please?

@rallytime
Copy link
Contributor

Yeah! @alexbleotu Your state module is using range so you need to use the one from the six lib included with salt. You can do that by simply adding from salt.ext.six.moves import range to your list of imports.

The Python 3 lint errors are not very helpful in the normal violation display, but you can find them in the console output of the lint build. (Totally not intuitive, I know, but works for now. We need to figure out what that output doesn't transfer to the jenkins error display.)

@alexbleotu
Copy link
Contributor Author

Ah cool @rallytime! Thanks for the info! Will fix ASAP

@cachedout
Copy link
Contributor

I didn't actually know that. Thanks, @rallytime !

@alexbleotu
Copy link
Contributor Author

@cachedout Finally pylint done and all tests relevant to the change pass

Copy link
Contributor

Choose a reason for hiding this comment

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

ret_dict is a list? This is a bit confusing. :]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old incarnation of the code. Will fix

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO ress and res as variable names is very hard to read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to move these private dictionary difference functions over to salt.utils.dictdiffer?

Copy link
Contributor Author

@alexbleotu alexbleotu Sep 21, 2017

Choose a reason for hiding this comment

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

Yep, I already created a RecursiveDictDiffer in salt.utils.dictdiffer which should be used here (it's in a different pull request). This is pretty old code however and I didn't have time to update it. It's not such a trivial change. That's why I've upstreamed this code and will revisit later.

@alexbleotu
Copy link
Contributor Author

@cachedout rebased and added the pyVmomi incompatibility check

@alexbleotu
Copy link
Contributor Author

Somehow some failing unit tests appeared due to #43645. Will fix ASAP

@alexbleotu
Copy link
Contributor Author

some more test fixes

@rallytime
Copy link
Contributor

Thank you very much for fixing up those tests, @alexbleotu! I guess we missed those failures when merging the other PR. I have fixed the boto and openscap test failures in other PRs.

@rallytime rallytime merged commit 16b8ec8 into saltstack:develop Sep 25, 2017
@alexbleotu alexbleotu deleted the dvs_states-gh branch September 26, 2017 11:25
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.

3 participants