On 07/05/2017 12:51 PM, Radostin Stoyanov wrote:
> Log messages from the virtBootstrap's logger are stored in string
> buffer and shown in case of failure.
>
> The bootstrap method is called at the end of the "create dialog" (when
> "Finish" button is clicked).
>
> For coordination we use global variable "self.container_bootstrap_flag"
> Assigned values are:
>
> 1. "CT_BOOTSTRAP_NOT_STARTED" - Container bootstrap is not started.
> This is the initial value of the flag variable.
> 2. "CT_BOOTSTRAP_RUNNING" - Container bootstrap is running.
> This is when the bootstrap process has started and
> "_validate_final_page()" method should wait until the process
> finish. The validation method is called again from the
> finished callback of the asynchronous process.
> 3. OS tree path is used as value when the container bootstrap is
> finished successfully (no exception was thrown).
> Setting this value allows "_validate_final_page()" to return True
> and "_finish_clicked()" to proceed.
I'm having a trouble following this. Particularly it's not clear what cases
you are exactly trying to handle with this logic. FWIW caching data
(container_bootstrap_flag) about the current install in vmmCreate variables is
very tricky to handle correctly so I aim to do it only when necessary. Let's
see if we can avoid it here:
Seems like the cases we need to handle are:
- virt-bootstrap fails. user reclicks Finish. we should just reattempt
everything like we did before
- virt-bootstrap succeeds, but something later in the install fails, like XML
define. if user re-clicks 'finish' we don't want to attempt virt-bootstrap
again, just use the directory path
- Something else?
So it seems like we just need some way to say 'don't try to call
virt-bootstrap on this directory, just use it'. We already have that in the
'install-oscontainer-bootstrap' checkbox. So can we just uncheck that box if
virt-bootstrap succeeds?
Some particular questions/comments inline
> ---
> virtManager/create.py | 113 +++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 112 insertions(+), 1 deletion(-)
>
> diff --git a/virtManager/create.py b/virtManager/create.py
> index 5007d77..f823ab7 100644
> --- a/virtManager/create.py
> +++ b/virtManager/create.py
> @@ -21,6 +21,7 @@
> import logging
> import pkgutil
> import os
> +import cStringIO
> import threading
> import time
>
> @@ -68,6 +69,10 @@ DEFAULT_MEM = 1024
> OS_COL_IS_SHOW_ALL) = range(4)
>
>
> +(CT_BOOTSTRAP_NOT_STARTED,
> + CT_BOOTSTRAP_RUNNING) = range(2)
> +
> +
> #####################
> # Pretty UI helpers #
> #####################
> @@ -142,6 +147,8 @@ class vmmCreate(vmmGObjectUI):
> self._netlist = None
> self._mediacombo = None
>
> + self.container_bootstrap_flag = CT_BOOTSTRAP_NOT_STARTED
> +
> self._addstorage = vmmAddStorage(self.conn, self.builder, self.topwin)
> self.widget("storage-align").add(self._addstorage.top_box)
> def _browse_file_cb(ignore, widget):
> @@ -448,6 +455,7 @@ class vmmCreate(vmmGObjectUI):
> src_model = (self.widget("install-oscontainer-source-url-combo")
> .get_model())
> _populate_media_model(src_model, self.config.get_container_urls())
> + self.container_bootstrap_flag = CT_BOOTSTRAP_NOT_STARTED
>
>
>
> @@ -1993,7 +2001,9 @@ class vmmCreate(vmmGObjectUI):
> if not fs:
> return self.err.val_err(_("An OS directory path is required."))
>
> - if self._get_config_oscontainer_bootstrap():
> + # If container bootstrap is enabled and was not perfromed yet
> + if (self._get_config_oscontainer_bootstrap()
> + and self.container_bootstrap_flag != fs):
>
> src_url = self._get_config_oscontainer_source_url()
> user = self._get_config_oscontainer_source_username()
> @@ -2276,6 +2286,31 @@ class vmmCreate(vmmGObjectUI):
> _mark_vmm_device(nic)
> self._guest.add_device(nic)
>
> + container_bootstrap = self._get_config_oscontainer_bootstrap()
> + # If creating new container and "container bootstrap" is enabled
> + if self._guest.os.is_container() and container_bootstrap:
> + # Get destination path
> + dest = self.widget("install-oscontainer-fs").get_text()
> +
> + if self.container_bootstrap_flag == CT_BOOTSTRAP_RUNNING:
> + return False # Stop if container bootstrap is already running
> +
In what case can we reach this code in the CT_BOOTSTRAP_RUNNING state? When
the bootstrap is running the create wizard can't be interacted with, so I'm
confused
> + # If the bootstrap was not yet performed on the current destination
> + # path.
> + # This check handles the case when container creations has failed
> + # after bootstrap and the user has changed the destination path.
> + elif self.container_bootstrap_flag != dest:
> + # Start container bootstrap
> + src_url = self._get_config_oscontainer_source_url()
> + user = self._get_config_oscontainer_source_username()
> + passwd = self._get_config_oscontainer_source_password()
> + insecure = self._get_config_oscontainer_isecure()
> +
> + self._container_image_bootstrap(src_url, dest, user, passwd,
> + insecure)
> + # Wait for container bootstrap to complete
> + return False
> +
> return True
>
Triggering this from the validation page isn't ideal. I think
_create_directory_tree should be called from _do_async_install, so there's
only one async dialog handling the whole install process. Then you don't need
to play games with exiting early in page validation etc, and it's likely
needed to give the intended interaction in the 'Configure before install' case
>
> @@ -2407,6 +2442,10 @@ class vmmCreate(vmmGObjectUI):
> ##########################
>
> def _finish_clicked(self, src_ignore):
> + # Stop if container bootstrap is running
> + if self.container_bootstrap_flag == CT_BOOTSTRAP_RUNNING:
> + return
> +
Same question, can we reach this with CT_BOOTSTRAP_RUNNING ?
> # Validate the final page
> page = self.widget("create-pages").get_current_page()
> if self._validate(page) is not True:
> @@ -2583,3 +2622,75 @@ class vmmCreate(vmmGObjectUI):
> self.err.show_err(_("Error continue install: %s") % str(e))
>
> return True
> +
> +
> + def _create_directory_tree(self, asyncjob, src, dest, user, passwd, insecure):
> + """
> + Call bootstrap method from virtBootstrap.
> + """
> + try:
> + import virtBootstrap
> + except ImportError as err:
> + asyncjob.set_error("Please install virt-bootstrap", err)
> +
We already check for virtBootstrap earlier, so I say drop this explicit error
handling
> + # Use string buffer to store log messages
> + log_stream = cStringIO.StringIO()
> +
> + # Get virt-bootstrap logger
> + vbLogger = logging.getLogger('virtBootstrap')
> + vbLogger.setLevel(logging.DEBUG)
> + vbLogger.addHandler(logging.StreamHandler(log_stream))
> +
> + # Key word arguments to be passed
> + kwargs = {'uri': src, 'dest': dest, 'not_secure': insecure}
> + if user and passwd:
> + kwargs['username'] = user
> + kwargs['password'] = passwd
> +
> + logging.debug('Start container bootstrap')
> + try:
> + virtBootstrap.bootstrap(**kwargs)
> + # Set the flag value to destination to indicate success
> + self.container_bootstrap_flag = dest
> + except Exception as err:
> + asyncjob.set_error(err, log_stream.getvalue())
> + except:
> + asyncjob.set_error("Container bootstrap has failed",
> + log_stream.getvalue())
> +
> +
> + def _container_image_bootstrap(self, src, dest, user, passwd, insecure):
> + """
> + Launch virt-bootstrap as async job
> + """
> +
> + params = [src, dest, user, passwd, insecure]
> +
> + logging.debug("Start asynchronous job to bootstrap container.")
> + # Set flag to running
> + self.container_bootstrap_flag = CT_BOOTSTRAP_RUNNING
> + parentobj = self._customize_window or self
> + progWin = vmmAsyncJob(self._create_directory_tree, params,
> + self._container_bootstrap_finished_cb,
> + [parentobj],
> + _("Bootstrap Container"),
> + _("The process of download and untar the "
> + "container image may take a few minutes "
> + "to complete."),
> + parentobj.topwin)
> + progWin.run()
> +
> +
> + def _container_bootstrap_finished_cb(self, error, details, parentobj):
> + """
> + Thread callback - See self._container_image_bootstrap()
> + """
> +
> + if error:
> + error = (_("Unable to complete container bootstrap: %s") % error)
> + parentobj.err.show_err(error, details=details)
> + # Reset flag
> + self.container_bootstrap_flag = CT_BOOTSTRAP_NOT_STARTED
> + else:
> + # Proceed with domain creation
> + self._finish_clicked(None)
>
Particularly calling 'finish_clicked' indicates to me things need to be
arranged differently
Thanks,
Cole
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
