Ways to improve WVC

[Real-Gecko]

Hey there! I think we need to discuss some things about development of WVC to avoid confusion with further development. Things to discuss are:

• [x] code formatting
• [ ] test coverage
• [ ] migrations squashing
• [ ] installation script
• [ ] views functionality split
• [ ] django-guardian

Feel free to suggest some other things to discuss in terms of development.

[Real-Gecko]

I use VS Code for development and I use yapf with following settings:

    "python.formatting.yapfArgs": [
        "--style",
        "{based_on_style: pep8, column_limit: 127}"
    ],

I also use VS Code feature "Sort imports" for sorting imports of python libs. That's why you may see a lot of code shifting in my PRs without actual change in functionality. To avoid confusion I suggest we decide which formatter we use, to keep code same for all devs.

[Real-Gecko]

Test coverage the thing I'm currently working on, slowly but surely we're getting there.

[Real-Gecko]

I think we need to perform either migration squashing or redo migrations from scratch in the end of current dev cycle, when all changes will be settled, cause we're currently getting a lot of them. We can also try and create a way to migrate from older version of WVC to newer, maybe with management command.

[Real-Gecko]

We may create shell script for new installations and move all pre install tasks to there, like creation of admin user, maybe add ability to set some settings without actually editing settings.py etc.

[catborise]

hi @Real-Gecko i am a system engineer more than software developer. I never worked with a developer team, because of that i am not experienced with team working. but i prefer/tend to experience it. i can follow your guidance.

[catborise]

migration squashing with after some period will be fine.(i did it with python2-->3 conversion.)

i moved setttings to appsettings page. for now settings.py is slimmed down.

i move them to appsettings but i think your admin app will be more suitable for that configs. it can be moved... (i am creating settings with setting.objects.using(db_alias).bulk_create migration but there should be more suitable way. i need help with that :) )

[catborise]

We may create shell script for new installations and move all pre install tasks to there, like creation of admin user, maybe add ability to set some settings without actually editing settings.py etc.

after the lastest updates this approach(moving all pre install tasks) may be more suitable way.

i have never write coverage tests, i do not now how to... i should work on it.. i need time.

[Real-Gecko]

i am a system engineer more than software developer

What IDE do you use? Does it integrate with YAPF if so than everything is easy. I created conf/requirements-dev.txt, it adds yapf, coverage and django-debug-toolbar to existing requirements.

YAPF is used by me to auto format source files on save, really helps to keep code readable. However other devs may prefer other ways to format code, that's why I decided to discuss the issue. Settings for yapf I use ar in post 2. Try it out.

Coverage is used to run tests with coverage info, really helps to see what parts of your code are not tested. I use following command to run tests:

venv/bin/coverage run --source '.' --omit 'venv/*' manage.py test

And this one to see the results:

venv/bin/coverage html && firefox ${workspaceFolder}/htmlcov/index.html

You can use any browser you want instead of FF.

And finally Debug Toolbar really helps too. Installation and set up is pretty easy too.

[Real-Gecko]

i moved setttings to appsettings page. for now settings.py is slimmed down.

Yeah, that's cool, ability to change settings on the fly is always better than messing with settings.py.

i move them to appsettings but i think your admin app will be more suitable for that configs. it can be moved...

Well that's not hard to do: one view, one model, one template. Let's do this

i am creating settings with setting.objects.using(db_alias).bulk_create migration but there should be more suitable way. i need help with that :)

Populating data with migrations is acceptable, but Django devs recommend using fixtures, imo it's more flexible way, it will require some work during testing, but that's not something I would worry about.

[Real-Gecko]

i have never write coverage tests, i do not now how to... i should work on it.. i need time.

It sounds harder than it actually is. If you read tests I created you'll see they're pretty simple, but help a lot. Here's computes index page test:

    def test_index(self):
        response = self.client.get(reverse('computes'))
        self.assertEqual(response.status_code, 200)

Simply GET index page and check that response status is 200.

[catborise]

before that i was using pycharm, it is really nice ide for python(license is over and community edition lack of some capabilities). Now i started using vscode. i can add yapf settings for sync.

[Real-Gecko]

I see a lot of views doing more than one task which makes them hard to maintain and test. I think we need to split views functionality like so: one view - one task. For example list users is one view, create user is another, update is another etc.

