Skip to content

fix(kube): use disable+ so local-storage doesn't clobber servicelb/traefik disable - #6232

Open
naiming-zededa wants to merge 1 commit into
lf-edge:masterfrom
naiming-zededa:naiming-k3s-conf
Open

fix(kube): use disable+ so local-storage doesn't clobber servicelb/traefik disable#6232
naiming-zededa wants to merge 1 commit into
lf-edge:masterfrom
naiming-zededa:naiming-k3s-conf

Conversation

@naiming-zededa

Copy link
Copy Markdown
Contributor

Description

k3s only appends to a "disable" list across config.yaml.d files when
the key uses the "+" suffix; a bare "disable: local-storage" in
03-enc-disable-local-path.yaml was replacing the base config.yaml's
"disable: [servicelb, traefik]" instead of adding to it, silently
re-enabling k3s's built-in servicelb on every fresh cluster-init.
see:
https://docs.k3s.io/installation/configuration#value-merge-behavior

PR dependencies

How to test and validate this PR

with this patch, we should see the svclb and traefik disabled in pods and svc.

Changelog notes

fix(kube): use disable+ so local-storage doesn't clobber servicelb/traefik disable

PR Backports

Checklist

  • I've provided a proper description
  • I've added the proper documentation
  • I've tested my PR on amd64 device
  • I've tested my PR on arm64 device
  • I've written the test verification instructions
  • I've set the proper labels to this PR

For backport PRs (remove it if it's not a backport):

  • I've added a reference link to the original PR
  • PR's title follows the template

And the last but not least:

  • I've checked the boxes above, or I've provided a good reason why I didn't
    check them.

Please, check the boxes above after submitting the PR in interactive mode.

@rene

rene commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

@claude

@claude

claude Bot commented Jul 28, 2026

Copy link
Copy Markdown

Claude finished @rene's task in 56s —— View job


Code Review

  • Read PR diff and changed file
  • Verified base config & k3s merge semantics
  • Checked for other clobbering disable: writes

Verdict: The fix is correct.

Analysis

The base pkg/kube/config.yaml (loaded as /etc/rancher/k3s/config.yaml) sets:

disable:
  - servicelb
  - traefik

The 03-enc-disable-local-path.yaml drop-in lands in /etc/rancher/k3s/config.yaml.d/ (K3S_CONFIG_DIR in pkg/kube/lib/config.sh:15,25). Per k3s value-merge behavior, a bare disable: in a drop-in replaces the base list, so disable: local-storage silently dropped the servicelb/traefik disables — re-enabling k3s's built-in servicelb (and traefik) on every fresh cluster-init. Switching to disable+: local-storage appends instead, yielding [servicelb, traefik, local-storage]. That matches the documented semantics and the stated bug.

I checked the other drop-ins (04-multinode-watch-cache.yaml, longhorn-cfg.yaml, the user-override file, and the cluster-init/utils scripts) — none of them write a disable: list, so there's no remaining clobber path.

Minor note

  • The docs checkbox is unchecked in the PR body. No dedicated doc likely needs updating here, but the inline comment added in the file is a good self-documenting touch.

Nothing blocking — small, well-scoped, correctly-reasoned fix.
· naiming-k3s-conf

disable: local-storage
#
# "+" appends to the base config.yaml's "disable" list (servicelb, traefik)
# instead of replacing it: https://docs.k3s.io/installation/configuration#value-merge-behavior

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this comment is overkill, disable+ is intuitive and it's even pointing to a documentation file, something like appends to the base config.yaml's "disable" list is more than enough....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sure. updated.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey @naiming-zededa :) I thought we wanted to use local-storage for single node eve-k zks mode. Is that no longer the case ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hi @zedi-pramodh , EVE is truely a community driven open-source project :-)
I think it still is, there is a section during making the k3s config file:

    Config_cluster_type_get
    cluster_type=$?
    if [ "$cluster_type" -eq "$CLUSTER_TYPE_UNSPECIFIED" ]; then
            if ! Registration_Applied; then
                   cp "${KUBE_MANIFESTS_SRC_DIR}/${K3S_CONFIG_FILE_DISABLE_LOCAL_PATH}" "${K3S_CONFIG_DIR}/${K3S_CONFIG_FILE_DISABLE_LOCAL_PATH}"
            else
                   rm "${K3S_CONFIG_DIR}/${K3S_CONFIG_FILE_DISABLE_LOCAL_PATH}"
            fi

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@naiming-zededa my question really is why even disable local-storage ? It will be useful for single node eve-k for ZKS and also in ENC mode it can just stay there. I did not exactly remember why we disabled it earlier !! sorry.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zedi-pramodh my guess is that, we used to always disable, but later on (5 months ago), @andrewd-zededa added this code to allow the ZKS use case, Andrew can comment more:

commit 82777e40d3fd3a0fe55c1be07a1882cb3aa9d89f
Author: Andrew Durbin <andrewd@zededa.com>
Date:   Wed Feb 18 16:38:46 2026 -0700

    Enable local-path storage for all cluster types.
    
    Some K3S_BASE mode use cases require local-path storage,
    for simplicity just enable it for all modes.
    
    Single node and CLUSTER_TYPE_REPLICATED_STORAGE continue
    to define all PVCs as longhorn.
    CLUSTER_TYPE_K3S_BASE will now allow for local volumes.
    
    Local-path storage is configured in the k3s config.yaml
    base config file to place all volumes in
    /persist/vault/volumes by default.
    
    Signed-off-by: Andrew Durbin <andrewd@zededa.com>

…aefik disable

 k3s only appends to a "disable" list across config.yaml.d files when
 the key uses the "+" suffix; a bare "disable: local-storage" in
 03-enc-disable-local-path.yaml was replacing the base config.yaml's
 "disable: [servicelb, traefik]" instead of adding to it, silently
 re-enabling k3s's built-in servicelb on every fresh cluster-init.
 see:
 https://docs.k3s.io/installation/configuration#value-merge-behavior

Signed-off-by: naiming-zededa <naiming@zededa.com>
@codecov

codecov Bot commented Jul 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 23.31%. Comparing base (5d69266) to head (79b70ce).
⚠️ Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6232      +/-   ##
==========================================
+ Coverage   22.93%   23.31%   +0.37%     
==========================================
  Files         510      520      +10     
  Lines       93473    95189    +1716     
==========================================
+ Hits        21440    22193     +753     
- Misses      70292    71072     +780     
- Partials     1741     1924     +183     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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.

3 participants