openwisp / django-ipam

The development of this project has moved to openwisp-ipam
https://github.com/openwisp/openwisp-ipam
BSD 3-Clause "New" or "Revised" License
78 stars 28 forks source link

[bug] Subnet page for `/8` or `/16` are not accessible #66

Closed atb00ker closed 4 years ago

atb00ker commented 5 years ago

The subnet information page for /8 or /16 networks is not accessible. The reason seems to be that there are too many ip adresses in one page hence loading all of them cause the site to not respond.

atb00ker commented 5 years ago

A possible solution would be to introduce pagination.

nemesifier commented 5 years ago

@atb00ker pagination could work, @hispanico what do you think? Have you tried the same with phpipam?

NoumbissiValere commented 5 years ago

@atb00ker i wish to address this issue. can you tell me the file(s) am suppose to look at? thanks.

NoumbissiValere commented 5 years ago

i have been able to identify the files and how they function. but am unable to make the pagination_class instance of ListViewPagination class has its effect in the subnet_list_ipaddress page. please can you help me with that?

atb00ker commented 5 years ago

Please share the error that you are having and what action is causing that problem, that will help me help you debug the problem! :smile:

NoumbissiValere commented 5 years ago

@atb00ker i have applied the pagination and the information page for /16 can be viewed. but that of /8 is not still loading. but i think if i can supply an optimum pagination size (a page size that will work for both /8 and /16), the problem will be solved. I am not able to find this optimum pagination size please can you help me with that? thanks.

NoumbissiValere commented 5 years ago

@atb00ker can i send the commit of what i have done so that you can see what i'am talking about?

atb00ker commented 5 years ago

@NoumbissiValere Yes, that'll be great! Please open a pull request marked as WIP! I think the optimum size will be 255 IPs per page, as it's working for the small networks! :smile:

NoumbissiValere commented 5 years ago

ok. do that right away

atb00ker commented 5 years ago

@nemesisdesign @NoumbissiValere

Thanks to @NoumbissiValere 's work here #71 , i've tried some diagnosis as it turns out:

total = [host for host in instance.subnet.hosts()]

Is taking a lot of time since very high number of hosts are present in larger networks. ( /10 or above )!

What do you think should be our approach here? :smile:

To begin with, i'll look into phpipam's implementation and get back here as soon as possible! :smile:

NoumbissiValere commented 5 years ago

ok @atb00ker . This i what i had in mind: What we are presently doing is Fix length subnetting with which it's possible to have millions of IP addresses in a subnet which a network Engineer who knows his work should never do because his subnet will be tooooo big and difficult to manage.

NoumbissiValere commented 5 years ago

I will propose us to use Variable length subneting(VLSM). with this approach, subnets are created as per the number of hosts who will be present in the subnet.

NoumbissiValere commented 5 years ago

with this approach, when a user supply his network address, the system will ask him how many host will be present in this subnet he wishes to create. with the data provided, django-ipam will subnet the network address supplied just for that amount of hosts and the remains from this can be used to do another subnet with number of hosts provided

NoumbissiValere commented 5 years ago

so for example if a user supply a network address as 10.0.0.0/8, and requires a subnet of say 500 users, the system will provide a subnet with just this amount of users. and now the remaining network address after the subnet say 10.0.1.30/14 (i have not done the calculation so that network address might not be correct) can be use as the network address for a new subnet which will have specific number of hosts and it goes on and on until the address can not longer accommodate hosts

NoumbissiValere commented 5 years ago

now when it comes to viewing the ip addresses, the user will select the subnet for which he wishes to view ip addresses and the system will display to him only IP addresses for that subnet. and he might do same for the other subnets and view their Ip addresses. with this approach, we can not expect a subnet of /8 because no network engineer will create a single subnet with 16,777,214 users because no network engineer in the world can manage this effeciently. more over, it will increase the performance of the system because instead of asking the system to display to u a bunch of IP addresses, we will ask the system to display the number of IP addresses as per number of hosts which usually is very small (in hundreds).

NoumbissiValere commented 5 years ago

and as we can see from the example above, we are creating nested subnet since a subnet can be created from the remains of another. and equally a subnet can be modified by modifying the number of hosts in that subnet (ofcourse less than the actually one) this will result to the creation of a subnet in a subnet in a subnet which increases the level of nesting. this is possible because this is the very concept on which VLSM is based on.

