Move overriding Scylla Operator's log level to CI scripts and make it configurable #2297
Conversation
|
@rzetelskik: GitHub didn't allow me to request PR reviews from the following users: rzetelskik. Note that only scylladb members and repo collaborators can review this PR, and authors cannot review their own PRs. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
scylla-operator-bot
bot
added
do-not-merge/work-in-progress
rzetelskik
force-pushed
the
operator-loglevel-ci-script
branch
from
674ff94 to
4570486
Compare
scylla-operator-bot
bot
added
size/M
rzetelskik
force-pushed
the
operator-loglevel-ci-script
branch
4 times, most recently
from
6e104a8 to
7e42f88
Compare
scylla-operator-bot
bot
added
size/L
rzetelskik
changed the title
rzetelskik
changed the title
rzetelskik
force-pushed
the
operator-loglevel-ci-script
branch
2 times, most recently
from
4f970b5 to
67740d8
Compare
rzetelskik
force-pushed
the
operator-loglevel-ci-script
branch
6 times, most recently
from
76ad542 to
235da58
Compare
|
/priority important-soon |
scylla-operator-bot
bot
added
priority/important-soon
rzetelskik
changed the title
scylla-operator-bot
bot
removed
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Assuming that the in place editing pattern for the operator & manager kustomizations is already present in these scripts (as opposed to creating a dependent kustomization), I agree that following the established way of doing things is the right decision (whilst a rewrite to a declarative approach is strongly preferable for maintainability reasons).
hack/.ci/run-e2e-gke-release.sh
Outdated
| SO_SCYLLA_OPERATOR_LOGLEVEL="${SO_SCYLLA_OPERATOR_LOGLEVEL:-4}" | ||
| export SO_SCYLLA_OPERATOR_LOGLEVEL | ||
|
|
There was a problem hiding this comment.
What do you think about creating a _run-e2e-common.inc.sh file with some of the bash boilerplate? (to get rid of the "repeated constants" code smell)
that would then be included using something like
source "$( dirname "${BASH_SOURCE[0]}" )/_run-e2e-common.inc.sh"There was a problem hiding this comment.
The idea behind separate scripts was to make the config platform-dependent and allow for configuring it in the CI jobs. The values here are meant as platform-dependent defaults, but as you can see, most of them repeat across different platforms.
This particular one could be embedded in ci-deploy.sh and ci-deploy-release.sh scripts, but it won't hold for some other examples (e.g. resources, as in #2276, because it will prevent us from using it locally and/or on less powerful setups).
I think having the shared sourced script could clean things up a bit, but I'm wondering if it's not going to obfuscate where the values are coming from in the end (given they could then come from the CI job, the shared script, or the e2e script)?
I'll make it your call. 😉
There was a problem hiding this comment.
The option of embedding the default log level in ci-deploy.sh and ci-deploy-release.sh directly (and inlining the loglevel patch in the kustomization yaml) looks most promising to me (because this config is unlikely to be environment-dependent and adding excessive logic here doesn't add clear benefit)
Let's do it (that is: drop the conditional, embed the loglevel patch directly in the kustomization) if you think the same way. Structuring the config in a streamlined way would require a fundamental restructuring of the deploy machinery - something we won't do in this PR.
I have a couple of similar PRs in queue, so maybe it's worth considering. I'm not sure how would you see this done though? |
I have started looking and it was more complex than I managed to produce a complete recipe for in a timeboxed effort. But basically the idea is that:
I am more than happy to try explore this tomorrow, or pair program. Kustomize has one important limitation that it doesn't like accepting external context that we'd need to work around somehow. |
I though about it before but I figured it wouldn't make that much of a difference since the patches would still have to be conditionally modified and concatenated to the kustomize file. It might make things more understandable, but I don't think it will change the flow in these scripts too much, so it seems mostly orthogonal to this PR. |
rzetelskik
force-pushed
the
operator-loglevel-ci-script
branch
from
235da58 to
721987b
Compare
rzetelskik
changed the title
scylla-operator-bot
bot
added
the
do-not-merge/work-in-progress
rzetelskik
force-pushed
the
operator-loglevel-ci-script
branch
2 times, most recently
from
cc4e7d3 to
ce2750c
Compare
scylla-operator-bot
bot
added
size/M
rzetelskik
force-pushed
the
operator-loglevel-ci-script
branch
from
ce2750c to
ecb7300
Compare
rzetelskik
force-pushed
the
operator-loglevel-ci-script
branch
2 times, most recently
from
0228d81 to
e981aa9
Compare
rzetelskik
changed the title
scylla-operator-bot
bot
removed
the
do-not-merge/work-in-progress
|
@mflendrich I believe I addressed all of your comments - let me know if we're on the same page now |
scylla-operator-bot
bot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mflendrich, rzetelskik 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 |
scylla-operator-bot
bot
added
the
approved
#2304 (comment) |
Description of your changes: Currently, the Operator's and manager controller's loglevels are patched after they have already been deployed, which causes a rolling restart. It prevents us from giving operator guaranteed QoS in CI with resources >= half of what's available because PDB blocks the restart and everything gets stuck.
This PR moves the adjustment to CI scripts and makes the value configurable through e2e scripts.
Which issue is resolved by this Pull Request:
Resolves #2296
/cc