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

add ingress-perf config template to be overwirte #634

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

qiliRedHat
Copy link
Collaborator

@qiliRedHat qiliRedHat commented Oct 11, 2023

Type of change

  • Refactor
  • New feature
  • Bug fix
  • Optimization
  • Documentation Update

Description

Customize the ingress-perf config file, for example for tuningPatch, use different ingress replica numbers, thread numbers.

Related Tickets & Documents

Testing

  • Please describe the System Under Test.
  • Please provide detailed steps to perform tests related to this code change.
    example ENV_VARS:
    CONFIG_TEMPLATE=config/standard-template.yml
    TUNING_PATCH="'{"spec":{"nodePlacement": {"nodeSelector": {"matchLabels": {"node-role.kubernetes.io/infra": ""}}}, "replicas": 3}}'"
    CONNECTIONS=100
    SAMPLES=1
    DURATION=1m
    CONCURRENCY=10
    SERVER_REPLICAS=40
    REQUEST_TIMEOUT=20s
    DELAY=20s
  • How were the fix/results from this change verified? Please provide relevant screenshots or results.

@qiliRedHat qiliRedHat marked this pull request as draft October 11, 2023 14:08
@qiliRedHat
Copy link
Collaborator Author

I want to do two changes for ‘tuningPatch’ for a same test config, e.g.
oc -n openshift-ingress-operator patch ingresscontroller/default --type=merge -p '{"spec":{"tuningOptions": {"threadCount": 8}}}'
oc -n openshift-ingress-operator patch ingresscontroller/default --type=merge -p '{"spec":{"nodePlacement": {"nodeSelector": {"matchLabels": {"node-role.kubernetes.io/infra": ""}}}, "replicas": 3}}'
I failed to put them together in one ‘tuningPatch’ https://github.com/cloud-bulldozer/ingress-perf/blob/main/pkg/runner/tuning.go#L35

@qiliRedHat qiliRedHat marked this pull request as ready for review January 15, 2024 08:01
@qiliRedHat
Copy link
Collaborator Author

@rsevilla87 PTAL, in some of the tests such as more ingress replicas, I hope to be able to customize the parameters in the configuration files and avoid adding new configuration files for each non-standard tests configurations.

@rsevilla87
Copy link
Member

rsevilla87 commented Feb 6, 2024

The change looks good technically, but I wonder, that if what you need new configurations, why don't you add more config files rather than templating the existing...
We're trying to avoid this kind of templates as much as possible as they are difficult to track. Same applies to other tools

@rsevilla87
Copy link
Member

rsevilla87 commented Feb 6, 2024

As an alternative, what do you think about adding new script custom-run.sh, rather than modifying run.sh. That way changes on run.sh won't be able to affect CPT in any way

@qiliRedHat
Copy link
Collaborator Author

The change looks good technically, but I wonder, that if what you need new configurations, why don't you add more config files rather than templating the existing... We're trying to avoid this kind of templates as much as possible as they are difficult to track. Same applies to other tools

@rsevilla87 Thanks for the comment, I'll consider to use new configurations. I can overwrite config as env var so no need to use custom-run.sh.

Please let me know if you have some suggestion for #634 (comment)

Copy link

openshift-ci bot commented Jun 24, 2024

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: qiliRedHat
Once this PR has been reviewed and has the lgtm label, please assign rsevilla87 for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@@ -18,6 +18,24 @@ TOLERANCY=${TOLERANCY:-20}
OS=$(uname -s)
HARDWARE=$(uname -m)

export TUNING_PATCH=${TUNING_PATCH:-"'{\"spec\":{\"nodePlacement\": {\"nodeSelector\": {\"matchLabels\": {\"node-role.kubernetes.io/infra\": \"\"}}}, \"replicas\": 2}}'"}
Copy link
Member

Choose a reason for hiding this comment

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

Rather than modifying run.sh, can you push this changes into a new file custom-run.sh? that way we won't touch any of the current logic in CPT

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have used new configurations to run different replica numbers. I find t is more easier than custom-run.sh for me that I can just provide the new config file as an env var, no need to update the CI to use a new cmd like custom-run.sh. So I think I can close this one.

@rsevilla87 If you have any suggestion to
#634 (comment), it could be helpful. Now to tune the thread number, I do it outside of ingress-perf with oc command before the ingress-perf test.

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.

2 participants