[catborise]

i do not know why retspen choose that approach but it is useful to slim down urls. for me, it is fine to seperate small ones but instances/views.py can be painful to split :)

[Real-Gecko]

urls.py is just like route map user request can take to receive required answer, so it's ok for it to be big, but big views that do more than one job is bad imo.

I'm currently working on computes where I've already split some functionality to smaller views. Instances are on the way.

[Real-Gecko]

Correct me if I'm wrong: we establish new connection to libvirt host every time there's any operation on instance. Am I right?

[Real-Gecko]

Never mind, look like I've figured it out.

[catborise]

i think yes it starts new connection everytime but with the help of keep alive protocol, connections are cached. libvirt service on the host keep up open the connection to serve requests fastly...

[Real-Gecko]

Yep, there's wvmConnectionManager class that pools all connections. I was just confused by wvmInstance close method.

[Real-Gecko]

I'm currently working on instances app, some serious optimizations in progress. Please avoid any work in this app, to prevent source code clashing during PR.

[catborise]

👍 currently i am not working on webvirtcloud master.. be careful instances app is a behemoth. :)

[Real-Gecko]

Yep, but it's worth it, unoptimized compute instances view: more than 5 seconds to response, 41 database requests.

Optimized: 1.5 seconds, 2 database queries.

[catborise]

wow 41 queries... it should be max five or six queries. 1. get compute 2.get instances, 3.update records, 4.check duplicates, 5.add missings, something must be out of control.

i found it: def refresh_instance_database(comp, inst_name, info) loops for all instances and updates all of them. that causes making queries relative to instance count. i am wondering how to solve that.

[Real-Gecko]

Currently I replaced it with this code:

def refr(compute):
    domains = compute.proxy.wvm.listAllDomains()
    domain_names = [d.name() for d in domains]
    # Delete instances that're not on host
    Instance.objects.filter(compute=compute).exclude(name__in=domain_names).delete()
    # Create instances that're not in DB
    names = Instance.objects.filter(compute=compute).values_list('name', flat=True)
    for domain in domains:
        if domain.name() not in names:
            Instance(compute=compute, name=domain.name(), uuid=domain.UUIDString()).save()

But I don't like it too, cause it does not connect instances from WVM with domains on host. If we try to connect 'em will get query hell again. So currently I think about workaround.

One solution is to get Instance objects from DB and work with it, but that way we don't get VM status on host.

[Real-Gecko]

OK, I think I know what to do with VM status. I added wvmInstance to Instance model that we can use to get info about domain directly via libvirt:

    @cached_property
    def proxy(self):
        return wvmInstance(
            self.compute.hostname,
            self.compute.login,
            self.compute.password,
            self.compute.type,
            self.name,
        )

Result is looking good:

>>> i = Instance.objects.get(name='test')
>>> i.proxy.instance.info()
[5, 4194304, 4194304, 2, 0]
>>> 

[catborise]

i think you will seperate allinstances and host only intances list refreshing.

there is not need to check duplicate uuid and names on one host. but we should handle that among other hosts. duplicate instance definition on other hosts could cause unwanted problems.

proxy with @cached_property is a nice approach. i like it.

[Real-Gecko]

duplicate instance definition on other hosts could cause unwanted problems.

Actually I think no, even if there will two VMs with same UUID, but on different hosts it will not cause trouble. Cause this VMs are well on different hosts :D

But current code definitely troublesome:

    instances = Instance.objects.filter(name=inst_name)
    if instances.count() > 1:
        for i in instances:
            user_instances_count = UserInstance.objects.filter(instance=i).count()
            if user_instances_count == 0:
                addlogmsg(request.user.username, i.name, _("Deleting due to multiple(Instance Name) records."))
                i.delete()

As it does not take into account the fact that there could be instances with the same name on different computes.

[catborise]

yes same name could cause confusion but doesn't same uuid cause migration prevention?

you are right unique name and uuid can be provided efficiently only within a cluster. these actions not a complete solution but only trivial checking

[Real-Gecko]

yes same name could cause confusion but doesn't same uuid cause migration prevention?

Yep it's possible even though possibility of UUID clashing is really low, however I think in this case libvirt will throw some kind of error we will show to user, and then that's admin's task to deal with this tricky situation. In our case it's important to keep our DB in sync with hosts.

