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
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 18 additions & 23 deletions api/v1/ytsaurus_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"

"github.com/ytsaurus/ytsaurus-k8s-operator/pkg/consts"
)

// EmbeddedPersistentVolumeClaim is an embedded version of k8s.io/api/core/v1.PersistentVolumeClaim.
Expand Down Expand Up @@ -645,11 +647,14 @@ type YtsaurusSpec struct {
//+optional
EnableFullUpdate bool `json:"enableFullUpdate"`
//+optional
//+kubebuilder:validation:Enum={"","Nothing","MasterOnly","DataNodesOnly","TabletNodesOnly","ExecNodesOnly","StatelessOnly","Everything"}
// UpdateSelector is an experimental field. Behaviour may change.
// If UpdateSelector is not empty EnableFullUpdate is ignored.
//+optional
// Deprecated: UpdateSelector is an experimental field. Behaviour may change.
UpdateSelector UpdateSelector `json:"updateSelector"`

//+optional
// Controls the components that should be updated during the update process.
UpdateSelectors []ComponentUpdateSelector `json:"updateSelectors,omitempty"`

NodeSelector map[string]string `json:"nodeSelector,omitempty"`
Tolerations []corev1.Toleration `json:"tolerations,omitempty"`

Expand Down Expand Up @@ -727,26 +732,16 @@ type TabletCellBundleInfo struct {

type UpdateSelector string

const (
// UpdateSelectorUnspecified means that selector is disabled and would be ignored completely.
UpdateSelectorUnspecified UpdateSelector = ""
// UpdateSelectorNothing means that no component could be updated.
UpdateSelectorNothing UpdateSelector = "Nothing"
// UpdateSelectorMasterOnly means that only master could be updated.
UpdateSelectorMasterOnly UpdateSelector = "MasterOnly"
// UpdateSelectorTabletNodesOnly means that only data nodes could be updated
UpdateSelectorDataNodesOnly UpdateSelector = "DataNodesOnly"
// UpdateSelectorTabletNodesOnly means that only tablet nodes could be updated
UpdateSelectorTabletNodesOnly UpdateSelector = "TabletNodesOnly"
// UpdateSelectorExecNodesOnly means that only tablet nodes could be updated
UpdateSelectorExecNodesOnly UpdateSelector = "ExecNodesOnly"
// UpdateSelectorStatelessOnly means that only stateless components (everything but master, data nodes, and tablet nodes)
// could be updated.
UpdateSelectorStatelessOnly UpdateSelector = "StatelessOnly"
// 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}}

type ComponentUpdateSelector struct {
//+optional
ComponentType consts.ComponentType `json:"componentType,omitempty"`
//+optional
ComponentGroup consts.ComponentGroup `json:"componentGroup,omitempty"`
//+optional
ComponentName string `json:"componentName,omitempty"`

// TODO(#325): Add rolling options
}

type UpdateFlow string

Expand Down
20 changes: 20 additions & 0 deletions api/v1/zz_generated.deepcopy.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 15 additions & 11 deletions config/crd/bases/cluster.ytsaurus.tech_ytsaurus.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34657,18 +34657,22 @@ spec:
uiImage:
type: string
updateSelector:
description: UpdateSelector is an experimental field. Behaviour may
change.
enum:
- ""
- Nothing
- MasterOnly
- DataNodesOnly
- TabletNodesOnly
- ExecNodesOnly
- StatelessOnly
- Everything
description: 'Deprecated: UpdateSelector is an experimental field.
Behaviour may change.'
type: string
updateSelectors:
description: Controls the components that should be updated during
the update process.
items:
properties:
componentGroup:
type: string
componentName:
type: string
componentType:
type: string
type: object
type: array
useIpv4:
default: false
type: boolean
Expand Down
136 changes: 72 additions & 64 deletions controllers/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -433,61 +433,81 @@
componentNames []string
}

func canUpdateComponent(selector ytv1.UpdateSelector, component consts.ComponentType) bool {
switch selector {
case ytv1.UpdateSelectorNothing:
return false
case ytv1.UpdateSelectorMasterOnly:
return component == consts.MasterType
case ytv1.UpdateSelectorDataNodesOnly:
return component == consts.DataNodeType
case ytv1.UpdateSelectorTabletNodesOnly:
return component == consts.TabletNodeType
case ytv1.UpdateSelectorExecNodesOnly:
return component == consts.ExecNodeType
case ytv1.UpdateSelectorStatelessOnly:
switch component {
case consts.MasterType:
return false
case consts.DataNodeType:
return false
case consts.TabletNodeType:
return false
}
return true
case ytv1.UpdateSelectorEverything:
return true
default:
return false
func getFlowFromComponent(component consts.ComponentType) ytv1.UpdateFlow {
if component == consts.MasterType {
return ytv1.UpdateFlowMaster
}
if component == consts.TabletNodeType {
return ytv1.UpdateFlowTabletNodes
}
if component == consts.DataNodeType || component == consts.ExecNodeType {
return ytv1.UpdateFlowFull
}
return ytv1.UpdateFlowStateless
}
l0kix2 marked this conversation as resolved.
Show resolved Hide resolved
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?


func canUpdateComponent(selectors []ytv1.ComponentUpdateSelector, component components.Component) (bool, error) {
for _, selector := range selectors {
if selector.ComponentType != "" {
if selector.ComponentType == component.GetType() {
return true, nil
}
} else if selector.ComponentName != "" {
if selector.ComponentName == component.GetName() {
return true, nil
}
} else {
switch selector.ComponentGroup {
case consts.ComponentGroupEverything:
return true, nil
case consts.ComponentGroupNothing:
return false, nil
case consts.ComponentGroupStateful:
if component.GetType() == consts.DataNodeType || component.GetType() == consts.TabletNodeType {
return true, nil
}
case consts.ComponentGroupStateless:
if component.GetType() != consts.DataNodeType && component.GetType() != consts.TabletNodeType && component.GetType() != consts.MasterType {
return true, nil
}
default:
return false, fmt.Errorf("unexpected component group %s", selector.ComponentGroup)
}
}
}
return false, nil
}

// chooseUpdateFlow considers spec and decides if operator should proceed with update or block.
// Block case is indicated with non-empty blockMsg.
// If update is not blocked, updateMeta containing a chosen flow and the component names to update returned.
func chooseUpdateFlow(spec ytv1.YtsaurusSpec, needUpdate []components.Component) (meta updateMeta, blockMsg string) {
configuredSelector := spec.UpdateSelector
if configuredSelector == ytv1.UpdateSelectorUnspecified {
if spec.EnableFullUpdate {
configuredSelector = ytv1.UpdateSelectorEverything
} else {
configuredSelector = ytv1.UpdateSelectorStatelessOnly
}
configuredSelectors := spec.UpdateSelectors
if len(configuredSelectors) == 0 && spec.EnableFullUpdate {
configuredSelectors = []ytv1.ComponentUpdateSelector{{ComponentGroup: consts.ComponentGroupEverything}}
}
needFullUpdate := false

var canUpdate []string
var cannotUpdate []string
needFullUpdate := false
flows := make(map[ytv1.UpdateFlow]struct{})

for _, comp := range needUpdate {
componentType := comp.GetType()
componentName := comp.GetName()
if canUpdateComponent(configuredSelector, componentType) {
upd, err := canUpdateComponent(configuredSelectors, comp)
if err != nil {
return updateMeta{}, err.Error()
}
if upd {
canUpdate = append(canUpdate, componentName)
flows[getFlowFromComponent(componentType)] = struct{}{}
} else {
cannotUpdate = append(cannotUpdate, componentName)
}
if !canUpdateComponent(ytv1.UpdateSelectorStatelessOnly, componentType) && componentType != consts.DataNodeType {

statelessCheck, err := canUpdateComponent([]ytv1.ComponentUpdateSelector{{ComponentGroup: consts.ComponentGroupStateless}}, comp)

Check failure on line 509 in controllers/sync.go

View workflow job for this annotation

GitHub Actions / Run checks

SA4006: this value of `err` is never used (staticcheck)
if !statelessCheck && componentType != consts.DataNodeType {
needFullUpdate = true
}
}
Expand All @@ -499,37 +519,25 @@
return updateMeta{}, "All components are uptodate"
}

switch configuredSelector {
case ytv1.UpdateSelectorEverything:
if len(configuredSelectors) == 1 && configuredSelectors[0].ComponentGroup == consts.ComponentGroupEverything {
if needFullUpdate {
return updateMeta{
flow: ytv1.UpdateFlowFull,
componentNames: nil,
}, ""
return updateMeta{flow: ytv1.UpdateFlowFull, componentNames: nil}, ""
} else {
return updateMeta{
flow: ytv1.UpdateFlowStateless,
componentNames: canUpdate,
}, ""
}
case ytv1.UpdateSelectorMasterOnly:
return updateMeta{
flow: ytv1.UpdateFlowMaster,
componentNames: canUpdate,
}, ""
case ytv1.UpdateSelectorTabletNodesOnly:
return updateMeta{
flow: ytv1.UpdateFlowTabletNodes,
componentNames: canUpdate,
}, ""
case ytv1.UpdateSelectorDataNodesOnly, ytv1.UpdateSelectorExecNodesOnly, ytv1.UpdateSelectorStatelessOnly:
return updateMeta{
flow: ytv1.UpdateFlowStateless,
componentNames: canUpdate,
}, ""
default:
return updateMeta{}, fmt.Sprintf("Unexpected update selector %s", configuredSelector)
return updateMeta{flow: ytv1.UpdateFlowStateless, componentNames: canUpdate}, ""
}
}

if len(flows) == 0 {
return updateMeta{}, fmt.Sprintf("Unexpected state: no flows for components {%s}", strings.Join(canUpdate, ", "))
}

if len(flows) == 1 {
for flow := range flows {
return updateMeta{flow: flow, componentNames: canUpdate}, ""
}
}
// If more than one flow is possible, we choose to follow full update flow.
return updateMeta{flow: ytv1.UpdateFlowFull, componentNames: canUpdate}, ""
}

func (r *YtsaurusReconciler) Sync(ctx context.Context, resource *ytv1.Ytsaurus) (ctrl.Result, error) {
Expand Down
30 changes: 19 additions & 11 deletions docs/api.md
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,23 @@ _Appears in:_
| `imagePullSecrets` _[LocalObjectReference](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#localobjectreference-v1-core) array_ | | | |


#### ComponentUpdateSelector







_Appears in:_
- [YtsaurusSpec](#ytsaurusspec)

| Field | Description | Default | Validation |
| --- | --- | --- | --- |
| `componentKind` _[ComponentType](#componenttype)_ | | | |
| `componentGroup` _[ComponentGroup](#componentgroup)_ | | | |


#### ControllerAgentsSpec


Expand Down Expand Up @@ -2048,16 +2065,6 @@ _Underlying type:_ _string_
_Appears in:_
- [YtsaurusSpec](#ytsaurusspec)

| Field | Description |
| --- | --- |
| `` | UpdateSelectorUnspecified means that selector is disabled and would be ignored completely.<br /> |
| `Nothing` | UpdateSelectorNothing means that no component could be updated.<br /> |
| `MasterOnly` | UpdateSelectorMasterOnly means that only master could be updated.<br /> |
| `DataNodesOnly` | UpdateSelectorTabletNodesOnly means that only data nodes could be updated<br /> |
| `TabletNodesOnly` | UpdateSelectorTabletNodesOnly means that only tablet nodes could be updated<br /> |
| `ExecNodesOnly` | UpdateSelectorExecNodesOnly means that only tablet nodes could be updated<br /> |
| `StatelessOnly` | UpdateSelectorStatelessOnly means that only stateless components (everything but master, data nodes, and tablet nodes)<br />could be updated.<br /> |
| `Everything` | UpdateSelectorEverything means that all components could be updated.<br />With this setting and if master or tablet nodes need update all the components would be updated.<br /> |


#### UpdateState
Expand Down Expand Up @@ -2206,7 +2213,8 @@ _Appears in:_
| `oauthService` _[OauthServiceSpec](#oauthservicespec)_ | | | |
| `isManaged` _boolean_ | | true | |
| `enableFullUpdate` _boolean_ | | true | |
| `updateSelector` _[UpdateSelector](#updateselector)_ | UpdateSelector is an experimental field. Behaviour may change.<br />If UpdateSelector is not empty EnableFullUpdate is ignored. | | Enum: [ Nothing MasterOnly DataNodesOnly TabletNodesOnly ExecNodesOnly StatelessOnly Everything] <br /> |
| `updateSelector` _[UpdateSelector](#updateselector)_ | Deprecated: UpdateSelector is an experimental field. Behaviour may change. | | |
| `updateSelectors` _[ComponentUpdateSelector](#componentupdateselector) array_ | Controls the components that should be updated during the update process. | | |
| `nodeSelector` _object (keys:string, values:string)_ | | | |
| `tolerations` _[Toleration](https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.28/#toleration-v1-core) array_ | | | |
| `bootstrap` _[BootstrapSpec](#bootstrapspec)_ | | | |
Expand Down
9 changes: 9 additions & 0 deletions pkg/consts/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,12 @@ const (
ChytType ComponentType = "CHYT"
SpytType ComponentType = "SPYT"
)

type ComponentGroup string

const (
ComponentGroupStateless ComponentGroup = "Stateless"
ComponentGroupStateful ComponentGroup = "Stateful"
ComponentGroupEverything ComponentGroup = "Everything"
ComponentGroupNothing ComponentGroup = "Nothing"
)
Loading
Loading