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

Reserved network locations for Studio and KDP #12703

Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
41b0989
Remove dynamic field and its setter after migrating to field location…
LianaHarris360 Oct 2, 2024
8b4b872
Update task helper to prevent enqueuing new tasks when is_local is False
LianaHarris360 Oct 2, 2024
9704b4a
Use hardcoded ID to create reserved network locations for KDP
LianaHarris360 Oct 2, 2024
8a96989
Update reset_connection_states to handle reserved locations for Studi…
LianaHarris360 Oct 2, 2024
b9eb839
Update NetworkLocationSerializer to only allow static models to be cr…
LianaHarris360 Oct 3, 2024
9976dd6
Annotate dynamic attribute when location_type is dynamic within Netwo…
LianaHarris360 Oct 3, 2024
5ffd66c
Add syncable filter in NetworkLocationViewSet to include KDP and Stud…
LianaHarris360 Oct 3, 2024
5698dc9
Change DATA_PORTAL_INSTANCE_ID to DATA_PORTAL_BASE_INSTANCE_ID
LianaHarris360 Oct 3, 2024
4b785f0
Add test for enqueue_network_location_update_with_backoff when not_lo…
LianaHarris360 Oct 3, 2024
c1a5767
Remove use of dynamic field for NetworkLocation model
LianaHarris360 Oct 4, 2024
d51a97d
Remove filtering for KDP network locations before enqueuing
LianaHarris360 Oct 22, 2024
c4830a9
Enforce NetworkLocationSerializer to use StaticNetworkLocation model
LianaHarris360 Oct 22, 2024
7d0de1b
Add correct KDP instance_id
LianaHarris360 Oct 22, 2024
5dcb456
Return dynamic locations in addition to static ones
LianaHarris360 Oct 22, 2024
4969474
Remove duplicate KDP application setting
LianaHarris360 Nov 13, 2024
2d2b5af
Remove annotated dynamic attribute
LianaHarris360 Nov 13, 2024
9aa66e1
Refactor NetworkLocationViewSet to return dynamic locations
LianaHarris360 Nov 19, 2024
a425e4f
Remove NetworkLocationViewSet get_queryset redundancy
LianaHarris360 Nov 19, 2024
b97e1d3
Add API test cases for returned static, dynamic, and reserved network…
LianaHarris360 Nov 20, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions kolibri/core/discovery/api.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
from django.db.models import Q
from django_filters.rest_framework import DjangoFilterBackend
from rest_framework import decorators
from rest_framework import viewsets
Expand All @@ -20,6 +21,8 @@
from kolibri.core.api import BaseValuesViewset
from kolibri.core.api import ValuesViewset
from kolibri.core.device.permissions import NotProvisionedHasPermission
from kolibri.core.discovery.well_known import CENTRAL_CONTENT_BASE_INSTANCE_ID
from kolibri.core.discovery.well_known import DATA_PORTAL_BASE_INSTANCE_ID
from kolibri.core.utils.urls import reverse_path


Expand All @@ -34,6 +37,27 @@ class NetworkLocationViewSet(viewsets.ModelViewSet):
"instance_id",
]

