fix: Move game creator to Django #1798

faucomte97 · 2023-07-31T15:23:22Z

This PR replaces #1575 as the owner of the repo that PR originates from has been archived.

This change is

faucomte97

Reviewable status: 0 of 61 files reviewed, 2 unresolved discussions

aimmo/views.py line 124 at r1 (raw file):

        return Response(response)

    def destroy(self, request, *args, **kwargs):

Check if this is being used

aimmo/game_manager/game_manager.py line 73 at r1 (raw file):

            self.game_ingress_manager.add_game_path_to_ingress(game_name=game_name)
        except ApiException as e:
            # TODO: Filter out no-ingress exception on localhost

Look more into what's going on here

faucomte97

Reviewable status: 0 of 61 files reviewed, 4 unresolved discussions

aimmo/game_manager/game_manager.py line 26 at r1 (raw file):

    ) -> None:
        self.api: CoreV1Api = CoreV1Api()
        self.api_client: ApiClient = ApiClient()

Validate we're connecting to the client correctly

aimmo/game_manager/game_manager.py line 92 at r1 (raw file):

        return self.game_server_manager.delete_game_server(game_id=game_id)

    def recreate_game_server(self, game_id: int, game_data_updates: dict = None) -> None:

See if we can just update instead of recreating

faucomte97

Reviewable status: 0 of 61 files reviewed, 6 unresolved discussions

aimmo-game/Pipfile line 20 at r1 (raw file):

[packages]
#aimmo-game = {editable = true, path = "."}

Remove

aimmo-game/simulation/django_communicator.py line 7 at r1 (raw file):

import aiohttp

LOGGER = logging.getLogger(__name__)

Remove this

Code quote:

import aiohttp

LOGGER = logging.getLogger(__name__)

SKairinos

Reviewed 24 of 61 files at r1, all commit messages.
Reviewable status: 24 of 61 files reviewed, 17 unresolved discussions (waiting on @faucomte97)

aimmo/models.py line 177 at r1 (raw file):

            game_manager = GameManager()
            game_manager.create_game_secret(game_id=self.id, token=self.auth_token)
            game_manager.create_game_server(game_id=self.id, game_data=GameSerializer(self).data)

this feels redundant. why serialize data we already have? why not just pass the data in directly?

aimmo/models.py line 202 at r1 (raw file):

class GameSerializer(serializers.Serializer):

I'm not sure this serializer is needed but if it were, why not make it a ModelSerializer since where serializing the Game model?

aimmo/views.py line 124 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Check if this is being used

let me know about this. if the game server is deleted here, then where/how are game servers deleted?

aimmo/views.py line 173 at r1 (raw file):

                # Re-raise exception so that atomic transaction reverts
                raise exception

besides aborting the db transaction, do we have any graceful feedback or handling of this exception?

aimmo/game_manager/game_manager.py line 26 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Validate we're connecting to the client correctly

how does our Django App Engine instance connect/authenticate with our GKE instance?

aimmo/game_manager/game_manager.py line 43 at r1 (raw file):

        return f"game-{game_id}"

    def create_game_secret(self, game_id: int, token: str):

why do we need to create a game secret?

aimmo/game_manager/game_manager.py line 73 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Look more into what's going on here

this code seems dangerous. it could make it possible to create a gamer server + service without an ingress path, meaning the game will be non-contactable. Also, if there's no ingress on localhost, how are requests getting routed locally?

aimmo/game_manager/game_manager.py line 89 at r1 (raw file):

        except ApiException as e:
            LOGGER.error(e)
        self.game_service_manager.delete_game_service(game_id=game_id)

defensive programming - if an exception is raised when deleting the service, the server will not be deleted and remain orphaned. we need to be careful about orphaned instances in our system.

aimmo/game_manager/game_manager.py line 92 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

See if we can just update instead of recreating

I think the confusion stems from how the system is modelled. Conceptually, we're not recreating the game, we're creating a new game. But technically, the way we've implemented it, we recycle game ids (which is conceptually wrong). Let's discuss this further and see if you agree.

aimmo/game_manager/game_secret_manager.py line 12 at r1 (raw file):

class GameSecretManager:

did we confirm if this is needed? if this is to ensure secure communication with our game servers, couldn't this just be achieved with CORS in our ingress?

aimmo/game_manager/game_server_manager.py line 39 at r1 (raw file):

                            "game-id": str(game_id),
                        },
                        "annotations": game_data,

why is this annotation needed? was it meant to be an environment variable?

aimmo/game_manager/game_server_manager.py line 55 at r1 (raw file):

            return result["status"]["gameServerName"]

    def delete_game_server(self, game_id: int) -> dict:

seems we have a few functions that delete game servers. can you help me understand the need for each on a call?

