This repository was archived by the owner on Aug 29, 2018. It is now read-only.
assert_proxies_disabled: Fix failures#6304
Open
Miciah wants to merge 1 commit intoopenshift:masterfrom
Open
Conversation
Change `assert_proxies_disabled` and `assert_proxies_not_disabled` to call `assert_gear_status_in_proxy` once for each proxy gear and pass `assert_gear_status_in_proxy` a list of target gears. Factor `assert_proxies_status` out of `assert_proxies_disabled` and `assert_proxies_not_disabled`. Change `assert_gear_status_in_proxy` to accept a list of target gears, use that list to construct a list of gear names to look for in the proxy's HAProxy status page, and pass the test if, and only if, the length of the list of matching gears with the expected status in the status page is equal to the number of target gears. The above changes solve the following issues: • First, `assert_gear_status_in_proxy` previously was taking a proxy gear and a target gear and was checking that both the specified proxy gear and the target gear had the specified status in the proxy gear. However, the only callers of `assert_gear_status_in_proxy` were `assert_proxies_not_disabled` and `assert_proxies_disabled`, and both of these methods called `assert_gear_status_in_proxy` for each pair of gears in both orderings. Consequently, `assert_gear_status_in_proxy` was performing redundant checks. For example, first `assert_proxies_disabled` would call `assert_gear_status_in_proxy` with `proxy=gear1` and `target_gear=gear1`, and so `assert_gear_status_in_proxy` would check that `gear1` had the right status for `gear1`. Later, `assert_proxies_disabled` would call `assert_gear_status_in_proxy` with `proxy=gear1` and `target_gear=gear2`, and so `assert_gear_status_in_proxy` would check that `gear1` had the right status for `gear1` again as well as checking that it had the right status for `gear2`. This issue is addressed by reworking the logic in `assert_proxies_disabled` and `assert_proxies_not_disabled` to use a single loop and pass a list of target gears to `assert_gear_status_in_proxy`. • Second, the logic in `assert_gear_status_in_proxy` that was intended to check the status of both gears was allowing the test to succeed even when only one gear had the expected status. This issue is addressed by using a hash to keep track of which gears are found to have the correct status. • Finally, in certain cases, `assert_gear_status_in_proxy` was getting the status from the wrong proxy gear because the primary gear was aliased by the secondary gear (see commit 4e6cb00). In test runs where this happened, the node would forward requests that used the primary proxy's host name to the secondary proxy, and so the returned status would show the secondary gear under the name "local-gear" rather than the expected name, causing the test to fail to find the secondary proxy, thus causing spurious test failures. This issue is addressed by building a more inclusive list of expected gear names and checking only that the expected number of matching gears have the expected status, so the test should pass even if it gets the wrong status page and one of the gears that otherwise would not be under the name "local-gear" is under that name. In `assert_gear_status_in_proxy`, construct the gear name by taking the host name with the domain name removed, instead of trying to extract the application name or gear UUID from the host name and constructing the gear name from that. The logic to extract the application name from the host name worked by grabbing the first part of the host name up to the first hyphen, but sometimes gears have host names and gear-registry entry names of the form "<namespace>-<application>-<n>-<namespace>" instead of the expected "<application>-<namespace>" or "<gearuuid>-<namespace>", and so the logic would erroneously take the namespace as the application name/gear UUID, look for a gear named "<namespace>-<namespace>", and consequently fail. Delete the redundant `assert_match` in `assert_proxies_disabled` that prevented the flunk from triggering. Fix the assertion flunk message in `assert_proxies_disabled` so that it prints the correct gear names instead of raising a `NoMethodError` exception for `target_gear.name`.
9d35126 to
edb9ea0
Compare
|
Evaluated for online test up to edb9ea0 |
|
Online Test Results: SUCCESS (https://ci.dev.openshift.redhat.com/jenkins/job/test_pull_requests/9020/) (Extended Tests: gear) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Change
assert_proxies_disabledandassert_proxies_not_disabledto callassert_gear_status_in_proxyonce for each proxy gear and passassert_gear_status_in_proxya list of target gears.Factor
assert_proxies_statusout ofassert_proxies_disabledandassert_proxies_not_disabled.Change
assert_gear_status_in_proxyto accept a list of target gears, use that list to construct a list of gear names to look for in the proxy's HAProxy status page, and pass the test if, and only if, the length of the list of matching gears with the expected status in the status page is equal to the number of target gears.The above changes solve the following issues:
• First,
assert_gear_status_in_proxypreviously was taking a proxy gear and a target gear and was checking that both the specified proxy gear and the target gear had the specified status in the proxy gear. However, the only callers ofassert_gear_status_in_proxywereassert_proxies_not_disabledandassert_proxies_disabled, and both of these methods calledassert_gear_status_in_proxyfor each pair of gears in both orderings. Consequently,assert_gear_status_in_proxywas performing redundant checks. For example, firstassert_proxies_disabledwould callassert_gear_status_in_proxywithproxy=gear1andtarget_gear=gear1, and soassert_gear_status_in_proxywould check thatgear1had the right status forgear1. Later,assert_proxies_disabledwould callassert_gear_status_in_proxywithproxy=gear1andtarget_gear=gear2, and soassert_gear_status_in_proxywould check thatgear1had the right status forgear1again as well as checking that it had the right status forgear2. This issue is addressed by reworking the logic inassert_proxies_disabledandassert_proxies_not_disabledto use a single loop and pass a list of target gears toassert_gear_status_in_proxy.• Second, the logic in
assert_gear_status_in_proxythat was intended to check the status of both gears was allowing the test to succeed even when only one gear had the expected status. This issue is addressed by using a hash to keep track of which gears are found to have the correct status.• Finally, in certain cases,
assert_gear_status_in_proxywas getting the status from the wrong proxy gear because the primary gear was aliased by the secondary gear (see commit 4e6cb00). In test runs where this happened, the node would forward requests that used the primary proxy's host name to the secondary proxy, and so the returned status would show the secondary gear under the name "local-gear" rather than the expected name, causing the test to fail to find the secondary proxy, thus causing spurious test failures. This issue is addressed by building a more inclusive list of expected gear names and checking only that the expected number of matching gears have the expected status, so the test should pass even if it gets the wrong status page and one of the gears that otherwise would not be under the name "local-gear" is under that name.In
assert_gear_status_in_proxy, construct the gear name by taking the host name with the domain name removed, instead of trying to extract the application name or gear UUID from the host name and constructing the gear name from that. The logic to extract the application name from the host name worked by grabbing the first part of the host name up to the first hyphen, but sometimes gears have host names and gear-registry entry names of the form "<namespace>-<application>-<n>-<namespace>" instead of the expected "<application>-<namespace>" or "<gearuuid>-<namespace>", and so the logic would erroneously take the namespace as the application name/gear UUID, look for a gear named "<namespace>-<namespace>", and consequently fail.Delete the redundant
assert_matchinassert_proxies_disabledthat prevented the flunk from triggering.Fix the assertion flunk message in
assert_proxies_disabledso that it prints the correct gear names instead of raising aNoMethodErrorexception fortarget_gear.name.[test] [extended:gear]