make volumeClient failure fatal only when needed #75
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sjenning The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
size/S
|
/test e2e-openstack |
|
/test e2e-openstack Thanks for the PR, the change is reasonable and the code looks good, however the CI is failing for unrelated reasons. Hoping to see at least one green run before we merge this. |
|
/test e2e-openstack |
| klog.Infof("Creating a bootable volume from image %v.", config.RootVolume.SourceUUID) | ||
|
|
||
| if is.volumeClient == nil { | ||
| return nil, fmt.Errorf("Volume requested but block storage service is not available") |
There was a problem hiding this comment.
Thanks! I like the change, but I'd prefer to display the original error when creating the client too. Would you be open to adding that?
There was a problem hiding this comment.
This could be achieved with client lazy-loading, by storing volumeClient in InstanceService as a closure in NewInstanceServiceFromCloud.
volumeClient := func() (*gophercloud.ServiceClient, error) {
return openstack.NewBlockStorageV3(provider, gophercloud.EndpointOpts{
Region: clientOpts.RegionName,
})
}
Depending on the arguments passed to kubeadm, it may or may not output more data to stdout. Since we're shelling out and depending on the kubeadm cli to generate the token, this commit fixes the token parsing by splitting the output into lines and getting the very last one. This is far from ideal, as in we should not be depending on kubeadm to generate the token. Until that is fixed, this patch seems to be a good compromise. Closes openshift#74
Depending on the arguments passed to kubeadm, it may or may not output more data to stdout. Since we're shelling out and depending on the kubeadm cli to generate the token, this commit fixes the token parsing by splitting the output into lines and getting the very last one. This is far from ideal, as in we should not be depending on kubeadm to generate the token. Until that is fixed, this patch seems to be a good compromise. Closes openshift#74
5 participants
Currently, this provider will not work on an Openstack platform without Cinder, even if nothing specifies a root volume, which is the only need for it.
This PR makes the failure to create a
volumeClientfatal only if we actually need to create a volume.@eparis @erich @rhowe