aimmo/game_manager/game_service_manager.py line 45 at r1 (raw file):

        for resource in resources.items:
            LOGGER.info("Removing service: {}".format(resource.metadata.name))
            self.api.delete_namespaced_service(resource.metadata.name, K8S_NAMESPACE)

defensive programming - wrap in a try/except and log any exceptions

aimmo/tests/test_game_manager.py line 37 at r1 (raw file):

def test_create_game_server(game_manager: GameManager, game_id, game_data):

what's the value of these tests? I say this because:

we're not making assertions
we're not testing our code/functionality? Seems we're testing the k8s client more.

aimmo/tests/test_middleware.py line 84 at r1 (raw file):

        assert Game.objects.all().count() == 2

    @patch("aimmo.views.GameManager")

why mock this if we're not doing anything with it?

SKairinos

Reviewed 27 of 61 files at r1.
Reviewable status: 51 of 61 files reviewed, 17 unresolved discussions (waiting on @faucomte97)

aimmo-game/Pipfile line 20 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove

just a reminder

aimmo-game/simulation/django_communicator.py line 7 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Remove this

reminder

faucomte97

Reviewable status: 51 of 61 files reviewed, 16 unresolved discussions (waiting on @SKairinos)

aimmo/models.py line 177 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

this feels redundant. why serialize data we already have? why not just pass the data in directly?

Done.

aimmo/views.py line 124 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

let me know about this. if the game server is deleted here, then where/how are game servers deleted?

Delete this endpoint

aimmo/views.py line 173 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

besides aborting the db transaction, do we have any graceful feedback or handling of this exception?

Done.

aimmo/game_manager/game_manager.py line 43 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

why do we need to create a game secret?

Add a TODO for the improved solution

faucomte97

Reviewable status: 51 of 61 files reviewed, 14 unresolved discussions (waiting on @faucomte97 and @SKairinos)

aimmo/models.py line 202 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

I'm not sure this serializer is needed but if it were, why not make it a ModelSerializer since where serializing the Game model?

Added a TODO to replace with ModelSerializer.

aimmo/game_manager/game_manager.py line 43 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Add a TODO for the improved solution

Done.

aimmo/game_manager/game_manager.py line 73 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

this code seems dangerous. it could make it possible to create a gamer server + service without an ingress path, meaning the game will be non-contactable. Also, if there's no ingress on localhost, how are requests getting routed locally?

Dun

aimmo/game_manager/game_manager.py line 89 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

defensive programming - if an exception is raised when deleting the service, the server will not be deleted and remain orphaned. we need to be careful about orphaned instances in our system.

no

aimmo/game_manager/game_manager.py line 92 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

I think the confusion stems from how the system is modelled. Conceptually, we're not recreating the game, we're creating a new game. But technically, the way we've implemented it, we recycle game ids (which is conceptually wrong). Let's discuss this further and see if you agree.

no

aimmo/game_manager/game_secret_manager.py line 12 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

did we confirm if this is needed? if this is to ensure secure communication with our game servers, couldn't this just be achieved with CORS in our ingress?

no

aimmo/game_manager/game_server_manager.py line 39 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

why is this annotation needed? was it meant to be an environment variable?

no

aimmo/game_manager/game_server_manager.py line 55 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

seems we have a few functions that delete game servers. can you help me understand the need for each on a call?

no

aimmo/game_manager/game_service_manager.py line 45 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

defensive programming - wrap in a try/except and log any exceptions

Done.

aimmo/tests/test_game_manager.py line 37 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

what's the value of these tests? I say this because:

we're not making assertions

we're not testing our code/functionality? Seems we're testing the k8s client more.

It seems like it just checks that the 3 subcalls of the create_game_server function are being called properly, I guess to make sure that the K8s calls are executed fine? 😕

aimmo/tests/test_middleware.py line 84 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

why mock this if we're not doing anything with it?

It is used, by the save() function which calls the GameManager to create the game servers.

aimmo-game/Pipfile line 20 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

just a reminder

Done.

aimmo-game/simulation/django_communicator.py line 7 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

reminder

Done.

SKairinos

Reviewed 5 of 61 files at r1, 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faucomte97)

aimmo/views.py line 124 at r1 (raw file):

Previously, faucomte97 (Florian Aucomte) wrote…

Delete this endpoint

Are you going to delete it now or are do you want to just add a TODO for now?

SKairinos

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @faucomte97)

faucomte97

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @SKairinos)

aimmo/views.py line 124 at r1 (raw file):

Previously, SKairinos (Stefan Kairinos) wrote…

Are you going to delete it now or are do you want to just add a TODO for now?

Dun now

SKairinos

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @faucomte97)

codecov · 2023-10-13T18:47:23Z

