Feature/mesh switch time by julianschuh · Pull Request #3 · freifunk-darmstadt/ffda-domain-director

julianschuh · 2018-12-27T22:57:36Z

This PR adds the ability to set a custom switch time per Mesh (via CLI) in order to migrate meshes manually and not all at once.

For this purpose, the column manual_switch_time is added to the Mesh model and the switch time selection logic is updated.

The command used to set the switch time for a mesh is domain_director set-switch-time [meshid] [switch_time as unix timestamp]

Additionally, some methods were renamed to make clear(er) what they are actually doing.

Feedback welcome ;-)

Thanks for your great work!

blocktrron · 2018-12-28T14:38:02Z

    def set_domain(mesh_id, domain, decision_criteria):
        Mesh.update(domain=domain, decision_criteria=int(decision_criteria)).where(Mesh.id == mesh_id).execute()

+    def set_manual_switch_time(mesh_id, manual_switch_time):


This method should also be static

blocktrron · 2018-12-28T14:42:05Z

Thanks for your PR. This is actually something i wanted myself for a long time!

This might be a fundamental design question but i would prefer to have a http endpoint for changing mesh properties such as target domain or switch time.

We needed to adjust the target domain of multiple meshes already which was done by manually shutting down the director and editing the SQLite databse. I would like to avoid the "shutting down of director" part and provide an http endpoint which sits behind basic auth of an nginx or something along those lines.

What do you whink about that?

julianschuh · 2018-12-28T16:39:30Z

Thanks for your feedback.

I thought about adding a HTTP endpoint, but ultimately decided against it due to having to handle authentication and so on. Handling the authentication via nginx would certainly be possible, but I usually try to avoid such setups as they are quite error prone (are all relevant paths protected? are the public parts available freely? and so on.)

If an authentication-less solution would be acceptable it should be quite easy.

With the current solution it should not be required to restart the director: You can invoke the domain directory binary with the sub-command, and it should then just update the db and stop again. As far as I know SQLite can handle cases where multiple threads/processes access the same database. The next time a node sends a request the new data should be returned (in theory).

blocktrron · 2019-01-07T15:26:55Z

First of all: Sorry for my reply being late. I haven't noticed you already replied.

I would still very much prefer the HTTP endpoint solution. I think the protection should be pretty easy by protecting everything contained in '/admin'.

I have the concern that we might want to have some extensibility in terms of what an admin might want to set. The CLI might get a bit fiddely then.

I will habe to think a bit about this. In the meantime, i would merge b99742a and 29f9621 and also work on a migration strategy for the database.

julianschuh · 2019-01-08T14:38:19Z

No worries, thanks for getting back :)

I'm currently working on a simple HTTP endpoint to update mesh settings, which should be easily expandable in the future.

I plan on pushing the changed in the next couple of days™ :)

julianschuh · 2019-01-23T09:26:16Z

So, I added a first version of the http endpoint for modifying the mesh properties. What do you think? What is missing in your eyes?

Thanks in advance!

Best Regards

mweinelt · 2019-01-23T12:52:20Z

+    force = 'force' in request.values and request.values['force'].lower() in ['true', 'yes', '1']
+
+    if not force and (switch_time_parsed - now).total_seconds() < 0:
+        return 'Specified switch time lies in the past. Force to set value.', 400


Do you have a use case for force setting a switch time in the past?

the rationale here is to be able to force a node to switch as quickly as possible by setting the switch time to the past

mweinelt · 2019-01-23T12:54:57Z

+        return
+    except ValueError:
+        print('Invalid meshid specified')
+        return


exit with sys.exit(code) and code > 0

mweinelt · 2019-01-23T12:55:10Z

+        mesh_db_entry = Mesh.select().where(Mesh.id == int(kwargs['meshid'])).get()
+    except DoesNotExist:
+        print('Mesh with ID {} not found.'.format(kwargs['meshid']))
+        return


mweinelt · 2019-01-23T12:56:00Z

+        switch_time_parsed = datetime.utcfromtimestamp(switch_time)
+    except ValueError:
+        print('Invalid switch time specified')
+        return


