Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Set envoy log level as error to default in OSM #1679

Merged

Conversation

snehachhabria
Copy link
Contributor

This PR :

  • sets the default value of the log level for the envoy to error when OSM is installed
  • exposes envoy-log-level as an install feature flag so that the demo and cli can run in the debug mode for envoy
  • updates the demo and .env file to have the log level as debug

Please mark with X for applicable areas.

  • New Functionality [ ]
  • Documentation [ ]
  • Install [X]
  • Control Plane [ ]
  • CLI Tool [X]
  • Certificate Management [ ]
  • Networking [ ]
  • Metrics [ ]
  • SMI Policy [ ]
  • Security [ ]
  • Tests / CI System [ ]
  • Other [ ]

Please answer the following questions with yes/no.

  • Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution? no

@snehachhabria snehachhabria requested a review from a team as a code owner September 9, 2020 23:51
@@ -30,6 +30,7 @@ IMAGE_PULL_POLICY="${IMAGE_PULL_POLICY:-Always}"
ENABLE_EGRESS="${ENABLE_EGRESS:-false}"
MESH_CIDR=$(./scripts/get_mesh_cidr.sh)
DEPLOY_WITH_SAME_SA="${DEPLOY_WITH_SAME_SA:-false}"
ENVOY_LOG_LEVEL="${ENVOY_LOG_LEVEL:-debug}"
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor

Choose a reason for hiding this comment

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

Should the default be error?

Copy link
Member

Choose a reason for hiding this comment

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

For the local development runs and the CI no, so that we can override the defaults in the install cli. Otherwise it would make it hard to debug CI failures.
We could set it to error here and override using env variables, but most use cases of the automated demo script are from CI and local development runs.

eduser25
eduser25 previously approved these changes Sep 10, 2020
Copy link
Member

@shashankram shashankram left a comment

Choose a reason for hiding this comment

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

Looks good, some comments inline regarding validation and unit tests.

cmd/cli/install_test.go Show resolved Hide resolved
cmd/cli/install.go Outdated Show resolved Hide resolved
cmd/cli/install.go Outdated Show resolved Hide resolved
cmd/cli/install.go Outdated Show resolved Hide resolved
shashankram
shashankram previously approved these changes Sep 10, 2020
ksubrmnn
ksubrmnn previously approved these changes Sep 10, 2020
@@ -104,6 +104,10 @@ export BOOKWAREHOUSE_NAMESPACE=bookwarehouse
# optional: Whether to deploy traffic split policy or not
# Default: true
# export DEPLOY_TRAFFIC_SPLIT=true

# optional: specify the log level for the enovy's
# Default: debug
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# Default: debug
# Default: error

Copy link
Member

Choose a reason for hiding this comment

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

This is reflecting the default value for the demo script which is debug.

draychev
draychev previously approved these changes Sep 10, 2020
Copy link
Contributor

@draychev draychev left a comment

Choose a reason for hiding this comment

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

This is great!

@snehachhabria snehachhabria merged commit 76147c6 into openservicemesh:main Sep 10, 2020
@snehachhabria snehachhabria deleted the updatingEnvoylogLevel branch October 19, 2020 23:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants