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

Add multi-machine setup script #2347

Merged
merged 8 commits into from Aug 1, 2023

Conversation

okurz
Copy link
Member

@okurz okurz commented Jul 26, 2023

@okurz okurz marked this pull request as draft July 26, 2023 20:42
@okurz okurz force-pushed the feature/setup_mm_poo132134_133025 branch from 1f7fccd to 9bd2d9d Compare July 31, 2023 11:59
@okurz okurz marked this pull request as ready for review July 31, 2023 21:24
@okurz okurz force-pushed the feature/setup_mm_poo132134_133025 branch from 9bd2d9d to 9cca1aa Compare July 31, 2023 21:53
@okurz
Copy link
Member Author

okurz commented Aug 1, 2023

Added commit "Include os-autoinst-setup-multi-machine in doc+package"

@okurz okurz force-pushed the feature/setup_mm_poo132134_133025 branch from afb76d3 to 228c7a0 Compare August 1, 2023 06:10
@okurz
Copy link
Member Author

okurz commented Aug 1, 2023

I don't understand

[  133s] found conflict of os-autoinst-4.6.1690870233.228c7a0-lp155.1624.1.x86_64 with os-autoinst-openvswitch-4.6.1690870233.228c7a0-lp155.1624.1.x86_64:
[  133s]   - /usr/bin/os-autoinst-setup-multi-machine

anyone has an idea?

CMakeLists.txt Outdated Show resolved Hide resolved
@okurz okurz force-pushed the feature/setup_mm_poo132134_133025 branch from 228c7a0 to 55e0554 Compare August 1, 2023 08:47
@Martchus
Copy link
Contributor

Martchus commented Aug 1, 2023

Maybe that new script gets pulled into the main package as well via that macro: https://en.opensuse.org/openSUSE:Packaging_Conventions_RPM_Macros#%perl_gen_filelist

I found the use of this macro very confusing/unintuitive. So if it goes in the way I suggest to simply remove the use of %perl_gen_filelist and list those files explicitly.

@okurz okurz force-pushed the feature/setup_mm_poo132134_133025 branch from ed441a8 to 11d9834 Compare August 1, 2023 09:52
@okurz
Copy link
Member Author

okurz commented Aug 1, 2023

Maybe that new script gets pulled into the main package as well via that macro: https://en.opensuse.org/openSUSE:Packaging_Conventions_RPM_Macros#%perl_gen_filelist

I found the use of this macro very confusing/unintuitive. So if it goes in the way I suggest to simply remove the use of %perl_gen_filelist and list those files explicitly.

Ok, for now I just removed the explicit mention of the file

@Martchus
Copy link
Contributor

Martchus commented Aug 1, 2023

But then it will end up in the wrong package.

@okurz
Copy link
Member Author

okurz commented Aug 1, 2023

But then it will end up in the wrong package.

Well, I wouldn't care. It's just a simple shell script :)

EDIT: A better explanation is: os-autoinst-setup-multi-machine can actually handle to install os-autoinst-openvswitch if it is not installed yet and also the setup script does not have additional dependencies so the script should be in the main package

@okurz
Copy link
Member Author

okurz commented Aug 1, 2023

Observed jobs on w23 still not working as in https://openqa.opensuse.org/tests/3473166#step/firewalld_policy_objects/138 when tap devices were still not in the right zone.

Added two commits

  • os-autoinst-setup-multi-machine: Use more common 'br0' as ethernet
  • os-autoinst-setup-multi-machine: Ensure devices are created in correct zone

@okurz okurz force-pushed the feature/setup_mm_poo132134_133025 branch from dd7aab0 to a900bef Compare August 1, 2023 12:24
action="\$1"
bridge="\$2"
ovs-vsctl set bridge \$bridge stp_enable=true
# TODO add entries according to your network topology
Copy link
Member

Choose a reason for hiding this comment

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

What is this TODO telling me? Do we need to add things here? Are we expecting users to edit the script before using it? If so, make it fail, so they know to edit it

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the admin reading this needs to patch up the script afterwards. If it would fail then wicked would likely ignore the failure anyway.

@mergify mergify bot merged commit 9eca513 into os-autoinst:master Aug 1, 2023
15 checks passed
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.

None yet

3 participants