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

Add multiple update selectors #383

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

wilwell
Copy link
Contributor

@wilwell wilwell commented Nov 4, 2024

No description provided.

@wilwell wilwell force-pushed the wilwell/multiple-update-selectors branch 2 times, most recently from 3be85ec to e9c2100 Compare November 5, 2024 12:08
@wilwell wilwell force-pushed the wilwell/multiple-update-selectors branch 6 times, most recently from ae5affa to fae9001 Compare December 5, 2024 11:50
@wilwell wilwell force-pushed the wilwell/multiple-update-selectors branch 5 times, most recently from a6175c8 to 06fd1a8 Compare December 16, 2024 13:56
@l0kix2
Copy link
Collaborator

l0kix2 commented Dec 18, 2024

Please rebase on the main branch since #387 is merged — which have couple of new stages in the flows

// UpdateSelectorEverything means that all components could be updated.
// With this setting and if master or tablet nodes need update all the components would be updated.
UpdateSelectorEverything UpdateSelector = "Everything"
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not remove updateSelector stuff for now. We are shooting ourselves in the leg that way, since we ourselves need some time to migrate from updateSelector to updateSelectors.
Let's just add deprecation comment and remove it it next releases when we migrate.

I suggest we have some function, which would receive ytsaurus spec and return updateSelectors values.
It should be easy to unittest it for all major cases, which are:

  1. if updateSelectors is not nil/empty list — we return it as is
  2. if updateSelectors is nil/empty list and updateSelector != UpdateSelectorUnspecified — we convert it's value to the updateSelectors value. Rules are:
UpdateSelectorNothing > []ComponentUpdateSelector{{ComponentGroup: consts.ComponentGroupNone}}
UpdateSelectorStatelessOnly > []ComponentUpdateSelector{{ComponentGroup: consts. ComponentGroupStateless}}
UpdateSelectorEverything > []ComponentUpdateSelector{{ComponentGroup: consts.ComponentGroupEverything}}

UpdateSelectorMasterOnly > []ComponentUpdateSelector{{ComponentType: consts.MasterType}}
UpdateSelectorDataNodesOnly > []ComponentUpdateSelector{{ComponentType: consts.DataNodes}}
UpdateSelectorExecNodesOnly > []ComponentUpdateSelector{{ComponentType: consts.ExecNodes}}
UpdateSelectorTabletNodesOnly > []ComponentUpdateSelector{{ComponentType: consts.TabletNodes}}
  1. if updateSelectors is nil/empty list and updateSelector == UpdateSelectorUnspecified we convert enableFullUpdate to the updateSelectors value. Rules are:
enableFullUpdate=true > []ComponentUpdateSelector{{ComponentGroup: consts.ComponentGroupEverything}}
enableFullUpdate=false => []ComponentUpdateSelector{{ComponentGroup: consts.ComponentGroupStateless}}

api/v1/ytsaurus_types.go Outdated Show resolved Hide resolved
api/v1/ytsaurus_types.go Outdated Show resolved Hide resolved
pkg/consts/types.go Outdated Show resolved Hide resolved
return ytv1.UpdateFlowFull
}
return ytv1.UpdateFlowStateless
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have combination of selectors that wouldn't be possible to cover with existing flow (like like master+http proxies will go to everything and will update tablet nodes which wasn't allowed by selector) we have to make changes in flows code.
Also we've discovered that we already have checks like needSchedulerUpdate and needQueryTrackerUpdate we simplify code a bit and just have one "EverythingFlow" with added ifs for the master and tablet nodes in the required places. Other flows and flow field itself can be removed with this change — that would make less copypaste and possible errors in the code.

The thing which is bothers me that this flows is not test covered very well and it is rather expensive (in the meaning of test time and code amount) to test all the required cases in e2e.
So I suggest we convert this flows code with switch-cases in a different shape. We introduce some struct like this

type flowStep struct {
  name string
  condition func(ytv1.Ytsaurus) bool
  onSuccess func() err
}

and have some function to build flow from the list of exact components which require update

func buildFlow(components) []flowStep{
    var steps []flowStep
    if components.Contain(masterType) {
        steps = append(flowStep{
             name: "safe mode enabled",
             condition: func(ytsaurus ytv1.Ytsaurus) {
                 return resource.Status.UpdateStatus.State == `ytv1.UpdateStateWaitingForSafeModeEnabled`
             },
             onSuccess: func(ytsaurus ytv1.Ytsaurus) {
                 return ytsaurus.SaveUpdateState(ctx, ytv1.UpdateStateWaitingForTabletCellsSaving)
              }
        })
    }
    ...
    return steps
}

(I'm not sure that this exact fields and functions should be there, but something similar should work).

That way we can cover our flow building with fast unittest good enough and cover all the important cases and be sure not to break stuff in flow logic.

So the whole code for this PR will look like this:

  1. convert fullUpdate+updateSelector+updateSelectors to the updateSelectors list
  2. get components which require update and filter them with updateSelectors — set it to the updating components field of UpdateState and set state=Updating
  3. generate flow for the list of the componenents
  4. apply the flow

all 1-3 steps can be untitested that way.

I think it is better to do it in the separate PR and after that use that code in this PR.
What do you think? Does it make sense to you?

@wilwell wilwell force-pushed the wilwell/multiple-update-selectors branch 5 times, most recently from 935eb92 to 39016b9 Compare December 18, 2024 18:08
@wilwell wilwell force-pushed the wilwell/multiple-update-selectors branch from 39016b9 to 683a5b9 Compare December 18, 2024 23:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants