resmgr: better expression validation, cleaner key resolution. #256
Conversation
klihub
force-pushed
the
fixes/prefixed-label-keys-in-expressions
branch
from
3666878
to
8d1494d
Compare
|
Let me take another look at this... |
klihub
force-pushed
the
fixes/prefixed-label-keys-in-expressions
branch
2 times, most recently
from
b940530
to
26a18e1
Compare
klihub
changed the title
|
@askervin Updated with a few additional cleanups. |
klihub
force-pushed
the
fixes/prefixed-label-keys-in-expressions
branch
3 times, most recently
from
11dae45
to
d94dcbc
Compare
|
I'd suggest to make the first commit a separate PR to make this cleaner to review. WDYT? |
|
...BC I'm a bit struggling to comprehend the change 😅 Is this part of the config api or not? Why not put this under |
klihub
force-pushed
the
fixes/prefixed-label-keys-in-expressions
branch
2 times, most recently
from
95874cb
to
c3be4c4
Compare
No, it's not part of the config API. It is used for other things, too. Now it is also used for configuration. |
Yeah, sorry, I just noticed that after posting my comment... |
"too" -> so is it part of the config API or not? 😅 I think if it's used in the config API it should be considered part of that (as that's our most obvious and best-defined user-facing API). I'm not sure if this deserves a separate top-level api if there's no self-contained implementation of it 🤔 |
It has always been a top-level API, but Jukka moved it to a slightly incorrect package path. The moving PR now fixes that. |
One way to look at it is like this: if I tomorrow nuke the full config API and switch to running with hard-coded or automatically created configuration, that API is still external and still needed. Affinity and anti-affinity expressions in the topology-aware policy rely on it. That is where it came from. And it is still a user-facing API: pods and containers can be annotated with it.
If this is considered a show-stopper, of course I can move it... I'm just not keen on it because 1) this is very close to the original idea and how it was in CRI Resource Manager, 2) just because something is used in/referenced by our config API does not mean that it must be defined there, especially if it predates our config API, and 3) moving to a different location totally screws me with the rest of the commits above in the stack. |
That's true
I don't think it's a show-stopper. I'm just trying to understand/figure out the principles how/what/where the api definitions should be maintained.
We don't need to do what CRI-RM did 😅 The question is was pondering is really where should the API definitions be maintained. Is there a difference with CRD, gRPC, annotation etc APIs. Looking at this, I think it is probably a good principle for us to have all user-facing APIs under
That is true. But if this was included in the config API I would include it there as in practice it would've become part of it (both maintained in the same project). Also one reason being that it would make it clear that changing/updating it would essentially change the config API, too. But, now looking at it I can see that it's NOT included in the config API so I don't think it belongs there. So my earlier question/comment on this is resolved. |
Yes, I totally agree with that. I shouldn't have brought up CRI-RM at all.
I think your question and pondering was valid and I am glad that you voiced your concern. If nothing else, it forced me to think about it and try to articulate it and then try to decide if what I say really looks like a valid justification or if it is just an excuse... Basically I think there are two reasonable ways to organize the externally visible APIs. 1) either blow them apart and put each logically self-contained bit together/under the component to where it the most closely belongs to, or 2) use a single rooted subtree under which all of them are put by some logic. IMO, we explicitly chose to go with 2, when we reworked the configuration. But the expression API was left untouched and it was according to 1 above.
That is still a valid comment/concern albeit a more genera one. Maybe one way to address that would be to make the |
klihub
force-pushed
the
fixes/prefixed-label-keys-in-expressions
branch
from
c3be4c4
to
691ea47
Compare
Simplify/clean up key reference resolution in expressions, fixing resolution to allow label keys with '/'. This should allow using domain-prefixed label keys in affinity expressions. Signed-off-by: Krisztian Litkey <[email protected]>
klihub
force-pushed
the
fixes/prefixed-label-keys-in-expressions
branch
from
691ea47
to
e3f6afe
Compare
Notes: this PR is stacked on top of
Simplify/clean up key reference resolution in expressions, fixing resolution to allow label keys with '/'. This should allow using domain-prefixed label keys in affinity expressions.