[Real-Gecko]

I've pushed my WIP Instances branch into forked repo for testing and fun. Anyone willing to test and suggest is free to do so. WARNING: heavy WIP, not for production use.

[Real-Gecko]

I base.html to contain this:

    {% include 'navbar.html' %}

    <div role="main" class="container">
      <div class"row">
        <div class="col-sm-12">
          <div class="float-right">
            {% block page_header_extra %}
            {% endblock page_header_extra %}
          </div>
          <h3 class="page-header">
            {% block page_header %}
            {% endblock page_header %}
          </h3>
        </div>
      </div>
      {% bootstrap_messages %}
      {% block content %}{% endblock %}
    </div>

So there will be no need to write page header in every template, things like search bar can go to page_header_extra and all messages added by django.contrib.messages framework will go below page header.

{% block page_header %}{% trans "Instances" %}{% endblock page_header %}

{% block page_header_extra %}
{% if request.user.is_superuser %}
    {% include 'create_inst_block.html' %}
{% endif %}
{% if all_host_vms or all_user_vms %}
    <input id="filter" class="form-control" type="text" placeholder="{% trans 'Search' %}">
{% endif %}
{% endblock page_header_extra %}

{% block content %}
...

[catborise]

although effort or code length does not much change, if alignment problems does not occur, this way is more elegant than current. i like it

[Real-Gecko]

although effort or code length does not much change, if alignment problems does not occur, this way is more elegant than current. i like it

This is just an example how things can be optimized, not a final version, anyone willing to suggest improvements is free to do so.

[catborise]

as far as i see grouped view for index page is removed, practically. group view: nongrouped view:

all_host_vms = Instance.objects.all().prefetch_related('userinstance_set') does not contain host info, doesn't it?
what do you think about that?

[Real-Gecko]

I think replacing it with either accordion or tabs which will contain tables with instances. Current implementation does not work right anyway, try sorting the table by name or RAM you'll see what I mean.

[Real-Gecko]

Accordion does not look bad IMO

[catborise]

i added grouped view because there is not any info about host usage. especially ram usage. This info very valuable to get idea host load. other than that already nongrouped view is like that.

[Real-Gecko]

I'll commit soon to instances branch, you'll see how it looks

[Real-Gecko]

I've pushed changes de20f810a27ab528aff6699a33ab7fbb607be4bf, not the best looking for right now, but you get the idea, most important is the optimization of templates and DB queries.

[catborise]

i think, database queries not criitical. query delay impact is very low compare to libvirt calls. focusing on queries would not affect very much.

[Real-Gecko]

Actually, as far as I can tell from testing rewritten views DB queries impact is huge :)

[catborise]

i cannot run your branch. gives error always i get that error

[Real-Gecko]

Is it the same error as in 2c8f092992bf657977c268e806a300cee3e30fce? If so, try commenting out 'webvirtcloud.middleware.ExceptionMiddleware', in your settings.py and see where error is.

[catborise]

not reachable host caueses, i think. trying to get cpu from host info causes that.

other problem in instances.html: {% widthratio vm.proxy.instance.info.1 1024 1 %} MiB</td>(Exception has occurred: VariableDoesNotExist Failed lookup for key [vm] in) i think this should be instance

and other in in instance_actions.html

<form action="" method="post" role="form" aria-label="Shortcut instance action form">{% csrf_token %}
    <input type="hidden" name="name" value="{{ inst }}" />

(Exception has occurred: VariableDoesNotExist Failed lookup for key [inst] in)

[Real-Gecko]

not reachable host caueses, i think. trying to get cpu from host info causes that.

Hmm, I cannot reproduce, can you remove problematic compute and retry.

[Real-Gecko]

other problem in instances.html: {% widthratio vm.proxy.instance.info.1 1024 1 %} MiB</td>(Exception has occurred: VariableDoesNotExist Failed lookup for key [vm] in) i think this should be instance

and other in in instance_actions.html

Fixed those.

[catborise]

add a compute with fake name ip pass user. it accepts

[Real-Gecko]

I tried, I do not get the error you describe, but instead I get another one :D

[catborise]

after instance_action.html fix, click show console.: negative token generation :)

[catborise]

forceoff power off powercycle does not work...

[catborise]

you are a good backender :) i am crying :)

wouldnt be nice implementation like this, if we could...