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

Add toleration support to ServiceLB DaemonSet #10687

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

aleskxyz
Copy link
Contributor

@aleskxyz aleskxyz commented Aug 9, 2024

Proposed Changes

This pull request introduces a new feature that allows ServiceLB to handle Kubernetes tolerations defined in a service's annotations. The changes include:

  • Implementation of the getTolerations method, which retrieves and validates tolerations from a service's annotations in JSON or YAML string format.
  • Updates to the newDaemonSet method to append the retrieved tolerations to the DaemonSet's pod tolerations.

These changes enable users to customize tolerations directly through service annotations, providing greater flexibility in pod scheduling.

Types of Changes

  • New Feature

Verification

To verify the changes:

  1. Create a Kubernetes Service with the LoadBalancer type and svccontroller.k3s.cattle.io/tolerations annotation containing valid JSON or YAML formatted tolerations list.
  2. Deploy the service and ensure that the corresponding ServiceLB DaemonSet's pods include the specified tolerations.
  3. Verify that the system handles invalid or malformed tolerations gracefully, emitting appropriate error messages.

yaml example:

apiVersion: v1
kind: Service
metadata:
  annotations:
    svccontroller.k3s.cattle.io/tolerations: |
      - effect: NoSchedule
        key: public
        operator: Exists
      - effect: NoExecute
        key: disk
        operator: Equal
        value: ssd

  name: ingress-nginx-controller
  namespace: ingress-nginx
spec:
  type: LoadBalancer
  ports:
  - name: http
    port: 80
    protocol: TCP
    targetPort: http
  selector:
    app.kubernetes.io/name: ingress-nginx

json example:

apiVersion: v1
kind: Service
metadata:
  annotations:
    svccontroller.k3s.cattle.io/tolerations: |
      [
        {
          "effect": "NoSchedule",
          "key": "public",
          "operator": "Exists"
        },
        {
          "effect": "NoExecute",
          "key": "disk",
          "operator": "Equal",
          "value": "ssd"
        }
      ]

  name: ingress-nginx-controller
  namespace: ingress-nginx
spec:
  type: LoadBalancer
  ports:
  - name: http
    port: 80
    protocol: TCP
    targetPort: http
  selector:
    app.kubernetes.io/name: ingress-nginx

Testing

Linked Issues

This PR addresses issue #1988.

User-Facing Change

- **New Feature**: Users can now define Kubernetes tolerations for ServiceLB DaemonSet directly in the `svccontroller.k3s.cattle.io/tolerations` annotation on services. 

Further Comments

@aleskxyz aleskxyz requested a review from a team as a code owner August 9, 2024 12:33
Copy link
Contributor

@brandond brandond left a comment

Choose a reason for hiding this comment

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

This looks good, thanks! I have one nit, and a question about whether or not there is existing upstream validation code we can reuse.

Can you provide an example of both json and yaml annotation values?

Signed-off-by: Alireza Eskandari <alireza.eskandari@wsd.com>
@aleskxyz
Copy link
Contributor Author

aleskxyz commented Aug 9, 2024

This looks good, thanks! I have one nit, and a question about whether or not there is existing upstream validation code we can reuse.

Can you provide an example of both json and yaml annotation values?

Thanks for review. I have updated the code and remove the redundant check.
I have also updated the description of the PR with example for both yaml and json

Copy link

codecov bot commented Aug 12, 2024

Codecov Report

Attention: Patch coverage is 25.00000% with 18 lines in your changes missing coverage. Please review.

Project coverage is 39.90%. Comparing base (0ee714d) to head (01d4714).
Report is 1 commits behind head on master.

Files Patch % Lines
pkg/cloudprovider/servicelb.go 25.00% 17 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (0ee714d) and HEAD (01d4714). Click for more details.

HEAD has 2 uploads less than BASE
Flag BASE (0ee714d) HEAD (01d4714)
unittests 1 0
e2etests 7 6
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #10687      +/-   ##
==========================================
- Coverage   49.64%   39.90%   -9.75%     
==========================================
  Files         179      162      -17     
  Lines       14955    14333     -622     
==========================================
- Hits         7424     5719    -1705     
- Misses       6168     7437    +1269     
+ Partials     1363     1177     -186     
Flag Coverage Δ
e2etests 35.91% <25.00%> (-10.44%) ⬇️
inttests 36.58% <25.00%> (-0.06%) ⬇️
unittests ?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@brandond brandond merged commit 22fb704 into k3s-io:master Aug 12, 2024
29 checks passed
Copy link

@aghodus aghodus left a comment

Choose a reason for hiding this comment

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

Ios17

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.

4 participants