ytsaurus / ytsaurus-k8s-operator

Kubernetes operator for YTsaurus.
https://ytsaurus.tech
Other
35 stars 25 forks source link

UpdateSelector needs to be decoupled #325

Open Krock21 opened 3 months ago

Krock21 commented 3 months ago

UpdateSelector has fixed amount of values now that do not cover all combinations of possible and reasonable updates.

In my case I wanted to update HttpProxy and QueryTrackers, and only UpdateSelector: StatelessOnly allows me to do it and it forces me to update execNodes and DataNodes too.

I suggest either:

koct9i commented 3 months ago
type ComponentUpdateSelector struct {
  Component CompontntKind // required enum
  Name string // optional instance group name
  Update true // default false

  // future stuff:
  // Rolling bool
  // DeleteAllDataAndRemoveAllVolumesDuringUpdate bool
}

type YtsaurusSpec struct {
  ...
  UpdateSelectors []ComponentUpdateSelector
  ...
}
spec:
  updateSelectors:
    - component: Masters
      update: false
    - component: ExecNodes
      name: test
      update: true
l0kix2 commented 1 month ago

I guess some groups of component kinds should be supported (something like current StatelessOnly and stuff, but need think bit better about it than last time)

koct9i commented 1 month ago

As I see there are three levels:

  1. Masters
  2. Stateful - DataNodes and TabletNodes
  3. Stateless - all others

So, we could rearrange existing enum to reflect that. And reuse it in new selector.

l0kix2 commented 1 month ago

Also we need: 1) to preserve current default behaviour: "update everything" for users who don't care about selective updates 2) to have "all updates blocked" switch.

From the config ux/code perspective it would be convienient to have empty value as nothing and group selector "Everything", but it will require "Everything" to be default value here on crd level (which I don't like very much, but guess it is fine). I don't know if list field can have default value though.

koct9i commented 1 month ago

Does not update and show explaining status is safe and sane default option.

l0kix2 commented 1 month ago

Does not update and show explaining status is safe and sane default option.

I don't know if our users are expecting that they will need to expicitly set field in their spec to be able to update cluster, but maybe it is ok. Need to discuss

koct9i commented 1 month ago

I expect that all known users are smart enough.

l0kix2 commented 1 month ago

I guess if we are adding new field we can switch the default to "No updates" if we introduce some kind of "UpdateBlocked" status for the resource (with event maybe)

l0kix2 commented 1 month ago

So to sum things up: 1) We are adding new field updateSelectors instead updateSelector as @koct9i suggested https://github.com/ytsaurus/ytsaurus-k8s-operator/issues/325#issuecomment-2296072041 2) default empty value means no updates allowed 3) users can specify any component (even one dnd of many by name) in updateSelectors 4) we will have some groups of components: Everything, Masters, Stateless, Statefull (maybe Group field to ComponentUpdateSelector should be added). 5) we need new status UpdateBlocked or smth to signalize that update is needed but not allowed by selectors

IsFulUpdate and updateSelector fields should be converted in relevant updateSelectors value in the component manager.