Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add OpenStack as a supported platform #89

Merged
merged 1 commit into from
Oct 16, 2018
Merged

Add OpenStack as a supported platform #89

merged 1 commit into from
Oct 16, 2018

Conversation

flaper87
Copy link
Contributor

@flaper87 flaper87 commented Oct 5, 2018

I'm adding support for OpenStack to the various operators, controllers, and providers that manage an openshift cluster.

I'm not sure if this PR should depend on others so I'd really appreciate some guidance in the process. 😄

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 5, 2018
@wking
Copy link
Member

wking commented Oct 5, 2018

images:

error: unable to upload new layer (0): Patch https://registry.svc.ci.openshift.org/v2/ci-op-n7xnxtmq/release/blobs/uploads/366fa0dc-673f-4cae-8b52-ac4d1f096bdf?_state=Co6UHx_0IOuMH8d4klUWwSfQFAl352HY2XT-iHRctp17Ik5hbWUiOiJjaS1vcC1uN3hueHRtcS9yZWxlYXNlIiwiVVVJRCI6IjM2NmZhMGRjLTY3M2YtNGNhZS04YjUyLWFjNGQxZjA5NmJkZiIsIk9mZnNldCI6MCwiU3RhcnRlZEF0IjoiMjAxOC0xMC0wNVQwOTo0OTowMS45MDI0NTcxMDlaIn0%3D: operator "machine-api-operator" failed to map images: requested mapping for "docker.io/openshift/origin-openstack-machine-controllers:v4.0", but no input image could be located

So getting origin-openstack-machine-controllers pushed (with an openshift/cluster-api-provider-openstack matching this) blocks this PR.

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 6, 2018
enxebre
enxebre reviewed Oct 8, 2018
View reviewed changes
pkg/render/config.go
ReleaseChannel string `json:"releaseChannel"`
ContainerLinuxVersion string `json:"containerLinuxVersion"`
Replicas string `json:"replicas"`
WithCreds bool `json:"withCreds"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WithCreds is used in aws so if set the actuator use those credentials to interact with the aws api, otherwise it uses the ami role in the given machine. How would this be handle for openstack?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would be handled by reading a clouds.yaml file from the system. It's a similar architecture to AWS, just different file format.

@enxebre
Copy link
Member

enxebre commented Oct 8, 2018

thanks a lot! @flaper87 dropped a few comments/questions

@flaper87
Copy link
Contributor Author

flaper87 commented Oct 8, 2018

@wking @enxebre thanks a bunch for the reviews.

So getting origin-openstack-machine-controllers pushed (with an openshift/cluster-api-provider-openstack matching this) blocks this PR.

What's the process to get cluster-api-provider-openstack pushed to the openshift registry? I noticed the respective aws project upstream has been forked under the openshift organization. Should the same be done with OpenStack? Do we have any docs on how I can build and upload those images?

@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2018
@enxebre
Copy link
Member

enxebre commented Oct 8, 2018 •
edited
Loading

@flaper87 we'll need to go through https://jira.coreos.com/browse/CLOUD-145 to add the openstack images to the release process
For building the images from a given commit upstream with could add automation here and embed the openstack binaries on the mao image. @bison @ingvagabund @vikaschoudhary16 thoughts?

@flaper87 flaper87 force-pushed the openstack branch 3 times, most recently from 9021207 to ba4bf5d Compare October 10, 2018 14:37
@flaper87
Copy link
Contributor Author

I think this is ready for another review. I'm working on an e2e-openstack job in parallel but I don't think we should block this patch on that. We'll need more iterations on this.

enxebre
enxebre reviewed Oct 15, 2018
View reviewed changes
machines/openstack/worker.machineset.yaml Outdated
@@ -0,0 +1,45 @@
---
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are planning to move the machineSet specs generation into the installer, so may be move this into that repo, so then we just make sure we deploy the right machine controller here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. So, IIUC, for now, I should just remove this file from the commit and that should be enough. Right?

enxebre
enxebre reviewed Oct 15, 2018
View reviewed changes
pkg/operator/sync.go Outdated Show resolved Hide resolved
enxebre
enxebre reviewed Oct 15, 2018
View reviewed changes
pkg/render/config.go Outdated Show resolved Hide resolved
enxebre
enxebre reviewed Oct 15, 2018
View reviewed changes
pkg/render/config.go Outdated Show resolved Hide resolved
enxebre
enxebre reviewed Oct 15, 2018
View reviewed changes
pkg/render/render_test.go Outdated Show resolved Hide resolved
@flaper87
Copy link
Contributor Author

/test images

@flaper87
Copy link
Contributor Author

/test e2e-aws

enxebre
enxebre reviewed Oct 16, 2018
View reviewed changes
pkg/render/config.go Outdated
@@ -42,12 +43,20 @@ type AWSConfig struct {
WithCreds bool `json:"withCreds"`
}

// OpenStackConfig contains specific config for OpenStack
type OpenStackConfig struct {
ClusterName string `json:"clusterName"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you won't need these values, ClusterName can be something arbitrary and clusterID can be generated on the spec it self, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok ok, I'll remove it 😄

@openshift-ci-robot openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 16, 2018
@enxebre
Copy link
Member

enxebre commented Oct 16, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 16, 2018
@enxebre
Copy link
Member

enxebre commented Oct 16, 2018

/approved

@enxebre enxebre added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2018
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by:

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:

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

@openshift-merge-robot openshift-merge-robot merged commit 5e6bdb9 into openshift:master Oct 16, 2018
@flaper87 flaper87 deleted the openstack branch October 16, 2018 12:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@enxebre enxebre enxebre left review comments

@smarterclayton smarterclayton smarterclayton left review comments

@wking wking wking left review comments

@frobware frobware Awaiting requested review from frobware

@trawler trawler Awaiting requested review from trawler

Assignees

@enxebre enxebre

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@flaper87 @wking @enxebre @openshift-ci-robot @smarterclayton @openshift-bot @openshift-merge-robot