-
Notifications
You must be signed in to change notification settings - Fork 164
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good @JaryN .
Can we run some tests like v2v_migrations.py so that we can validate that the HTTP 503 error is gone .
I detected some fixture changes in commit 6c9b997c4d819557502b633104404829322a6654 Show fixturesThe global fixture
The global fixture
The global fixture
The local fixture
The local fixture
The local fixture
The local fixture
The local fixture
The local fixture
The local fixture
The local fixture
The local fixture
Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃 |
No HTTP 503 failure in test run 👍 |
Would you mind rebasing this Pull Request against latest master, please? |
@sshveta Is it normal that those v2v tests are failing so much? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but it looks like no tests ran in the most recent PRT run?
Nothing really changed since I ran this with results I was happy with. It is not easy to do proper testing. I was asked to test with V2V tests, but many of them are failing for reasons I believe are not caused by this PR. At least we saw no HTTP errors this should be fixing. This is touching many tests but it is quite trivial change. Except of the name change, I added just some wait_for. I believe the smoke tests likely did hit the changed function. |
cfme/utils/appliance/__init__.py
Outdated
@@ -23,6 +23,7 @@ | |||
from manageiq_client.api import APIException | |||
from manageiq_client.api import ManageIQClient as VanillaMiqApi | |||
from urllib3.exceptions import ConnectionError | |||
from wait_for import _get_timeout_secs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This private method is triggering an exception within a test on PRT, why is it being imported?
it looks like its being used to process a timeout
kwarg, before passing to wait_for as num_sec, can't we just directly pass the timeout and let wait_for process it?
> appliance.wait_for_miq_ready()
cfme/tests/test_appliance.py:443:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
cfme/utils/log.py:177: in newfunc
return func(*args, **kwargs)
cfme/utils/appliance/__init__.py:1486: in wait_for_miq_ready
num_secs = _get_timeout_secs(timeout)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
kwargs = 900
def _get_timeout_secs(kwargs):
> if "timeout" in kwargs and kwargs["timeout"] is not None:
E TypeError: argument of type 'int' is not iterable
.cfme_venv/lib64/python3.7/site-packages/wait_for/__init__.py:36: TypeError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DOH! How could I miss this?
I removed the _get_timeout_secs
as it is private, but I had to make the wait_for_miq_ready
accepting num_sec
, not timeout
I did check all usages in pycharm. All uses of timeout
should be now replaced with num_sec
Introduce wait_for_miq_ready, remove wait_for_web_ui
We were hitting HTTP 503 when accessing MIQ API right after the server
got restarted. We were waiting for Web UI availability, but api, though
served on similar URL, was handled probably by separate process and it could
happen the requests to it returned HTTP 503 when the rest of the UI was working.
This patch renames
wait_for_web_ui
towait_for_miq_ready
and adds theblocking call for waiting for the API to it.
No work though has been done to remove any other stale API objects somewhere
else if there are any!
Test results advocacy:
cfme/tests/test_appliance.py/test_codename_in_log