Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend OpenVSwitch modules #58987

Merged
merged 1 commit into from
Dec 9, 2022
Merged

Conversation

smarsching
Copy link
Contributor

@smarsching smarsching commented Nov 19, 2020

What does this PR do?

This PR adds functions to the OpenVSwitch modules for two purposes:

  • Creating bridges that have a parent bridge. Such bridges can be used in OpenVSwitch for having a bridge that is associated with a specific (tagged) VLAN on a parent bridge.
  • Updating values in the OpenVSwitch database. This can be used to update arbitrary properties of OpenVSwitch objects. For example, it can be used to assign a fixed MAC address to the interface associated with a certain OpenVSwitch port.

In detail, the following changes are made to the code:

  • The openvswitch_bridge.present(…) state function is modified, adding two new optional parameters for the parent bridge and VLAN ID.
  • The openvswitch.bridge_create(…) execution function is modified, adding two new optional parameters for the parent bridge and VLAN ID. This function is used by salt.states.openvswitch_bridge.present(…).
  • The openvswitch.bridge_to_parent(…) execution function is added. This function is used by salt.states.openvswitch_bridge.present(…) to determine the parent of an existing bridge.
  • The openvswitch.bridge_to_vlan(…) execution function is added. This function is used by salt.states.openvswitch_bridge.present(…) to determine the VLAN ID of an existing bridge.
  • The openvswitch_db state modules is added. This module has a single function called openvswitch_db.managed. This function can be used to make sure that a record in the OpenVSwitch database has certain values.
  • The openvswitch.db_get execution function is added. This function is used by openvswitch_db.managed(…) to read a value from the OpenVSwitch database.
  • The openvswitch.db_set execution function is added. This function is used by `openvswitch_db.managed(…) to write a value to the OpenVSwitch database.

What issues does this PR fix or reference?

Fixes: #58986
There was an earlier PR (#50564) essentially providing the same changes that got merged into develop, but was never ported to master.

Merge requirements satisfied?

Commits signed with GPG?

No

@smarsching smarsching requested a review from a team as a code owner November 19, 2020 16:30
@smarsching smarsching requested review from Akm0d and removed request for a team November 19, 2020 16:30
@sagetherage sagetherage requested review from dmurphy18 and removed request for Akm0d March 5, 2021 23:49
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

@smarsching Are you still interested in this PR ?, if unresponsive the PR shall be closed after several days.
Test cases need addressing as well as resolving conflicts

@smarsching
Copy link
Contributor Author

@dmurphy18 I rebased the PR against the current master branch and addressed all linting issues. Are there any other things that need to be changed?

I saw that two checks (pr-centos-7-x86_64-py3-pycryptodome-pytest and pr-centos-7-x86_64-py3-tcp-pytest) failed, but the log output does not give any information about why they failed (at least no information that I could decipher). Could you have a look please and tell me, so that I can try to reproduce and fix these problems if needed?

@dmurphy18
Copy link
Contributor

@smarsching Thanks for the updates, will take a look and let you know what I find

@smarsching
Copy link
Contributor Author

@dmurphy18 Thanks for having another look. It seems like merging the master branch triggered another run of the checks, which all succeeded now. Maybe the two failed checks were simply causes by a temporary hiccup in the CI system.

Please let me know whether any more actions are needed before this can be merged.

@dmurphy18 dmurphy18 added the Sulfur v3006.0 release code name and version label Nov 29, 2022
Copy link
Contributor

@dmurphy18 dmurphy18 left a comment

Choose a reason for hiding this comment

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

Have some style changes
The test files are the old test/unit and while still valid, wondering if you would be able to make them pytests since that is what is preferred moving forward ?

salt/modules/openvswitch.py Outdated Show resolved Hide resolved
salt/modules/openvswitch.py Show resolved Hide resolved
salt/modules/openvswitch.py Show resolved Hide resolved
salt/modules/openvswitch.py Show resolved Hide resolved
salt/modules/openvswitch.py Show resolved Hide resolved
salt/modules/openvswitch.py Outdated Show resolved Hide resolved
salt/modules/openvswitch.py Outdated Show resolved Hide resolved
salt/modules/openvswitch.py Outdated Show resolved Hide resolved
salt/states/openvswitch_bridge.py Outdated Show resolved Hide resolved
salt/states/openvswitch_db.py Show resolved Hide resolved
@smarsching
Copy link
Contributor Author

Thank you very much for your review and your comments regarding the style. I will address the changes that you requested, but it will probably take a few days before I find the time to do so. I will notify you when the code is ready for another review.

@Ch3LL
Copy link
Contributor

Ch3LL commented Dec 5, 2022

We are attempting to get everything merged in by this Friday. Just letting you know if you want to get this in before then.

@smarsching smarsching force-pushed the issue-58986 branch 2 times, most recently from 3ccf391 to 9d4602d Compare December 9, 2022 11:26
@smarsching
Copy link
Contributor Author

@Ch3LL Thanks for letting me know. Unfortunately, I couldn’t get to it earlier this week due to other, more urgent projects. Maybe you still have a change to merge it today.

@dmurphy18 I addressed your change requests regarding documentation style. All functions that have been newly introduced or that have had parameters added now use the newer style. I also added “versionadded” to all newly introduced modules, functions, and parameters. The unit tests have been converted to the new “pytest” style. I am now waiting for all the checks to complete and hope that I have not introduced any new problems with these changes. 😬

@smarsching
Copy link
Contributor Author

It looks like all the “-pytest” checks failed, but with a quite unspecific message “script returned exit code 20”. Unfortunately, the build log is truncated and thus does not contain any useful information either, so now I am not sure whether these failures are caused by my changes (converting the tests to pytest) or whether they appeared because I rebased the branch on the newest version of the master branch.

@dmurphy18 Do you have any idea how I could get a more concrete error message from this jobs that might at least give me a hint to the cause of the problems?

@smarsching
Copy link
Contributor Author

After managing to create a local test setup, I got a meaningful error message that allowed me to figure out what’s happening: The tests that were converted to pytest should also have been moved to tests/pytest. After doing this, the tests run, but there still is a problem with them. As soon as I have fixed this (probably in an hour or so), I will push the changes. With a bit of luck, this will resolve all issues and make the checks succeed.

@dmurphy18
Copy link
Contributor

dmurphy18 commented Dec 9, 2022

FYI - @waynew Holds Test Clinics on Twitch, which deals with issues writing tests for Salt https://www.twitch.tv/saltprojectoss by example, that is, fixing tests, migrating unit tests to pytest, taking a PR and resolving test failures by updating the test, etc.

Salt now only accepts new tests if they are pytests. Can still modify / update existing unit tests, but these are being transitioned to pytest, as time allows (background task).

Thanks for moving your tests over to pytest, which as you found reside under tests/pytest. There are also helpers for pytest with Salt, see https://pytest-salt-factories.readthedocs.io/en/stable/contents.html and https://pytest-skip-markers.readthedocs.io/en/latest/contents.html#table-of-contents

This adds the new openvswitch_db state module. It also  adds the new
functions bridge_to_parent, bridge_to_vlan, db_get, and db_set to the
openvswitch execution module.

Besides, it adds two new optional parameters parent and vlan to the
openvswitch_bridge.present state module function and the
openvswitch.bridge_create execution module function.
@smarsching
Copy link
Contributor Author

@dmurphy18 Thanks for your reply. I think that I managed to sort out the problems with the tests. All checks except for the “pr-windows-2019-x64-py3-pytest” check succeeded now. That check is still in progress. It looks to me like it cannot progress due to some problem with an EC2 instance.

As it seems like all issues have been resolved, could you please give this PR another review, so that it can be merged?

@smarsching
Copy link
Contributor Author

@dmurphy18 Just so that you know, the last check has completed as well and all checks are marked as successful now.

@dmurphy18
Copy link
Contributor

Thanks @smarsching , will review soon

@garethgreenaway garethgreenaway merged commit 3c9baed into saltstack:master Dec 9, 2022
@smarsching smarsching deleted the issue-58986 branch December 9, 2022 22:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Sulfur v3006.0 release code name and version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Extend OpenVSwitch modules with VLAN and DB functions
4 participants