Codecov Report

Merging #1798 (d5e7661) into master (a7574d4) will increase coverage by 4.19%.
Report is 110 commits behind head on master.
The diff coverage is 84.84%.

@@            Coverage Diff             @@
##           master    #1798      +/-   ##
==========================================
+ Coverage   66.32%   70.51%   +4.19%     
==========================================
  Files         179      189      +10     
  Lines        3661     3938     +277     
  Branches      255      272      +17     
==========================================
+ Hits         2428     2777     +349     
+ Misses       1201     1132      -69     
+ Partials       32       29       -3

Files	Coverage Δ
aimmo-game-worker/simulation/utils.py	`100.00% <100.00%> (ø)`
aimmo/__init__.py	`100.00% <100.00%> (ø)`
aimmo/app_settings.py	`100.00% <100.00%> (ø)`
aimmo/exceptions.py	`100.00% <100.00%> (ø)`
aimmo/game_manager/__init__.py	`100.00% <100.00%> (ø)`
aimmo/game_manager/constants.py	`100.00% <100.00%> (ø)`
aimmo/game_manager/game_secret_manager.py	`100.00% <100.00%> (ø)`
aimmo/middleware/game_limit_exceeded.py	`100.00% <100.00%> (ø)`
aimmo/migrations/0003_auto_20160802_1418.py	`48.27% <ø> (ø)`
aimmo/migrations/0025_generate_auth_token.py	`100.00% <100.00%> (ø)`
... and 42 more

... and 19 files with indirect coverage changes

faucomte97

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @faucomte97)

faucomte97 added 2 commits July 21, 2023 15:27

Move game creator logic to Django

c7455fa

Merge master

0e6be24

faucomte97 self-assigned this Jul 31, 2023

faucomte97 mentioned this pull request Jul 31, 2023

fix: Remove game-creator and move relevant logic into the aimmo django app #1575

Closed

4 tasks

faucomte97 added 18 commits August 16, 2023 16:36

Merge master

4d2ef3a

Finish merging conflicts

5660520

Move serializers to models.py

4535cf4

Fix tests

379dac3

Fix tests again

e61d5c4

Fix tests again again

9b69682

Clean pipfiles and dependencies

f5db464

Clean up, increase local fleet to 15 and reduce retry from 60 to 10

0aabc02

Try adding back pytest as test package for ci

633b864

Add back all dev packages to setup.py

c269cbd

Test removing the patch method (not needed?)

f48717d

Clean tests

c30a2c0

Merge conflicts

002f410

Try using prints instead of logger

00fdd45

Remove final logger

5061a8b

Replace views loggers with print

88fe73e

Recreate game service on worksheet change

b470dbb

Cleanup

9957a0f

faucomte97 marked this pull request as ready for review September 4, 2023 11:57

faucomte97 commented Sep 12, 2023

View reviewed changes

SKairinos suggested changes Sep 20, 2023

View reviewed changes

SKairinos reviewed Sep 20, 2023

View reviewed changes

faucomte97 commented Sep 26, 2023

View reviewed changes

faucomte97 commented Oct 3, 2023

View reviewed changes

Feedback

7802ec3

SKairinos approved these changes Oct 9, 2023

View reviewed changes

SKairinos suggested changes Oct 9, 2023

View reviewed changes

Delete unused endpoint

642e904

faucomte97 commented Oct 12, 2023

View reviewed changes

SKairinos approved these changes Oct 13, 2023

View reviewed changes

faucomte97 added 2 commits October 13, 2023 17:56

Fix test

23f6ff7

Add URL log

d5e7661

faucomte97 commented Oct 16, 2023

View reviewed changes

faucomte97 merged commit 7c2fe3e into master Oct 16, 2023
7 of 8 checks passed

faucomte97 deleted the remove_game_creator_new branch October 16, 2023 15:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Move game creator to Django #1798

fix: Move game creator to Django #1798

faucomte97 commented Jul 31, 2023 •

edited

Loading

faucomte97 left a comment

faucomte97 left a comment

faucomte97 left a comment

SKairinos left a comment

SKairinos left a comment

faucomte97 left a comment

faucomte97 left a comment

SKairinos left a comment

SKairinos left a comment

faucomte97 left a comment

SKairinos left a comment

codecov bot commented Oct 13, 2023

faucomte97 left a comment

fix: Move game creator to Django #1798

fix: Move game creator to Django #1798

Conversation

faucomte97 commented Jul 31, 2023 • edited Loading

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

faucomte97 left a comment

Choose a reason for hiding this comment

SKairinos left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 13, 2023

Codecov Report

faucomte97 left a comment

Choose a reason for hiding this comment

faucomte97 commented Jul 31, 2023 •

edited

Loading