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.

Sign up for GitHub

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

Jump to bottom

Cluster Operator dynamic logging configuration #3328

Merged
merged 16 commits into from
Aug 7, 2020
Merged

Cluster Operator dynamic logging configuration #3328

merged 16 commits into from
Aug 7, 2020

Conversation

sknot-rh
Copy link
Member

Type of change

  • Enhancement / new feature

Description

The CO is deployed as a Deployment so it is not possible just change envvar without rolling update. We need to use an external confingmap (maybe we should allow to configure its name).
Whenever this configmap is updated, the volume is updated and thanks to monitorInterval entry in the log4j2.properties the configuration is reloaded.

Checklist

  • Write tests
  • Make sure all tests pass
  • Update documentation
  • Check RBAC rights for Kubernetes / OpenShift roles
  • Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
  • Reference relevant issue(s) and close them after merging
  • Update CHANGELOG.md
  • Supply screenshots for visual changes, such as Grafana dashboards

@sknot-rh sknot-rh added this to the 0.19.0 milestone Jul 16, 2020
scholzj
scholzj reviewed Jul 16, 2020
View reviewed changes
...-operator/src/main/resources/config-maps/050-ConfigMap-strimzi-cluster-operator-logging.yaml Outdated Show resolved Hide resolved
cluster-operator/src/main/resources/log4j2.properties Outdated Show resolved Hide resolved
...s/helm2/strimzi-kafka-operator/templates/050-ConfigMap-strimzi-cluster-operator-logging.yaml Outdated Show resolved Hide resolved
helm-charts/helm2/Makefile Outdated Show resolved Hide resolved
helm-charts/helm3/Makefile Outdated Show resolved Hide resolved
...s/helm3/strimzi-kafka-operator/templates/050-ConfigMap-strimzi-cluster-operator-logging.yaml Outdated Show resolved Hide resolved
install/cluster-operator/050-ConfigMap-strimzi-cluster-operator-logging.yaml Outdated Show resolved Hide resolved
install/cluster-operator/051-Deployment-strimzi-cluster-operator.yaml Outdated Show resolved Hide resolved
systemtest/src/main/java/io/strimzi/systemtest/resources/operator/BundleResource.java
@@ -20,7 +20,7 @@
public class BundleResource {
private static final Logger LOGGER = LogManager.getLogger(BundleResource.class);

public static final String PATH_TO_CO_CONFIG = "../install/cluster-operator/050-Deployment-strimzi-cluster-operator.yaml";
public static final String PATH_TO_CO_CONFIG = "../install/cluster-operator/060-Deployment-strimzi-cluster-operator.yaml";
Copy link
Member

Choose a reason for hiding this comment

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

@Frawless Will these renamings play well with upgrade tests which use different versions?

Copy link
Member

Choose a reason for hiding this comment

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

Interesting question, in upgrade tests, we change this file only for upgrade to HEAD, for older versions we basically do kubectl apply -f /tmp/upgrade-54556445/strimzi-0.x.y/install/cluster-operator and for HEAD Standa did proper changes from my POV. Let's try to run upgrade tests and we will see.

@Frawless
Copy link
Member

@strimzi-ci run tests profile=upgrade testcase=StrimziUpgradeST#testUpgradeStrimziVersion

samuel-hawker
samuel-hawker reviewed Jul 17, 2020
View reviewed changes
install/cluster-operator/060-Deployment-strimzi-cluster-operator.yaml
@@ -17,6 +17,10 @@ spec:
strimzi.io/kind: cluster-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we switching 50 to 60? Are none of the numbers between 40-50 appropriate for the new CM?

Copy link
Member Author

@sknot-rh sknot-rh Jul 17, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

04x are for the CRDs. The numbers are basically just an aid for listing files. We need to have ConfigMap file before Deployment. Switching Deployment to 060 makes some space to any future ConfigMap. Maybe @scholzj has more comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd also argue 51 would also be appropriate for the CM (keeping deployment as 50) as the CM should not need to be created prior to the deployment due to the dynamic file watcher?

Copy link
Member

Choose a reason for hiding this comment

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

Well, we used different numbers for different types of resources so far. This really just continues.

@strimzi-ci
Copy link

✔️ Test Summary ✔️

TEST_PROFILE: upgrade
EXCLUDED_GROUPS: networkpolicies,flaky
TEST_CASE: StrimziUpgradeST#testUpgradeStrimziVersion
TOTAL: 1
PASS: 1
FAIL: 0
SKIP: 0
BUILD_NUMBER: 1226
BUILD_ENV: oc cluster up

samuel-hawker
samuel-hawker reviewed Jul 17, 2020
View reviewed changes
cluster-operator/scripts/cluster_operator_run.sh

if [ -f /opt/strimzi/custom-config/log4j2.properties ]; then
# if ConfigMap was not mounted and thus this file was not created, use properties file from the classpath
export JAVA_OPTS="${JAVA_OPTS} -Dlog4j2.configurationFile=file:/opt/strimzi/custom-config/log4j2.properties"
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we not instead always deploy the CM and pre-seed the CM with the properties from the properties file?
Easier to predict behaviour if we don't have this if check here, I can imagine us having to suggets users restart operator pod to make the operator run through this logic again.

Copy link
Member Author

Choose a reason for hiding this comment

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

We are deploying it always. @scholzj thinks there may be some edge cases when the CM is not deployed/mounted as a volume. In that case, we should use at least some defaults (what is done by loading log4j2.properties from classpath).

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 echo something to say that a static logging configuration is being used because no config was found, and therefore that dynamic log config update won't work.

scholzj
scholzj reviewed Jul 23, 2020
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

The code looks good to me. But I think we need to have also some docs:

CHANGELOG.md Outdated Show resolved Hide resolved
sknot-rh
sknot-rh commented Jul 23, 2020
View reviewed changes
documentation/modules/ref-operator-cluster.adoc Outdated
@@ -121,6 +118,31 @@ The DNS domain name is added to the Kafka broker certificates used for hostname
+
If you are using a different DNS domain name suffix in your cluster, change the `KUBERNETES_SERVICE_DNS_DOMAIN` environment variable from the default to the one you are using in order to establish a connection with the Kafka brokers.

== Logging configuration
Copy link
Member Author

Choose a reason for hiding this comment

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

@PaulRMellor @laidan6000
Could you please check these changes?

Copy link
Contributor

Choose a reason for hiding this comment

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

@stanlyDoge 👍

scholzj
scholzj reviewed Jul 23, 2020
View reviewed changes
Copy link
Member

@scholzj scholzj left a comment

Choose a reason for hiding this comment

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

Some nits to the docs. But looks mostly good.

documentation/modules/ref-operator-cluster.adoc Outdated Show resolved Hide resolved
documentation/modules/ref-operator-cluster.adoc Outdated Show resolved Hide resolved
@scholzj scholzj requested a review from PaulRMellor July 23, 2020 17:44
@scholzj scholzj modified the milestones: 0.19.0, 0.20.0 Jul 23, 2020
scholzj
scholzj reviewed Jul 23, 2020
View reviewed changes
CHANGELOG.md Outdated
@@ -22,6 +22,7 @@
* enable/disable metrics in the KafkaBridge custom resource
* new Grafana dashboard for the bridge metrics
* Support dynamically changeable logging in the Entity Operator and Kafka Bridge
* Configure Cluster Operator logging using ConfigMap instead of environment variable and support dynamic changes
Copy link
Member

Choose a reason for hiding this comment

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

FYI: Looks like this will not get into 0.19.0, so you might need to move this to 0.20.0.

Copy link
Member

Choose a reason for hiding this comment

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

@stanlyDoge I still see this here in 0.19.0, should you remove it as asked by Jakub?

@sknot-rh sknot-rh requested a review from scholzj July 24, 2020 09:17
sknot-rh added 13 commits August 4, 2020 09:46
@sknot-rh
rename files
71d1741 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
delete file
ba67fbe 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
default conf
29b5ef6 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
commit
4a93f45 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
remove redundant vars
c73aa66 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
Sam's comments
e56fb57 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
consistency
7ecf878 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
comment
7e8be11 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
docu
ad7cdfa 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
docomments
c24a8d6 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
rebase + move to 0.20.0
eff4bd2 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
addressing comments
276ae0a 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh
avoid logging
961c674 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh sknot-rh requested a review from ppatierno August 4, 2020 12:28
ppatierno
ppatierno reviewed Aug 4, 2020
View reviewed changes
install/cluster-operator/060-Deployment-strimzi-cluster-operator.yaml Outdated
@@ -17,6 +17,10 @@ spec:
strimzi.io/kind: cluster-operator
spec:
serviceAccountName: strimzi-cluster-operator
volumes:
- name: co-logging-volume
Copy link
Member

Choose a reason for hiding this comment

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

even in this case the volume could be about co-configuration not just logging?

ppatierno
ppatierno requested changes Aug 4, 2020
View reviewed changes
helm-charts/helm2/strimzi-kafka-operator/values.yaml Outdated
@@ -9,7 +9,8 @@ image:
repository: strimzi
name: operator
tag: latest
logLevel: INFO
logVolume: cluster-operator-logging-volume
Copy link
Member

Choose a reason for hiding this comment

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

should be co-config-volume as for the Helm 3 values YAML.

@sknot-rh
another comment
a89fc7e 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
tombentley
tombentley approved these changes Aug 4, 2020
View reviewed changes
cluster-operator/scripts/cluster_operator_run.sh

if [ -f /opt/strimzi/custom-config/log4j2.properties ]; then
# if ConfigMap was not mounted and thus this file was not created, use properties file from the classpath
export JAVA_OPTS="${JAVA_OPTS} -Dlog4j2.configurationFile=file:/opt/strimzi/custom-config/log4j2.properties"
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 echo something to say that a static logging configuration is being used because no config was found, and therefore that dynamic log config update won't work.

documentation/modules/ref-operator-cluster.adoc Outdated Show resolved Hide resolved
documentation/modules/ref-operator-cluster.adoc Outdated Show resolved Hide resolved
helm-charts/helm2/strimzi-kafka-operator/templates/050-ConfigMap-strimzi-cluster-operator.yaml Outdated Show resolved Hide resolved
@sknot-rh
Tom's comments
69f6a4f 
Signed-off-by: Stanislav Knot <sknot@redhat.com>
@sknot-rh sknot-rh mentioned this pull request Aug 6, 2020
Closed
sknot-rh
sknot-rh commented Aug 7, 2020
View reviewed changes
cluster-operator/scripts/cluster_operator_run.sh
# if ConfigMap was not mounted and thus this file was not created, use properties file from the classpath
export JAVA_OPTS="${JAVA_OPTS} -Dlog4j2.configurationFile=file:/opt/strimzi/custom-config/log4j2.properties"
else
echo "Configuration file log4j2.properties not found. Using default static logging setting. Dynamic updates of logging configuration will not work."
Copy link
Member Author

@sknot-rh sknot-rh Aug 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

@laidan6000 Can you please review this message?

@scholzj scholzj merged commit ee43ba6 into strimzi:master Aug 7, 2020
@im-konge im-konge mentioned this pull request Aug 10, 2020
Merged
2 tasks
@Frawless Frawless mentioned this pull request Aug 18, 2020
Merged
1 task
@sknot-rh sknot-rh deleted the co-dyn-log branch August 26, 2020 06:35
klalafaryan pushed a commit to klalafaryan/strimzi-kafka-operator that referenced this pull request Oct 21, 2020
@sknot-rh @klalafaryan
Cluster Operator dynamic logging configuration (strimzi#3328)
9cffe6b 
* co dyn log

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* rename files

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* delete file

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* default conf

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* commit

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* remove redundant vars

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* Sam's comments

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* consistency

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* comment

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* docu

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* docomments

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* rebase + move to 0.20.0

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* addressing comments

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* avoid logging

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* another comment

Signed-off-by: Stanislav Knot <sknot@redhat.com>

* Tom's comments

Signed-off-by: Stanislav Knot <sknot@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@d-laing d-laing d-laing requested changes

@PaulRMellor PaulRMellor PaulRMellor approved these changes

@ppatierno ppatierno ppatierno approved these changes

@scholzj scholzj scholzj approved these changes

@tombentley tombentley tombentley approved these changes

@Frawless Frawless Awaiting requested review from Frawless

@samuel-hawker samuel-hawker Awaiting requested review from samuel-hawker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.20.0
Development

Successfully merging this pull request may close these issues.
9 participants
@sknot-rh @Frawless @strimzi-ci @tombentley @scholzj @ppatierno @d-laing @samuel-hawker @PaulRMellor