MM-50808: Add versions to actions #565

agarciamontoro · 2023-03-01T09:49:38Z

Summary

When running newer versions of the load-test against older versions of Mattermost, many of the actions that the agents perform are not implemented in the server, which artificially increases the rate of errors.

Fortunately, the userAction struct in simulcontroller already had this logic implemented: setting the minServerVersion in the actions, they are performed only when the server's version is greater than or equal to it.

Unfortunately, none of the actions had this field set, so all actions were running for all versions.

This PR adds the minimum server version to each action. Some comments:

The basic actions (create post, view channel, change team) have the lowest version accepted by the load-test.
The lowest version accepted by the load-test is defined to be 4.0.0, since all requests are based in API v4.

I've made a couple of additional changes:

The type of minServerVersion is no longer string, but semver.Version. We were parsing these strings every time we ran an action, so I decided to use semver.MustParse once when we initialize the versions. Then we can skip all additional parsings, which reduces the number of errors we can get.
The same was done to the simulcontroller.serverVersion. It is parsed once when it is initialized (this time it can error, we cannot do a semver.MustParse since it's an external value), and used as a semver.Version elsewhere.
The simulcontroller now checks at the beginning whether the server version is at least 4.0.0. Just some defensive code in case someone tries to load-test a very old server.
The actions are now filtered by their version once before running the agent's loop, instead of checking them every time they were picked (which I believe slightly altered the predefined weights when the actions were skipped).
Tests were updated to reflect the changes. In particular, I had to overwrite the server version in some of them so that the simulcontroller could initialize correctly.

I recommend you review by commit, it will be easier to understand the changes, and the messages should be self-explanatory.

I'm gonna start a test with older versions and see how this behaves.

Ticket Link

https://mattermost.atlassian.net/browse/MM-50808

streamer45

Looks good 👍

streamer45 · 2023-03-02T19:56:56Z

loadtest/control/simulcontroller/controller.go

-			frequency: 4,
+			run:              switchChannel,
+			frequency:        4,
+			minServerVersion: initialVersion,


I guess we could have defaulted to no/empty version meaning initialVersion to avoid adding this everywhere, unless we really want to be explicit.

streamer45 · 2023-03-02T20:01:27Z

loadtest/control/simulcontroller/controller.go

@@ -54,6 +55,9 @@ func New(id int, user user.User, config *Config, status chan<- control.UserStatu
 	}, nil
 }

+// All requests are based in API v4, so the absolute minimum version required is 4.0.0
+var initialVersion = semver.MustParse("4.0.0")


Not very strong opinions on this but I am not sure it would make a lot of sense to support anything prior to v5. This repository was created around 5.17 time and I don't believe it was ever executed against previous versions.

Right, and if we are not going to test again a certain version. I think it's just better to restrict.

streamer45 · 2023-03-02T20:02:26Z

loadtest/store/memstore/store.go

@@ -980,7 +980,7 @@ func (s *MemStore) ServerVersion() (string, error) {
 	return s.serverVersion, nil
 }

-// SetProfileImage sets as stored the profile image for the given user.


I was good at copy pasting 🤦‍♂️

isacikgoz

LGTM 👍

agarciamontoro · 2023-03-17T12:58:40Z

Running a smoke-test in Postgres. I'll merge as soon as it finishes.

agarciamontoro · 2023-03-27T18:14:03Z

Status update on this: I did run the tests, but I had so many errors that the server ended up crashing. With the rest of the work going on, I didn't have a chance to properly debug it, so this is paused until I have some time to find the root cause (which I suspect is related to version-specific code inside some actions that, although cover old endpoints, should only be executed in more modern versions).

agarciamontoro · 2023-07-14T16:33:56Z

I'm thinking that we should aggressively simplify this PR. Following the decision on https://mattermost.atlassian.net/browse/MM-51011, where we avoided a specific change that would allow supporting versions prior to 6.0 because that version was out of support, we could set the minimum version for all actions to 6.3 (which is also out of support at this point, but still used by some customers). That version is known to work with the tool, and if there are some quirks to solve, they'll be more approachable than issues with 5.x versions.

Thoughts, @agnivade, @streamer45?

streamer45 · 2023-07-14T17:26:38Z

Make sense, also not opposed to support from the last supported ESR onwards.

agnivade · 2023-07-17T04:16:28Z

