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

Prepare two synchronized machines to test PCW #17429

Closed
wants to merge 2 commits into from

Conversation

ilausuch
Copy link
Contributor

@ilausuch ilausuch commented Jul 17, 2023

PCW is executed in a microos and is setup using ansible. This PR creates the environment for two VMs to test the ansible playbooks and selenium tests.

The microos is ready with all the needed packages.

zypper_call('in -y iputils git');

assert_script_run('mkdir /root/.ssh');
assert_script_run('curl -f -v ' . autoinst_url . '/data/slenkins/ssh/id_rsa > /root/.ssh/id_rsa');
Copy link
Member

Choose a reason for hiding this comment

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

Please generate new keys and put them into data/publiccloud/pcw/.

Copy link
Contributor

Choose a reason for hiding this comment

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

We should not use pre-generated keys in test runs, but rather generate new transient keys for each test run.

tests/publiccloud/pcw_target.pm Outdated Show resolved Hide resolved
Comment on lines 25 to 28
assert_script_run('zypper ar http://download.suse.de/ibs/SUSE:/CA/openSUSE_Tumbleweed/ SUSE_CA');
assert_script_run('zypper --gpg-auto-import-keys ref');
assert_script_run('transactional-update -n -c pkg install ca-certificates-suse htop iftop iotop atop telnet nmap jq git rsync wget parted tcpdump screen sqlite3 fortune iputils nginx dehydrated dehydrated-nginx podman podman-cni-config cockpit cockpit-podman toolbox python3-selinux');
process_reboot(trigger => 1);
Copy link
Member

Choose a reason for hiding this comment

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

This is already done in Ansible.

Copy link
Member

Choose a reason for hiding this comment

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

yes I would expect only python3 install here , all other things should happen via ansible script if we currently missing something we need to add this into ansible script not here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean python3-selinux?
why python3 shouldn't be in the other repo?

Copy link
Member

Choose a reason for hiding this comment

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

Try to remove this as a whole.

Copy link
Member

@asmorodskyi asmorodskyi Jul 17, 2023

Choose a reason for hiding this comment

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

Try to remove this as a whole.

no we do need to install python on target ( it is not available by default on MicroOS and requirement by ansible )

do you mean python3-selinux?

no I mean that except installation of python package target should not be changed by the test at all , no addition of extra repos , no extra packages installation . the whole point of this test is to check that on clean VM this ansible playbook can create workable system. With set of this zypper addrepo ... / zypper in ... calls you removing "on clean VM"

Copy link
Member

Choose a reason for hiding this comment

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

Anton, there is no Python in the list right now. Isn't Python on MicroOS by default? And if it's not, then Ivan will find out latest.

Copy link
Member

Choose a reason for hiding this comment

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

Anton, there is no Python in the list right now. Isn't Python on MicroOS by default? And if it's not, then Ivan will find out latest.

two times when I was doing this setup manually I was forced to install python , but you can double-check with someone who knows MicroOS better

Copy link
Contributor

Choose a reason for hiding this comment

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

python3 is not present by default I think. However this things are moving rather quickly, it's anyways good to install it explicitly, just to ensure it's present.

lib/network_utils.pm Outdated Show resolved Hide resolved
@asmorodskyi
Copy link
Member

at least for me pcw_client is a bit confusing . I would call it ansible_client and ansible_target . After all playbook will install not only PCW there

@ilausuch ilausuch added the WIP Work in progress label Jul 17, 2023
@ilausuch ilausuch force-pushed the test_pcw branch 6 times, most recently from b81c1cd to f507064 Compare July 18, 2023 11:24
@grisu48
Copy link
Contributor

grisu48 commented Jul 18, 2023

Do we really need selenium? Wouldn't it be sufficient to fetch the /health.json from the newly installed PCW instance and check if it returns the expected status object?

{"status": "ok"}

@asmorodskyi
Copy link
Member

Do we really need selenium?

  1. I think this discussion goes far away from this PR . this is something what we will decide later on
  2. depending what expected coverage for the test . I think at the end we will have several tests based on that some of them will concentrate covering ansible playbook and they won't need selenium . Others will aim give better coverage of PCW functionality and here we might use selenium

@github-actions
Copy link

Great PR! Please pay attention to the following items before merging:

Files matching lib/**.pm:

  • Consider adding or extending unit tests in t/

This is an automatically generated QA checklist based on modified files.

lib/network_utils.pm Outdated Show resolved Hide resolved
@ricardobranco777
Copy link
Contributor

ricardobranco777 commented Jul 18, 2023

Do we really need selenium? Wouldn't it be sufficient to fetch the /health.json from the newly installed PCW instance and check if it returns the expected status object?

{"status": "ok"}

I do believe that Selenium tests would fit nicely here, because the one that we have in GitHub interacts directly with Django, and in this case we have Nginx + Django.

@ilausuch (or anyone planning to add such a test):

OPTIONAL: Run the tests using a Docker image like it's done with HAWK:

lib/network_utils.pm Outdated Show resolved Hide resolved
tests/publiccloud/ansible_target.pm Outdated Show resolved Hide resolved
@ilausuch ilausuch force-pushed the test_pcw branch 2 times, most recently from ac4cf5e to 6311e33 Compare July 19, 2023 11:43
Copy link
Member

@asmorodskyi asmorodskyi left a comment

Choose a reason for hiding this comment

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

LGTM

@ilausuch ilausuch force-pushed the test_pcw branch 4 times, most recently from f0455a0 to 4cc47e8 Compare July 20, 2023 10:23
loadtest 'boot/boot_to_desktop';
loadtest 'publiccloud/ansible_client';
} elsif (get_var('PUBLIC_CLOUD_PCW_TARGET')) {
loadtest 'microos/disk_boot';
Copy link
Member

Choose a reason for hiding this comment

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

it won't work like this see line 201

Copy link
Member

Choose a reason for hiding this comment

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

if (check_var('PUBLIC_CLOUD_PREPARE_TOOLS', 1)) {
        load_create_publiccloud_tools_image();
    }
    elsif (check_var('PUBLIC_CLOUD_TOOLS_CLI', 1)) {
        load_publiccloud_cli_tools();
    }
elsif (get_var('PUBLIC_CLOUD_PCW_TARGET')) {
            loadtest 'microos/disk_boot';
            loadtest 'publiccloud/ansible_target';
}
    else {
        loadtest 'boot/boot_to_desktop';
        if (get_var('PUBLIC_CLOUD_MIGRATION')) {
            my $args = OpenQA::Test::RunArgs->new();
            loadtest('publiccloud/upload_image', run_args => $args);
            loadtest('publiccloud/migration', run_args => $args);
        } elsif (check_var('PUBLIC_CLOUD_DOWNLOAD_TESTREPO', 1)) {
            load_publiccloud_download_repos();
        } elsif (get_var('PUBLIC_CLOUD_QAM')) {
            load_maintenance_publiccloud_tests();
        } elsif (get_var('PUBLIC_CLOUD_PCW_CLIENT')) {
             loadtest 'publiccloud/ansible_client';
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, understood. In my wayt a previous loadtest is added

Comment on lines 267 to 274
if (check_var('PUBLIC_CLOUD_ANSIBLE_CLIENT', 1)) {
loadtest 'boot/boot_to_desktop';
loadtest 'publiccloud/ansible_client';
}
elsif (check_var('PUBLIC_CLOUD_ANSIBLE_TARGET', 1)) {
loadtest 'microos/disk_boot';
loadtest 'publiccloud/ansible_target';
}
Copy link
Member

Choose a reason for hiding this comment

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

I would move this out of load_slem_on_pc_tests().

zypper_call('in -y iputils git');

assert_script_run('mkdir /root/.ssh');
assert_script_run('curl -f -v ' . autoinst_url . '/data/slenkins/ssh/id_rsa > /root/.ssh/id_rsa');
Copy link
Member

Choose a reason for hiding this comment

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

Please either allow password login and generate the key in the test or at least generate our own key in data/publiccloud/

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's please not have pre-generated keys as they always expose a certain security risk. Unless really really really necessary, let's generate per-test key pairs.

Copy link
Member

Choose a reason for hiding this comment

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

Let's please not have pre-generated keys as they always expose a certain security risk. Unless really really really necessary, let's generate per-test key pairs.

IMHO it is too much . we can not apply common security rules into short living test VM running in internal network . Generating per-test key pairs coming with very high price -> in case debug is needed you can access VM only via VNC

Copy link
Member

Choose a reason for hiding this comment

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

if we really want to care about security of test VM running on internal network let's hide ssh key in worker config so it will be protected on same level as some other info which has much much more value

@ilausuch ilausuch force-pushed the test_pcw branch 2 times, most recently from b4223fa to 3e67a9a Compare July 26, 2023 12:25
PCW is executed in a microos and is setup using ansible.
This PR creates the environment for two VMs to test
the ansible playbooks and selenium tests.

The microos is ready with all the needed packages.

https://progress.opensuse.org/issues/130144
@ilausuch ilausuch force-pushed the test_pcw branch 5 times, most recently from b4e5146 to d528b0c Compare July 27, 2023 10:52
@ilausuch
Copy link
Contributor Author

We decided to move in an other direction. See the ticket

@ilausuch ilausuch closed this Jul 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants