Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add a second master server, rework the defines #112

Merged
merged 10 commits into from Jul 11, 2018

Conversation

Projects
None yet
3 participants
@illwieckz
Copy link
Member

commented Jun 20, 2018

  • add a second master server
  • retrieve motd from first working master server
  • extend the globalservers command to query every master server at once
  • always resolve master server to not have to restart servers when master ip changes
  • deduplicate server lists from multiple master servers
  • drop “send gamestats to master server” support, see Unvanquished/Unvanquished#1083
add a second master server, rework the defines
- add a second master server
- motd and gamestat server can be something else than first one
  using specific defines makes code easier to read
- defines all masters at the same place, even empty ones

@illwieckz illwieckz changed the title add a second master server, motd and gamestat server can be something else add a second master server, rework the defines Jun 20, 2018

@illwieckz illwieckz force-pushed the illwieckz:masterserver2 branch from 6e356a1 to 8ddb2d0 Jun 20, 2018

@slipher

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

LGTM

I believe that this will cause servers to report to both masters, but that the client lacks any logic for trying alternative servers. The user would still have to type a command to get servers from a master other than the 1st one.

#define MASTER3_SERVER_NAME ""
#define MASTER4_SERVER_NAME ""
#define MASTER5_SERVER_NAME ""
#define MOTD_MASTER_SERVER_NAME MASTER1_SERVER_NAME

This comment has been minimized.

Copy link
@DolceTriade

DolceTriade Jun 20, 2018

Contributor

Doesn't this mean that if master 1 goes down, these two won't work. I'd rather move back to a cvar scheme...

This comment has been minimized.

Copy link
@illwieckz

illwieckz Jun 20, 2018

Author Member

This was already a case and worst than that, it was using the hardcoded first master server even if the first master server cvar was updated. There was a similar behavior for the gamestat sending (sending to the first hardcoded master server only). See my new commits attempting to fix that.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

I updated the PR to make the server sending gamestat to every master server and retrieving motd from first working master server.

@DolceTriade
Copy link
Contributor

left a comment

For now, since as slipher notes, this mainly only affects the servers, I think this is ok. however, when we do the client side implementation, we definitely need to make it smarter to be able to detect that a specific master server is down and be able to apply necessary fallback logic to try other master servers.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

A better client implementation would be to merge the server lists (like the XQF server browser does). Basically we would have to add to the internal server list any server found in a list from a master server that was not already added to the internal list before.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

Extending the globalservers command to allow something like that would be good:

globalservers * 86 full empty
@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

I just added a commit to do that btw I don't know yet where we have to deduplicate the received lists.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 20, 2018

If I'm reading things the right way there is already some deduplication for both packets and addresses:

for ( i = 0; i < numservers; ++i )
{
if ( !strcmp( s, NET_AdrToStringwPort( addresses[ i ] ) ) )
{
// found: replace with the preferred address, exit both loops
addresses[ i ] = cls.serverLinks[ j ];
addresses[ i ].type = NET_TYPE( cls.serverLinks[ j ].type );
addresses[ i ].port = ( addresses[ i ].type == netadrtype_t::NA_IP ) ? cls.serverLinks[ j ].port4 : cls.serverLinks[ j ].port6;
duplicate = true;
break;
}
}

enable globalservers command to query every master server
use it this way:

```
globalservers * 86 full empty
```

@illwieckz illwieckz force-pushed the illwieckz:masterserver2 branch from 5e0a3ff to baf2869 Jun 21, 2018

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

Also, if I read it right if two master servers reply the same list of servers in the same order a false-positive warning about duplicate packet can occurs. This is probably OK.

@DolceTriade

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2018

Well, that would happen everytime someone fetched a list, so it probably not the best idea, but we can fix that when we deal with the client code.

illwieckz added a commit to illwieckz/Unvanquished that referenced this pull request Jun 21, 2018

@slipher

This comment has been minimized.

Copy link
Member

commented Jun 23, 2018

What is MOTD, is it even used ever? Can we delete it?

@slipher

This comment has been minimized.

Copy link
Member

commented Jun 23, 2018

The global stats thing seems deletable too. It's a fairly pointless idea since it is so easy to vandalize.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2018

is it even used ever? Can we delete it?

I like this kind of question! 😁

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2018

what is globalstats for? what does it do?