NoumbissiValere commented 5 years ago

Thus this proposal will not only solve the /8 and /16 not accessible and the nested subnets problems we had identified, but it will equally increase the performance of the system.

NoumbissiValere commented 5 years ago

@atb00ker and @nemesisdesign what do you think about the proposal.

NoumbissiValere commented 5 years ago

Am a networking student who love and is passionate about programming so if this proposal is accepted, i will love to propose it as my GSoC 2019 project. i understand very well the concept of subneting and VLSM in particular so i believe i can solve it. more over it will increase my skills and make me learn more.

nemesifier commented 4 years ago

Adding this as a Google Code-In task.

I think we should use JS based pagination.

If django does the rendering, the server must generate the whole HTML, the client must download it and the browser then renders it anyway. If django returns only a JSON with the list of ip addresses (one for available and one for taken), the server will return the response a lot faster, the client downloads it also faster because the content is smaller, then JS is used to render only the subnet visualization part, which if we limit the number of adresses loaded at each time in a sensible way will be fast (eg: 255 addresses for ipv4).

I just think that we should not use pages: we have to to use vertical scrolling. Here's some more details:

This should do the trick. It's not super hard, but obviously not trivial.

pawelplsi commented 4 years ago

I'm working on this GCI task and I'd like to suggest a little improvement. IMO the idea of returning two arrays - available and used generates an unnecessary problem in frontend/js implementation - we have to sort the addresses somehow to display them in the right order, wouldn't it be simpler both in terms of implementation and code cleanness to return one list containing objects composed of IP address and a boolean indicating whether the address is available or not? This approach also lets us easily use Django's pagination mechanism instead of writing our own. What do You think?

atb00ker commented 4 years ago

@pawelplsi okay, I checked, available and used are just integers telling how many IPs are used and now many are free, this might be useful for the graphical library used to make that pie chart subnet's change page. :smile:

Currently, total and used_ip are send to the change_form.html page, where total is all the IPs and used_ip is the IP addresses used. :smile:

I don't see a particular reason to change this but I don't see any particular reason to hold to this either.

If the frontend code will be cleaner in changing it to a bool value, I think it may be justified to change it.

CC: @nemesisdesign

pawelplsi commented 4 years ago

@atb00ker Sorry, but I think You're slightly missing the point there

* we should add a JSON view (accessible only to users logged in the admin) which returns a JSON object (dictionary) with two keys: `available` and `used`, **each one being a list** of ip addresses;

It wasn't about total and used IP but available and used, which generates a need to sort the IP's client-side. Also, the approach I suggested wasn't meant to clean-up frontend, but server - returning one array such as [ { 'address':'xxx.xxx.xxx.xxx', 'used':true }, ... ] lets us DRY up the code and reuse Django's pagination and just query our API for a specific page instead of implementing our own logic (start parameter, 255 max size).

nemesifier commented 4 years ago

@pawelplsi yes you're right, go ahead.

pawelplsi commented 4 years ago

@nemesisdesign Okay, could you explain why do we need a box with seperate scrollbar for it? It doesn't seem really convenient to use to me.

nemesifier commented 4 years ago

@pawelplsi because the subnet could be huge so the page could become huge.

The best thing we can do is to design a specific part of the page to contain the visualization and make this scrollable, but at the same time you could add a button that allows to show that box in full-screen mode (implies adding another button to exit full-screen mode) this would be the best outcome.

pawelplsi commented 4 years ago

I think we can implement such behavior by moving the infinitely scrollable view to another admin sub-page/url, embedding it in subnet admin page using iframe and adding a button opening that view in a fullscreen-sized popup. Do you think it's a good way to go?

nemesifier commented 4 years ago

@pawelplsi no because that would cause the loading of the iframe page to hang anyway.

The proposal is the way to go.

pawelplsi commented 4 years ago

@nemesisdesign Of course, I know infinite scroll is the way to go and I have it implemented, I want to put it in iframe in order to implement feature you described there, it can't hang then, yes?

nemesifier commented 4 years ago

Why do you think we need an iframe? I don't see the need of an iframe. We can use css (overflow-x: scroll and define a min and max height) to display the scrollable box, we don't need to have an <iframe>, that's quite an obsolete HTML construct which should be used sparingly only when there's no alternative, it's usually used only to embed content from different websites and domains.

Anyway, open the PR, if the implementation works I will help you to improve the UI if needed.