def get_queryset(self):
syncable = self.request.query_params.get("syncable", None)
if syncable == "1":
# Include KDP's reserved location
queryset = NetworkLocation.objects.filter(
Q(location_type=LocationTypes.Static)
| Q(id=DATA_PORTAL_BASE_INSTANCE_ID)
)
elif syncable == "0":
# Include Studio's reserved location
queryset = NetworkLocation.objects.filter(
Q(location_type=LocationTypes.Static)
| Q(id=CENTRAL_CONTENT_BASE_INSTANCE_ID)
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't quite 100% to what we need. For background, the reasoning behind this filter is to return locations that either are syncable (for facility data syncing) or can be used for content import. Studio can be used for content import but syncing. KDP can be used for syncing but not content import. As coded, this wouldn't return dynamic locations, which are similar to static in that we assume they can do both.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this filter should consistently return static and dynamic locations, and only include KDP/Studio reserved locations based on the syncable value?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep!

else:
# Exclude both KDP and Studio
queryset = NetworkLocation.objects.filter(
location_type__in=[LocationTypes.Static, LocationTypes.Dynamic]
)
return queryset
bjester marked this conversation as resolved.
Show resolved Hide resolved

def get_object(self, id_filter=None):
"""
Override get_object to use the unrestricted queryset for the detail view
Expand All @@ -47,6 +71,7 @@ def get_object(self, id_filter=None):
for filter_key in ("id", "instance_id"):
try:
obj = queryset.get(**{filter_key: id_filter})
obj.dynamic = obj.location_type == LocationTypes.Dynamic
bjester marked this conversation as resolved.
Show resolved Hide resolved
break
except NetworkLocation.DoesNotExist:
pass
Expand Down
11 changes: 0 additions & 11 deletions kolibri/core/discovery/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,17 +131,6 @@ def available(self):

return self.connection_status == ConnectionStatus.Okay

@property
def dynamic(self):
return self.location_type == LocationTypes.Dynamic

@dynamic.setter
def dynamic(self, value):
"""
TODO: remove this setter once we've migrated to the new location_type field
"""
self.location_type = LocationTypes.Dynamic if value else LocationTypes.Static

@property
def reserved(self):
return self.location_type == LocationTypes.Reserved
Expand Down
8 changes: 4 additions & 4 deletions kolibri/core/discovery/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,20 +3,19 @@
from rest_framework.serializers import ValidationError

from .models import ConnectionStatus
from .models import NetworkLocation
from .models import PinnedDevice
from .models import StaticNetworkLocation
from .utils.network import errors
from .utils.network.client import NetworkClient
from kolibri.core.serializers import HexOnlyUUIDField


class NetworkLocationSerializer(serializers.ModelSerializer):
class Meta:
model = NetworkLocation
model = StaticNetworkLocation
fields = (
"id",
"available",
"dynamic",
bjester marked this conversation as resolved.
Show resolved Hide resolved
"nickname",
"base_url",
"device_name",
Expand All @@ -30,10 +29,10 @@ class Meta:
"subset_of_users_device",
"connection_status",
"is_local",
"location_type",
)
read_only_fields = (
"available",
"dynamic",
"device_name",
"instance_id",
"added",
Expand All @@ -45,6 +44,7 @@ class Meta:
"subset_of_users_device",
"connection_status",
"is_local",
"location_type",
)

def validate(self, data):
Expand Down
58 changes: 49 additions & 9 deletions kolibri/core/discovery/tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
from kolibri.core.discovery.utils.network.connections import update_network_location
from kolibri.core.discovery.well_known import CENTRAL_CONTENT_BASE_INSTANCE_ID
from kolibri.core.discovery.well_known import CENTRAL_CONTENT_BASE_URL
from kolibri.core.discovery.well_known import DATA_PORTAL_BASE_INSTANCE_ID
from kolibri.core.discovery.well_known import DATA_PORTAL_SYNCING_BASE_URL
from kolibri.core.tasks.decorators import register_task
from kolibri.core.tasks.job import Priority
from kolibri.core.tasks.main import job_storage
Expand Down Expand Up @@ -135,7 +137,10 @@ def _update_connection_status(network_location):
logger.error(e)
logger.warning(
"Failed to update connection status for {} location {}".format(
"dynamic" if network_location.dynamic else "static", network_location.id
"dynamic"
if network_location.location_type is LocationTypes.Dynamic
else "static",
network_location.id,
)
)

Expand Down Expand Up @@ -184,6 +189,14 @@ def _enqueue_network_location_update_with_backoff(network_location):
dependent on how many connection faults have occurred
:type network_location: NetworkLocation
"""
# Check if the network location is local before proceeding
if not network_location.is_local:
logger.info(
"Network location {} is not local. Skipping enqueue.".format(
network_location.id
)
)
return
# exponential backoff depending on how many faults/attempts we've had
next_attempt_minutes = 2 ** network_location.connection_faults
logger.debug(
Expand Down Expand Up @@ -332,17 +345,36 @@ def dispatch_broadcast_hooks(hook_type, instance):

def _refresh_reserved_locations():
"""
TODO handle this a bit smarter with: https://github.com/learningequality/kolibri/issues/10431
Refreshes the reserved network locations for Studio and Kolibri Data Portal
"""
# Delete existing reserved locations
NetworkLocation.objects.filter(location_type=LocationTypes.Reserved).delete()
NetworkLocation.objects.create(

# Create or update Studio reserved location
NetworkLocation.objects.update_or_create(
id=CENTRAL_CONTENT_BASE_INSTANCE_ID,
instance_id=CENTRAL_CONTENT_BASE_INSTANCE_ID,
nickname="Kolibri Studio",
base_url=CENTRAL_CONTENT_BASE_URL,
location_type=LocationTypes.Reserved,
is_local=False,
kolibri_version="0.16.0",
defaults={
"instance_id": CENTRAL_CONTENT_BASE_INSTANCE_ID,
"nickname": "Kolibri Studio",
"base_url": CENTRAL_CONTENT_BASE_URL,
"location_type": LocationTypes.Reserved,
"is_local": False,
"kolibri_version": "0.16.0",
},
)

# Create or update Kolibri Data Portal reserved location
NetworkLocation.objects.update_or_create(
id=DATA_PORTAL_BASE_INSTANCE_ID,
defaults={
"instance_id": DATA_PORTAL_BASE_INSTANCE_ID,
"nickname": "Kolibri Data Portal",
"base_url": DATA_PORTAL_SYNCING_BASE_URL,
"location_type": LocationTypes.Reserved,
"is_local": False,
"application": "Kolibri Data Portal",
"kolibri_version": "0.16.0",
},
)


Expand Down Expand Up @@ -380,3 +412,11 @@ def reset_connection_states(broadcast_id):
job_id=generate_job_id(TYPE_CONNECT, static_location_id),
args=(static_location_id,),
)

# For KDP, set the application to 'Kolibri Data Portal' without enqueuing update task
kdp_location = NetworkLocation.objects.filter(
id=DATA_PORTAL_BASE_INSTANCE_ID
).first()
if kdp_location:
kdp_location.application = "Kolibri Data Portal"
LianaHarris360 marked this conversation as resolved.
Show resolved Hide resolved
kdp_location.save()
6 changes: 3 additions & 3 deletions kolibri/core/discovery/test/test_connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from django.test import TestCase

from ..models import ConnectionStatus
from ..models import LocationTypes
from ..models import NetworkLocation
from ..utils.network import errors
from ..utils.network.client import NetworkClient
Expand All @@ -19,7 +20,6 @@ def setUp(self):
self.mock_location = mock.MagicMock(
spec=NetworkLocation(),
id="mock_location_id",
dynamic=False,
instance_id=None,
connection_status=ConnectionStatus.Unknown,
connection_faults=0,
Expand Down Expand Up @@ -141,12 +141,12 @@ def test_okay(self):
self.assertEqual(self.mock_location.connection_faults, 0)

def test_okay__dynamic(self):
self.mock_location.dynamic = True
self.mock_location.location_type = LocationTypes.Dynamic
update_network_location(self.mock_location)
self.assertNotEqual(self.mock_location.last_known_ip, "192.168.101.101")

def test_okay__static(self):
self.mock_location.dynamic = False
self.mock_location.location_type = LocationTypes.Static
update_network_location(self.mock_location)
self.assertEqual(self.mock_location.last_known_ip, "192.168.101.101")
self.assertTrue(self.mock_location.is_local)
Expand Down
8 changes: 0 additions & 8 deletions kolibri/core/discovery/test/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,14 +28,6 @@ def test_property__is_kolibri(self):
location.application = "kolibri"
self.assertTrue(location.is_kolibri)

def test_property__dynamic(self):
static = StaticNetworkLocation()
self.assertFalse(static.dynamic)
dynamic = DynamicNetworkLocation()
self.assertTrue(dynamic.dynamic)
reserved = NetworkLocation(location_type=LocationTypes.Reserved)
self.assertFalse(reserved.dynamic)

def test_property__reserved(self):
static = StaticNetworkLocation()
self.assertFalse(static.reserved)
Expand Down
8 changes: 4 additions & 4 deletions kolibri/core/discovery/test/test_network_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import kolibri
from ..models import ConnectionStatus
from ..models import LocationTypes
from ..models import NetworkLocation
from ..utils.network import errors
from ..utils.network.client import NetworkClient
Expand Down Expand Up @@ -190,7 +191,7 @@ def test_build_for_network_location__previously_not_okay(self):
spec=NetworkLocation(),
base_url="url.qqq",
connection_status=ConnectionStatus.Unknown,
dynamic=True,
location_type=LocationTypes.Dynamic,
)
client = NetworkClient.build_from_network_location(network_loc)
# should have resolved the base url to something different
Expand All @@ -205,7 +206,7 @@ def test_build_for_network_location__failure(self):
spec=NetworkLocation(),
base_url="url.qqq",
connection_status=ConnectionStatus.Unknown,
dynamic=True,
location_type=LocationTypes.Dynamic,
)
with self.assertRaises(errors.NetworkLocationNotFound):
NetworkClient.build_from_network_location(network_loc)
Expand All @@ -218,7 +219,7 @@ def test_build_for_network_location__no_raise(self):
spec=NetworkLocation(),
base_url="url.qqq",
connection_status=ConnectionStatus.ConnectionFailure,
dynamic=True,
location_type=LocationTypes.Dynamic,
)
try:
NetworkClient.build_from_network_location(network_loc)
Expand All @@ -233,7 +234,6 @@ def test_build_for_network_location__static(self):
spec=NetworkLocation(),
base_url="url.qqq",
connection_status=ConnectionStatus.Unknown,
dynamic=False,
)
try:
NetworkClient.build_from_network_location(network_loc)
Expand Down
24 changes: 22 additions & 2 deletions kolibri/core/discovery/test/test_tasks.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from ..models import ConnectionStatus
from ..models import DynamicNetworkLocation
from ..models import LocationTypes
from ..models import NetworkLocation
from ..models import StaticNetworkLocation
from ..tasks import _dispatch_discovery_hooks
Expand Down Expand Up @@ -183,7 +184,7 @@ def setUp(self):
kolibri_version="0.15.11",
instance_id=mock_device_info.get("instance_id"),
subset_of_users_device=False,
dynamic=True,
location_type=LocationTypes.Dynamic,
)
self.task = unwrap(unwrap(remove_dynamic_network_location))

