-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Test] Add a feature to start workspaces in separate namespaces #22901
Conversation
@@ -0,0 +1,13 @@ | |||
kind: DevWorkspace |
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.
the name is misleading should probably be simple-with-editor
or the better name would be ephemeral-with-editor
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.
I have added new samples. Review it, please.
@@ -63,6 +77,17 @@ function parseArguments() { | |||
done | |||
} | |||
|
|||
function cleanup() { | |||
echo "Clean up the environment" | |||
kubectl delete dw --all >/dev/null 2>&1 |
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.
isn't it too risky, shoud we explicitly specify namespace that should be used
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.
In this case it will be only current namespace but it will be better specify namespace. Thanks.
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.
Done #22901 (comment)
@SkorikSergey basically, I have concerns about implementation of the dw, dwt, namespace removal. Can we be sure that only those that were created by the script would be removed?
|
@ibuziuk I have updated cleanup func. Now only created test namespaces and dw will be deleted.
|
@SkorikSergey let's make the cleanup process more robust and label all the k8s resources created during the load testing. After that we can use selector for cleanup e.g. oc delete all --selector |
@@ -1,19 +1,20 @@ | |||
# Overview | |||
This script tests how well OpenShift environment can handle running simultaneously many of workspaces. It evaluates the performance of the system under test by checking the average results across all pods and identifying failures that occur during the testing process. | |||
This script tests the performance of an OpenShift environment by running multiple workspaces simultaneously. It evaluates the system under test by checking the average results across all pods and identifying any failures that occur during the testing process. |
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.
is it OpenShift specific, or would it work on any k8s?
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.
I have checked it only on Openshift.
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.
ack, it probably not going to work on vanilla k8s, but it is ok for the time-being
|
||
if [ $start_separately = true ]; then | ||
echo "Delete test namespaces" | ||
kubectl delete namespace --selector=test-type=load-tests >/dev/null 2>&1 || true |
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.
can we add selector definition to the top of the script ?
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.
Ok
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.
@SkorikSergey good job! please, add the badge for starting load test workspace on workspaces.openshift.com (can be done in a separate PR)
What does this PR do?
This PR:
--s
argument (e.g.,./load-test.sh --s
)samples
folder./load-test.sh --help
)What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-5633
How to test this PR?
load-test.sh
script from test/e2e/performance/load-tests. Set number of started workspaces by -c parameter(like./load-test.sh -c 5
)./load-test.sh -l https://gist.githubusercontent.com/SkorikSergey/2eb152db36ed27a0736e0b367ee7f435/raw/dffdb84aaa8336977d9a1f25afe7749bb5c06e9a/simple-devworskpace.yaml
)Logs
PR Checklist
As the author of this Pull Request I made sure that:
What issues does this PR fix or reference
andHow to test this PR
completedReviewers
Reviewers, please comment how you tested the PR when approving it.