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

Move resolution of developerMode and additionalScyllaDBArguments to Operator #2137

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

zimnx
Copy link
Collaborator

@zimnx zimnx commented Oct 1, 2024

Moves the resolution of developerMode and additionalScyllaDBArguments from the sidecar to the Operator.
This change addresses an issue where modifications to these arguments did not trigger a rolling restart of the ScyllaDB cluster.
By moving the logic to the Operator, any changes to these fields now correctly trigger a rolling restart as intended.

Since the sidecar no longer needs to fetch the ScyllaCluster resource for these parameters, it no longer requires permissions to perform GET operations on ScyllaCluster objects.
However, to ensure a safe rolling upgrade of the Operator, these permissions are not removed yet.

Additionally, this commit removes support for the deprecated cpuset field. It is now treated as if it is always set to true regardless of its value.

Requires:

Fixes #1389

@zimnx zimnx added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Oct 1, 2024
@scylla-operator-bot scylla-operator-bot bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 1, 2024
@scylla-operator-bot scylla-operator-bot bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 1, 2024
@zimnx zimnx force-pushed the sidecar-parameters branch from a4eae74 to cf5b5bd Compare October 1, 2024 15:58
@tnozicka
Copy link
Contributor

tnozicka commented Oct 2, 2024

Additionally, this commit removes support for the deprecated cpuset field. It is now treated as if it is always set to true regardless of its value.

This should go in a dedicated PR, especially for visibility.

@zimnx zimnx force-pushed the sidecar-parameters branch from cf5b5bd to 352c0e4 Compare October 2, 2024 10:25
@zimnx zimnx force-pushed the sidecar-parameters branch from 352c0e4 to f4f707b Compare October 2, 2024 13:47
@zimnx zimnx changed the title Move resolution of developerMode and additionalScyllaDBArguments to Operator [WIP] Move resolution of developerMode and additionalScyllaDBArguments to Operator Oct 2, 2024
@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2024
@zimnx zimnx force-pushed the sidecar-parameters branch from f4f707b to 2b93f00 Compare October 2, 2024 14:06
@zimnx zimnx force-pushed the sidecar-parameters branch from 2b93f00 to 3c1b469 Compare October 2, 2024 14:11
@zimnx
Copy link
Collaborator Author

zimnx commented Oct 2, 2024

This should go in a dedicated PR, especially for visibility.

extracted to separate PR #2139

@zimnx zimnx changed the title [WIP] Move resolution of developerMode and additionalScyllaDBArguments to Operator Move resolution of developerMode and additionalScyllaDBArguments to Operator Oct 2, 2024
@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 2, 2024
@zimnx zimnx requested review from tnozicka and rzetelskik October 2, 2024 14:38
@zimnx
Copy link
Collaborator Author

zimnx commented Oct 2, 2024

/hold for prereq #2139

@scylla-operator-bot scylla-operator-bot bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 2, 2024
@zimnx zimnx force-pushed the sidecar-parameters branch from 3c1b469 to 858d3ba Compare October 7, 2024 09:35
@scylla-operator-bot scylla-operator-bot bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2024
@zimnx
Copy link
Collaborator Author

zimnx commented Oct 7, 2024

#2139 landed
/hold cancel

@scylla-operator-bot scylla-operator-bot bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 7, 2024
@zimnx zimnx force-pushed the sidecar-parameters branch from 858d3ba to a31126b Compare October 7, 2024 09:37
@scylla-operator-bot scylla-operator-bot bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 7, 2024
@zimnx zimnx force-pushed the sidecar-parameters branch 2 times, most recently from a578a31 to 281d62b Compare October 7, 2024 11:06
@zimnx
Copy link
Collaborator Author

zimnx commented Oct 8, 2024

@rzetelskik @tnozicka ptal

@zimnx zimnx force-pushed the sidecar-parameters branch 3 times, most recently from 9a9a07b to adb86f2 Compare October 8, 2024 14:24
Copy link
Member

@rzetelskik rzetelskik left a comment

Choose a reason for hiding this comment

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

lgtm
/assign tnozicka

@zimnx zimnx force-pushed the sidecar-parameters branch from adb86f2 to 85f60ec Compare October 9, 2024 11:58
zimnx added 2 commits October 9, 2024 17:06
…to Operator

Moves the resolution of developerMode and additionalScyllaDBArguments from the sidecar to the Operator.
This change addresses an issue where modifications to these arguments did not trigger a rolling restart of the ScyllaDB cluster.
By moving the logic to the Operator, any changes to these fields now correctly trigger a rolling restart as intended.

Since the sidecar no longer needs to fetch the ScyllaCluster resource for these parameters, it no longer requires permissions to perform GET operations on ScyllaCluster objects.
However, to ensure a safe rolling upgrade of the Operator, these permissions are not removed yet.
@zimnx zimnx force-pushed the sidecar-parameters branch from 85f60ec to b55a97a Compare October 9, 2024 15:06
Copy link
Contributor

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

thanks for the updates
overall lgtm, but 2 nits would simplify branching and a testing matrix

@zimnx zimnx requested a review from tnozicka October 10, 2024 14:12
@tnozicka
Copy link
Contributor

/approve
/lgtm

@scylla-operator-bot scylla-operator-bot bot added the lgtm Indicates that a PR is ready to be merged. label Oct 11, 2024
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rzetelskik, tnozicka, zimnx

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@scylla-operator-bot scylla-operator-bot bot merged commit 6e96cd1 into scylladb:master Oct 11, 2024
12 checks passed
@zimnx zimnx deleted the sidecar-parameters branch October 11, 2024 08:01
zimnx added a commit to zimnx/scylla-operator that referenced this pull request Dec 4, 2024
ScyllaDB contianer image enables developerMode if it's not explicitly disabled.
It's the opposite logic to what we had in the past, which was changed in scylladb#2137 by mistake.
This restores previous logic of explicitly disabling developerMode unless it's enabled in the API object.
zimnx added a commit to zimnx/scylla-operator that referenced this pull request Dec 5, 2024
ScyllaDB contianer image enables developerMode if it's not explicitly disabled.
It's the opposite logic to what we had in the past, which was changed in scylladb#2137 by mistake.
This restores previous logic of explicitly disabling developerMode unless it's enabled in the API object.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Changes to some of the config values do not trigger a rolling restart
3 participants