oamg / leapp

Leapp - Application & OS Modernization Framework (For in-place upgrades, follow leapp-repository)
https://oamg.github.io/leapp/
Apache License 2.0
87 stars 70 forks source link

Demo UI proposal #43

Closed ncoghlan closed 7 years ago

ncoghlan commented 7 years ago

The current demo is hardcoded to migrate from a particular local VM hostname to a particular target VM hostname. The demo is also hardcoded to assume a particular port will be exposed on the target machine, and it implicitly deletes all previously migrated macrocontainers from the target machine when doing a migration.

These limitations combine to make it difficult to conduct a fluid demo that covers multiple different migration scenarios.

Changing that requires at least the following:

  1. A way of choosing the target machine
  2. A way of choosing the source machine
  3. A way to trigger a migration from the chosen source to the chosen target
  4. A way to explicitly clear previously migrated containers from the target machine
  5. A way to customise the ports exposed post migration

My proposal for handling the first three points would be to do the following:

For the fourth point (assuming the above changes):

For the final point (assuming the above changes):

ncoghlan commented 7 years ago

A couple of possible simplifications to the above:

  1. Keep centos7-target as a hardcoded target for now. This means being able to skip the radio selection buttons and the UI logic to pass the selected target to the migration command, and may be worth doing, since we don't have any current plans or questions that would require a demo with multiple target VMs.

  2. For the initial restructuring of the UI, keep centos6-app-vm as the only supported source, and then update the Migrate button to be row dependent later. This seems less beneficial, since we already know we want to handle multiple source VMs in a single demo session, so there's no chance of a "maybe we'll never need it" outcome.

ncoghlan commented 7 years ago

A solid argument in favour of allowing selectable targets: it pre-empts the question of "Can I choose a different target?" :)

ncoghlan commented 7 years ago

Adding a note prompted by @pcahyna: as part of this, we should add an integration test that actually starts the demo.

Some complications that brings beyond any of the current integration tests:

ncoghlan commented 7 years ago

@podvody looked up what Cockpit uses, and it's PhantomJS: https://github.com/cockpit-project/cockpit/blob/master/test/common/phantom-command

Looking at https://github.com/dhamaniasad/HeadlessBrowsers doesn't give me any reason to question their choice, so I think we should run with it as well.

ncoghlan commented 7 years ago

A bit of research into the current state of the art for PhantomJS based testing from behave brought me to Nicole Harris's write-up here: http://whoisnicoleharris.com/2015/03/19/bdd-part-two.html

Her recommendation is to use http://splinter.readthedocs.io/en/latest/index.html as a higher level abstraction layer over Selenium & PhantomJS, and that seems like a good idea to me.

ncoghlan commented 7 years ago

I spent some time looking at/thinking about the automated testing problem this morning, and I think it's tricky enough that it should be separated into its own issue: https://github.com/leapp-to/prototype/issues/46

Specifically, consider the suggested basic test scenario that just goes to the plugin page and checks it renders within a certain time limit:

Given that the Cockpit service is running on the testing host # skip scenario if not
  And the demonstration plugin is installed # ensure appropriate symlinks are in place
  And the demonstration user exists  # create unique-to-the-test-run user (destroy at end)
  And the demonstration VMs are running  # run demo/start_vms.sh as the demo user
When the demonstration user visits the Kernel Flexibility page
Then the local VMs should be listed within 15 seconds

While all of that is feasible (given that the tests will be run either as root, or by a user with sudo access), adding the required testing infrastructure and step definitions is all orthogonal to the actual changes we want to make to the demo itself.

mfranczy commented 7 years ago

Regarding to fourth point of this issue:

A way to explicitly clear previously migrated containers from the target machine

For the fourth point (assuming the above changes):

move the clearing of previously migrated macrocontainers to the Ansible provisioning playbook for the target VM

Maybe it would be better to implement destroy-container within leapp-tool than clear previously migrated containers by Ansible playbooks.

It should be quick to implement, how I see it: leap-tool destroy-container centos7-target

I could take docker rm command from start_container function https://github.com/leapp-to/prototype/blob/master/src/leappto/cli.py#L90 and then add new destroy_container function

What do you think?

ncoghlan commented 7 years ago

@mfranczy The main downside I see to that is that the affected test cases would need a new "Given" step to see "And any existing macrocontainers on {target} have been destroyed".

That's not a big downside though, since we don't have many test cases at this point, so if it's easier than the Ansible option, go for it.

We can always refactor it again later if we decide we want to.

ncoghlan commented 7 years ago

Closing this based on the layout updates in #51 that switched to a two-column layout with source selection on the left and target selection on the right.