Replace FSM’s old registry pattern with newly suggested pattern #14266
Conversation
|
@kisunji |
Hey! If you rebase from edit: spoke too soon! #14257 should fix the issue. Until then you can ignore the integration test failures; we can work around it |
|
Ahh ok, that makes a lot more sense. Thanks! I also just realized that I opened this PR with my dev branch instead of the proper branch |
That's totally fine. Is this ready for review? It will need some enterprise-side changes on our end but looks simple enough |
|
yup it should be ready for review! |
Mathew-Estafanous
force-pushed
the
mathew.estafanous/dev
branch
from
1aa00f4
to
0158a67
Compare
agent/consul/fsm/commands_oss.go
Outdated
| return commands | ||
| } | ||
|
|
||
| func registerEnterpriseCommands(registry map[structs.MessageType]command, fsm *FSM) {} |
There was a problem hiding this comment.
Could you move most of this file to a new file called commands.go and leave the empty registerEnterpriseCommands in commands_oss.go with this directive at the top:
//go:build !consulent
On the enterprise side we have another file called commands_ent.go which will have the enterprise implementation.
agent/consul/fsm/snapshot_oss.go
Outdated
| } | ||
| return nil | ||
| } | ||
| func RegisteredRestorer() map[structs.MessageType]restorer { |
There was a problem hiding this comment.
same comment as commands_oss.go. Could the common code go to snapshot.go, leaving only registerEnterpriseRestorers in the oss-version?
There was a problem hiding this comment.
do you need registerEnterprisePersisters or is it only for Restorers?
There was a problem hiding this comment.
Also should I add
//go:build !consulent
to the top of snapshot_oss.go as well or is it just for commands_oss.go?
There was a problem hiding this comment.
Yeah please add it! We would want registerEnterprisePersisters as well.
Mathew-Estafanous
force-pushed
the
mathew.estafanous/dev
branch
2 times, most recently
from
dfffe30
to
8c22485
Compare
Mathew-Estafanous
force-pushed
the
mathew.estafanous/dev
branch
from
8c22485
to
58b3eba
Compare
agent/consul/fsm/snapshot.go
Outdated
| @@ -104,3 +83,934 @@ func (s *snapshot) Persist(sink raft.SnapshotSink) error { | |||
| func (s *snapshot) Release() { | |||
| s.state.Close() | |||
| } | |||
|
|
|||
| func RegisteredRestorer() map[structs.MessageType]restorer { | |||
| registry := map[structs.MessageType]restorer{ | |||
There was a problem hiding this comment.
One last feedback I found while working on the enterprise side:
We actually lost the "this is already registered" check on this registry map.
It's not a big deal on OSS since the compiler natively checks for duplicate keys but in ENT I need to do something like:
func registerEnterpriseRestorers(m map[structs.MessageType]restorer) {
m[structs.EnterpriseType1] = restoreEnterpriseType1
m[structs.EnterpriseType2] = restoreEnterpriseType2
m[structs.EnterpriseType3] = restoreEnterpriseType3
}which can't check for overwriting keys.
Do you think we could make a registerRestorer func similar to what you did in commands.go and add some validation?
I could simply add it to the enterprise side only but it would be nice to have the OSS and ENT versions both call common code.
There was a problem hiding this comment.
Just pushed those changes.
I also thought that it might be better to make a small(ish) change to command.go by removing the named struct (entry) and just making it an anonymous struct. I thought that it would prevent misuse or confusion, since the name is pretty vague by itself.
kisunji
added
pr/no-changelog
Mathew-Estafanous
force-pushed
the
mathew.estafanous/dev
branch
from
684d34d
to
7a86e6a
Compare
agent/consul/fsm/snapshot.go
Outdated
| s.persistPeeringTrustBundles, | ||
| s.persistPeeringSecrets, | ||
| } | ||
| registerEnterprisePersisters(registry, s) |
There was a problem hiding this comment.
Found another issue while testing this. Unlike maps, slices are passed as copies so registerEnterprisePersisters cannot actually add persisters to the registry.
Can we change the signature so it simply returns a slice of only the enterprise persisters?
Description
As suggested by the linked issue, I transitioned to the new pattern for the FSM operations and snapshot registries. I wasn’t sure if any of the registries needed enterprise mapping as well, so I went ahead and created a noop function for each registry where enterprise mappings can be added.
Links
Working on issue #11740
This PR is a continuation of a previously merged PR, #12729