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

support for definining cmd arguments #8401

Merged
merged 3 commits into from
Feb 8, 2023
Merged

Conversation

koikonom
Copy link
Contributor

@koikonom koikonom commented Jan 25, 2023

Sometimes we need to pass new command line arguments to Redpanda, in order to enable functionality not necessarily managed by the Cluster spec. This PR adds a new field in the spec that allows us to do just that. For example a cluster spec that would use this feature would look like this:

apiVersion: redpanda.vectorized.io/v1alpha1
kind: Cluster
metadata:
  name: test-cluster
spec:
[... other ...]
  configuration:
    [... other ...]
    additionalCommandlineArguments:
      overprovisioned: ""
      default-log-level: "info"

Backports Required

  • none - not a bug fix
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v22.3.x
  • v22.2.x
  • v22.1.x

Release Notes

Improvements

  • Added a new field called additionalCommandlineArguments under the configuration block that allows additional command line arguments to be passed to the redpanda command line.

@CLAassistant
Copy link

CLAassistant commented Jan 25, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@koikonom koikonom marked this pull request as ready for review January 25, 2023 12:43
@koikonom koikonom requested a review from a team as a code owner January 25, 2023 12:43
Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

I see you need to sign CLA, write at least one unit test that uses additional command line flags and last but not least change the PR cover letter as release notes will not be rendered correctly.

0xdiba
0xdiba previously approved these changes Feb 6, 2023
Copy link
Contributor

@0xdiba 0xdiba left a comment

Choose a reason for hiding this comment

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

LGTM 🚀
Let's wait for @RafalKorepta (or the @redpanda-data/k8s team in general) to give the final 👍

Copy link
Contributor

@RafalKorepta RafalKorepta left a comment

Choose a reason for hiding this comment

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

Could you add one e2e test?

esteban
esteban previously approved these changes Feb 7, 2023
@koikonom koikonom dismissed stale reviews from esteban and 0xdiba via 89d226a February 7, 2023 17:29
@esteban
Copy link
Member

esteban commented Feb 7, 2023

Retrying failed tests.

esteban
esteban previously approved these changes Feb 7, 2023
Comment on lines +1 to +11
---
apiVersion: redpanda.vectorized.io/v1alpha1
kind: Cluster
metadata:
name: cluster
status:
replicas: 1
restarting: false
conditions:
- type: ClusterConfigured
status: "True"
Copy link
Member

Choose a reason for hiding this comment

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

Not in the full scope of this e2e test but it would be better if we could confirm if the redpanda command includes the new flags and not just if the container was launched. I think if we can confirm that it does apply the flags correctly that should be enough for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd expect this to be covered in the rest of the testing suite. See here for example: https://github.com/redpanda-data/redpanda/pull/8401/files#diff-0cfeba1779bdecfec8476b8602a73be2ab7de5892aec5463b2866bc66f4b42b2R805

Comment on lines +1 to +11
---
apiVersion: redpanda.vectorized.io/v1alpha1
kind: Cluster
metadata:
name: cluster
status:
replicas: 1
restarting: false
conditions:
- type: ClusterConfigured
status: "True"
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be moved to 00-assert.yaml as we need to wait for cluster to report configured condition.

"--default-log-level=debug",
)
args["overprovisioned"] = ""
args["kernel-page-cache"] = "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

Because of this change linter stated to complain:

pkg/resources/ingress.go:99:68: string `true` has 2 occurrences, make it a constant (goconst)
--
  | r.annotations["nginx.ingress.kubernetes.io/force-ssl-redirect"] = "true"

https://buildkite.com/redpanda/redpanda/builds/22710#01862dcd-a745-4e05-adec-77c7ba803d1e/408-477

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorted

@RafalKorepta RafalKorepta merged commit 7133138 into dev Feb 8, 2023
joejulian pushed a commit to joejulian/redpanda that referenced this pull request Mar 10, 2023
…tional_cmdline_args

support for definining cmd arguments
@koikonom koikonom deleted the additional_cmdline_args branch March 15, 2023 10:31
joejulian pushed a commit to joejulian/redpanda that referenced this pull request Mar 24, 2023
…ditional_cmdline_args

support for definining cmd arguments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants