Skip to content

Commit

Permalink
fix(coordinator): panic when BadVersion in coordinator (#512)
Browse files Browse the repository at this point in the history
## Motivation

The coordinator will stop all the operations when it stuck in the
metadata `bad version` case. this PR is trying to trigger panic when bad
version to help auto-restart. the bad version might caused by many
cases:


1. operator/maintainer update the status config.
2. another coordinator updated metadata before shutdown.


> the coordinator is using lock to ensure all the metadata operation is
invoke sequentially. which means we don't have race conditions in the
process.

## Modification

- panic directly when met `BadVersion`

## Alternatives

- Update to the latest version when update metadata get `BadVersion`.
but we have to change some logics to ensure get version before building
the new status to avoid unexpected override.
  • Loading branch information
mattisonchao authored Sep 13, 2024
1 parent f8b82de commit ad609a0
Show file tree
Hide file tree
Showing 4 changed files with 11 additions and 10 deletions.
4 changes: 2 additions & 2 deletions coordinator/impl/metadata_configmap.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,13 @@ func (m *metadataProviderConfigMap) Store(status *model.ClusterStatus, expectedV
}

if version != expectedVersion {
return version, ErrMetadataBadVersion
panic(ErrMetadataBadVersion)
}

data := configMap(m.name, status, expectedVersion)
cm, err := K8SConfigMaps(m.kubernetes).Upsert(m.namespace, m.name, data)
if k8serrors.IsConflict(err) {
return version, ErrMetadataBadVersion
panic(ErrMetadataBadVersion)
}
version = Version(cm.ResourceVersion)
m.metadataSize.Store(int64(len(data.Data["status"])))
Expand Down
2 changes: 1 addition & 1 deletion coordinator/impl/metadata_file.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ func (m *metadataProviderFile) Store(cs *model.ClusterStatus, expectedVersion Ve
}

if expectedVersion != existingVersion {
return MetadataNotExists, ErrMetadataBadVersion
panic(ErrMetadataBadVersion)
}

newVersion = incrVersion(existingVersion)
Expand Down
2 changes: 1 addition & 1 deletion coordinator/impl/metadata_memory.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (m *metadataProviderMemory) Store(cs *model.ClusterStatus, expectedVersion
defer m.Unlock()

if expectedVersion != m.version {
return MetadataNotExists, ErrMetadataBadVersion
panic(ErrMetadataBadVersion)
}

m.cs = cs.Clone()
Expand Down
13 changes: 7 additions & 6 deletions coordinator/impl/metadata_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,13 +59,14 @@ func TestMetadataProvider(t *testing.T) {
assert.Equal(t, MetadataNotExists, version)
assert.Nil(t, res)

newVersion, err := m.Store(&model.ClusterStatus{
Namespaces: map[string]model.NamespaceStatus{},
}, "")
assert.ErrorIs(t, err, ErrMetadataBadVersion)
assert.Equal(t, MetadataNotExists, newVersion)
assert.PanicsWithError(t, ErrMetadataBadVersion.Error(), func() {
_, err := m.Store(&model.ClusterStatus{
Namespaces: map[string]model.NamespaceStatus{},
}, "")
assert.NoError(t, err)
})

newVersion, err = m.Store(&model.ClusterStatus{
newVersion, err := m.Store(&model.ClusterStatus{
Namespaces: map[string]model.NamespaceStatus{},
}, MetadataNotExists)
assert.NoError(t, err)
Expand Down

0 comments on commit ad609a0

Please sign in to comment.