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

QE: Lazy initialization of Twopence nodes #7334

Merged
merged 1 commit into from
Aug 8, 2023
Merged

Conversation

srbarrios
Copy link
Member

@srbarrios srbarrios commented Aug 1, 2023

What does this PR change?

Related card https://github.com/SUSE/spacewalk/issues/22101

Note: This is a Prove of Concept. Might need a follow up refactor.

Lazy initialization of Twopence nodes:

When we start the test framework, we load the Ruby file twopence_init.rb, this is a plain Ruby file that at the beginning of the file start declaring variables for each fqdn of every systems we handle in the test framework, inside that we call twopence_init() passing by parameter the fqdn, and spend some time establishing a connection with each client.

If a twopence node can't be initialized this cause a failure in the test framework and it stops, which might be good, but the side effect of this is the need to initialize all when we start and that can take a lot of time.

The proposal is to don't initialize any twopence node when the twopence_init.rb file is loaded, but instead we have a getter method to retrieve the specific twopence node we want to use in our step definitions, and the first time we call this getter, we check if the variable is null or has a value, if it has a value, we return the value, if not we call twopence_init() method to initialize that node.

That would decouple responsibilities and outside the test framework, we don't need to care about which clients we will be using in order to initialize them.

GUI diff

No difference.

  • DONE

Documentation

Test coverage

  • Cucumber tests were refactored

  • DONE

Links

Ports:

  • Manager-4.3

  • DONE

Changelogs

Make sure the changelogs entries you are adding are compliant with https://github.com/uyuni-project/uyuni/wiki/Contributing#changelogs and https://github.com/uyuni-project/uyuni/wiki/Contributing#uyuni-projectuyuni-repository

If you don't need a changelog check, please mark this checkbox:

  • No changelog needed

If you uncheck the checkbox after the PR is created, you will need to re-run changelog_test (see below)

Re-run a test

If you need to re-run a test, please mark the related checkbox, it will be unchecked automatically once it has re-run:

  • Re-run test "changelog_test"
  • Re-run test "backend_unittests_pgsql"
  • Re-run test "java_pgsql_tests"
  • Re-run test "schema_migration_test_pgsql"
  • Re-run test "susemanager_unittests"
  • Re-run test "javascript_lint"
  • Re-run test "spacecmd_unittests"

Copy link
Contributor

@Bischoff Bischoff left a comment

Choose a reason for hiding this comment

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

lgtm

@srbarrios
Copy link
Member Author

Waiting to pass the full acceptance tests, but looks quite good on my first tests.

@srbarrios srbarrios force-pushed the qe-lazy-init-twopence branch 5 times, most recently from 47ef6bc to 9bc64a6 Compare August 1, 2023 20:37
@maximenoel8
Copy link
Contributor

maximenoel8 commented Aug 2, 2023

I like the changes ! Just few questions to be sure I understand.
Now we will not open ssh sessions with all the nodes at the beginning but only when we want to interact with one node ( awesome ). And if we do more than one call during a feature, we will reuse the node ssh session correct ?
The node session will be close at the end of each Feature ( file.feature ) correct ?

@maximenoel8
Copy link
Contributor

maximenoel8 commented Aug 2, 2023

Having a method run with get_target('server') makes sense but I'm a little bit surprise that we also create a ssh session to get the full_hostname.
Do we have situations where the get_target('server').full_hostname is different from ENV['SERVER'] ?
If you don't want to have a direct ENV call in the features, we could create a function get_full_hostname('server').
What do you think ?

@srbarrios
Copy link
Member Author

I like the changes ! Just few questions to be sure I understand.
Now we will not open ssh sessions with all the nodes at the beginning but only when we want to interact with one node ( awesome ). And if we do more than one call during a feature, we will reuse the node ssh session correct ?
The node session will be close at the end of each Feature ( file.feature ) correct ?

First of all, clarify something, a twopence node instance will not keep a ssh session open, it just keep all the details so it can make ssh calls to a node.
Each ssh call is independent and will close the session after the reply.

And then, we use a global variable to define a hashmap that will contain the twopence node instances, so the nodes once created, will live during all test framework life, they will not be deleted per feature.

@srbarrios
Copy link
Member Author

Having a method run with get_target('server') makes sense but I'm a little bit surprise that we also create a ssh session to get the full_hostname.
Do we have situations where the get_target('server').full_hostname is different from ENV['SERVER'] ?
If you don't want to have a direct ENV call in the features, we could create a function get_full_hostname('server').
What do you think ?

The idea is to have all what we could need from a target contained in the Lavanda Object as attributes.
We could maybe safe one ssh call by reusing the info from the environement variable, that's fair, but also we could perfectly pass the IP instead of the fqdn in the environment variable... so that's not a fully reliable source to get the fqdn.

For now, I would not refactor that part, at least not as part of this PR. Because it could bring some unexpected corner cases.

@srbarrios srbarrios force-pushed the qe-lazy-init-twopence branch 4 times, most recently from 3ee1c76 to 003a418 Compare August 8, 2023 10:29
@srbarrios srbarrios merged commit 2fc0089 into master Aug 8, 2023
2 of 8 checks passed
@srbarrios srbarrios deleted the qe-lazy-init-twopence branch August 8, 2023 10:35
@nodeg nodeg mentioned this pull request Aug 11, 2023
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants