Add new syntax / notation for selecting namespaces and their labels

[Serializator]

Currently this PR is a draft and meant for receiving feedback on the parsing. Once the parsing is reviewed I will continue refactoring the usages of the "old" namespace list option to the new one.

What this PR does / why we need it: This PR extends the existing syntax / notation (BC) to allow for selecting namespaces based on their labels. It does not yet implement the filtering upon those aforementioned labels, this will be a separate PR.

How does this change affect the cardinality of KSM: It does not affect cardinality.

Which issue(s) this PR fixes: n/a

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Serializator To complete the pull request process, please assign tariq1890 after the PR has been reviewed. You can assign the PR to them by writing /assign @tariq1890 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment Approvers can cancel approval by writing /approve cancel in a comment

[Serializator]

I accidentally created this PR as a normal PR. Could someone with write access to the repository change it to a draft please?

[mrueg]

I was always hoping we could profit from upstream logic (e.g. https://github.com/kubernetes/apimachinery/tree/v0.23.1/pkg/labels ) and not invent that parser ourselves. Maybe we could add another flag --namespaces-selectors (or similar) that allows you to use specify conditions on how namespaces get selected?

[Serializator]

I was always hoping we could profit from upstream logic (e.g. https://github.com/kubernetes/apimachinery/tree/v0.23.1/pkg/labels ) and not invent that parser ourselves. Maybe we could add another flag --namespaces-selectors (or similar) that allows you to use specify conditions on how namespaces get selected?

I agree completely in terms of not inventing the parser ourselves. Of course the parser you mentioned is much more extensive and reliable than the one I wrote so I would prefer using that one as well.

I don't think that adding another option (--namespaces-selectors in this case) is the way to go. That would mean there would be 3 options acting upon and deciding what namespaces are either in- or excluded.

• --namespaces
• --namespaces-denylist
• --namespaces-selectors

I would like to take this opportunity to merge --namespaces and --namespaces-denylist together so that there is one --namespaces option with the purpose of either in- or excluding namespaces, rather than 2 / 3 separate options.

Though I understand that this approach poses some BC questions.

Important; I read through the README of the aforementioned repository and there is this part that stood out.

Things you should NOT do

• Add API types to this repo. This is for the machinery, not for the types.
• Directly modify any files under pkg in this repo. Those are driven from k8s.io/kubernetes/staging/src/k8s.io/apimachinery.
• Expect compatibility. This repo is direct support of Kubernetes and the API isn't yet stable enough for API guarantees.

[Serializator]

//cc @fpetkovski what do you think of the above?

[fpetkovski]

If we can use the parser from api-machinery, that would surely reduce the maintenance burden on KSM. I wouldn't worry too much about the compatibility disclaimer though. Changing the label parser would likely be a disruptive change to the entire ecosystem and we can also add tests to make sure we catch breaking changes if they happen.

I agree that introducing --namespaces-selector might be redundant if we can extend --namespaces in a backwards compatible manner. However I wouldn't try to merge --namespaces and --namespaces-denylist yet. We already have this pattern established for different cli args and I would prefer keeping the options separate in order to maintaining consistency.

[Serializator]

I can see what you mean in terms of keeping --namespaces and --namespaces-denylist separate.

I looked through the syntax for parsing labels in API machinery and I do believe we can reuse it but we will still need to do a bit of parsing ourselves as the parser in API machinery is for labels into a selector. In our case the syntax is:

namespace-a[<selector>], namespace-b

This means the parser in API machinery can be used for the <selector> part but we still need to do the parsing of namespace[...] itself. Which is not too bad. [ and ] are not characters allowed within the API machinery so we can safely assume that they either indicate the start or end of a selector (<selector>) for a given namespace.

[fpetkovski]

This looks good to me, we just need to fix the CI before merging. Would you like to continue working on the rest of the implementation in a separate PR?

[Serializator]

Sorry for the late response, I've been quite busy the last couple of weeks / days.

@fpetkovski, I've been thinking about ways to implement this but one bottleneck I came across is the fact that the Kubernetes SDK / API does not allow for getting resources from multiple namespaces at once I believe. To implement this we'll need to get all namespaces and get metrics for each one of those. At the same time these namespaces need to be watched in case a namespace is created or deleted to keep the in-memory namespaces in KSM in-sync.

Do you have an idea of ways to implement this in a more performant (and clean) way?

[fpetkovski]

@Serializator sorry I completely missed your comment.

Yes you're right that filtering by namespace might not be as simple, especially if we want to filter by namespace labels and also update exported metrics when those labels change. The only solution I see is doing this filtering in memory as you pointed out.

I think it's fine to do this as long as we don't worsen the performance of the filtering functionality (only by namespace names).

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[Serializator]

I will try and get some work done on this soon.

/remove-lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Reopen this PR with /reopen
• Mark this PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.