allow lease filtering by exporter labels #168
Conversation
📝 WalkthroughWalkthroughCopies selector matchLabels into Lease.ObjectMeta.Labels during protobuf conversion, adds an implicit "lease-ended DoesNotExist" label requirement to ListLeases when OnlyActive is nil/true, and introduces Python client-side label-selector parsing/filtering plus unit and e2e tests for selector behavior. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/api/v1alpha1/lease_helpers.go (1)
198-203: Make a defensive copy of selector labels to prevent unintended selector mutations.
ObjectMeta.LabelsandSpec.Selector.MatchLabelscurrently point to the same map object. Label mutations elsewhere in the codebase (e.g., inlease_controller.gowhere system labels are added) will also mutate the selector and change its filtering semantics. Make a defensive copy for metadata labels.Proposed fix
+ labelsCopy := make(map[string]string, len(selector.MatchLabels)) + for k, v := range selector.MatchLabels { + labelsCopy[k] = v + } return &Lease{ ObjectMeta: metav1.ObjectMeta{ Namespace: key.Namespace, Name: key.Name, - Labels: selector.MatchLabels, + Labels: labelsCopy, },
|
hmm.. seems this is not enough.. must be another issue.. set it draft for now investigating.. |
| Context("when creating a lease with selector labels", func() { | ||
| It("should copy selector matchLabels to lease metadata labels", func() { | ||
| pbLease := &cpb.Lease{ | ||
| Selector: "board-type=virtual,env=test", |
There was a problem hiding this comment.
Can you also test what happens when you have a selector like
board-type=virtual,!env
board-type=virtual,firmware in (v2, v3)
board-type=virtual,firmware!=v3
all those are valid selectors ... and I believe ... now after seing your patch (sorry, because this was my idea) that we cannot map it directly to labels :-/
There was a problem hiding this comment.
hmm.. yep seems to be a limitation of the current one. the question is whatever we see value in some filtering rather than no filtering, or we better do it now the proper way so we don't have to get back to it later on 🤔
also claude suggests two solutions for this one:
- Store the full selector as a serialized annotation
or - client-side filtering
wonder wdyt? also if you think we must have it support complex filters from day one, should we postpone this PR for if seems like a low priority at the moment, or it's something that looks useful for the 0.8 release? 🤔
side note, I added 4296b66 as filtering wasn't working. tested some basics ones with it and it works now.
There was a problem hiding this comment.
for client-side filtering, we can keep the existing code, to pre-filter by matchLabels, so the server still does the heavy lifting, and then post-process it on the client to remove what doesn't match the complex filter criteria. performance-wise it's better than pure client-side, with the only caveat of having the filtering implementation code scattered across both.
There was a problem hiding this comment.
I think that's a good compromise.
Doing heavy filtering on the server without labels will be expensive, because you need to traverse all, but letting the server do the optimized part, and then the client (eventually, don't spend time on it now) ... do the rest...
👍 for this.
There was a problem hiding this comment.
eventually, don't spend time on it now
haven't seen this part of the comment 😆 so ended up implementing something quickly c13d074
evakhoni
force-pushed
the
lease-labels
branch
from
4296b66 to
bef3e10
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@python/packages/jumpstarter/jumpstarter/client/grpc.py`:
- Around line 315-323: The client-side filter_by_selector function returns a new
LeaseList but preserves the original next_page_token, which can break pagination
after items are removed; update filter_by_selector (in jumpstarter.client.grpc)
to clear the pagination token by returning LeaseList(leases=filtered,
next_page_token=None) when client-side filtering is applied (i.e., when
filter_selector is provided and filtering occurs), and ensure selector_contains
is still used for membership checks so callers know pagination is no longer
valid after filtering.
🧹 Nitpick comments (3)
python/packages/jumpstarter/jumpstarter/client/selectors.py (1)
47-49: Missing handling for unrecognized selector parts.When a selector part doesn't match any of the defined patterns, it's silently ignored. This could lead to unexpected behavior if users provide malformed selectors.
Consider logging a warning or raising an error for unrecognized parts to help users identify typos or unsupported syntax.
💡 Optional improvement
# key (Exists) - bare key without operator elif re.match(r"^[a-zA-Z0-9_./-]+$", part): match_expressions.append((part, "exists", [])) + else: + # Optionally warn about unrecognized selector syntax + import logging + logging.warning(f"Unrecognized selector part ignored: {part}") return match_labels, match_expressionspython/packages/jumpstarter/jumpstarter/client/selectors_test.py (1)
6-35: Test coverage could be expanded for edge cases andparse_label_selector.The tests cover core scenarios well, but consider adding:
Direct tests for
parse_label_selector- Currently onlyselector_containsis tested. Edge cases in parsing (malformed input, whitespace handling, empty values) would benefit from direct coverage.Edge cases missing:
key notin (...)operatorkey!=valueoperator- Multiple expressions combined
- Empty selector vs empty filter
- Whitespace variations in values
💡 Additional test cases
def test_notin_expression(self): assert selector_contains("env notin (dev, staging)", "env notin (dev, staging)") is True def test_not_equal_expression(self): assert selector_contains("board!=jetson", "board!=jetson") is True def test_empty_selector_empty_filter(self): assert selector_contains("", "") is True def test_whitespace_in_values(self): # Verify whitespace handling in comma-separated values assert selector_contains("env in (dev, prod)", "env in (dev,prod)") is TrueAs per coding guidelines: "Test coverage should be comprehensive, focusing on end-to-end testing with server and client interactions."
python/packages/jumpstarter-cli/jumpstarter_cli/get.py (1)
57-57: Redundant filtering: same selector applied both server-side and client-side.The same
selectoris passed to bothlist_leases(filter=selector, ...)(server-side) and.filter_by_selector(selector)(client-side). This means:
- For matchLabels: Filtering happens twice unnecessarily
- For matchExpressions: Server-side filtering may not support them, so client-side is needed
The implementation works correctly but is inefficient for simple label selectors. Consider:
- Only applying client-side filtering when needed (e.g., when matchExpressions are present)
- Or documenting that this double-filtering is intentional as a workaround
Given the PR comment that this "is not fully resolving the problem," this may be an interim solution.
💡 Optional optimization
from jumpstarter.client.selectors import parse_label_selector # Only apply client-side filtering if matchExpressions are present _, match_exprs = parse_label_selector(selector) if selector else (None, []) leases = config.list_leases(filter=selector, only_active=not show_all) if match_exprs: leases = leases.filter_by_selector(selector)
evakhoni
force-pushed
the
lease-labels
branch
from
c13d074 to
1024551
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/packages/jumpstarter/jumpstarter/client/selectors_test.py (1)
9-35: Add test cases for whitespace/invalid segments and additional operators.Consider adding coverage for
!=,notin, bare-keyexists, and whitespace variants likeboard = rpito lock in the expected parsing behavior and prevent regressions.
| # Split by comma, but not inside parentheses | ||
| parts = re.split(r",(?![^()]*\))", selector) | ||
|
|
||
| for part in parts: | ||
| part = part.strip() | ||
| if not part: | ||
| continue | ||
|
|
||
| # key in (v1, v2, ...) | ||
| if m := re.match(r"^([a-zA-Z0-9_./-]+)\s+in\s+\(([^)]*)\)$", part): | ||
| key, values = m.groups() | ||
| match_expressions.append((key, "in", [v.strip() for v in values.split(",")])) | ||
| # key notin (v1, v2, ...) | ||
| elif m := re.match(r"^([a-zA-Z0-9_./-]+)\s+notin\s+\(([^)]*)\)$", part): | ||
| key, values = m.groups() | ||
| match_expressions.append((key, "notin", [v.strip() for v in values.split(",")])) | ||
| # !key (DoesNotExist) | ||
| elif m := re.match(r"^!([a-zA-Z0-9_./-]+)$", part): | ||
| match_expressions.append((m.group(1), "!exists", [])) | ||
| # key!=value | ||
| elif m := re.match(r"^([a-zA-Z0-9_./-]+)!=(.+)$", part): | ||
| key, value = m.groups() | ||
| match_expressions.append((key, "!=", [value])) | ||
| # key=value or key==value | ||
| elif m := re.match(r"^([a-zA-Z0-9_./-]+)==?(.+)$", part): | ||
| key, value = m.groups() | ||
| match_labels[key] = value | ||
| # key (Exists) - bare key without operator | ||
| elif re.match(r"^[a-zA-Z0-9_./-]+$", part): | ||
| match_expressions.append((part, "exists", [])) | ||
|
|
There was a problem hiding this comment.
Prevent invalid/unsupported selector segments from becoming a no‑op filter.
parse_label_selector silently ignores unparsed segments. Combined with selector_contains treating empty requirements as True, inputs like board = rpi (or any unsupported token) can incorrectly match all leases. Please validate segments and either fail fast or treat invalid requirements as non-matching; also tolerate optional whitespace and strip values to avoid accidental invalidation.
💡 Suggested fix (one option)
- # Split by comma, but not inside parentheses
- parts = re.split(r",(?![^()]*\))", selector)
+ # Split by comma, but not inside parentheses
+ parts = re.split(r",(?![^()]*\))", selector)
+ invalid_parts: list[str] = []
for part in parts:
part = part.strip()
if not part:
continue
# key in (v1, v2, ...)
- if m := re.match(r"^([a-zA-Z0-9_./-]+)\s+in\s+\(([^)]*)\)$", part):
+ if m := re.match(r"^([a-zA-Z0-9_./-]+)\s+in\s*\(([^)]*)\)$", part):
key, values = m.groups()
- match_expressions.append((key, "in", [v.strip() for v in values.split(",")]))
+ vals = [v.strip() for v in values.split(",") if v.strip()]
+ match_expressions.append((key, "in", vals))
# key notin (v1, v2, ...)
- elif m := re.match(r"^([a-zA-Z0-9_./-]+)\s+notin\s+\(([^)]*)\)$", part):
+ elif m := re.match(r"^([a-zA-Z0-9_./-]+)\s+notin\s*\(([^)]*)\)$", part):
key, values = m.groups()
- match_expressions.append((key, "notin", [v.strip() for v in values.split(",")]))
+ vals = [v.strip() for v in values.split(",") if v.strip()]
+ match_expressions.append((key, "notin", vals))
# !key (DoesNotExist)
elif m := re.match(r"^!([a-zA-Z0-9_./-]+)$", part):
match_expressions.append((m.group(1), "!exists", []))
# key!=value
- elif m := re.match(r"^([a-zA-Z0-9_./-]+)!=(.+)$", part):
+ elif m := re.match(r"^([a-zA-Z0-9_./-]+)\s*!=\s*(.+)$", part):
key, value = m.groups()
- match_expressions.append((key, "!=", [value]))
+ match_expressions.append((key, "!=", [value.strip()]))
# key=value or key==value
- elif m := re.match(r"^([a-zA-Z0-9_./-]+)==?(.+)$", part):
+ elif m := re.match(r"^([a-zA-Z0-9_./-]+)\s*==?\s*(.+)$", part):
key, value = m.groups()
- match_labels[key] = value
+ match_labels[key] = value.strip()
# key (Exists) - bare key without operator
elif re.match(r"^[a-zA-Z0-9_./-]+$", part):
match_expressions.append((part, "exists", []))
+ else:
+ invalid_parts.append(part)
+
+ if invalid_parts:
+ raise ValueError(f"invalid label selector segment(s): {', '.join(invalid_parts)}")There was a problem hiding this comment.
just checked, on get exporters this works
jmp get exporters -l 'board-type = virtual'
so updated the code to support this as well
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Fetch the diff from the GitHub compare URL to see the changes made
gh api repos/jumpstarter-dev/jumpstarter/compare/102455192a620747e3aca27aad0186328a38069f...8d851b1e258b3edb9c5ab47ceb552b4c52900a06 \
--jq '.files[] | select(.filename == "python/packages/jumpstarter/jumpstarter/client/selectors.py") | .patch'
Length of output: 3198
@evakhoni Great, the whitespace tolerance around operators looks good now! 👍
One concern from my original review remains: the parser still silently skips any segment it doesn't recognize. For example, board-type = virtual, unsupported syntax here would parse only the first part and ignore the second, potentially leading to unintended broad matches. Consider adding an else clause that either raises a ValueError or collects unmatched parts to help catch typos and unsupported syntax early.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
evakhoni
force-pushed
the
lease-labels
branch
from
1024551 to
8d851b1
Compare
2 participants
Mirrors the labels from
spec.selector.matchLabelstometadata.labelsduring lease creation. This allows the controller to filter leases
server-side when using label selectors (e.g.,
jmp get leases -l key=val).Fixes: #36
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.