Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UpdateSelector needs to be decoupled #325

Open
Krock21 opened this issue Aug 19, 2024 · 9 comments
Open

UpdateSelector needs to be decoupled #325

Krock21 opened this issue Aug 19, 2024 · 9 comments
Labels
enhancement New feature or request

Comments

@Krock21
Copy link
Member

Krock21 commented Aug 19, 2024

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:

  • Making UpdateSelector masked and composable from every component, like QUERY_TRACKER | HTTP_PROXY
  • Splitting it to many different selectors who are true by default: updateQueryTracker: true; updateHttpProxy=true...
@koct9i
Copy link
Collaborator

koct9i commented Aug 19, 2024

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
Copy link
Collaborator

l0kix2 commented Oct 21, 2024

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
Copy link
Collaborator

koct9i commented Oct 22, 2024

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.

@koct9i koct9i added the enhancement New feature or request label Oct 22, 2024
koct9i added a commit to koct9i/ytsaurus-k8s-operator that referenced this issue Oct 23, 2024
@l0kix2
Copy link
Collaborator

l0kix2 commented Oct 23, 2024

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
Copy link
Collaborator

koct9i commented Oct 23, 2024

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

@l0kix2
Copy link
Collaborator

l0kix2 commented Oct 23, 2024

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
Copy link
Collaborator

koct9i commented Oct 23, 2024

I expect that all known users are smart enough.

koct9i added a commit to koct9i/ytsaurus-k8s-operator that referenced this issue Oct 23, 2024
koct9i added a commit to koct9i/ytsaurus-k8s-operator that referenced this issue Oct 23, 2024
@l0kix2
Copy link
Collaborator

l0kix2 commented Oct 24, 2024

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)

koct9i added a commit to koct9i/ytsaurus-k8s-operator that referenced this issue Oct 24, 2024
koct9i added a commit to koct9i/ytsaurus-k8s-operator that referenced this issue Oct 24, 2024
koct9i added a commit to koct9i/ytsaurus-k8s-operator that referenced this issue Oct 24, 2024
koct9i added a commit that referenced this issue Oct 25, 2024
@l0kix2
Copy link
Collaborator

l0kix2 commented Oct 25, 2024

So to sum things up:

  1. We are adding new field updateSelectors instead updateSelector as @koct9i suggested UpdateSelector needs to be decoupled #325 (comment)
  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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: No status
Development

No branches or pull requests

3 participants