@slipher

This comment has been minimized.

Copy link
Member

commented Jun 23, 2018

The game stats thing is mostly just the same information as the server status, plus which team won the game. So I guess gamestats was added in Tremulous for making the statistics on global humans win vs. aliens vs. draw rate.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2018

I see, a kind of cheap and unreliable http://stats.xonotic.org/ ?
In this case I'm OK to drop it: we have to do something better in either way

@slipher

This comment has been minimized.

Copy link
Member

commented Jun 23, 2018

We used to have something like that for the official server (with stats per player per weapon, etc.). Based on just parsing the game log I guess? Anyway, the information sent to the global stats server is very minimal - you only get a couple graphs like average number of players or team win percentages. Probably not too meaningful at the moment since most games are played by bots.

@illwieckz illwieckz force-pushed the illwieckz:masterserver2 branch from 4d97963 to aa5c5ce Jun 30, 2018

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2018

I removed the Windows 95 workaround for name resolution since we'll never support this OS, hence the master servers are now resolved when needed. It fixes Unvanquished/Unvanquished#1082.

That workaround was to avoid hitches on Windows 95 platform, by never resolving a master server again, but we want to resolve master servers again.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jun 30, 2018

I discovered that

  • server deduplication only worked on already known servers, not on multiple responses from server
  • packet deduplication was just deduplicating “n-th packet from any master server” whatever the content, meaning the first packet from first master server will make the engine drops the first packets from all other masters…

So I removed that useless packet deduplication and added some code to deduplicate servers themselve.

@slipher

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

You can remove the "send gamestat every master server" commit since we are deleting gamestats now. Also I believe we can delete master server MOTD as it is never used (not to be confused with server MOTD, which is used).

@slipher

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

Why should we care about detecting duplicate master servers? I don't see the point.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2018

Exactly, the former code was to detect duplicate master servers (by detecting duplicate master server replies), and there was no point about that. It just meant that even if you queried 5 master servers, only the faster master server to reply had his answer parsed… Since our master server is up again I was able to test our code with two master servers with different server lists, and the in-game server browser just displayed randomly the list from one master or the list from the other one, never giving the user a server list that merges both lists.

The new code parses all the packets received by any master server, but deduplicates the server list. And with that code we don't have to take care about duplicate packets.

The only case that is not handled (yet?) is if a poorly implemented master server sends multiple time the same server in one packet, in this case the server will appears two time in the list since the current deduplication deduplicates against the comprehensive list from previous packets, not the current one.

(hint, if I say “server” and not “master server” neither just “master” I talk about a “game server“, i.e. daemonded).

@slipher

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

Silly me, somehow I thought it was about comparing master server addresses.

char label[ MAX_FEATLABEL_CHARS ] = "";

Log::Debug( "CL_ServersResponsePacket" );

duplicate_count = 0;

This comment has been minimized.

Copy link
@slipher

slipher Jul 1, 2018

Member

Prefer to declare and initialize the variable on one line (int duplicate_count = 0;)

@DolceTriade

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2018

Master MOTD is not used because it wasn't carried over somehow when we redid our UI with librocket (whoops...). It's still set on the master server. I was going to use it to tell people to upgrade unv lol

illwieckz added a commit to illwieckz/Unvanquished that referenced this pull request Jul 1, 2018

@illwieckz illwieckz force-pushed the illwieckz:masterserver2 branch from 9b012c2 to e574262 Jul 1, 2018

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2018

This PR now drops support for gamestats. See Unvanquished/Unvanquished#1083

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jul 7, 2018

@DolceTriade you initially approved this PR but I added a lot of things since. Can you review it again?

Do we drop MOTD or not?

@DolceTriade

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2018

ya, we drop it.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

I added a commit to drop motd support.

To announce new versions it would be better to have a code that compares the current running version number with the one from servers in server browser and when connecting to them, something saying “hey, this server is in the future, do you wanna jump forward???

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

Oh and btw, the GetNews thing is more a game thing than an engine one.

@illwieckz illwieckz merged commit d6dd683 into DaemonEngine:for-0.51.0 Jul 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@DolceTriade

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

It's an engine thing because with QVMs, the game had no way to fetch external data. However, now that we can compile curl directly into the game/cgame, it can be a game thing.

@illwieckz

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.