Skip to content

[Kubernetes] Coerce Python bool tolerations to lowercase for taint match#9756

Merged
rohansonecha merged 1 commit into
skypilot-org:masterfrom
rohansonecha:sky-4496-bool-toleration-lowercase
May 29, 2026
Merged

[Kubernetes] Coerce Python bool tolerations to lowercase for taint match#9756
rohansonecha merged 1 commit into
skypilot-org:masterfrom
rohansonecha:sky-4496-bool-toleration-lowercase

Conversation

@rohansonecha

@rohansonecha rohansonecha commented May 29, 2026

Copy link
Copy Markdown
Collaborator

Summary

Follow-up to #9750. _str_or_empty (used by taint_is_tolerated to
compare toleration values against K8s taint values) coerces non-string
scalars to string so unquoted YAML values like value: 0 and
value: 1.5 still match real K8s taints. But for Python bool,
str(True) produces the Python-cased 'True' — while K8s stores
taint values as whatever Go's YAML serializer emits, which is always
lowercase ('true' / 'false'). Even kubectl taint nodes X foo=true:NoSchedule lands on the node as value: "true".

That silent mismatch means a config

tolerations:
  - key: foo
    operator: Equal
    value: true   # unquoted YAML → Python True → 'True'
    effect: NoSchedule

never matches a real K8s taint with value: "true" — the operator
sees 'True' != 'true' and the taint reads un-tolerated. The user has
to remember to quote the value (value: "true") to work around it.

Special-case bool inside _str_or_empty to lowercase. Mirrors what
Go's strconv.FormatBool / fmt.Sprintf("%t", ...) produce, so the
matcher stays in lockstep with how K8s actually stores taint values.

Test plan

  • Updates the yaml-bool / yaml-bool-false parametrize cases in
    test_taint_is_tolerated_str_coercion to use 'true' / 'false'
    taint values (the realistic K8s shape).
  • Adds two python-cased-bool-no-match cases asserting the coercion
    is one-directional / strict: a hypothetical taint stored as
    Python-cased 'True' must NOT match a Python-bool toleration. We
    fix the toleration side to match K8s reality, not loosen the
    comparison to be case-insensitive.
pytest tests/unit_tests/kubernetes/test_kubernetes_utils.py -k taint_is_tolerated_str_coercion

🤖 Generated with Claude Code


Open in Devin Review

`_str_or_empty` already coerces non-string toleration / taint values to
string so YAML-parsed scalars (`value: 0`, `value: 1.5`, `value: true`)
match against the K8s API's always-string taint fields. But `str(True)`
yields the Python-cased `'True'`, while K8s stores taint values as
whatever Go's YAML serializer emits — always lowercase (`'true'` /
`'false'`). A `kubectl taint nodes X foo=true:NoSchedule` ends up with
`value: "true"` on the node.

That silent mismatch means a config

    tolerations:
      - key: foo
        operator: Equal
        value: true       # unquoted YAML → Python True → 'True'
        effect: NoSchedule

would never match a real K8s taint with `value: "true"` — the operator
sees `'True' != 'true'` and the taint reads un-tolerated. The user has
to quote the value (`value: "true"`) to work around it.

Special-case `bool` inside `_str_or_empty` to lowercase. Mirrors what
Go's `strconv.FormatBool` / `fmt.Sprintf("%t", ...)` produce, so the
matcher stays in lockstep with how K8s actually stores taint values.

Tests:
- Updates `yaml-bool` / `yaml-bool-false` parametrize cases to use
  `'true'` / `'false'` taint values (the realistic K8s shape).
- Adds two `python-cased-bool-no-match` cases to assert strictness:
  a hypothetical taint stored as Python-cased `'True'` / `'False'`
  must NOT match a Python-bool toleration — the lowercase coercion
  is one-directional, not a case-insensitive compare.
@rohansonecha rohansonecha self-assigned this May 29, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request updates the _str_or_empty utility function in sky/provision/kubernetes/utils.py to explicitly convert boolean values to lowercase strings ('true' or 'false'), aligning with how Kubernetes stores taint values. Unit tests in tests/unit_tests/kubernetes/test_kubernetes_utils.py have been updated and expanded to cover these changes and prevent regressions. There are no review comments, and I have no feedback to provide.

@devin-ai-integration devin-ai-integration Bot left a comment

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.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 2 additional findings.

Open in Devin Review

@rohansonecha rohansonecha merged commit 4d87b8d into skypilot-org:master May 29, 2026
22 checks passed
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