Expand All @@ -194,7 +195,7 @@ def test_not_found(self, mock_dispatch):

@mock.patch("kolibri.core.discovery.tasks._dispatch_discovery_hooks")
def test_static_location(self, mock_dispatch):
self.network_location.dynamic = False
self.network_location.location_type = LocationTypes.Static
self.network_location.save()
self.task(self.broadcast_id, self.instance)
mock_dispatch.assert_not_called()
Expand Down Expand Up @@ -479,3 +480,22 @@ def test_enqueue_network_location_update_with_backoff__non_zero_faults(
next_attempt,
priority=Priority.LOW,
)

@mock.patch("kolibri.core.discovery.tasks.get_current_job")
def test_enqueue_network_location_update_with_backoff__not_local(
self, mock_get_current_job
):
current_job_mock = mock.MagicMock()
mock_get_current_job.return_value = current_job_mock
self.network_location.is_local = False

with mock.patch("kolibri.core.discovery.tasks.logger") as mock_logger:
_enqueue_network_location_update_with_backoff(self.network_location)
# 'retry_in' should not be called since is_local is False
current_job_mock.retry_in.assert_not_called()
# Verify the function logged the appropriate message
mock_logger.info.assert_called_once_with(
"Network location {} is not local. Skipping enqueue.".format(
self.network_location.id
)
)
3 changes: 2 additions & 1 deletion kolibri/core/discovery/utils/network/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from .urls import HTTP_PORTS
from .urls import HTTPS_PORTS
from kolibri.core.discovery.models import ConnectionStatus
from kolibri.core.discovery.models import LocationTypes
from kolibri.core.tasks.utils import get_current_job
from kolibri.core.utils.urls import join_url
from kolibri.utils.server import get_urls
Expand Down Expand Up @@ -98,7 +99,7 @@ def build_from_network_location(cls, network_location, timeout=None):
# expect that static network locations have an exact base_url, and only try different
# variations if we haven't already
if (
network_location.dynamic
network_location.location_type is LocationTypes.Dynamic
and network_location.connection_status == ConnectionStatus.Unknown
):
return cls.build_for_address(network_location.base_url, timeout=timeout)
Expand Down
3 changes: 2 additions & 1 deletion kolibri/core/discovery/utils/network/connections.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
from .client import NetworkClient
from .urls import parse_address_into_components
from kolibri.core.discovery.models import ConnectionStatus
from kolibri.core.discovery.models import LocationTypes
from kolibri.core.discovery.models import NetworkLocation


Expand Down Expand Up @@ -48,7 +49,7 @@ def capture_network_state(network_location, client):
# having validated the base URL, we can save that
network_location.base_url = client.base_url
# save the IP address for static locations
if not network_location.dynamic:
if network_location.location_type is not LocationTypes.Dynamic:
remote_ip = client.remote_ip

network_location.last_known_ip = remote_ip
Expand Down
4 changes: 4 additions & 0 deletions kolibri/core/discovery/well_known.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,7 @@
CENTRAL_CONTENT_BASE_INSTANCE_ID = generate_ecosystem_namespaced_uuid(
CENTRAL_CONTENT_BASE_URL
).hex

# AKA Kolibri Data Portal
DATA_PORTAL_SYNCING_BASE_URL = conf.OPTIONS["Urls"]["DATA_PORTAL_SYNCING_BASE_URL"]
DATA_PORTAL_BASE_INSTANCE_ID = "2a824768819aa2bec5cecbc06a31ec1e"