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

Allow passing a custom elasticsearch backend url to use for log forwarding #152

Merged
merged 1 commit into from
Apr 7, 2021

Conversation

dry923
Copy link
Member

@dry923 dry923 commented Mar 26, 2021

Description

Added the ability for a user to pass an external elasticsearch url to use for a custom log forwarder backend.

Fixes

@@ -28,8 +28,13 @@ fi

function install() {
# create cluster logging and elasticsearch resources
log "Creating cluster logging and elastisearch resources"
envsubst < ./files/logging-stack.yml | oc create -f -
if [[ $DEPLOY_ES == "true" ]]; then
Copy link
Member

Choose a reason for hiding this comment

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

This will skip the logging operator/fluentd setup as well, maybe modify the logging-stack template to selectively install Elasticsearch when DEPLOY_ES is set? Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

As it stands, if you don't set DEPLOY_ES to true or provide a custom es url then the script will pretty much just run the cleanup and then timeout waiting for the logger to come up. We could
a) key off the custom url only and otherwise default to deploying;
b) Allow it to setup a stub cluster logging instance with pretty much no configuration
c) Make the cleanup optional so it could verify that the cluster logging is "up"

I think option c would provide the most flexibility long term but am open to other options as well

Copy link
Member

Choose a reason for hiding this comment

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

The intention of this variable is to have the ability to just setup the logging operator, fluentd stack when ES is not used as the backend and forward the logs to the custom ES right? This conditional will skip the entire logging stack install like you mentioned. We can tweak the spec to conditionally skip the elasticsearch setup when custom ES url is defined: https://github.com/cloud-bulldozer/e2e-benchmarking/blob/master/workloads/logging/files/logging-stack.yml#L82. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

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

@chaitanyaenr I inverted the if on the install to simply be if the custom es url is defined use it otherwise do the install per normal. Does that resolve your concerns?

workloads/logging/deploy_logging_stack.sh Outdated Show resolved Hide resolved
@dry923 dry923 force-pushed the es_backend branch 2 times, most recently from 46e6394 to b5c84af Compare April 6, 2021 14:05
Copy link
Member

@chaitanyaenr chaitanyaenr 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
Copy link
Member

rsevilla87 commented Apr 7, 2021

Seems like with the current implementation some CRDs are not initially available

$ export CUSTOM_ES_URL=blablabla
$ ./deploy_logging_stack.sh 
Wed 7 Apr 10:45:41 UTC 2021: Checking if oc client is installed
NAME      VERSION                             AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.8.0-0.nightly-2021-04-05-174735   True        False         17h     Cluster version is 4.8.0-0.nightly-2021-04-05-174735
Wed 7 Apr 10:45:44 UTC 2021: Deteting openshift-logging/openshift-operators-redhat namespaces if exists
Wed 7 Apr 10:45:51 UTC 2021: Installing the necessary objects for setting up elastic and logging operators and creating a cluster logging instance
Wed 7 Apr 10:45:51 UTC 2021: Creating cluster logging with custom elasticsearch backend
namespace/openshift-operators-redhat created
namespace/openshift-logging created
operatorgroup.operators.coreos.com/openshift-operators-redhat created
operatorgroup.operators.coreos.com/cluster-logging created
subscription.operators.coreos.com/cluster-logging created
unable to recognize "STDIN": no matches for kind "ClusterLogging" in version "logging.openshift.io/v1"
unable to recognize "STDIN": no matches for kind "ClusterLogForwarder" in version "logging.openshift.io/v1"

@dry923
Copy link
Member Author

dry923 commented Apr 7, 2021

Seems like with the current implementation some CRDs are not initially available

$ export CUSTOM_ES_URL=blablabla
$ ./deploy_logging_stack.sh 
Wed 7 Apr 10:45:41 UTC 2021: Checking if oc client is installed
NAME      VERSION                             AVAILABLE   PROGRESSING   SINCE   STATUS
version   4.8.0-0.nightly-2021-04-05-174735   True        False         17h     Cluster version is 4.8.0-0.nightly-2021-04-05-174735
Wed 7 Apr 10:45:44 UTC 2021: Deteting openshift-logging/openshift-operators-redhat namespaces if exists
Wed 7 Apr 10:45:51 UTC 2021: Installing the necessary objects for setting up elastic and logging operators and creating a cluster logging instance
Wed 7 Apr 10:45:51 UTC 2021: Creating cluster logging with custom elasticsearch backend
namespace/openshift-operators-redhat created
namespace/openshift-logging created
operatorgroup.operators.coreos.com/openshift-operators-redhat created
operatorgroup.operators.coreos.com/cluster-logging created
subscription.operators.coreos.com/cluster-logging created
unable to recognize "STDIN": no matches for kind "ClusterLogging" in version "logging.openshift.io/v1"
unable to recognize "STDIN": no matches for kind "ClusterLogForwarder" in version "logging.openshift.io/v1"

@rsevilla87 Yeah I opened an issue about that here: #154

Effectively we're trying to apply the cr to quickly after apply the crd. If you re-run it now it will work without an issue. I was going to look into fixing that outside of this PR as it is pre-existing to this.

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!

@rsevilla87 rsevilla87 merged commit 02279f6 into cloud-bulldozer:master Apr 7, 2021
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.

3 participants