mirror of
https://github.com/milvus-io/milvus.git
synced 2026-07-21 02:05:41 +00:00
fix: stop runtime overlay from shadowing etcd-persisted mq.type after WAL switch (#51543)
issue: #51497 After a successful WAL switch via `POST /management/wal/alter`, `GET /management/config/get?keys=mq.type` kept returning the pre-switch value with `"source":"RuntimeSource"` until the coordinator restarted, even though the ack callback had already persisted the new mq.type into etcd (and linearizably refreshed the local EtcdSource). External operators polling that endpoint to confirm switch completion never saw the new value. ### Root cause `InitAndSelectWALName()` saved the startup-resolved WAL name into the config manager's **runtime overlay**, and `Manager.GetConfig` serves the overlay with top priority — permanently shadowing the EtcdSource for every reader in the process. The overlay write existed only to feed `ProcessImmutableConfigs`, which pins `GetConfig`'s value into etcd create-if-absent at first boot (without it, the literal `"default"` would be persisted). ### Fix Replace the overlay bridge with an explicit per-key **renderer** on the persistence path, so mq.type reads fall through to the etcd source and naturally follow a WAL switch: - `ProcessImmutableConfigs(renderers map[string]func(raw string) string)`: a renderer converts a placeholder raw value (mq.type's literal `"default"`) into the concrete value at create-if-absent persist time. It never runs when etcd already holds the key (a completed switch survives restart), and it also covers the key-absent-from-all-sources case (raw is passed as `""`). Generic: any future immutable config with a placeholder value can plug in its own renderer. - `InitAndSelectWALName()` no longer writes the runtime overlay and returns the resolved `message.WALName`; the coordinator startup passes a mq.type renderer backed by that name. - `dependency.HealthCheck` resolves the literal `"default"` to the actually-enabled MQ before probing (this gap predates the overlay Save: since #36822, deployments running with `mq.type: default` probed nothing and always reported unhealthy). This also fixes two other readers of the shadowed value: - the `HandleAlterWAL` same-type short-circuit: re-posting the completed target no longer re-broadcasts, and a rollback to the original MQ is no longer silently swallowed with "already configured"; - the proxy MQ health check (`/_cluster/dependencies`) no longer keeps probing the old MQ forever after a switch. ### Behavior note `MQCfg.Type.GetValue()` now follows etcd at runtime instead of being frozen at process start: the coordinator sees a switch immediately (linearizable refresh in the ack callback); other nodes converge within one EtcdSource poll cycle (default `refreshInterval` 5s). Audited readers: `mustSelectMQType` / `MustSelectWALName` resolve `"default"` themselves and are startup-scoped; the AlterWAL idempotency check and the proxy health check are exactly the readers this change fixes. ### Verification - New tests (all watched failing first): renderer converts placeholder before first persist / existing etcd value never overwritten nor re-rendered / no-renderer keys unchanged / key absent from all sources still pinned via renderer (`pkg/config`); `InitAndSelectWALName` leaves mq.type served from its original source, not `RuntimeSource` (`streamingutil/util`); `HealthCheck("default")` resolves to the enabled MQ (`dependency`). - Full package runs green locally: `pkg/config`, `internal/util/streamingutil/util`, `internal/util/dependency` (with `-tags dynamic,test -gcflags="all=-N -l"`). - `cmd/roles`, `internal/coordinator`, `internal/distributed/streaming` type-checked via `go vet` locally (no current-master segcore build on this machine); relying on CI for full compilation. A live-cluster alter → config/get end-to-end pass is still pending — the switch-visibility chain (etcd write → `GetConfig` returns the new value) is covered at unit level by the existing `TestAlterConfigsInEtcd`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Signed-off-by: tinswzy <zhenyuan.wei@zilliz.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
10cd30c66e
commit
b9621d7d32
+8
-4
@@ -416,10 +416,14 @@ func (mr *MilvusRoles) Run() {
|
||||
|
||||
// Persist immutable configurations at startup, such as mqType paramItem
|
||||
if (mr.EnableRootCoord && mr.EnableDataCoord && mr.EnableQueryCoord) || mr.EnableMixCoord {
|
||||
// Initialize the actual walName instead of default
|
||||
util.InitAndSelectWALName()
|
||||
// persist immutable configs if necessary
|
||||
if err := paramtable.GetBaseTable().Manager().ProcessImmutableConfigs(); err != nil {
|
||||
// Resolve the actual walName instead of default
|
||||
walName := util.InitAndSelectWALName()
|
||||
// persist immutable configs if necessary; mq.type's literal "default" is
|
||||
// rendered to the resolved walName so the value pinned in etcd is always
|
||||
// a concrete WAL name (issue #51497)
|
||||
if err := paramtable.GetBaseTable().Manager().ProcessImmutableConfigs(map[string]func(string) string{
|
||||
paramtable.Get().MQCfg.Type.Key: func(string) string { return walName.String() },
|
||||
}); err != nil {
|
||||
mlog.Error(context.TODO(), "failed to process immutable configs", mlog.Err(err))
|
||||
return
|
||||
}
|
||||
|
||||
@@ -15,6 +15,7 @@ import (
|
||||
"github.com/milvus-io/milvus/pkg/v3/objectstorage"
|
||||
"github.com/milvus-io/milvus/pkg/v3/util/merr"
|
||||
"github.com/milvus-io/milvus/pkg/v3/util/paramtable"
|
||||
"github.com/milvus-io/milvus/pkg/v3/util/typeutil"
|
||||
)
|
||||
|
||||
const (
|
||||
@@ -173,6 +174,14 @@ type Factory interface {
|
||||
}
|
||||
|
||||
func HealthCheck(mqType string) *common.MQClusterStatus {
|
||||
if mqType == mqTypeDefault {
|
||||
// "default" is a placeholder meaning "auto-select by enabled MQs"; resolve
|
||||
// it to the concrete type before probing. The same resolution already
|
||||
// succeeded at factory init, so it cannot panic here on a running node.
|
||||
params := paramtable.Get()
|
||||
mqType = mustSelectMQType(paramtable.GetRole() == typeutil.StandaloneRole, mqType,
|
||||
mqEnable{params.RocksmqEnable(), params.PulsarEnable(), params.KafkaEnable(), params.WoodpeckerEnable()})
|
||||
}
|
||||
clusterStatus := &common.MQClusterStatus{MqType: mqType}
|
||||
switch mqType {
|
||||
case mqTypeRocksmq:
|
||||
|
||||
@@ -6,6 +6,7 @@ import (
|
||||
"github.com/stretchr/testify/assert"
|
||||
|
||||
"github.com/milvus-io/milvus/pkg/v3/util/paramtable"
|
||||
"github.com/milvus-io/milvus/pkg/v3/util/typeutil"
|
||||
)
|
||||
|
||||
func TestValidateMQType(t *testing.T) {
|
||||
@@ -86,3 +87,18 @@ func TestHealthCheck(t *testing.T) {
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestHealthCheckResolvesDefaultMQType(t *testing.T) {
|
||||
paramtable.Init()
|
||||
|
||||
status := HealthCheck(mqTypeDefault)
|
||||
|
||||
// "default" is a placeholder, not a real MQ type: it must be resolved to the
|
||||
// actually-enabled MQ before probing, otherwise the status reports an unknown
|
||||
// type with health=false forever (issue #51497).
|
||||
assert.NotEqual(t, mqTypeDefault, status.MqType)
|
||||
params := paramtable.Get()
|
||||
expected := mustSelectMQType(paramtable.GetRole() == typeutil.StandaloneRole, mqTypeDefault,
|
||||
mqEnable{params.RocksmqEnable(), params.PulsarEnable(), params.KafkaEnable(), params.WoodpeckerEnable()})
|
||||
assert.Equal(t, expected, status.MqType)
|
||||
}
|
||||
|
||||
@@ -25,14 +25,15 @@ type walEnable struct {
|
||||
}
|
||||
|
||||
// InitAndSelectWALName init and select wal name.
|
||||
func InitAndSelectWALName() {
|
||||
// The resolved name is intentionally NOT saved into the runtime config overlay:
|
||||
// the overlay would take priority over the etcd source forever, hiding a later
|
||||
// WAL switch (mq.type updated in etcd) from every reader in this process.
|
||||
// Persisting the resolved name is handled by ProcessImmutableConfigs with a
|
||||
// renderer at coordinator startup instead.
|
||||
func InitAndSelectWALName() message.WALName {
|
||||
walName := MustSelectWALName()
|
||||
message.RegisterDefaultWALName(walName)
|
||||
mqTypeKey := paramtable.Get().MQCfg.Type.Key
|
||||
err := paramtable.Get().Save(mqTypeKey, walName.String())
|
||||
if err != nil {
|
||||
panic(err)
|
||||
}
|
||||
return walName
|
||||
}
|
||||
|
||||
// MustSelectWALName select wal name.
|
||||
|
||||
@@ -5,6 +5,7 @@ import (
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
|
||||
"github.com/milvus-io/milvus/pkg/v3/config"
|
||||
"github.com/milvus-io/milvus/pkg/v3/streaming/util/message"
|
||||
"github.com/milvus-io/milvus/pkg/v3/util/paramtable"
|
||||
)
|
||||
@@ -85,3 +86,20 @@ func TestWoodpeckerLocalStorageInClusterMode(t *testing.T) {
|
||||
assert.Equal(t, message.WALNameWoodpecker, mustSelectWALName(true, message.WALNameWoodpecker.String(), walEnable{true, true, true, true}))
|
||||
})
|
||||
}
|
||||
|
||||
func TestInitAndSelectWALNameDoesNotWriteRuntimeOverlay(t *testing.T) {
|
||||
paramtable.Init()
|
||||
|
||||
walName := InitAndSelectWALName()
|
||||
|
||||
assert.NotEqual(t, message.WALNameUnknown, walName)
|
||||
assert.Equal(t, walName, message.GetDefaultWALName())
|
||||
|
||||
// mq.type must keep being served from its original config source. Writing the
|
||||
// resolved name into the runtime overlay would permanently shadow the etcd
|
||||
// source and hide a later WAL switch from every reader (issue #51497).
|
||||
mqTypeKey := paramtable.Get().MQCfg.Type.Key
|
||||
source, _, err := paramtable.GetBaseTable().Manager().GetConfig(mqTypeKey)
|
||||
assert.NoError(t, err)
|
||||
assert.NotEqual(t, config.RuntimeSource, source)
|
||||
}
|
||||
|
||||
+24
-3
@@ -502,24 +502,45 @@ func (m *Manager) GetEtcdSource() (*EtcdSource, bool) {
|
||||
return etcdSourceImpl, true
|
||||
}
|
||||
|
||||
func (m *Manager) ProcessImmutableConfigs() error {
|
||||
// ProcessImmutableConfigs persists immutable configs into etcd (create-if-absent).
|
||||
// renderers optionally converts a placeholder raw value (e.g. mq.type's literal
|
||||
// "default") into the concrete value to persist, keyed by config key. A renderer
|
||||
// runs only when the key is not yet persisted in etcd; an existing etcd value is
|
||||
// never overwritten or re-rendered.
|
||||
func (m *Manager) ProcessImmutableConfigs(renderers map[string]func(raw string) string) error {
|
||||
etcdSourceImpl, ok := m.GetEtcdSource()
|
||||
if !ok {
|
||||
mlog.Info(context.TODO(), "etcd source not enable,skip processing immutable configs")
|
||||
return nil
|
||||
}
|
||||
|
||||
normalizedRenderers := make(map[string]func(string) string, len(renderers))
|
||||
for key, render := range renderers {
|
||||
normalizedRenderers[formatKey(key)] = render
|
||||
}
|
||||
|
||||
var saveErrors []error
|
||||
var savedConfigs []string
|
||||
m.immutableKeys.Range(func(key string) bool {
|
||||
render, hasRenderer := normalizedRenderers[key]
|
||||
confgSourceName, configValue, getConfigErr := m.GetConfig(key)
|
||||
if getConfigErr != nil {
|
||||
mlog.Warn(context.TODO(), "failed to get config", mlog.String("key", key), mlog.Err(getConfigErr))
|
||||
return true
|
||||
if !hasRenderer {
|
||||
mlog.Warn(context.TODO(), "failed to get config", mlog.String("key", key), mlog.Err(getConfigErr))
|
||||
return true
|
||||
}
|
||||
// the key exists in no source: the renderer alone decides the value to pin
|
||||
confgSourceName, configValue = "", ""
|
||||
}
|
||||
|
||||
_, getFromEtcdErr := etcdSourceImpl.GetConfigurationByKey(key)
|
||||
if errors.Is(getFromEtcdErr, ErrKeyNotFound) {
|
||||
if hasRenderer {
|
||||
rendered := render(configValue)
|
||||
mlog.Info(context.TODO(), "rendered immutable config value before persisting",
|
||||
mlog.String("key", key), mlog.String("rawValue", configValue), mlog.String("renderedValue", rendered))
|
||||
configValue = rendered
|
||||
}
|
||||
mlog.Info(context.TODO(), "immutable config not exist in etcd, saving to persistent storage",
|
||||
mlog.String("fromSource", confgSourceName), mlog.String("key", key), mlog.String("value", configValue))
|
||||
if err := m.SaveConfigToEtcd(etcdSourceImpl, key, configValue); err != nil {
|
||||
|
||||
@@ -474,3 +474,103 @@ func TestAlterConfigsInEtcd(t *testing.T) {
|
||||
}, time.Second*5, 100*time.Millisecond)
|
||||
})
|
||||
}
|
||||
|
||||
func TestProcessImmutableConfigsRenderer(t *testing.T) {
|
||||
cfg, _ := embed.ConfigFromFile("../../configs/advanced/etcd.yaml")
|
||||
cfg.Dir = "/tmp/milvus/test_process_immutable_renderer"
|
||||
e, err := embed.StartEtcd(cfg)
|
||||
assert.NoError(t, err)
|
||||
defer e.Close()
|
||||
defer os.RemoveAll(cfg.Dir)
|
||||
|
||||
mgr, _ := Init(WithEtcdSource(&EtcdInfo{
|
||||
Endpoints: []string{cfg.AdvertiseClientUrls[0].Host},
|
||||
KeyPrefix: "test-immutable-renderer",
|
||||
RefreshInterval: 10 * time.Millisecond,
|
||||
}))
|
||||
|
||||
etcdSource, ok := mgr.GetEtcdSource()
|
||||
assert.True(t, ok, "should get etcd source")
|
||||
|
||||
t.Run("renderer converts placeholder value before first persist", func(t *testing.T) {
|
||||
mgr.SetConfig("render.mq.type", "default")
|
||||
mgr.ImmutableUpdate("render.mq.type")
|
||||
|
||||
err := mgr.ProcessImmutableConfigs(map[string]func(string) string{
|
||||
"render.mq.type": func(raw string) string {
|
||||
assert.Equal(t, "default", raw)
|
||||
return "woodpecker"
|
||||
},
|
||||
})
|
||||
assert.NoError(t, err)
|
||||
|
||||
v, err := etcdSource.GetConfigurationByKey(formatKey("render.mq.type"))
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, "woodpecker", v)
|
||||
})
|
||||
|
||||
t.Run("existing etcd value is not overwritten and renderer is not applied", func(t *testing.T) {
|
||||
err := mgr.UpdateConfigInEtcd(etcdSource, "render.existing", "pulsar")
|
||||
assert.NoError(t, err)
|
||||
|
||||
mgr.SetConfig("render.existing", "default")
|
||||
mgr.ImmutableUpdate("render.existing")
|
||||
|
||||
err = mgr.ProcessImmutableConfigs(map[string]func(string) string{
|
||||
"render.existing": func(raw string) string {
|
||||
t.Errorf("renderer must not run for a key already persisted in etcd")
|
||||
return "must-not-be-used"
|
||||
},
|
||||
})
|
||||
assert.NoError(t, err)
|
||||
|
||||
v, err := etcdSource.GetConfigurationByKey(formatKey("render.existing"))
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, "pulsar", v)
|
||||
})
|
||||
|
||||
t.Run("key without renderer persists raw value unchanged", func(t *testing.T) {
|
||||
mgr.SetConfig("render.plain", "rawvalue")
|
||||
mgr.ImmutableUpdate("render.plain")
|
||||
|
||||
err := mgr.ProcessImmutableConfigs(nil)
|
||||
assert.NoError(t, err)
|
||||
|
||||
v, err := etcdSource.GetConfigurationByKey(formatKey("render.plain"))
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, "rawvalue", v)
|
||||
})
|
||||
}
|
||||
|
||||
func TestProcessImmutableConfigsRendererKeyAbsentFromSources(t *testing.T) {
|
||||
cfg, _ := embed.ConfigFromFile("../../configs/advanced/etcd.yaml")
|
||||
cfg.Dir = "/tmp/milvus/test_process_immutable_renderer_absent"
|
||||
e, err := embed.StartEtcd(cfg)
|
||||
assert.NoError(t, err)
|
||||
defer e.Close()
|
||||
defer os.RemoveAll(cfg.Dir)
|
||||
|
||||
mgr, _ := Init(WithEtcdSource(&EtcdInfo{
|
||||
Endpoints: []string{cfg.AdvertiseClientUrls[0].Host},
|
||||
KeyPrefix: "test-immutable-renderer-absent",
|
||||
RefreshInterval: 10 * time.Millisecond,
|
||||
}))
|
||||
|
||||
etcdSource, ok := mgr.GetEtcdSource()
|
||||
assert.True(t, ok, "should get etcd source")
|
||||
|
||||
// The key exists in no source at all: a registered renderer must still be able
|
||||
// to produce the value to pin into etcd (raw is passed as empty string).
|
||||
mgr.ImmutableUpdate("render.absent")
|
||||
err = mgr.ProcessImmutableConfigs(map[string]func(string) string{
|
||||
"render.absent": func(raw string) string {
|
||||
assert.Equal(t, "", raw)
|
||||
return "woodpecker"
|
||||
},
|
||||
})
|
||||
assert.NoError(t, err)
|
||||
|
||||
v, err := etcdSource.GetConfigurationByKey(formatKey("render.absent"))
|
||||
assert.NoError(t, err)
|
||||
assert.Equal(t, "woodpecker", v)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user