From ffb51f0ca2c527091560dc967bd34507351618eb Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 13 Apr 2020 15:23:45 -0700 Subject: [PATCH 1/4] pkg/destroy/ovirt: Cleanup VM logging Fixing some of the logging from 05177839ab (ovirt: Implement destroy, 2019-10-28, #1948) to avoid things like: $ openshift-install destroy cluster --dir one INFO searching VMs by tag=one-6xlb7 INFO Found %!s(int=3) VMs INFO Stopping VM one-6xlb7-master-1 : errors: %s%!(EXTRA ) ... because the final 'err' had no %s in the template string to consume it. This commit fixes the two cases which had an 'errors: %s' using up the final template %s, but leaves a lot of other potential logging cleanup in this package alone. --- pkg/destroy/ovirt/destroyer.go | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/pkg/destroy/ovirt/destroyer.go b/pkg/destroy/ovirt/destroyer.go index a2f201c22d2..96d4eb14ef9 100644 --- a/pkg/destroy/ovirt/destroyer.go +++ b/pkg/destroy/ovirt/destroyer.go @@ -93,23 +93,27 @@ func (uninstaller *ClusterUninstaller) stopVM(vmsService *ovirtsdk.VmsService, v vmService := vmsService.VmService(vm.MustId()) // this is a teardown, stopping instead of shutting down. _, err := vmService.Stop().Send() - uninstaller.Logger.Infof("Stopping VM %s : %s", vm.MustName(), "errors: %s", err) - if err != nil { - uninstaller.Logger.Debugf("Failed stopping VM %s : %s", vm.MustName(), err) + if err == nil { + uninstaller.Logger.Infof("Stopping VM %s", vm.MustName()) + } else { + uninstaller.Logger.Debugf("Failed to stop VM %s : %s", vm.MustName(), err) } waitForDownDuration := time.Minute * 10 err = vmService.Connection().WaitForVM(vm.MustId(), ovirtsdk.VMSTATUS_DOWN, waitForDownDuration) - if err != nil { - uninstaller.Logger.Warnf("Waiting %d for VM %s to power-off", waitForDownDuration, vm.MustName()) + if err == nil { + uninstaller.Logger.Infof("VM %s powered off", vm.MustName()) + } else { + uninstaller.Logger.Warnf("Waited %d for VM %s to power-off: %s", waitForDownDuration, vm.MustName(), err) } } func (uninstaller *ClusterUninstaller) removeVM(vmsService *ovirtsdk.VmsService, vm *ovirtsdk.Vm) { vmService := vmsService.VmService(vm.MustId()) _, err := vmService.Remove().Send() - uninstaller.Logger.Infof("Removing VM %s : %s", vm.MustName(), "errors: %s", err) - if err != nil { - uninstaller.Logger.Debugf("Failed removing VM %s : %s", vm.MustName(), err) + if err == nil { + uninstaller.Logger.Infof("Removing VM %s", vm.MustName()) + } else { + uninstaller.Logger.Debugf("Failed to remove VM %s : %s", vm.MustName(), err) } } From 84f8fe27a8151f3f2320ee89c99a42816c9cab9b Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Fri, 17 Apr 2020 14:45:57 -0700 Subject: [PATCH 2/4] assets/ovirt: clean up prompts This brings the language used in the prompts inline with the style used in the other platforms and adds some more detail to the help messages. --- pkg/asset/installconfig/ovirt/cluster.go | 4 ++-- pkg/asset/installconfig/ovirt/credentials.go | 18 +++++++++--------- pkg/asset/installconfig/ovirt/network.go | 5 ++--- pkg/asset/installconfig/ovirt/ovirt.go | 12 ++++++------ pkg/asset/installconfig/ovirt/storage.go | 4 ++-- 5 files changed, 21 insertions(+), 22 deletions(-) diff --git a/pkg/asset/installconfig/ovirt/cluster.go b/pkg/asset/installconfig/ovirt/cluster.go index f889a3feac0..f7cc2735352 100644 --- a/pkg/asset/installconfig/ovirt/cluster.go +++ b/pkg/asset/installconfig/ovirt/cluster.go @@ -29,8 +29,8 @@ func askCluster(c *ovirtsdk4.Connection, p *ovirt.Platform) (string, error) { clusterNames = append(clusterNames, cluster.MustName()) } err = survey.AskOne(&survey.Select{ - Message: "Select the oVirt cluster", - Help: "The oVirt cluster where the VMs will be created", + Message: "oVirt cluster", + Help: "The oVirt cluster where the VMs will be created.", Options: clusterNames, }, &clusterName, diff --git a/pkg/asset/installconfig/ovirt/credentials.go b/pkg/asset/installconfig/ovirt/credentials.go index 1d8427fdc79..d26c8adaaeb 100644 --- a/pkg/asset/installconfig/ovirt/credentials.go +++ b/pkg/asset/installconfig/ovirt/credentials.go @@ -12,8 +12,8 @@ func askCredentials() (Config, error) { err := survey.Ask([]*survey.Question{ { Prompt: &survey.Input{ - Message: "Enter oVirt's api endpoint URL", - Help: "oVirt engine api url, for example https://ovirt-engine-fqdn/ovirt-engine/api", + Message: "oVirt API endpoint URL", + Help: "The URL of the oVirt engine API. For example, https://ovirt-engine-fqdn/ovirt-engine/api.", }, Validate: survey.ComposeValidators(survey.Required), }, @@ -25,9 +25,9 @@ func askCredentials() (Config, error) { var ovirtCertTrusted bool err = survey.AskOne( &survey.Confirm{ - Message: "Is the installed oVirt certificate trusted?", + Message: "Is the oVirt CA bundle trusted locally?", Default: true, - Help: "", + Help: "In order to securly communicate with the oVirt engine, the certificate authority bundle must either be trusted by the local system or explicitly provided.", }, &ovirtCertTrusted, nil) @@ -48,8 +48,8 @@ func askCredentials() (Config, error) { ovirtURL.Host) err = survey.AskOne(&survey.Multiline{ - Message: "Enter oVirt's CA bundle", - Help: "Obtain oVirt CA bundle from " + pemURL, + Message: "oVirt CA bundle", + Help: fmt.Sprintf("The oVirt CA bundle can be downloaded from %s.", pemURL), }, &c.CABundle, survey.ComposeValidators(survey.Required)) @@ -61,8 +61,8 @@ func askCredentials() (Config, error) { err = survey.Ask([]*survey.Question{ { Prompt: &survey.Input{ - Message: "Enter ovirt-engine username", - Help: "The user must have permissions to create VMs and disks on the Storage Domain with the same name as the OpenShift cluster", + Message: "oVirt engine username", + Help: "The user must have permissions to create VMs and disks on the Storage Domain with the same name as the OpenShift cluster.", Default: "admin@internal", }, Validate: survey.ComposeValidators(survey.Required), @@ -75,7 +75,7 @@ func askCredentials() (Config, error) { err = survey.Ask([]*survey.Question{ { Prompt: &survey.Password{ - Message: "Enter password", + Message: "oVirt engine password", Help: "", }, Validate: survey.ComposeValidators(survey.Required, authenticated(&c)), diff --git a/pkg/asset/installconfig/ovirt/network.go b/pkg/asset/installconfig/ovirt/network.go index a05b02c6579..6690a3aa757 100644 --- a/pkg/asset/installconfig/ovirt/network.go +++ b/pkg/asset/installconfig/ovirt/network.go @@ -29,9 +29,8 @@ func askNetwork(c *ovirtsdk4.Connection, p *ovirt.Platform) error { networkNames = append(networkNames, network.MustName()) } err = survey.AskOne(&survey.Select{ - Message: "Select the oVirt network", - Help: "The oVirt network of the deployed VMs. 'ovirtmgmt' is the default network - it is recommended " + - "to work with a dedicated network per OpenShift cluster", + Message: "oVirt network", + Help: "The oVirt network of the deployed VMs. 'ovirtmgmt' is the default network. It is recommended to use a dedicated network for each OpenShift cluster.", Options: networkNames, }, &networkName, diff --git a/pkg/asset/installconfig/ovirt/ovirt.go b/pkg/asset/installconfig/ovirt/ovirt.go index f97054f366b..705d069df44 100644 --- a/pkg/asset/installconfig/ovirt/ovirt.go +++ b/pkg/asset/installconfig/ovirt/ovirt.go @@ -55,8 +55,8 @@ func Platform() (*ovirt.Platform, error) { err = survey.Ask([]*survey.Question{ { Prompt: &survey.Input{ - Message: "Enter the internal API Virtual IP", - Help: "Make sure the IP address is not in use", + Message: "Internal API virtual IP", + Help: "This is the virtual IP address that will be used to address the OpenShift control plane. Make sure the IP address is not in use.", Default: "", }, Validate: survey.ComposeValidators(survey.Required), @@ -69,8 +69,8 @@ func Platform() (*ovirt.Platform, error) { err = survey.Ask([]*survey.Question{ { Prompt: &survey.Input{ - Message: "Enter the internal DNS Virtual IP", - Help: "Make sure the IP address is not in use", + Message: "Internal DNS virtual IP", + Help: "This is the virtual IP address that will be used to address the DNS server internal to the cluster. Make sure the IP address is not in use.", Default: "", }, Validate: survey.ComposeValidators(survey.Required), @@ -83,8 +83,8 @@ func Platform() (*ovirt.Platform, error) { err = survey.Ask([]*survey.Question{ { Prompt: &survey.Input{ - Message: "Enter the ingress IP ", - Help: "Make sure the IP address is not in use", + Message: "Ingress virtual IP", + Help: "This is the virtual IP address that will be used to address the OpenShift ingress routers. Make sure the IP address is not in use.", Default: "", }, Validate: survey.ComposeValidators(survey.Required), diff --git a/pkg/asset/installconfig/ovirt/storage.go b/pkg/asset/installconfig/ovirt/storage.go index d05c1d9220b..614d4dd16d8 100644 --- a/pkg/asset/installconfig/ovirt/storage.go +++ b/pkg/asset/installconfig/ovirt/storage.go @@ -29,8 +29,8 @@ func askStorage(c *ovirtsdk4.Connection, p *ovirt.Platform, clusterName string) domainNames = append(domainNames, domain.MustName()) } err = survey.AskOne(&survey.Select{ - Message: "Select the oVirt storage domain", - Help: "The storage domain will be used to create the disks of all the cluster nodes", + Message: "oVirt storage domain", + Help: "The storage domain will be used to create the disks of all the cluster nodes.", Options: domainNames, }, &storageDomainName, From 0fbaba75f48818e3a1b1f4a4f38937bc6201fa8d Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Fri, 17 Apr 2020 15:47:50 -0700 Subject: [PATCH 3/4] destroy/ovirt: clean up logging This is a follow-up to d947a588c6. There were a few incorrect formatting verbs and a few redundant logging statements. --- pkg/destroy/ovirt/destroyer.go | 35 +++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/pkg/destroy/ovirt/destroyer.go b/pkg/destroy/ovirt/destroyer.go index 96d4eb14ef9..693e7fd4c7d 100644 --- a/pkg/destroy/ovirt/destroyer.go +++ b/pkg/destroy/ovirt/destroyer.go @@ -33,27 +33,31 @@ func (uninstaller *ClusterUninstaller) Run() error { } defer con.Close() - err = uninstaller.removeVms(con) - uninstaller.Logger.Errorf("Removing VMs - error: %s", err) - err = uninstaller.removeTag(con) - uninstaller.Logger.Errorf("Removing Tag - error: %s", err) - err = uninstaller.removeTemplate(con) - uninstaller.Logger.Errorf("Removing Template - error: %s", err) + if err := uninstaller.removeVMs(con); err != nil { + uninstaller.Logger.Errorf("Failed to remove VMs: %s", err) + } + if err := uninstaller.removeTag(con); err != nil { + uninstaller.Logger.Errorf("Failed to remove tag: %s", err) + } + if err := uninstaller.removeTemplate(con); err != nil { + uninstaller.Logger.Errorf("Failed to remove template: %s", err) + } + return nil } -func (uninstaller *ClusterUninstaller) removeVms(con *ovirtsdk.Connection) error { +func (uninstaller *ClusterUninstaller) removeVMs(con *ovirtsdk.Connection) error { // - find all vms by tag name=infraID vmsService := con.SystemService().VmsService() searchTerm := fmt.Sprintf("tag=%s", uninstaller.Metadata.InfraID) - uninstaller.Logger.Infof("searching VMs by %s", searchTerm) + uninstaller.Logger.Debugf("Searching VMs by %s", searchTerm) vmsResponse, err := vmsService.List().Search(searchTerm).Send() if err != nil { return err } // - stop + delete VMS vms := vmsResponse.MustVms().Slice() - uninstaller.Logger.Infof("Found %s VMs", len(vms)) + uninstaller.Logger.Debugf("Found %d VMs", len(vms)) wg := sync.WaitGroup{} wg.Add(len(vms)) for _, vm := range vms { @@ -77,10 +81,9 @@ func (uninstaller *ClusterUninstaller) removeTag(con *ovirtsdk.Connection) error if tagsServiceListResponse != nil { for _, t := range tagsServiceListResponse.MustTags().Slice() { if t.MustName() == uninstaller.Metadata.InfraID { + uninstaller.Logger.Infof("Removing tag %s", t.MustName()) _, err := tagsService.TagService(t.MustId()).Remove().Send() - uninstaller.Logger.Infof("Removing tag %s : %s", t.MustName(), "errors: %s", err) if err != nil { - uninstaller.Logger.Debugf("Failed removing tag %s : %s", t.MustName(), err) return err } } @@ -96,14 +99,14 @@ func (uninstaller *ClusterUninstaller) stopVM(vmsService *ovirtsdk.VmsService, v if err == nil { uninstaller.Logger.Infof("Stopping VM %s", vm.MustName()) } else { - uninstaller.Logger.Debugf("Failed to stop VM %s : %s", vm.MustName(), err) + uninstaller.Logger.Errorf("Failed to stop VM %s: %s", vm.MustName(), err) } waitForDownDuration := time.Minute * 10 err = vmService.Connection().WaitForVM(vm.MustId(), ovirtsdk.VMSTATUS_DOWN, waitForDownDuration) if err == nil { uninstaller.Logger.Infof("VM %s powered off", vm.MustName()) } else { - uninstaller.Logger.Warnf("Waited %d for VM %s to power-off: %s", waitForDownDuration, vm.MustName(), err) + uninstaller.Logger.Warnf("Waited %d for VM %s to power off: %s", waitForDownDuration, vm.MustName(), err) } } @@ -113,7 +116,7 @@ func (uninstaller *ClusterUninstaller) removeVM(vmsService *ovirtsdk.VmsService, if err == nil { uninstaller.Logger.Infof("Removing VM %s", vm.MustName()) } else { - uninstaller.Logger.Debugf("Failed to remove VM %s : %s", vm.MustName(), err) + uninstaller.Logger.Errorf("Failed to remove VM %s: %s", vm.MustName(), err) } } @@ -122,8 +125,7 @@ func (uninstaller *ClusterUninstaller) removeTemplate(con *ovirtsdk.Connection) search, err := con.SystemService().TemplatesService(). List().Search(fmt.Sprintf("name=%s", uninstaller.Metadata.InfraID)).Send() if err != nil { - uninstaller.Logger.Errorf("Couldn't find a template with name %s", uninstaller.Metadata.InfraID) - return nil + return fmt.Errorf("couldn't find a template with name %s", uninstaller.Metadata.InfraID) } if result, ok := search.Templates(); ok { // the results can potentially return a list of template @@ -133,7 +135,6 @@ func (uninstaller *ClusterUninstaller) removeTemplate(con *ovirtsdk.Connection) service := con.SystemService().TemplatesService().TemplateService(tmp.MustId()) _, err := service.Remove().Send() if err != nil { - uninstaller.Logger.Errorf("Failed to remove Template %s %s", tmp.MustName(), tmp.MustId()) return err } } From 4cf6223cb5ce09466ac7f50c48fb2f348f1704d0 Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Sat, 18 Apr 2020 09:36:36 -0700 Subject: [PATCH 4/4] installconfig/ovirt: fix CA prompt and add warning This is a follow-on to 3eccc348. After looking at this more closely, I realized that the cert bundle is only prompted if the CA is trusted locally. This means that if the CA isn't trusted, the communication with the oVirt engine will be insecure. I added a warning to reflect that reality. --- pkg/asset/installconfig/ovirt/credentials.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/asset/installconfig/ovirt/credentials.go b/pkg/asset/installconfig/ovirt/credentials.go index d26c8adaaeb..3afecceac52 100644 --- a/pkg/asset/installconfig/ovirt/credentials.go +++ b/pkg/asset/installconfig/ovirt/credentials.go @@ -4,6 +4,7 @@ import ( "fmt" "net/url" + "github.com/sirupsen/logrus" "gopkg.in/AlecAivazis/survey.v1" ) @@ -25,9 +26,9 @@ func askCredentials() (Config, error) { var ovirtCertTrusted bool err = survey.AskOne( &survey.Confirm{ - Message: "Is the oVirt CA bundle trusted locally?", + Message: "Is the oVirt CA trusted locally?", Default: true, - Help: "In order to securly communicate with the oVirt engine, the certificate authority bundle must either be trusted by the local system or explicitly provided.", + Help: "In order to securly communicate with the oVirt engine, the certificate authority must be trusted by the local system.", }, &ovirtCertTrusted, nil) @@ -48,14 +49,16 @@ func askCredentials() (Config, error) { ovirtURL.Host) err = survey.AskOne(&survey.Multiline{ - Message: "oVirt CA bundle", - Help: fmt.Sprintf("The oVirt CA bundle can be downloaded from %s.", pemURL), + Message: "oVirt certificate bundle", + Help: fmt.Sprintf("The oVirt certificate bundle can be downloaded from %s.", pemURL), }, &c.CABundle, survey.ComposeValidators(survey.Required)) if err != nil { return c, err } + } else { + logrus.Warning("Communication with the oVirt engine will be insecure.") } err = survey.Ask([]*survey.Question{