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

fix(rancher-logging): [4.10] add priorityClassName to DS resources #31

Merged
merged 4 commits into from
Feb 25, 2025

Conversation

jmeza-xyz
Copy link
Contributor

Issue:

rancher/rancher#49048 && SURE-9662

Solution

Added priorityClassName to DS resources for k3s/rke/rk2 only if value is populated in values.yaml

      {{- with .Values.priorityClassName }}
      priorityClassName: {{ . }}
      {{- end }}

Added podPriorityClassName in Logging/FluentBitAgent generic templates with the option to set it in the global or fluentd/fluentbit keys.

    {{- with (default .Values.priorityClassName .Values.fluentd.podPriorityClassName) }}
    podPriorityClassName: {{ . }}
    {{- end }}

    {{- with (default .Values.priorityClassName .Values.fluentd.podPriorityClassName) }}
    podPriorityClassName: {{ . }}
    {{- end }}
# values.yaml addition

fluentbit:
  podPriorityClassName: ""
fluentd:
  podPriorityClassName: ""

QA Testing Considerations

Example values.yaml used for testing. Comment out the podPriorityClassName to test priorityClassName: global-level-test is passed down to all resources.

additionalLoggingSources:
  rke2:
    enabled: true
debug: true
fluentbit:
  podPriorityClassName: resource-level-test
  resources:
    limits:
      memory: 512Mi
    requests:
      cpu: 20m
      memory: 512Mi
fluentd:
  replicas: 1
  podPriorityClassName: resource-level-test
  resources:
    limits:
      memory: 512Mi
    requests:
      cpu: 20m
      memory: 512Mi
priorityClassName: global-level-test
global:
  seLinux:
    enabled: true

Copy link
Member

@mallardduck mallardduck left a comment

Choose a reason for hiding this comment

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

Before your first commit here, I think we should increment the package version. Then make charts, then apply the commits that exist here.

So in order:

  1. Increment Rancher Logging version - in package.yaml increment the -rancher.{num} part
  2. PACKAGE=rancher-logging/4.10 make charts
  3. Cherry pick commit 1 from this,
  4. PACKAGE=rancher-logging/4.10 make charts

(note cannot cherry pick commit 2 from this as mark charts results are different due to step 1 commit)

@mallardduck mallardduck changed the title rfe(rancher-logging): [14.0] add priorityClassName to DS resources fix(rancher-logging): [4.10] add priorityClassName to DS resources Feb 24, 2025
@jmeza-xyz jmeza-xyz force-pushed the rancher-logging-priorityClassName branch from 02ec3ad to d5bfff5 Compare February 24, 2025 23:43
@jmeza-xyz jmeza-xyz force-pushed the rancher-logging-priorityClassName branch from d5bfff5 to 8133fbf Compare February 25, 2025 00:40
@jmeza-xyz
Copy link
Contributor Author

Before your first commit here, I think we should increment the package version. Then make charts, then apply the commits that exist here.

So in order:

1. Increment Rancher Logging version - in package.yaml increment the `-rancher.{num}` part

2. PACKAGE=rancher-logging/4.10 make charts

3. Cherry pick commit 1 from this,

4. PACKAGE=rancher-logging/4.10 make charts

(note cannot cherry pick commit 2 from this as mark charts results are different due to step 1 commit)

Understood, made the changes in sequence as requested. Please let me know if I need to do anything else. Thanks!

@mallardduck mallardduck merged commit b35fb05 into rancher:main Feb 25, 2025
@mallardduck
Copy link
Member

Cool - now that this is merged, you can create a new PR (or update the other one) similar what I mentioned here: rancher/charts#5212 (review)

@jmeza-xyz
Copy link
Contributor Author

Cool - now that this is merged, you can create a new PR (or update the other one) similar what I mentioned here: rancher/charts#5212 (review)

Will do, thanks!

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

Successfully merging this pull request may close these issues.

2 participants