✨ Introduce some control over controller verbosity #2041
Conversation
kcp-ci-bot
added
dco-signoff: yes
MikeSpreitzer
force-pushed
the
control-verbosity
branch
from
20924bb
to
1e644d0
Compare
|
/cc @nirrozenbaum |
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| This test is an executable variant of the "multi-cluster workload deployment with kubectl" scenario in [the examples doc](../../../docs/content/direct/examples.md). This test has the same prerequisites as the cited one. This test can test either (a) the local copy of the repo or (b) the release identified in the kubestellar PostCreateHook (which will be the last release created, regardless of quality, except for that brief time when it identifies the release about to be made). Testing the local copy is the default behavior; to test the release identified in the PostCreateHook, pass `--released` on the command line of `run-test.sh`. | |||
|
|
|||
| The kubestellar controller-manager will be invoked with `-v=2` unless otherwise specified on the command line with `--kubestellar-controller-manager-verbosity $number`. This verbosity can not be set to a value other than 2 when using `--released`. | |||
|
|
|||
| The transport controller will be invoked with `-v=4` unless othewise specified on the command line with `--transport-controller-verbosity $number`. | |||
There was a problem hiding this comment.
Seems like a very high level. Developers usually push a lot of printouts into V4 (@nirrozenbaum ? )
There was a problem hiding this comment.
The defaults are the pre-existing behavior.
There was a problem hiding this comment.
I know, this is why I pointed to @nirrozenbaum I thought 4 is too high for our release default.
There was a problem hiding this comment.
@MikeSpreitzer if level 4 seems like a reasonable output , I'm OK with it.
| runAsNonRoot: true | ||
| serviceAccountName: kubestellar-controller-manager | ||
| terminationGracePeriodSeconds: 10 | ||
| This file is overwritten by `make chart` (whose results are not kept in Git) and ignored by `make local-chart`. |
There was a problem hiding this comment.
So why not deleting the file ?
There was a problem hiding this comment.
I considered that, and thought that leaving an explanation in place would be more helpful.
There was a problem hiding this comment.
This seems strange. It's as if we will leave such a place holder for all our generated files that are not in git. I don't see the value in this
There was a problem hiding this comment.
I think that this one is a fairly special case. First, note that currently this file is under Git control, so that means that this is a reasonable thing to expect. More importantly, it is part of a suite of files --- the ones that make up the Helm chart definition. The rest of the suite is in the Git-controlled contents, and the reader will would reasonably wonder about it if this were not here. Finally, note that this is not the only Git-controlled place where we have a note saying that it gets replaced.
There was a problem hiding this comment.
I still strongly believe we should just remove it and instead put the comment that the file is generate in the docs and/or in the helm definition.
However, I will not block the PR because of this. I leave it up to you ...
There was a problem hiding this comment.
@ezrasilvera: how would we put this comment "in the Helm definition"? This file is part of the Helm definition.
MikeSpreitzer
force-pushed
the
control-verbosity
branch
from
1e644d0
to
d512749
Compare
kcp-ci-bot
added
the
needs-rebase
Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>
MikeSpreitzer
force-pushed
the
control-verbosity
branch
from
d512749
to
8c5ec11
Compare
kcp-ci-bot
removed
the
needs-rebase
|
We can delete the file in the future once we agree how to document it |
|
LGTM label has been added. Git tree hash: 67fefb768dcb0efed1651f69696a3bc0426aa0d1
|
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MikeSpreitzer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kcp-ci-bot
added
the
approved
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
✨ Introduce some control over controller verbosity Signed-off-by: Effi Ofer <effi.ofer@gmail.com>
Summary
This PR introduces some control over the logging verbosity of the kubestellar controller-manager and the transport controller. The controllers themselves already had command line flags for this, but the higher level constructs passed constant values. This PR generalizes the kubestellar controller-manager Helm chart so that it can be told what verbosity to pass in. This PR generalizes the scripts/deploy-transport-controller.sh script. This PR also generalizes the test/e2e/common/setup-kubestellar.sh and test/e2e/multi-cluster-deployment/run-test.sh scripts.
This PR does NOT change the kubestellar PostCreateHook, even though PostCreateHooks can have user-defined parameters in release 0.5.0 and later of KubeFlex. One reason is that KubeStellar currently only requires release 0.4.2 of KubeFlex. The other reason is that a PostCreateHook parameter can not have a default value; every usage of the PCH has to specify the value to use for that parameter.
Related issue(s)
This addresses part of #2042