OpenStack provider support #144
Conversation
openshift-ci-robot
added
the
do-not-merge/work-in-progress
|
Can one of the admins verify this patch? |
openshift-ci-robot
added
the
size/XXL
|
One initial comment - the patch series contains several fixup patches (variable names etc), it might be worth rebasing to squash those into the related feature patch - agree having a series of incremental additions vs one huge patch is a good idea though. |
flaper87
force-pushed
the
openstack
branch
2 times, most recently
from
e9f9165 to
fcf08dc
Compare
|
@hardys good point. I had actually done that after submitting the PR and thought the push had gone through |
steps/assets/openstack/main.tf
Outdated
|
|
||
| data "ignition_user" "ssh_authorized_key" { | ||
| name = "core" | ||
| ssh_authorized_keys = ["${data.openstack_compute_keypair_v2.openstack_ssh_key.public_key}"] |
There was a problem hiding this comment.
I wonder if we can simplify this a bit by adding support for metadata to the ignition openstack provider?
E.g atm it only reads the user_data from the metadata service/config-drive not http://169.254.169.254/openstack/2012-08-10/meta_data.json which will contain some predictable data like the hostname and ssh key?
If we added that it'd also make the initial boot of nodes with empty user_data easier as at least the ssh keys etc could be set up without explicit configuration.
|
I see golint failing, so: /lint ^this should give us inline comments for any issues. [Edit: looks like I still need to enable that plugin for this repo.] |
|
openshift/release#1205 is live now, so trying again: /lint |
openshift-ci-robot
left a comment
openshift-ci-robot
left a comment
There was a problem hiding this comment.
@wking: 8 warnings.
Details
In response to this:
openshift/release#1205 is live now, so trying again:
/lint
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.
| Password string `json:"tectonic_openstack_credentials_password,omitempty" yaml:"password,omitempty"` | ||
| Token string `json:"tectonic_openstack_credentials_token,omitempty" yaml:"token,omitempty"` | ||
| UserDomainName string `json:"tectonic_openstack_credentials_user_domain_name,omitempty" yaml:"userDomainName,omitempty"` | ||
| UserDomainId string `json:"tectonic_openstack_credentials_user_domain_id,omitempty" yaml:"userDomainId,omitempty"` |
There was a problem hiding this comment.
Golint naming: struct field UserDomainId should be UserDomainID. More info.
| UserDomainName string `json:"tectonic_openstack_credentials_user_domain_name,omitempty" yaml:"userDomainName,omitempty"` | ||
| UserDomainId string `json:"tectonic_openstack_credentials_user_domain_id,omitempty" yaml:"userDomainId,omitempty"` | ||
| ProjectDomainName string `json:"tectonic_openstack_credentials_project_domain_name,omitempty" yaml:"projectDomainName,omitempty"` | ||
| ProjectDomainId string `json:"tectonic_openstack_credentials_project_domain_id,omitempty" yaml:"projectDomainId,omitempty"` |
There was a problem hiding this comment.
Golint naming: struct field ProjectDomainId should be ProjectDomainID. More info.
| UserDomainId string `json:"tectonic_openstack_credentials_user_domain_id,omitempty" yaml:"userDomainId,omitempty"` | ||
| ProjectDomainName string `json:"tectonic_openstack_credentials_project_domain_name,omitempty" yaml:"projectDomainName,omitempty"` | ||
| ProjectDomainId string `json:"tectonic_openstack_credentials_project_domain_id,omitempty" yaml:"projectDomainId,omitempty"` | ||
| DomainId string `json:"tectonic_openstack_credentials_domain_id,omitempty" yaml:"domainId,omitempty"` |
There was a problem hiding this comment.
Golint naming: struct field DomainId should be DomainID. More info.
| @@ -0,0 +1,115 @@ | |||
| package openstack | |||
There was a problem hiding this comment.
Golint comments: should have a package comment, unless it's in another file for this package. More info.
| Type string `json:"tectonic_openstack_worker_root_volume_type,omitempty" yaml:"type,omitempty"` | ||
| } | ||
|
|
||
| type Credentials struct { |
There was a problem hiding this comment.
Golint comments: exported type Credentials should have comment or be unexported. More info.
| } | ||
|
|
||
| type Credentials struct { | ||
| AuthUrl string `json:"tectonic_openstack_credentials_auth_url,omitempty" yaml:"authUrl,omitempty"` |
There was a problem hiding this comment.
Golint naming: struct field AuthUrl should be AuthURL. More info.
| Cloud string `json:"tectonic_openstack_credentials_cloud,omitempty" yaml:"cloud,omitempty"` | ||
| Region string `json:"tectonic_openstack_credentials_region,omitempty" yaml:"region,omitempty"` | ||
| UserName string `json:"tectonic_openstack_credentials_auth_url,omitempty" yaml:"userName,omitempty"` | ||
| UserId string `json:"tectonic_openstack_credentials_user_id,omitempty" yaml:"userId,omitempty"` |
There was a problem hiding this comment.
Golint naming: struct field UserId should be UserID. More info.
| Region string `json:"tectonic_openstack_credentials_region,omitempty" yaml:"region,omitempty"` | ||
| UserName string `json:"tectonic_openstack_credentials_auth_url,omitempty" yaml:"userName,omitempty"` | ||
| UserId string `json:"tectonic_openstack_credentials_user_id,omitempty" yaml:"userId,omitempty"` | ||
| TenantId string `json:"tectonic_openstack_credentials_tenant_id,omitempty" yaml:"tenantId,omitempty"` |
There was a problem hiding this comment.
Golint naming: struct field TenantId should be TenantID. More info.
|
When I tried this, the |
|
So in addition to the conflict, now that the etcd stuff got removed, this no longers works when rebased on master. |
openshift-bot
added
the
needs-rebase
|
Okay, that last commit resolves the crash from earlier. But we still need to rebase & fix the etcd situation. |
tomassedovic
force-pushed
the
openstack
branch
from
c967691 to
a6b2436
Compare
openshift-bot
removed
the
needs-rebase
|
This latest batch rebases on top of master. That means removal of the etcd nodes and addition of the bootstrap step. The tectonic deployment finishes successfully, but the ignition is failing with: So we'll need to investigate that further. |
openshift-bot
added
the
needs-rebase
tomassedovic
force-pushed
the
openstack
branch
from
bde2ae8 to
7afd2a6
Compare
|
@yifan-gu the provider still needs work to be fully functional, but we have enough here to create the OpenStack resources and finish ignition. Could we consider merging it as is and improving the code in followup PRs? Having to constantly rebase this is kind of painful. |
openshift-bot
removed
the
needs-rebase
|
@tomassedovic thanks for rebasing the PR. It sounds like we do have enough functionality implemented to consider merging this PR. @yifan-gu @wking what do you think? It'll be easier for us to contribute and for you to review if we can create smaller PRs for the remaining functionality that needs to be implemented. |
tomassedovic
force-pushed
the
openstack
branch
from
7afd2a6 to
47ed020
Compare
|
Thanks for rebasing it, @tomassedovic It'd be awesome to merge this PR and be able to break the remaining work into smaller pieces. |
wking
left a comment
wking
left a comment
There was a problem hiding this comment.
There are a lot of commits here, and some (e.g. 469c1fb202) seem to be fixups for earlier commits. If we're supposed to review commit by commit, can you squash those fixups into the commits they're fixing (this makes review easier, but can be a lot of work for the submitters)? Or, if you'd rather this be reviewed as a monolithic unit, can you squash it down to a single commit?
config.tf
Outdated
bogdando
left a comment
bogdando
left a comment
There was a problem hiding this comment.
I wanted to ask if there is a chance to adopt the alternative Terraform solution https://github.com/kubernetes-incubator/kubespray/tree/master/contrib/terraform ?
Would be really nice to collaborate with kubespray community on that.
|
@bogdando I'm not sure if that would make sense here, since AFAICS the kubespray terraform templates model the entire cluster, but this codebase is just installing a minimum bootstrap environment then using the cluster itself to scale out and add more nodes - so the two approaches atm are kind of different. Also I note that destroy is not longer handled via terraform - presumably that is so that the cluster-created resources can also be cleaned up on destroy, but I'm unclear if that's part of a longer term move away from terraform for deploy also, perhaps @wking can comment on the plan there. |
I looked at how the AWS destroy works now terraform doesn't destroy things - AFAICS awstagdeprovision is used to delete all-the-things in parallel based on a resource tag, e.g it's no longer really orchestrated but we expect the tags and retries to eventually enable cleanup. I think we can probably do the same for openstack, the server resources already have properties set related to the cluster, but we'll have to add tag data to all the network/subnet/port/router resources. I'm not aware of any similar code to awstagdeprovision for OpenStack, so unless anyone can suggest something I'll look at implementing something in pkg/destroy/openstack, similar to the libvirt one but I'll use tags/properties instead of a prefix. As mentioned by @russellb it'd be great if we can provide that via a follow-up, as rebasing this current large patch has been a massive time-sink. |
That seems fine. We don't have the libvirt destroyer enabled yet either. |
|
Note that currently, bootstrap doesn't get the ignition config because the router we create is missing the external gateway. You can fix it after the fact with: But we'll want to add a prompt/env var to configure it. |
|
Now that #373 has landed, can you squash in b1d6cf2? |
|
I think I already did. The only part left in that commit is aws bits.
…On Mon, Oct 1, 2018 at 1:13 PM W. Trevor King ***@***.***> wrote:
Now that #373 <#373> has
landed, can you squash in b1d6cf2
<b1d6cf2>
?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#144 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAS4CvZbNGiXpua7g5_ywVIbWfCoyY-zks5ugk1HgaJpZM4WA-ff>
.
--
Russell Bryant
|
|
/retest This looks good to me. Any last words before I |
|
Thanks for your help!
…On Mon, Oct 1, 2018 at 1:37 PM W. Trevor King ***@***.***> wrote:
/retest
This looks good to me. Any last words before I /lgtm?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#144 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAS4Cmj02nXFcp22gWhgUa372p8XLtI2ks5uglLngaJpZM4WA-ff>
.
--
Russell Bryant
|
russellb
force-pushed
the
openstack
branch
2 times, most recently
from
c38a45b to
0dd454a
Compare
crawford
dismissed
their stale review
I haven't reviewed this since all of the recent changes.
|
Sweet! This looks really close. I'll let @wking |
This commit includes support for OpenStack as a target deployment platform. There are still some things to implement, such as DNS and destroy support, that will come in future PRs. Contributors (in alphabetical order) include: Co-authored-by: Flavio Percoco <[email protected]> Co-authored-by: Jeremiah Stuever <[email protected]> Co-authored-by: Russell Bryant <[email protected]> Co-authored-by: Steven Hardy <[email protected]> Co-authored-by: Tomas Sedovic <[email protected]> Co-authored-by: W. Trevor King <[email protected]>
|
/test shellcheck |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flaper87, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
/test e2e-aws |
/retest |
|
Note that destroy support has been started via #391 |
15 participants
This PR adds an OpenStack module and the respective steps to run tectonic against an OpenStack cloud.
Fully implemented steps
Features implementation:
Installation/Configuration of the OpenStack provider (currently disabled)@yifan-gu r?