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

Fix UHS performance-tests target and JS test code #329

Merged
merged 13 commits into from
Nov 22, 2024

Conversation

richscott
Copy link
Collaborator

@richscott richscott commented Nov 7, 2024

  • Fix UHS container instantiation via helm
  • Change 'disown' shell usage for detaching UHS process to use 'nohup' wrapper prefix instead - 'disown' is available in full Bash, but not minimal /bin/sh versions.
  • Fix JS test code to use UHS-style /api/v1/... API prefix, and use correct status_text for parsing response.
  • Use recent yunikorn-core (GR fork) image version - this is fixed for now, but we will change it to latest once that tag is published for yk-core.

Fixes #309

- Fix UHS container instantiation via helm

- Change 'disown' shell usage for detaching UHS process to use 'nohup'
wrapper prefix instead - 'disown' is available in full Bash, but not
minimal /bin/sh versions.

- Remove use of 'UHS_TEST' as cluster-name - it does not appropriate
to propagate correctly through the several funcion sub-shells we
invoke for cluster creation, kind image loading, etc. Since we
are creating the cluster and tearing it down after tests run,
we should be safe with the default 'UHS' cluster name.

- Fix JS test code to use UHS-style /api/v1/... API prefix, and
use correct status_text for parsing response.

Fixes G-Research#309
@richscott richscott self-assigned this Nov 7, 2024
- Fix UHS container instantiation via helm

- Change 'disown' shell usage for detaching UHS process to use 'nohup'
wrapper prefix instead - 'disown' is available in full Bash, but not
minimal /bin/sh versions.

- Remove use of 'UHS_TEST' as cluster-name - it does not appropriate
to propagate correctly through the several funcion sub-shells we
invoke for cluster creation, kind image loading, etc. Since we
are creating the cluster and tearing it down after tests run,
we should be safe with the default 'UHS' cluster name.

- Fix JS test code to use UHS-style /api/v1/... API prefix, and
use correct status_text for parsing response.

Fixes G-Research#309
…corn-history-server into rich/performance-tests-fix
Copy link
Collaborator

@nikola-jokic nikola-jokic left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Collaborator

@nikola-jokic nikola-jokic left a comment

Choose a reason for hiding this comment

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

Wait, integration tests are failing, can you please fix that

Makefile Outdated
@@ -208,7 +208,7 @@ define start-cluster
@echo "**********************************"
@echo "Creating cluster"
@echo "**********************************"
@CLUSTER_NAME=uhs-test $(MAKE) create-cluster
$(MAKE) create-cluster
Copy link
Collaborator

@sudiptob2 sudiptob2 Nov 8, 2024

Choose a reason for hiding this comment

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

Creating a separate cluster for test seemed to be a good idea. Sometimes I keep Yunikorn running locally in a cluster and at the same time, I also want to run integration/perf test.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay - reverted back in commit 70d0db9

@richscott
Copy link
Collaborator Author

Wait, integration tests are failing, can you please fix that

All tests finally work now! Please re-review.

@@ -97,6 +97,10 @@ KIND_VERSION ?= latest
MINIKUBE ?= $(LOCALBIN_TOOLING)/minikube
MINIKUBE_VERSION ?= latest

# The release version of Yunikorn images
# used in integration and performance tests.
YK_VERSION=fab384e
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just have a small question.
So, we have to change this version every time we release a YK image.
Why don't we release with the latest tag?

Copy link
Contributor

Choose a reason for hiding this comment

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

I always wonder that!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We will change this to latest after we unleash our first v1.6.0-1-gr0 (or whatever) image release, and @pavlovic-ivan can also make a latest tag alias to that.

@richscott richscott merged commit 548d747 into G-Research:main Nov 22, 2024
9 checks passed
@richscott richscott deleted the rich/performance-tests-fix branch November 22, 2024 19:19
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.

performance-tests Make target is broken
4 participants