You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@druid.apache.org by "AdheipSingh (via GitHub)" <gi...@apache.org> on 2026/05/04 20:06:44 UTC
[PR] Feat/tier aware upgrade ordering [druid-operator]
AdheipSingh opened a new pull request, #17:
URL: https://github.com/apache/druid-operator/pull/17
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
<!-- Thanks for trying to help us make Druid Operator be the best it can be! Please fill out as much of the following information as is possible (where relevant, and remove it when irrelevant) to help make the intention and scope of this PR clear in order to ease review. -->
Fixes #XXXX.
<!-- Replace XXXX with the id of the issue fixed in this PR. Remove this section if there is no corresponding issue. Don't reference the issue in the title of this pull-request. -->
### Description
<!-- Describe the goal of this PR and the problem you encoutered while managing Druid clusters. Something like, "I have a Druid cluster managed with this operator and wanted to change XX on the cluster to enable YY usecase that I needed due to ZZ requirement.". If there is a corresponding issue (referenced above), it's not necessary to repeat the description here, however, you may choose to keep one summary sentence. -->
<!-- Describe the possible solutions and chosen one with the rationale. -->
<!-- Describe key changes made in the patch. -->
<hr>
This PR has:
- [ ] been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
- [ ] been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
- [ ] added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
- [ ] added documentation for new or modified features or behaviors.
<hr>
##### Key changed/added files in this PR
* `MyFoo`
* `OurBar`
* `TheirBaz`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For additional commands, e-mail: dev-help@druid.apache.org
Re: [PR] Feat/tier aware upgrade ordering [druid-operator]
Posted by "AdheipSingh (via GitHub)" <gi...@apache.org>.
AdheipSingh commented on PR #17:
URL: https://github.com/apache/druid-operator/pull/17#issuecomment-4418159494
Thanks @aruraghuwanshi for the review. Will get back this week.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For additional commands, e-mail: dev-help@druid.apache.org
Re: [PR] Feat/tier aware upgrade ordering [druid-operator]
Posted by "aruraghuwanshi (via GitHub)" <gi...@apache.org>.
aruraghuwanshi commented on code in PR #17:
URL: https://github.com/apache/druid-operator/pull/17#discussion_r3215216330
##########
apis/druid/v1alpha1/druid_types.go:
##########
@@ -252,6 +252,17 @@ type DruidSpec struct {
// +optional
Affinity *v1.Affinity `json:"affinity,omitempty"`
+ // OrderOfUpgrade defines the order in which node types are upgraded during a rolling deploy.
+ // If not specified, the default order is used: historical, overlord, middleManager, indexer, broker, coordinator, router.
+ // +optional
+ OrderOfUpgrade []string `json:"orderOfUpgrade,omitempty"`
Review Comment:
Looks like the generated CRD manifests may still need to be included for these new API fields.
I see zz_generated.deepcopy.go was updated in the PR, but running make manifests generate locally still produced additional generated drift, especially in config/crd/bases/druid.apache.org_druids.yaml and chart/crds/druid.apache.org_druids.yaml.
Without those CRD updates, users installing from the manifests or Helm chart may not be able to persist orderOfUpgrade, orderOfUpgradeOfTiers, or tier as expected.
##########
controllers/druid/ordering_test.go:
##########
@@ -68,4 +63,41 @@ var _ = Describe("Test ordering logic", func() {
Expect(orderedServiceGroups[7].key).Should(Equal("routers"))
})
})
+
+ Context("When creating a druid cluster with custom order and tiers", func() {
+ const filePath = "testdata/ordering-tiers.yaml"
+ var druid = &druidv1alpha1.Druid{}
+
+ It("Should create the druid object", func() {
+ By("Creating a new druid")
+ druidCR, err := readDruidClusterSpecFromFile(filePath)
+ Expect(err).Should(BeNil())
+ Expect(k8sClient.Create(ctx, druidCR)).To(Succeed())
+
+ By("Getting a newly created druid")
+ Eventually(func() bool {
+ err := k8sClient.Get(ctx, types.NamespacedName{Name: druidCR.Name, Namespace: druidCR.Namespace}, druid)
+ return err == nil
+ }, timeout, interval).Should(BeTrue())
+ })
+ It("Should return nodes ordered by custom nodeType order and tier order", func() {
Review Comment:
Could we add a small regression test around incomplete or invalid orderOfUpgrade inputs?
The happy path for tier ordering is covered, but the more risky behavior seems to be partial, unknown, or duplicate node type entries. A test showing that remaining configured nodes are still reconciled, or that the spec is rejected during validation, would make this safer to evolve.
##########
controllers/druid/ordering.go:
##########
@@ -18,36 +18,85 @@ under the License.
*/
package druid
-import "github.com/apache/druid-operator/apis/druid/v1alpha1"
+import (
+ "sort"
+
+ "github.com/apache/druid-operator/apis/druid/v1alpha1"
+)
var (
- druidServicesOrder = []string{historical, overlord, middleManager, indexer, broker, coordinator, router}
+ defaultDruidServicesOrder = []string{historical, overlord, middleManager, indexer, broker, coordinator, router}
)
type ServiceGroup struct {
- key string
- spec v1alpha1.DruidNodeSpec
+ key string
+ nodeType string
+ tier string
+ spec v1alpha1.DruidNodeSpec
}
-// getNodeSpecsByOrder returns all NodeSpecs f a given Druid object.
-// Recommended order is described at http://druid.io/docs/latest/operations/rolling-updates.html
func getNodeSpecsByOrder(m *v1alpha1.Druid) []*ServiceGroup {
+ nodeTypeOrder := defaultDruidServicesOrder
+ if len(m.Spec.OrderOfUpgrade) > 0 {
+ nodeTypeOrder = m.Spec.OrderOfUpgrade
Review Comment:
Could we make this path a little more defensive?
If orderOfUpgrade is set, it looks like the reconciler only returns node groups whose nodeType appears in that list. For a partial list, typo, or unknown node type, configured nodes could be skipped entirely.
Since deployDruidCluster uses this returned list to populate the resource name maps used by cleanup, skipped node groups may later look unused and be deleted.
Maybe we could either validate orderOfUpgrade as a complete/valid list for the configured node types, or treat it as a priority list and append any remaining configured node types in the default deterministic order.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For additional commands, e-mail: dev-help@druid.apache.org
Re: [PR] Feat/tier aware upgrade ordering [druid-operator]
Posted by "AdheipSingh (via GitHub)" <gi...@apache.org>.
AdheipSingh commented on code in PR #17:
URL: https://github.com/apache/druid-operator/pull/17#discussion_r3184302537
##########
controllers/druid/ordering.go:
##########
@@ -18,36 +18,85 @@ under the License.
*/
package druid
-import "github.com/apache/druid-operator/apis/druid/v1alpha1"
+import (
+ "sort"
+
+ "github.com/apache/druid-operator/apis/druid/v1alpha1"
+)
var (
- druidServicesOrder = []string{historical, overlord, middleManager, indexer, broker, coordinator, router}
+ defaultDruidServicesOrder = []string{historical, overlord, middleManager, indexer, broker, coordinator, router}
)
type ServiceGroup struct {
- key string
- spec v1alpha1.DruidNodeSpec
+ key string
+ nodeType string
+ tier string
+ spec v1alpha1.DruidNodeSpec
}
-// getNodeSpecsByOrder returns all NodeSpecs f a given Druid object.
-// Recommended order is described at http://druid.io/docs/latest/operations/rolling-updates.html
func getNodeSpecsByOrder(m *v1alpha1.Druid) []*ServiceGroup {
+ nodeTypeOrder := defaultDruidServicesOrder
+ if len(m.Spec.OrderOfUpgrade) > 0 {
+ nodeTypeOrder = m.Spec.OrderOfUpgrade
+ }
- scaledServiceSpecsByNodeType := map[string][]*ServiceGroup{}
- for _, t := range druidServicesOrder {
- scaledServiceSpecsByNodeType[t] = []*ServiceGroup{}
+ groupsByNodeType := map[string][]*ServiceGroup{}
+ for _, t := range nodeTypeOrder {
+ groupsByNodeType[t] = []*ServiceGroup{}
}
for key, nodeSpec := range m.Spec.Nodes {
- scaledServiceSpec := scaledServiceSpecsByNodeType[nodeSpec.NodeType]
- scaledServiceSpecsByNodeType[nodeSpec.NodeType] = append(scaledServiceSpec, &ServiceGroup{key: key, spec: nodeSpec})
+ sg := &ServiceGroup{
+ key: key,
+ nodeType: nodeSpec.NodeType,
+ tier: nodeSpec.Tier,
+ spec: nodeSpec,
+ }
+ groupsByNodeType[nodeSpec.NodeType] = append(groupsByNodeType[nodeSpec.NodeType], sg)
}
- allScaledServiceSpecs := make([]*ServiceGroup, 0, len(m.Spec.Nodes))
+ for nodeType, groups := range groupsByNodeType {
+ tierOrder := m.Spec.OrderOfUpgradeOfTiers[nodeType]
+ sortServiceGroups(groups, tierOrder)
+ groupsByNodeType[nodeType] = groups
+ }
- for _, t := range druidServicesOrder {
- allScaledServiceSpecs = append(allScaledServiceSpecs, scaledServiceSpecsByNodeType[t]...)
+ result := make([]*ServiceGroup, 0, len(m.Spec.Nodes))
+ for _, t := range nodeTypeOrder {
+ result = append(result, groupsByNodeType[t]...)
}
- return allScaledServiceSpecs
+ return result
+}
+
+func sortServiceGroups(groups []*ServiceGroup, tierOrder []string) {
Review Comment:
sortServiceGroups sorts nodes of the same node type to determine their rollout order. tierRank map will convert the tier order slice ["hot", "cold", "glacier"] into a lookup: {"hot": 0, "cold": 1, "glacier": 2}.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For additional commands, e-mail: dev-help@druid.apache.org
Re: [PR] Feat/tier aware upgrade ordering [druid-operator]
Posted by "AdheipSingh (via GitHub)" <gi...@apache.org>.
AdheipSingh commented on PR #17:
URL: https://github.com/apache/druid-operator/pull/17#issuecomment-4374174970
#### **Note**: **The following is review is purely written by me and not edited or reviewed or taken any inspiration by any AI.**
### Problem Statement
The issue raised is valid, as the operator reconciles multiple druid nodes based on purely nodeType. The underlying structure is ```map[string]nodeSpec```, within nodeSpec the only way to segregate nodes is based on nodeType. So if i have 10 historical with X,Y,Z names, they all will get upgraded at same time, which is a pure disruption.
I would propose for an implementation which fixes the problem at its core, and sticks to druid native design on how it represents nodeTypes and tiers.
### Approach
If i break down the main abstractions, we are dealing with 4 constructs. ( nodeType is present as of now, i plan to introduce order of upgrade, tiers and order of upgrade of tiers ).
- OrderOfUpgrade
- OrderOfUpgradeOfTiers
- NodeTypes
- Tiers
As a user i should be able to define the following for my druid nodes.
- i want to upgrade my druid nodes in a certain order defined.
- i want to define tiers within my druid nodes.
- i want to define the order of upgrades of my tiers for each druid node defined.
### Tiers
In druid we can categorize historicals as well as brokers into tiers. A tier which defines separate groups of Historicals and Brokers to receive different query assignments / loading rules etc.
How does this map in the operator spec ?
We introduce tier as a key within the nodeSpec, scoped same with nodeType.
```
historical-aws-az1:
tier: hot
nodeType: historical
historical-aws-az2:
tier: cold
nodeType:historical
broker-aws-az1:
tier: hot
nodeType: broker
broker-aws-az1:
tier: cold
nodeType: broker
.....
```
### OrderOfUpgrade
Remove the hardcoded order in the code, though the order is based on druid's recommendation, a lot of times i had a custom order of upgrade. The main for loop should construct an order based on this.
```
orderOfUpgrade:
- historical
- broker
- middlemanager
- coordinator
- router
- overlord
```
This is a []string structure and on each reconcile should be built.
### OrderOfUpgradeOfTiers
WIthin a nodeType we need to have the ability to define order of upgrade of nodeTypes. So that needs to be defined as a separate structure for each tier
OrderOfUpgradeOfTiers:
```
- histroricals:
- hot
- cold
- glacier
- broker
- hot
- cold
```
Here's a combined spec.
```
spec:
orderOfUpgrade:
- historical
- broker
- middleManager
- coordinator
- router
- overlord
orderOfUpgradeOfTiers:
historical:
- hot
- cold
- glacier
broker:
- hot
- cold
nodes:
historical-aws-az1:
tier: hot
nodeType: historical
historical-aws-az2:
tier: cold
nodeType: historical
historical-aws-az3:
tier: glacier
nodeType: historical
broker-aws-az1:
tier: hot
nodeType: broker
broker-aws-az2:
tier: cold
nodeType: broker
middlemanagers:
nodeType: middleManager
coordinators:
nodeType: coordinator
routers:
nodeType: router
overlords:
nodeType: overlord
```
Upgrade execution:
1. historical:hot → historical-aws-az1
2. historical:cold → historical-aws-az2
3. historical:glacier → historical-aws-az3
4. broker:hot → broker-aws-az1
5. broker:cold → broker-aws-az2
6. middleManager → middlemanagers
7. coordinator → coordinators
8. router → routers
9. overlord → overlords
### Implementation
Signature remains as it is
```
func getNodeSpecsByOrder(m *v1alpha1.Druid) []*ServiceGroup {
// Returns ordered list of ServiceGroups ready for sequential rollout
}
```
The existing service group should be extended to use :
```
type ServiceGroup struct {
key string
nodeType string
tier string
spec v1alpha1.DruidNodeSpec
}
```
An ideal service group after construction should look like this:
```
[
{key: "historical-aws-az1", nodeType: "historical", tier: "hot", spec: ...},
{key: "historical-aws-az2", nodeType: "historical", tier: "cold", spec: ...},
{key: "historical-aws-az3", nodeType: "historical", tier: "glacier", spec: ...},
{key: "broker-az1", nodeType: "broker", tier: "hot", spec: ...},
{key: "broker-az2", nodeType: "broker", tier: "cold", spec: ...},
{key: "mm-1", nodeType: "middleManager", tier: "", spec: ...},
]
```
This way we solve the problem at the core. I have half of the implementation already done, and would like to raise a PR and get feedback. Also this is needs to be backward compatible, so we make sure regardless of user specifying the above we fallback to the default/current way.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@druid.apache.org
For additional commands, e-mail: dev-help@druid.apache.org