mweinelt · 2019-01-23T12:56:14Z

+
+    if not force and (switch_time_parsed - now).total_seconds() < 0:
+        print('Specified switch time lies in the past. Force to set value.')
+        return


mweinelt · 2019-01-23T12:58:40Z



-def get_domain(lat, lon, domain_polygons, treshold_distance=0):
+def get_domain_by_location(lat, lon, domain_polygons, treshold_distance=0):


I think get_domain_from_location sounds even better. It's not a "group by" after all.

mweinelt · 2019-01-23T13:05:40Z

+def update_node_domain(node_id, polygons, wifis=None, api_key="test", lat=None, lon=None, accuracy=None,
                    default_domain=None,
-                    max_accuracy=250, treshold_distance=0, switch_time=-1, migrate_only_vpn=False):
+                    max_accuracy=250, treshold_distance=0, default_switch_time=-1, migrate_only_vpn=False):


underindentend for new func name

what exactly do you mean?

The parameters of this function call are under-indented.

done, i think

mweinelt · 2019-01-23T13:06:43Z

    mesh_id = Node.get_mesh_id(node_id)
    if mesh_id is None:
        domain = default_domain
+        out_switch_time = default_switch_time


I don't understand what "out" means in this case. Could likely be shortened to just "switch_time", no?

default_switch_time could also stay as just switch_time param, that gets overwritten if certain requirements are met.

I changed the name to switch_time, good idea.
regarding the parameter: I think it would be more clear to leave the parameter name default_switch_time. When calling the function with the parameter named default_* you would probably expect that the value could be changed within the function.

mweinelt · 2019-01-23T13:14:22Z

+
+    def test_admin_mesh(self):
+        rv = self.app.patch('/admin/mesh/999999/', data={})
+        self.assertEqual(rv.status_code, 404)


Also test for the HTTP 400 case here https://github.com/freifunk-darmstadt/ffda-domain-director/pull/3/files#diff-e852383f0904571fb8352b555a1566c1R28

did you link to the correct line? the references line does not seem to be part of my changes ;)

Possibly, what I mean is the abort(400) responses.

I added more tests for all the paths

…location

…t the functions changes the state of a node

allows to change a mesh’s switch time via HTTP

julianschuh · 2019-03-14T21:29:37Z

OK, i think I incorporated your feedback :)

I also rebased the branch on master.

Thanks for your time!

blocktrron self-requested a review December 28, 2018 14:26

blocktrron self-assigned this Dec 28, 2018

blocktrron reviewed Dec 28, 2018

View reviewed changes

mweinelt reviewed Jan 23, 2019

View reviewed changes

julianschuh added 9 commits March 14, 2019 22:25

add switch time override by domain

456d1ab

change name of get_domain function to more descriptive get_domain_by_…

94778a2

…location

rename get_node_domain function to update_node_domain to indicate tha…

601a59d

…t the functions changes the state of a node

implement CLI command to set the switch time for a mesh

b34fc95

mark set_manual_switch_time method as static

9114605

add first version of admin blueprint

158162b

allows to change a mesh’s switch time via HTTP

incorporated some changes suggested in code review

a438788

fix indentation for update_node_domain function

24efca2

add more detailed tests for admin interface

4f29e55

julianschuh force-pushed the feature/mesh_switch_time branch from 923d795 to 4f29e55 Compare March 14, 2019 21:26



		def get_domain(lat, lon, domain_polygons, treshold_distance=0):
		def get_domain_by_location(lat, lon, domain_polygons, treshold_distance=0):

Conversation

julianschuh commented Dec 27, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

blocktrron commented Dec 28, 2018

Uh oh!

julianschuh commented Dec 28, 2018

Uh oh!

blocktrron commented Jan 7, 2019

Uh oh!

julianschuh commented Jan 8, 2019

Uh oh!

julianschuh commented Jan 23, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mweinelt Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mweinelt Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mweinelt Jan 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julianschuh commented Mar 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mweinelt Jan 23, 2019 •

edited

Loading

mweinelt Jan 23, 2019 •

edited

Loading

mweinelt Jan 23, 2019 •

edited

Loading