targets/spiperf add Server FSM

[dmortondev]

This change is 

[dmortondev]

Some high-level comments:

• This is mostly a copy-paste-update of the client side.
• This introduces logging. Something that needs to be added to the client side.
• server_test.go has some dependencies on client_test.go w/r/t utility functions. Would it be more idiomatic to create test_utils.go and put them there?

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @djoyner)

---

targets/spiperf/fsm/server.go, line 12 at r1 (raw file):

	"github.com/Spirent/openperf/targets/spiperf/msg"
	"github.com/Spirent/openperf/targets/spiperf/openperf"
	"github.com/rs/zerolog"

When I checked out logrus the project page says it's now in maintenance mode. One of the next-gen packages they pointed to is zerolog. Giving it a try even though the performance isn't crucial for spiperf.

---

targets/spiperf/fsm/server.go, line 48 at r1 (raw file):

	// Logger log output facility
	Logger *zerolog.Logger

Decided to make this a public member so the caller of the FSM has more control over setting this up. One feature to look into is zerolog's support for sub-loggers that can have a prefix included in all their messages. The caller could create one of those sub-loggers, tell it to include "Server FSM" in its output, and the code here wouldn't need to know about any of that. It would just see an object to log to.

---

targets/spiperf/fsm/server.go, line 217 at r1 (raw file):

	cmd, cmdErr := s.waitForPeerCommand(msg.GetServerParametersType)
	if cmdErr != nil {

This construct determines if the error is fatal or non-fatal for the FSM. Right now it's two if's, one inside the other. To me this is more natural as a C++ dev. But I could see it as two sequential ifs:

if cmdErr != nil && cmd == nil{ //fatal}

if cmdErr != nil {//non-fatal but still error}

or a switch

switch {
case cmdErr != nil && cmd == nil:
// fatal

case cmdErr != nil:
//non-fatal

}

Not sure if reviewers have any guidance about the most idiomatic version?

[djoyner]

Re: the last question, you could factor these out, sure. But the file name should end in _test.go so that it doesn't get compiled outside of the unit test environment. You could put this code in fsm_suite_test.go.

Reviewed 2 of 5 files at r1.
Reviewable status: 2 of 5 files reviewed, 8 unresolved discussions (waiting on @djoyner and @dmorton-spirent)

---

targets/spiperf/fsm/server.go, line 162 at r1 (raw file):

		if !ok {
			s.Logger.Error().Msg("error reading from peer command channel.")
			return nil, &PeerError{What: "error reading from peer command channel."}

This strikes me as over-logging, especially considering that the owner of the FSM is likely to log the error returned by Run.

---

targets/spiperf/fsm/server.go, line 217 at r1 (raw file):

Previously, dmorton-spirent (Dan Morton) wrote…

This construct determines if the error is fatal or non-fatal for the FSM. Right now it's two if's, one inside the other. To me this is more natural as a C++ dev. But I could see it as two sequential ifs:

if cmdErr != nil && cmd == nil{ //fatal}

if cmdErr != nil {//non-fatal but still error}

or a switch

switch {
case cmdErr != nil && cmd == nil:
// fatal

case cmdErr != nil:
//non-fatal

}

Not sure if reviewers have any guidance about the most idiomatic version?

I'm not sure that any of these is more idiomatic than the others. The nested ifs are probably the most "normal" way most people would write it.

I think a bigger deal is that this construct is repeated multiple times, sometimes within the same method receiver.

---

targets/spiperf/fsm/server.go, line 304 at r1 (raw file):

	s.TestConfiguration.UpstreamRateBps = serverCfg.UpstreamRateBps
	s.TestConfiguration.DownstreamRateBps = serverCfg.DownstreamRateBps

Here the FSM is mutating the test config it was (presumably?) constructed with. Raises a couple of questions in my head:

1. Why mutate something that you were constructed with? Or, if it wasn't needed at construction-time, why expose this bit of internals?

2. Is the protocol implementation going to enforce single connection at a time behavior?

---

targets/spiperf/fsm/server.go, line 391 at r1 (raw file):

			Time("now", time.Now()).
			Dur("maximum delta", MaximumStartTimeDelta).
			Msg("requested start time is too far in the future")

I like this idea but would suggest that MaximumStartTimeDelta should be a command line parameter instead of a compile-time constant.

---

targets/spiperf/fsm/server.go, line 833 at r1 (raw file):

// on non-fatal error condition, returns <message, error>.
// on fatal error condition, returns <nil, error>.
func (s *Server) waitForPeerCommand(expectedType string) (*msg.Message, error) {

Is this different than the client's method receiver of the same name?

---

targets/spiperf/fsm/server.go, line 969 at r1 (raw file):

}

func (s *Server) startOpenperfCmdRepeater(ctx context.Context, cmd *openperf.Command, interval time.Duration) (responses chan interface{}, cancelFn context.CancelFunc) {

Likewise... any different from the client's version?