Skip to content

Commit

Permalink
fixes a dumb refactoring bug preventing isolation-groups from updating (
Browse files Browse the repository at this point in the history 
#6488)

* fixes a dumb refactoring bug affecting isolation-groups
  • Loading branch information
@davidporter-id-au
davidporter-id-au authored Dec 2, 2024
1 parent 2e3645f commit 018fff9
Show file tree
Hide file tree
Showing 12 changed files with 70 additions and 49 deletions.
16 changes: 8 additions & 8 deletions common/isolationgroup/defaultisolationgroupstate/state.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,11 +58,9 @@ func NewDefaultIsolationGroupStateWatcherWithConfigStoreClient(
) (isolationgroup.State, error) {
stopChan := make(chan struct{})

allIsolationGroups := getIsolationGroups()

config := defaultConfig{
IsolationGroupEnabled: dc.GetBoolPropertyFilteredByDomain(dynamicconfig.EnableTasklistIsolation),
AllIsolationGroups: allIsolationGroups,
AllIsolationGroups: getIsolationGroups,
}

return &defaultIsolationGroupStateHandler{
Expand All @@ -76,14 +74,14 @@ func NewDefaultIsolationGroupStateWatcherWithConfigStoreClient(
}, nil
}

func (z *defaultIsolationGroupStateHandler) AvailableIsolationGroupsByDomainID(ctx context.Context, domainID string, availablePollerIsolationGroups []string) (types.IsolationGroupConfiguration, error) {
func (z *defaultIsolationGroupStateHandler) AvailableIsolationGroupsByDomainID(ctx context.Context, domainID string, tasklistName string, availablePollerIsolationGroups []string) (types.IsolationGroupConfiguration, error) {
state, err := z.getByDomainID(ctx, domainID)
if err != nil {
return nil, fmt.Errorf("unable to get isolation group state: %w", err)
}
availableIsolationGroupsCfg := isolationGroupHealthyListToConfig(availablePollerIsolationGroups)
scope := z.createAvailableisolationGroupMetricsScope(domainID)
return availableIG(z.config.AllIsolationGroups, availableIsolationGroupsCfg, state.Global, state.Domain, scope), nil
scope := z.createAvailableisolationGroupMetricsScope(domainID, tasklistName)
return availableIG(z.config.AllIsolationGroups(), availableIsolationGroupsCfg, state.Global, state.Domain, scope), nil
}

func (z *defaultIsolationGroupStateHandler) IsDrained(ctx context.Context, domain string, isolationGroup string) (bool, error) {
Expand Down Expand Up @@ -164,9 +162,11 @@ func (z *defaultIsolationGroupStateHandler) get(ctx context.Context, domain stri
return ig, nil
}

func (z *defaultIsolationGroupStateHandler) createAvailableisolationGroupMetricsScope(domainID string) metrics.Scope {
func (z *defaultIsolationGroupStateHandler) createAvailableisolationGroupMetricsScope(domainID string, tasklistName string) metrics.Scope {
domainName, _ := z.domainCache.GetDomainName(domainID)
return z.metricsClient.Scope(metrics.GetAvailableIsolationGroupsScope).Tagged(metrics.DomainTag(domainName))
return z.metricsClient.Scope(metrics.GetAvailableIsolationGroupsScope).
Tagged(metrics.DomainTag(domainName)).
Tagged(metrics.TaskListTag(tasklistName))
}

// A simple explicit deny-based isolation group implementation
Expand Down
28 changes: 14 additions & 14 deletions common/isolationgroup/defaultisolationgroupstate/state_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return false },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {},
domainAffordance: func(mock *cache.MockDomainCache) {
Expand All @@ -109,7 +109,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
client.EXPECT().GetListValue(
Expand Down Expand Up @@ -139,7 +139,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
client.EXPECT().GetListValue(
Expand All @@ -164,7 +164,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
client.EXPECT().GetListValue(
Expand Down Expand Up @@ -193,7 +193,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
client.EXPECT().GetListValue(
Expand All @@ -213,7 +213,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {},
domainAffordance: func(mock *cache.MockDomainCache) {
Expand All @@ -227,7 +227,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
client.EXPECT().GetListValue(
Expand All @@ -247,7 +247,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
},
Expand All @@ -263,7 +263,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: []string{"zone-1", "zone-2"},
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
},
Expand All @@ -279,7 +279,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
availablePollerIsolationGroups: nil,
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
client.EXPECT().GetListValue(
Expand Down Expand Up @@ -311,7 +311,7 @@ func TestAvailableIsolationGroupsHandler(t *testing.T) {
config: td.cfg,
metricsClient: metrics.NewNoopMetricsClient(),
}
res, err := handler.AvailableIsolationGroupsByDomainID(context.TODO(), "domain-id", td.availablePollerIsolationGroups)
res, err := handler.AvailableIsolationGroupsByDomainID(context.TODO(), "domain-id", "a-tasklist", td.availablePollerIsolationGroups)
assert.Equal(t, td.expected, res)
if td.expectedErr != nil {
assert.Equal(t, td.expectedErr.Error(), err.Error())
Expand Down Expand Up @@ -353,7 +353,7 @@ func TestIsDrainedHandler(t *testing.T) {
requestIsolationgroup: "zone-3", // no config specified for this
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
client.EXPECT().GetListValue(
Expand All @@ -372,7 +372,7 @@ func TestIsDrainedHandler(t *testing.T) {
requestIsolationgroup: "zone-2", // no config specified for this
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return true },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {
client.EXPECT().GetListValue(
Expand All @@ -391,7 +391,7 @@ func TestIsDrainedHandler(t *testing.T) {
requestIsolationgroup: "zone-2", // no config specified for this
cfg: defaultConfig{
IsolationGroupEnabled: func(string) bool { return false },
AllIsolationGroups: []string{"zone-1", "zone-2", "zone-3"},
AllIsolationGroups: func() []string { return []string{"zone-1", "zone-2", "zone-3"} },
},
dcAffordance: func(client *dynamicconfig.MockClient) {},
domainAffordance: func(mock *cache.MockDomainCache) {
Expand Down
4 changes: 2 additions & 2 deletions common/isolationgroup/defaultisolationgroupstate/types.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,6 @@ type isolationGroups struct {
type defaultConfig struct {
// IsolationGroupEnabled is a domain-based configuration value for whether this feature is enabled at all
IsolationGroupEnabled dynamicconfig.BoolPropertyFnWithDomainFilter
// AllIsolationGroups is a static list of all the possible isolation group names
AllIsolationGroups []string
// AllIsolationGroups is all the possible isolation-groups available for a region
AllIsolationGroups func() []string
}
2 changes: 1 addition & 1 deletion common/isolationgroup/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,5 +43,5 @@ type State interface {

// AvailableIsolationGroupsByDomainID returns the available isolation zones for a domain.
// Takes into account global and domain zones
AvailableIsolationGroupsByDomainID(ctx context.Context, domainID string, availableIsolationGroups []string) (types.IsolationGroupConfiguration, error)
AvailableIsolationGroupsByDomainID(ctx context.Context, domainID string, tasklistName string, availableIsolationGroups []string) (types.IsolationGroupConfiguration, error)
}
8 changes: 4 additions & 4 deletions common/isolationgroup/isolation_group_mock.go
View file

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

9 changes: 5 additions & 4 deletions common/partition/default-partitioner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func NewDefaultPartitioner(
}
}

func (r *defaultPartitioner) GetIsolationGroupByDomainID(ctx context.Context, domainID string, wfPartitionData PartitionConfig, availablePollerIsolationGroups []string) (string, error) {
func (r *defaultPartitioner) GetIsolationGroupByDomainID(ctx context.Context, pollerInfo PollerInfo, wfPartitionData PartitionConfig) (string, error) {
if wfPartitionData == nil {
return "", ErrInvalidPartitionConfig
}
Expand All @@ -81,7 +81,7 @@ func (r *defaultPartitioner) GetIsolationGroupByDomainID(ctx context.Context, do
return "", ErrInvalidPartitionConfig
}

available, err := r.isolationGroupState.AvailableIsolationGroupsByDomainID(ctx, domainID, availablePollerIsolationGroups)
available, err := r.isolationGroupState.AvailableIsolationGroupsByDomainID(ctx, pollerInfo.DomainID, pollerInfo.TasklistName, pollerInfo.AvailableIsolationGroups)
if err != nil {
return "", fmt.Errorf("failed to get available isolation groups: %w", err)
}
Expand All @@ -90,7 +90,7 @@ func (r *defaultPartitioner) GetIsolationGroupByDomainID(ctx context.Context, do
return "", ErrNoIsolationGroupsAvailable
}

ig := r.pickIsolationGroup(wfPartition, available)
ig := r.pickIsolationGroup(wfPartition, available, pollerInfo)
return ig, nil
}

Expand All @@ -105,14 +105,15 @@ func mapPartitionConfigToDefaultPartitionConfig(config PartitionConfig) defaultW

// picks an isolation group to run in. if the workflow was started there, it'll attempt to pin it, unless there is an explicit
// drain.
func (r *defaultPartitioner) pickIsolationGroup(wfPartition defaultWorkflowPartitionConfig, available types.IsolationGroupConfiguration) string {
func (r *defaultPartitioner) pickIsolationGroup(wfPartition defaultWorkflowPartitionConfig, available types.IsolationGroupConfiguration, pollerInfo PollerInfo) string {
_, isAvailable := available[wfPartition.WorkflowStartIsolationGroup]
if isAvailable {
return wfPartition.WorkflowStartIsolationGroup
}
r.log.Debug("isolation group falling back to any zone",
tag.IsolationGroup(wfPartition.WorkflowStartIsolationGroup),
tag.PollerGroupsConfiguration(available),
tag.WorkflowTaskListName(pollerInfo.TasklistName),
tag.WorkflowID(wfPartition.WFID),
)
return ""
Expand Down
15 changes: 10 additions & 5 deletions common/partition/default-partitioner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ func TestPickingAZone(t *testing.T) {
log: testlogger.New(t),
isolationGroupState: nil,
}
res := partitioner.pickIsolationGroup(td.wfPartitionCfg, td.availablePartitionGroups)
res := partitioner.pickIsolationGroup(td.wfPartitionCfg, td.availablePartitionGroups, PollerInfo{})
assert.Equal(t, td.expected, res)
})
}
Expand All @@ -103,6 +103,7 @@ func TestPickingAZone(t *testing.T) {
func TestDefaultPartitioner_GetIsolationGroupByDomainID(t *testing.T) {

domainID := "some-domain-id"
sampleTasklist := "a-tasklist"
validIsolationGroup := types.IsolationGroupConfiguration{
"zone-2": {
Name: "zone-2",
Expand All @@ -129,7 +130,7 @@ func TestDefaultPartitioner_GetIsolationGroupByDomainID(t *testing.T) {
},
incomingContext: context.Background(),
stateAffordance: func(state *isolationgroup.MockState) {
state.EXPECT().AvailableIsolationGroupsByDomainID(gomock.Any(), domainID, isolationGroups).Return(validIsolationGroup, nil)
state.EXPECT().AvailableIsolationGroupsByDomainID(gomock.Any(), domainID, sampleTasklist, isolationGroups).Return(validIsolationGroup, nil)
},
expectedValue: "zone-2",
},
Expand All @@ -140,7 +141,7 @@ func TestDefaultPartitioner_GetIsolationGroupByDomainID(t *testing.T) {
},
incomingContext: context.Background(),
stateAffordance: func(state *isolationgroup.MockState) {
state.EXPECT().AvailableIsolationGroupsByDomainID(gomock.Any(), domainID, isolationGroups).Return(validIsolationGroup, nil)
state.EXPECT().AvailableIsolationGroupsByDomainID(gomock.Any(), domainID, sampleTasklist, isolationGroups).Return(validIsolationGroup, nil)
},
expectedValue: "",
},
Expand All @@ -151,7 +152,7 @@ func TestDefaultPartitioner_GetIsolationGroupByDomainID(t *testing.T) {
},
incomingContext: context.Background(),
stateAffordance: func(state *isolationgroup.MockState) {
state.EXPECT().AvailableIsolationGroupsByDomainID(gomock.Any(), domainID, isolationGroups).Return(
state.EXPECT().AvailableIsolationGroupsByDomainID(gomock.Any(), domainID, sampleTasklist, isolationGroups).Return(
types.IsolationGroupConfiguration{}, nil)
},
expectedValue: "",
Expand Down Expand Up @@ -179,7 +180,11 @@ func TestDefaultPartitioner_GetIsolationGroupByDomainID(t *testing.T) {
ig := isolationgroup.NewMockState(ctrl)
td.stateAffordance(ig)
partitioner := NewDefaultPartitioner(testlogger.New(t), ig)
res, err := partitioner.GetIsolationGroupByDomainID(td.incomingContext, domainID, td.partitionKeyPassedIn, isolationGroups)
res, err := partitioner.GetIsolationGroupByDomainID(td.incomingContext, PollerInfo{
DomainID: domainID,
TasklistName: sampleTasklist,
AvailableIsolationGroups: isolationGroups,
}, td.partitionKeyPassedIn)

assert.Equal(t, td.expectedValue, res)
assert.Equal(t, td.expectedError, err)
Expand Down
11 changes: 10 additions & 1 deletion common/partition/interface.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,9 +26,18 @@ package partition

import "context"

// PollerInfo captures relevant information from the poller side
type PollerInfo struct {
DomainID string
TasklistName string
// The isolation groups that are known to have pollers in them and are able to receive tasks
// for this domain and tasklist.
AvailableIsolationGroups []string
}

type Partitioner interface {
// GetIsolationGroupByDomainID gets where the task workflow should be executing. Largely used by Matching
// when determining which isolationGroup to place the tasks in.
// Implementations ought to return (nil, nil) for when the feature is not enabled.
GetIsolationGroupByDomainID(ctx context.Context, DomainID string, partitionKey PartitionConfig, availableIsolationGroups []string) (string, error)
GetIsolationGroupByDomainID(ctx context.Context, pollerinfo PollerInfo, partitionKey PartitionConfig) (string, error)
}
8 changes: 4 additions & 4 deletions common/partition/partitioning_mock.go
View file

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

2 changes: 1 addition & 1 deletion common/resource/resource_test_utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -165,7 +165,7 @@ func NewTest(

partitionMock := partition.NewMockPartitioner(controller)
mockZone := "zone1"
partitionMock.EXPECT().GetIsolationGroupByDomainID(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(mockZone, nil)
partitionMock.EXPECT().GetIsolationGroupByDomainID(gomock.Any(), gomock.Any(), gomock.Any()).AnyTimes().Return(mockZone, nil)

scope := tally.NewTestScope("test", nil)

Expand Down
Loading

0 comments on commit 018fff9

Please sign in to comment.