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

control-service: use full url for heartbeat tests and heartbeat tests run in multiple namespaces #2295

Merged
merged 17 commits into from
Jul 25, 2023

Conversation

murphp15
Copy link
Collaborator

@murphp15 murphp15 commented Jun 21, 2023

Why

We want to catch regressions in control plane if they are related to using the wrong namespace.

What

We now use different namespace for deployment and control.

How was this tested?

on a previous commit I enabled heartbeat steps and everything ran well.

@murphp15 murphp15 changed the title Person/murphp15/use full url for heartbeat tests control-service: use full url for heartbeat tests Jun 21, 2023
@antoniivanov
Copy link
Collaborator

antoniivanov commented Jun 21, 2023

Maybe we can use the public one https://iaclqhm5xk.execute-api.us-west-1.amazonaws.com ?

Added benefit some tests can run in the gitlab.com runners (instead of ours) .
I've been wanting to run vdk-hearbeat on windows for example.

The downside may be that if Control Service is re-created (deleted and created) the Load Balanced domain name would change and one need to manually update the API gateway (https://github.com/vmware/versatile-data-kit/wiki/Gitlab-CICD#api-gateway)

@murphp15 murphp15 force-pushed the person/murphp15/use_full_url_for_heartbest_tests branch from 11cab51 to 37a3c15 Compare June 27, 2023 08:36
@murphp15
Copy link
Collaborator Author

Maybe we can use the public one https://iaclqhm5xk.execute-api.us-west-1.amazonaws.com ?

Added benefit some tests can run in the gitlab.com runners (instead of ours) . I've been wanting to run vdk-hearbeat on windows for example.

The downside may be that if Control Service is re-created (deleted and created) the Load Balanced domain name would change and one need to manually update the API gateway (https://github.com/vmware/versatile-data-kit/wiki/Gitlab-CICD#api-gateway)

@tozka I don't agree with using the external url its needless IO on the network and not the pattern we should encourage.

@antoniivanov
Copy link
Collaborator

antoniivanov commented Jun 27, 2023

@tozka I don't agree with using the external url its needless IO on the network and not the pattern we should encourage.

You make a valid point, when it comes to jobs deployed / jobs runnning on the cloud ("cloud jobs").

Let me clarify the difference between local command runs and cloud runs.

  • Local command runs refer to executing jobs or other CLI commands on the local machine of the user, where the vdk is run on the same system where it's stored.
  • On the other hand, cloud runs involve deploying and executing jobs on a cloud-based platform (VDK Kubernetes runtime)

For cloud jobs, indeed, minimizing unnecessary network I/O by avoiding external URLs is a more efficient approach as it reduces unnecessary network traffic and latency and instability.

However vdk-heartbeat is designed to verify local CLI (Command Line Interface) commands and simulate how user uses VDK. For that case, it might be beneficial to use the external URL. The rationale behind this is that these tests simulate the conditions under which end-users would be interacting with our application. Typically, these users would be accessing our services via the external URL. Therefore, using the external URL in tests helps ensure that we're accurately replicating and testing the user experience.

As part of its test vdk-heartbeat also deploys a job and remotely executes it. And also prints the logs of that remote execution. That remote execution likely would use CONTROL_SERVICE_URL that is internal because it's part of the "cloud" but the rest of the vdk-heartbeat which runs commands outside of the cloud should use external address

@antoniivanov antoniivanov marked this pull request as draft July 14, 2023 10:36
@murphp15
Copy link
Collaborator Author

@tozka I am only coming back to this PR now.

I am setting the URL only in ini files which are used internally?
Where in this PR do you want me to use the full external URL?

@murphp15 murphp15 force-pushed the person/murphp15/use_full_url_for_heartbest_tests branch from fe5bb7d to ae21078 Compare July 24, 2023 11:33
@antoniivanov
Copy link
Collaborator

@tozka I am only coming back to this PR now.

I am setting the URL only in ini files which are used internally? Where in this PR do you want me to use the full external URL?

I forgot what was the problem being fixed here? Does it still exists? (perhaps update PR description?)

@murphp15 murphp15 changed the title control-service: use full url for heartbeat tests control-service: use full url for heartbeat tests and heartbeat tests run in multiple namespaces Jul 24, 2023
@murphp15 murphp15 marked this pull request as ready for review July 24, 2023 14:01
@DeltaMichael
Copy link
Contributor

DeltaMichael commented Jul 24, 2023

@murphp15
Copy link
Collaborator Author

@DeltaMichael it won't effect the frontend in anyway because deployments never make requests to the frontend.

@murphp15 murphp15 enabled auto-merge (squash) July 25, 2023 07:26
@murphp15 murphp15 merged commit 05f64f8 into main Jul 25, 2023
@murphp15 murphp15 deleted the person/murphp15/use_full_url_for_heartbest_tests branch July 25, 2023 07:58
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.

5 participants