Yep agree. If customers want to run tests with older MM versions, they can use older releases. There is no need to maintain backwards incompatibility beyond the last ESR.

agarciamontoro · 2024-04-22T17:39:43Z

Rebased this on top of master to get it through the finish line, a year and a month later! It's basically the same as before, so the description in the PR still holds, except for the minimum supported version, which is, by definition, the third-to-last ESR. So, for now, 7.8, according to https://docs.mattermost.com/upgrade/extended-support-release.html#what-are-the-current-supported-extended-support-release-versions. Let me know if you need any more context!

isacikgoz

Looks good overall, a couple of comments.

isacikgoz · 2024-04-26T15:02:04Z

loadtest/control/actions.go

@@ -960,16 +961,16 @@ func ReloadGQL(u user.User) UserActionResponse {
 			}
 		}

-		ver, err := u.Store().ServerVersion()
+		serverVersionString, err := u.Store().ServerVersion()


Do we still support graphql?

isacikgoz · 2024-04-26T15:04:50Z

loadtest/control/simulcontroller/controller_test.go

-		WebSocketURL: "ws://localhost:8065",
-	})
+func TestNew(t *testing.T) {
+	c, _ := newController(t)


what if we just close the chan after this :p

streamer45 · 2024-05-23T23:20:14Z

@agarciamontoro Given the above, do we still need all these checks against version 6.4.0?

agarciamontoro added the 2: Dev Review Requires review by a core committer label Mar 1, 2023

agarciamontoro requested review from streamer45 and isacikgoz March 1, 2023 09:49

streamer45 approved these changes Mar 2, 2023

View reviewed changes

isacikgoz approved these changes Mar 3, 2023

View reviewed changes

agarciamontoro added 3 commits April 22, 2024 19:27

Set ServerVersion in tests to MinSupportedVersion

d3722b6

Fix typo

4039c28

Use semver instead of plain strings

9c392f9

agarciamontoro force-pushed the MM-50808.add-versions-to-actions branch from f9cf8cf to 899aa15 Compare April 22, 2024 17:34

agarciamontoro requested review from streamer45 and isacikgoz April 22, 2024 17:35

Add minServerVersion to all actions

baa5327

agarciamontoro force-pushed the MM-50808.add-versions-to-actions branch from 899aa15 to baa5327 Compare April 22, 2024 17:38

isacikgoz reviewed Apr 26, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MM-50808: Add versions to actions #565

MM-50808: Add versions to actions #565

agarciamontoro commented Mar 1, 2023

streamer45 left a comment

streamer45 Mar 2, 2023

streamer45 Mar 2, 2023

isacikgoz Mar 3, 2023

streamer45 Mar 2, 2023

isacikgoz left a comment

agarciamontoro commented Mar 17, 2023

agarciamontoro commented Mar 27, 2023

agarciamontoro commented Jul 14, 2023

streamer45 commented Jul 14, 2023

agnivade commented Jul 17, 2023

agarciamontoro commented Apr 22, 2024 •

edited

isacikgoz left a comment

isacikgoz Apr 26, 2024

isacikgoz Apr 26, 2024

streamer45 commented May 23, 2024

MM-50808: Add versions to actions #565

Are you sure you want to change the base?

MM-50808: Add versions to actions #565

Conversation

agarciamontoro commented Mar 1, 2023

Summary

Ticket Link

streamer45 left a comment

Choose a reason for hiding this comment

streamer45 Mar 2, 2023

Choose a reason for hiding this comment

streamer45 Mar 2, 2023

Choose a reason for hiding this comment

isacikgoz Mar 3, 2023

Choose a reason for hiding this comment

streamer45 Mar 2, 2023

Choose a reason for hiding this comment

isacikgoz left a comment

Choose a reason for hiding this comment

agarciamontoro commented Mar 17, 2023

agarciamontoro commented Mar 27, 2023

agarciamontoro commented Jul 14, 2023

streamer45 commented Jul 14, 2023

agnivade commented Jul 17, 2023

agarciamontoro commented Apr 22, 2024 • edited

isacikgoz left a comment

Choose a reason for hiding this comment

isacikgoz Apr 26, 2024

Choose a reason for hiding this comment

isacikgoz Apr 26, 2024

Choose a reason for hiding this comment

streamer45 commented May 23, 2024

agarciamontoro commented Apr 22, 2024 •

edited