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

Wait for OPA to load rules in tests startup #5483

Merged
merged 11 commits into from
Dec 20, 2022

Conversation

zhiltsov-max
Copy link
Contributor

Motivation and context

OPA can take some time to load rules, but our tests don't wait for OPA, and start right after the server is loaded.
Sometimes it works, but in other times the tests may fail because OPA is still loading the rules.
This PR allows to wait for OPA during the test suite startup.

How has this been tested?

Checklist

License

  • I submit my code changes under the same MIT License that covers the project.
    Feel free to contact the maintainers if that's a concern.


def wait_for_opa():
for i in range(300):
logging.getLogger(__package__).debug(f"waiting for the opa to load ... ({i})")
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhiltsov-max , we have our own health check which have the logic.

def wait_for_server():
for _ in range(30):
for i in range(300):
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhiltsov-max , need to use our own health check.

@nmanovic nmanovic requested review from sizov-kirill and removed request for azhavoro December 17, 2022 08:48
@nmanovic
Copy link
Contributor

@kirill-sizov , could you please comment?

@sizov-kirill
Copy link
Contributor

sizov-kirill commented Dec 17, 2022

@kirill-sizov , could you please comment?

My comment is the same as yours: probably we should reuse our GET /api/server/healthcheck method.

I also noticed that in our docker-compose file cvat_server depends on cvat_opa, I think this is incorrect since after recent changes cvat_opa became depending on cvat_server because cvat_opa cannot bundle rules without cvat_server

@zhiltsov-max
Copy link
Contributor Author

zhiltsov-max commented Dec 17, 2022

Changed to use the server command, as recommended in the docs.

UPD: changed to the server as it looks more generic (e.g. for k8s deployments)

@@ -205,7 +223,7 @@ def kube_restore_data_volumes():
pod_name = _kube_get_server_pod_name()
kube_cp(
CVAT_DB_DIR / "cvat_data.tar.bz2",
f"{pod_name}:/tmp/cvat_data.tar.bz2",
f"{pod_name}:/tifmp/cvat_data.tar.bz2",
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhiltsov-max , is it typo?

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, fixed. Sometimes I occasionally can paste something with my laptop's touchpad.

response = requests.get(get_server_url("api/server/health/", format="json"))
try:
statuses = response.json()
if all(v == "working" for v in statuses.values()):
Copy link
Contributor

Choose a reason for hiding this comment

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

@zhiltsov-max , I believe it is better just to check the response status. If it is 200, the server is fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

sleep(5)
def wait_for_services():
for i in range(300):
logging.getLogger(__package__).debug(f"waiting for the server to load ... ({i})")
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably it is better to get the logger only once.

logger = logging.getLogger(__package__)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@nmanovic nmanovic merged commit 760f40d into develop Dec 20, 2022
@nmanovic nmanovic deleted the zm/fix-tests-startup-waiting branch December 20, 2022 15:56
mikhail-treskin pushed a commit to retailnext/cvat that referenced this pull request Jul 1, 2023
OPA can take some time to load rules, but our tests don't wait for OPA,
and start right after the server is loaded.
Sometimes it works, but in other times the tests may fail because OPA is
still loading the rules.
This PR allows to wait for OPA during the test suite startup.
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.

3 participants