scylladb / scylla-manager

The Scylla Manager
https://manager.docs.scylladb.com/stable/
Other
48 stars 33 forks source link

Add `label` parameter to cluster and tasks #3828

Closed karol-kokoszka closed 2 months ago

karol-kokoszka commented 2 months ago

Fixes https://github.com/scylladb/scylla-manager/issues/3219

cc: @d-helios @zimnx


Please make sure that:

Michal-Leszczynski commented 2 months ago

@karol-kokoszka this PR introduces additional string label for cluster/tasks, but I think that operator team requested it to be a map:

Proposed solution is to add a labels field to the clsuters. User defined key-values which might be useful for filtering, tagging or putting arbitrary values like hash of entire cluster we put in API.

The only thing we're requesting here is enriching the cluster/task structs with user-defined key-value pairs, allowing the users to annotate the objects - akin to https://kubernetes.io/docs/concepts/overview/working-with-objects/annotations/.

karol-kokoszka commented 2 months ago

We won't introduce any map, the field is expected to serve general purpose. DevOps wants to use it as well to add their labels. If operator wants to have the map, then can use JSON.

zimnx commented 2 months ago

What's the problem with having a map for DevOps? Single text fields is not extensible, doesn't allow for advanced querying or filtering. While with maps, users could search for clusters having particular key or set of keys.

rzetelskik commented 2 months ago

We won't introduce any map, the field is expected to serve general purpose. DevOps wants to use it as well to add their labels. If operator wants to have the map, then can use JSON.

They can use a map for a single key, we can't use a label for a map - we'd collide with a user whenever they'd try to amend it.

d-helios commented 2 months ago

What's the problem with having a map for DevOps? Single text fields is not extensible, doesn't allow for advanced querying or filtering. While with maps, users could search for clusters having particular key or set of keys.

just to let you know that map for us is also ok.

karol-kokoszka commented 2 months ago

@zimnx @rzetelskik I don't understand you guys. Here is a comment from the issue that is expected to be closed with this PR https://github.com/scylladb/scylla-manager/issues/3219#issuecomment-2031727386 It's talking about label text field.

Why you cannot use JSON ? Single text field is extensible, you can put whatever you want.

While with maps, users could search for clusters having particular key or set of keys.

There is no functionality in manager to search for clusters using particular key or set of keys basing on this new label. Nothing prevents you from building this kind of functionality on top of what SM returns. Besides, the label field is not part of the key in the backing DB, you need to scan to get the data and just filter.

It's still manager client app that must implement the search.

rzetelskik commented 2 months ago

Here is a comment from the issue that is expected to be closed with this PR https://github.com/scylladb/scylla-manager/issues/3219#issuecomment-2031727386

It's your comment, not ours. We specifically ask for a key-value store there.

Why you cannot use JSON ? Single text field is extensible, you can put whatever you want.

As long as the user doesn't fiddle with the label directly, sure. If they do, we lose all of the metadata and no longer know if this cluster/task should be controlled by the operator. And this is the only use case I care about personally here.

I don't really understand the pushback - it seem like a pretty standard approach? Grafana, prometheus, kubernetes all seem to use maps for labels.

karol-kokoszka commented 2 months ago

As long as the user doesn't fiddle with the label directly, sure. If they do, we lose all of the metadata and no longer know if this cluster/task should be controlled by the operator. And this is the only use case I care about personally here.

Makes sense, but the current implementation (in this PR), just allows to put all or nothing. If there is a map of label fields, than it implies that there is completely new CLI and API in sctool to manage these labels. It's achievable, but it won't be a part of 3.2.8. It's because it's manager who will not only need to update the label field, but just update the diff.

The plan for 3.2.8 is to release it by 6th of May and the release is driven by https://github.com/scylladb/scylla-manager/issues/3767

Guys, if the label as a single text field is no go, then we must postpone it for a couple of weeks.

zimnx commented 2 months ago

It's a no go for us. We can wait for it until next release.