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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
