Bug 1880443: fix Openstack machinesets #1141

joelddiaz · 2020-09-17T22:19:18Z

vendor in new installer with ability to provide our own OpenStack credentials fetching functions
provide custom credentials fetching functions that use the contents of the ClusterDeployment's secret as the credentials source
provide an empty (non-nil) list of Zones when creating MachinePools to MachineSets for OpenStack

joelddiaz · 2020-09-17T22:19:58Z

this depends on the changes in openshift/installer#4196

staebler · 2020-09-17T22:32:51Z

pkg/controller/remotemachineset/openstackactuator.go


+	"github.com/gophercloud/utils/openstack/clientconfig"


Suggested change

"github.com/gophercloud/utils/openstack/clientconfig"

"github.com/gophercloud/utils/openstack/clientconfig"

staebler · 2020-09-17T22:35:15Z

pkg/controller/remotemachineset/openstackactuator.go

+
+	credsSecret := &corev1.Secret{}
+	if err := kubeClient.Get(context.TODO(), credsSecretKey, credsSecret); err != nil {
+		return nil, err


Suggested change

return nil, err

return nil, errors.Wrap(err, "failed to get OpenStack credentials")

staebler · 2020-09-17T22:35:36Z

pkg/controller/remotemachineset/openstackactuator.go

+
+	cloudsYaml, ok := credsSecret.Data[constants.OpenStackCredentialsName]
+	if !ok {
+		return nil, errors.New("did not find credentials in the ClusterDeployment's secret")


Suggested change

return nil, errors.New("did not find credentials in the ClusterDeployment's secret")

return nil, errors.New("did not find credentials in the OpenStack credentials secret")

staebler · 2020-09-17T22:37:53Z

pkg/controller/remotemachineset/openstackactuator.go

+	var clouds clientconfig.Clouds
+	if err := yaml.Unmarshal(cloudsYaml, &clouds); err != nil {


How stable is clientconfig.Clouds? Do you anticipate any issues with supporting older OpenShift versions that may use an older version of this type?

This is a good question. Outside of this PR I've never dealt with this package before.
Now that you ask, I would worry not so much about versions of OpenShift but the version of OpenStack that this package will be making calls into. Is there someone more familiar with OpenStack that can perhaps answer this?

As far as OpenShift releases, older versions of the installer made you specify whether this "trunk" support was available, and now it is dynamic. And we mark the field as optional in Hive https://github.com/openshift/hive/blob/master/pkg/apis/hive/v1/openstack/platform.go#L19. This feels like we've just automated detection of what you previously had to specify.

staebler · 2020-09-17T22:39:51Z

pkg/controller/remotemachineset/openstackactuator.go

+	var secureClouds clientconfig.Clouds
+	return secureClouds.Clouds, nil


Isn't this just an nil map?

Suggested change

var secureClouds clientconfig.Clouds

return secureClouds.Clouds, nil

return nil, nil

staebler · 2020-09-17T22:46:58Z

pkg/controller/remotemachineset/openstackactuator.go

@@ -76,6 +76,7 @@ func (a *OpenStackActuator) GenerateMachineSets(cd *hivev1.ClusterDeployment, po
 	computePool := baseMachinePool(pool)
 	computePool.Platform.OpenStack = &installertypesosp.MachinePool{
 		FlavorName: pool.Spec.Platform.OpenStack.Flavor,
+		Zones:      []string{""},


The commit message is not quite right. It states that Hive is providing "an empty (but non-nil) list of zones." However, Hive is actually providing a list with a single empty entry.

I also think it is worth a comment in the code to explain what is happening here.

staebler · 2020-09-17T22:51:15Z

pkg/controller/remotemachineset/openstackactuator.go

@@ -76,6 +76,7 @@ func (a *OpenStackActuator) GenerateMachineSets(cd *hivev1.ClusterDeployment, po
 	computePool := baseMachinePool(pool)
 	computePool.Platform.OpenStack = &installertypesosp.MachinePool{
 		FlavorName: pool.Spec.Platform.OpenStack.Flavor,
+		Zones:      []string{""},


It looks like Hive needs to start supporting zones in MachinePools for OpenStack.
https://github.com/openshift/installer/blob/96579442a7526367e86cf916ec85089c18dfaf24/pkg/types/openstack/machinepool.go#L26-L29

Agreed. Created a Jira card on our backlog.

staebler · 2020-09-17T22:53:15Z

Is there any chance that we can re-enable the unit test for the OpenStack actuator as part of this PR? Or are we still far away from that since we have no way of mocking out the call to OpenStack that is baked into the installer?

openshift-ci-robot · 2020-09-18T14:40:36Z

@joelddiaz: This pull request references Bugzilla bug 1880443, which is invalid:

expected the bug to target the "4.6.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

WIP: Bug 1880443: fix Openstack machinesets

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

joelddiaz · 2020-09-18T15:30:56Z

Is there any chance that we can re-enable the unit test for the OpenStack actuator as part of this PR? Or are we still far away from that since we have no way of mocking out the call to OpenStack that is baked into the installer?

Not yet. We would need more reworking of the installer repo to allow us to stub out the OpenStack client.

joelddiaz · 2020-10-23T17:45:40Z

/assign @staebler
/cc @akhil-rane

this PR pre-dates us explicitly assigning and CCing people...

Use the new ability of the installer to provide OpenStack client options to allow use to provide the contents of the ClusterDeployment's credentials Secret as the "clouds.yaml" file.

The installer machineset generator will distribute all the replicas across the list of zones. If we do not provide a list of zones, we get back an empty list of machinesets. Follow what the installer does, and provide list of zones with exactly one member consisting of the empty string.

joelddiaz · 2020-11-10T18:40:58Z

staebler's hive watch has ended..
/assign @dgoodwin

dgoodwin · 2020-11-18T18:47:16Z

/lgtm

openshift-ci-robot · 2020-11-18T18:47:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dgoodwin, joelddiaz

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [dgoodwin,joelddiaz]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

joelddiaz · 2020-11-19T16:02:57Z

/bugzilla refresh

openshift-ci-robot · 2020-11-19T16:03:04Z

@joelddiaz: This pull request references Bugzilla bug 1880443, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.7.0) matches configured target release for branch (4.7.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot · 2020-11-23T22:41:38Z

@joelddiaz: All pull requests linked via external trackers have merged:

Bugzilla bug 1880443 has been moved to the MODIFIED state.

In response to this:

Bug 1880443: fix Openstack machinesets

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 17, 2020

openshift-ci-robot requested review from dgoodwin and twiest September 17, 2020 22:19

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 17, 2020

staebler reviewed Sep 17, 2020

View reviewed changes

joelddiaz changed the title ~~WIP: fix Openstack machinesets~~ WIP: Bug 1880443: fix Openstack machinesets Sep 18, 2020

openshift-ci-robot added the bugzilla/severity-unspecified Referenced Bugzilla bug's severity is unspecified for the PR. label Sep 18, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Sep 18, 2020

joelddiaz force-pushed the openstack-machinesets branch 2 times, most recently from c4f3120 to a8da86e Compare September 18, 2020 15:02

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2020

joelddiaz force-pushed the openstack-machinesets branch from a8da86e to 3a85fb3 Compare September 28, 2020 13:35

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 28, 2020

twiest removed their request for review October 1, 2020 18:19

openshift-ci-robot mentioned this pull request Oct 23, 2020

Bug 1880443: allow providing client options when generating OpenStack MachineSets openshift/installer#4196

Merged

joelddiaz force-pushed the openstack-machinesets branch from 3a85fb3 to 107fa0c Compare October 23, 2020 17:39

joelddiaz changed the title ~~WIP: Bug 1880443: fix Openstack machinesets~~ Bug 1880443: fix Openstack machinesets Oct 23, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 23, 2020

openshift-ci-robot assigned staebler Oct 23, 2020

openshift-ci-robot requested a review from akhil-rane October 23, 2020 17:45

joelddiaz force-pushed the openstack-machinesets branch from 107fa0c to f7de382 Compare October 23, 2020 18:34

Joel Diaz added 2 commits November 10, 2020 09:36

provide our own OpenStack config loading functions

e295799

Use the new ability of the installer to provide OpenStack client options to allow use to provide the contents of the ClusterDeployment's credentials Secret as the "clouds.yaml" file.

joelddiaz force-pushed the openstack-machinesets branch from f7de382 to 39bf33c Compare November 10, 2020 14:38

openshift-ci-robot assigned dgoodwin Nov 10, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2020

openshift-ci-robot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Nov 19, 2020

openshift-merge-robot merged commit cb41795 into openshift:master Nov 23, 2020

joelddiaz mentioned this pull request Dec 9, 2020

MachinePool not creating corresponding MachineSets on Openstack #1228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1880443: fix Openstack machinesets #1141

Bug 1880443: fix Openstack machinesets #1141

joelddiaz commented Sep 17, 2020

joelddiaz commented Sep 17, 2020

staebler Sep 17, 2020

staebler Sep 17, 2020

staebler Sep 17, 2020

staebler Sep 17, 2020

joelddiaz Sep 18, 2020

staebler Sep 17, 2020

staebler Sep 17, 2020

staebler Sep 17, 2020

joelddiaz Sep 18, 2020

staebler commented Sep 17, 2020

openshift-ci-robot commented Sep 18, 2020

joelddiaz commented Sep 18, 2020

joelddiaz commented Oct 23, 2020

joelddiaz commented Nov 10, 2020

dgoodwin commented Nov 18, 2020

openshift-ci-robot commented Nov 18, 2020

joelddiaz commented Nov 19, 2020

openshift-ci-robot commented Nov 19, 2020

openshift-ci-robot commented Nov 23, 2020


	"github.com/gophercloud/utils/openstack/clientconfig"
	"github.com/gophercloud/utils/openstack/clientconfig"

	return nil, err
	return nil, errors.Wrap(err, "failed to get OpenStack credentials")

	return nil, errors.New("did not find credentials in the ClusterDeployment's secret")
	return nil, errors.New("did not find credentials in the OpenStack credentials secret")

		var clouds clientconfig.Clouds
		if err := yaml.Unmarshal(cloudsYaml, &clouds); err != nil {

		var secureClouds clientconfig.Clouds
		return secureClouds.Clouds, nil

	var secureClouds clientconfig.Clouds
	return secureClouds.Clouds, nil
	return nil, nil

Bug 1880443: fix Openstack machinesets #1141

Bug 1880443: fix Openstack machinesets #1141

Conversation

joelddiaz commented Sep 17, 2020

joelddiaz commented Sep 17, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

staebler commented Sep 17, 2020

openshift-ci-robot commented Sep 18, 2020

joelddiaz commented Sep 18, 2020

joelddiaz commented Oct 23, 2020

joelddiaz commented Nov 10, 2020

dgoodwin commented Nov 18, 2020

openshift-ci-robot commented Nov 18, 2020

joelddiaz commented Nov 19, 2020

openshift-ci-robot commented Nov 19, 2020

openshift-ci-robot commented Nov 23, 2020