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

Update logging workload to OLO 5.6 #549

Merged
merged 1 commit into from
Apr 27, 2023

Conversation

morenod
Copy link
Collaborator

@morenod morenod commented Mar 22, 2023

Update code of logging workload to the docs published for 4.12 and OLO 5.6

@morenod morenod requested review from rsevilla87 and dry923 March 22, 2023 14:22
@dry923 dry923 requested a review from mukrishn March 22, 2023 14:28
Storage class to use for the persistent storage. The faster the storage, better the Elasticsearch performance.

### ES_STORAGE_SIZE
Default: `100G`
Each data node in the cluster is bound to a Persistent Volume Claim that requests the size specified using this variable from the cloud storage.

### ES_MEMORY_LIMITS
Default: `16Gi`
Copy link
Member

Choose a reason for hiding this comment

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

It looks like the default request and limit are the same. Do we really even need to set a limit here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

We can keep this variable as long as defaults make sense, which is the case :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Agree, request=limit makes this a guaranteed pod which has implications in the context of preemption - guaranteed pods are preempted last, so there's probably several reasons why we have requests=limit for ES.

Copy link
Member

Choose a reason for hiding this comment

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

I don't disagree that the values may be reasonable, etc. My thinking here was if we are testing the logging stack we shouldn't limit the backend data store as it is just something that we may hit (OOM of ES) which would negatively impact our tests but not be prudent to the results. I.e. if we cause ES to OOM it does NOT indicate a failed test. It just means we need to change those values and re-run. IMHO removing the limit just potentially saves us steps/debugging down the line.

workloads/logging/common.sh Outdated Show resolved Hide resolved
@morenod morenod force-pushed the logging_hypershift branch from f493509 to 7603128 Compare March 22, 2023 14:51
@@ -44,7 +44,7 @@ function cleanup() {
while [[ $( oc get projects | grep -w "openshift-logging\|openshift-operators-redhat") ]]; do
sleep 5
wait_time=$((wait_time+5))
if [[ $wait_time -ge $TIMEOUT ]]; then
if [[ "${wait_time}" -ge "${TIMEOUT}" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

Can we can reduce this whole function into:

cleanup() {
  oc delete --wait=true project openshift-logging openshift-operators-redhat --ignore-not-found --timeout=${TIMEOUT}
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

rewritted cleanup function to use the --wait

@morenod morenod force-pushed the logging_hypershift branch 3 times, most recently from 20178f5 to 3400d08 Compare April 3, 2023 11:21
@morenod morenod requested review from dry923 and rsevilla87 April 3, 2023 11:21
@morenod morenod force-pushed the logging_hypershift branch from 3400d08 to 6f9b6aa Compare April 4, 2023 09:48
@rsevilla87 rsevilla87 self-requested a review April 4, 2023 10:04
Copy link
Member

@rsevilla87 rsevilla87 left a comment

Choose a reason for hiding this comment

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

LGTM!

@morenod morenod force-pushed the logging_hypershift branch from 6f9b6aa to 6410a94 Compare April 4, 2023 10:46
@dry923 dry923 mentioned this pull request Apr 4, 2023
@morenod morenod force-pushed the logging_hypershift branch from 6410a94 to c5443bd Compare April 5, 2023 16:02
Copy link
Member

@krishvoor krishvoor 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

@mukrishn mukrishn left a comment

Choose a reason for hiding this comment

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

LGTM

@rsevilla87 rsevilla87 merged commit 7fab821 into cloud-bulldozer:master Apr 27, 2023
vishnuchalla pushed a commit that referenced this pull request Sep 6, 2023
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.

6 participants