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

Openshift machine api #1175

Merged
merged 2 commits into from
Feb 6, 2019
Merged

Openshift machine api #1175

merged 2 commits into from
Feb 6, 2019

Conversation

enxebre
Copy link
Member

@enxebre enxebre commented Feb 1, 2019 •
edited
Loading

We are transitioning the cluster API types from k8s.io over a machine.openshift.io group.
https://github.com/openshift/cluster-api/tree/91fca585a85b163ddfd119fd09c128c9feadddca
Although this give us a necessary level of autonomy today, we aim to consolidate back with upstream once there's a release of the API stable enough
For reference:
Backward compatible changes on machine api operator
openshift/machine-api-operator#185
openshift/machine-api-operator#188
openshift/machine-api-operator#191
AWS actuator support for new group
openshift/cluster-api-provider-aws#147

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 1, 2019
@enxebre enxebre force-pushed the openshift-machine-api branch from 0727ff9 to 6a7441a Compare February 1, 2019 16:12
@enxebre enxebre changed the title WIP - Openshift machine api Openshift machine api Feb 1, 2019
@openshift-ci-robot 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 Feb 1, 2019
@enxebre
Copy link
Member Author

enxebre commented Feb 1, 2019 •
edited
Loading

cc @dgoodwin @jwforres @derekwaynecarr

@enxebre
Copy link
Member Author

enxebre commented Feb 1, 2019

/test e2e-aws

1 similar comment
@enxebre
Copy link
Member Author

enxebre commented Feb 1, 2019

/test e2e-aws

@jwforres
Copy link
Member

jwforres commented Feb 1, 2019

cc @spadgett since this impacts the console

@spadgett spadgett mentioned this pull request Feb 1, 2019
Merged
@spadgett
Copy link
Member

spadgett commented Feb 1, 2019

Are there any API changes other than API group and version?

WIP console PR at openshift/console#1161

@enxebre
Copy link
Member Author

enxebre commented Feb 1, 2019

/test e2e-aws

1 similar comment
@enxebre
Copy link
Member Author

enxebre commented Feb 4, 2019

/test e2e-aws

@enxebre
Copy link
Member Author

enxebre commented Feb 4, 2019 •
edited
Loading

friendly ping @abhinavdahiya @wking @derekwaynecarr @smarterclayton

This was referenced Feb 4, 2019
Merged
Closed
Closed
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2019
abhinavdahiya
abhinavdahiya reviewed Feb 5, 2019
View reviewed changes
pkg/asset/machines/master.go
@@ -144,7 +145,20 @@ func masterPool(pools []types.MachinePool) types.MachinePool {
return types.MachinePool{}
}

func listFromMachines(objs []clusterapi.Machine) *metav1.List {
func listFromMachines(objs []machineapi.Machine) *metav1.List {
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change.. ?

Copy link
Member Author

@enxebre enxebre Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

because machines now belong to machine.openshift.io API group, hence the machineapi.Machine type. See relevant references in the description. Please let me know if this is not clear

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry wrong line, more as to why keep the deprecated one too? It that bring used?

Copy link
Member Author

@enxebre enxebre Feb 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@abhinavdahiya we are providing a backward compatible pivoting path for any actuator openshift/machine-api-operator#192 This PR finalises the pivot for aws. We'll put a follow up for libvirt right away and once the openstack actuator is updated to rely on machine.openshift.io and they put a PR analogous to this one for Opensatck we will fully drop support for cluster.k8s.io either here and on the machine api operator openshift/machine-api-operator#195

@enxebre enxebre force-pushed the openshift-machine-api branch from 6a7441a to 6d2764c Compare February 5, 2019 08:27
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2019
@enxebre
Copy link
Member Author

enxebre commented Feb 5, 2019

/test e2e-openstack

@enxebre
Copy link
Member Author

enxebre commented Feb 5, 2019 •
edited
Loading

openstack:
level=fatal msg="failed to fetch Terraform Variables: failed to load asset \"Install Config\": invalid \"install-config.yaml\" file: [platform.openstack.region: Unsupported value: \"RegionOne\": supported values: \"regionOne\", platform.openstack.computeFlavor: Unsupported value: \"\": supported values: \"m1.large\", \"m1.large2\", \"m1.tiny\", \"m1.small\", \"m1.medium\", \"m1.xlarge\"]"
libvirt:
error: Missing or incomplete configuration info. Please login or point to an existing, complete config file:

/test e2e-libvirt
/test e2e-openstack

@flaper87
Copy link
Contributor

flaper87 commented Feb 5, 2019

@enxebre the openstack job should be optional. We're aware that it's broken. Sadly, we need to move this job to another cloud and we're working on getting such cloud ready. 😄

@enxebre enxebre mentioned this pull request Feb 5, 2019
Closed
@derekwaynecarr
Copy link
Member

@flaper87 - given it’s broken, any objection to merging this and have openstack make similar update at a later date as part of restoring the job behavior?

@trown
Copy link

trown commented Feb 5, 2019

WRT openstack: I can manually test against my personal vexxhost tenant.

@enxebre
Copy link
Member Author

enxebre commented Feb 5, 2019

@trown thanks, just to clarify this openstack/libvirt fail are unrelated to this PR. As discussed with @flaper87 we are providing a backward compatible pivoting path for any actuator openshift/machine-api-operator#192 Once there's a PR analogous to this one for Opensatck we will fully drop support for cluster.k8s.io on the machine api operator openshift/machine-api-operator#195

@enxebre
Copy link
Member Author

enxebre commented Feb 6, 2019

cc @abhinavdahiya @wking

@wking
Copy link
Member

wking commented Feb 6, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, enxebre, wking

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 [abhinavdahiya,wking]

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

@enxebre
Copy link
Member Author

enxebre commented Feb 6, 2019

/test e2e-aws

3 similar comments
@enxebre
Copy link
Member Author

enxebre commented Feb 6, 2019

/test e2e-aws

@enxebre
Copy link
Member Author

enxebre commented Feb 6, 2019

/test e2e-aws

@enxebre
Copy link
Member Author

enxebre commented Feb 6, 2019

/test e2e-aws

@derekwaynecarr
Copy link
Member

e2e -aws failure was vpc limit.

/test e2e-aws

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Feb 6, 2019 •
edited
Loading

@enxebre: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
ci/prow/e2e-openstack cf60daa link /test e2e-openstack
ci/prow/e2e-libvirt cf60daa link /test e2e-libvirt

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@enxebre
Copy link
Member Author

enxebre commented Feb 6, 2019 •
edited
Loading

/test tf-lint

1 similar comment
@enxebre
Copy link
Member Author

enxebre commented Feb 6, 2019

/test tf-lint

@openshift-merge-robot openshift-merge-robot merged commit 809a7d0 into openshift:master Feb 6, 2019
@wking wking mentioned this pull request Feb 7, 2019
Merged
@wking wking mentioned this pull request Feb 8, 2019
Merged
@dgoodwin dgoodwin mentioned this pull request Feb 8, 2019
Merged
4 tasks
wking added a commit to wking/openshift-installer that referenced this pull request Feb 11, 2019
@wking
pkg/asset/machines: Convert Master to a WriteableAsset
7a396d9 
Since 3326b6b (pkg/tfvars: Pull from []Machine instead of
InstallConfig, 2018-11-28, openshift#792), we've been consuming structured
master configurations to generate Terraform variables.  But before
this commit, the Openshift asset was the WriteableAsset responsible
for the master configs, giving you the following issue:

1. openshift-install create manifests
  i. Master asset populated with structured data
  ii. Openshift asset pulls in Master, flattens to YAML, and writes to
      disk.

2. openshift-install create cluster
  i. Openshift asset reads master YAML from disk.
  ii. TerraformVariables asset pulls in Master, but it's empty.
  iii. Panic [1].

With this commit, responsibility for reading and writing master
manifests to disk gets shifted to the Master asset itself.

I've dropped the structured Master properties (Machines and
MachineDeprecated) in favor of new methods on the asset.  There are
three possible paths towards recovering the asset that we feed in to
TerraformVariables:

a. After rendering it with Generate.  This information had been
   structured before this commit, so no problems here.
b. After reading from the disk.  This information could be
   unmarshalled in Master.Load(), but...
c. After loading it from the state file.  There are no hooks for
   custom unmarshalling in this pathway, so while the generic YAML
   unmarshal would give us a structured machine object, it would not
   unmarshal the RawExtension that holds the provider spec.

The lack of custom-unmarshal hooks for (c) means there's no reliable
way to use Master properties to feed TerraformVariables structured
provider information.  With this commit, we use Master methods
instead.  That's just as efficient for the (b) and (c) cases, and
while it's an uneccessary (de)serialize cycle for (a), that's not too
expensive.

Unpacking the YAML data into the appropriate structures is a bit
hairy.  I'm not familiar with this code though, maybe there's a better
way.  It will help once we complete the shift to the OpenShift
machine-API types started in cf60daa (Pivot aws from cluster.k8s.io
into machine.openshift.io, 2019-02-01, openshift#1175).

I've also taken the opportunity to drop the list.  It saves us a few
lines of code for (de)serializing, and there's no upside to collecting
the Machine configs together in a single file.

The "glob, but not the master files" logic in the Openshift loader is
also a bit ugly.  Moving forward, I expect us to push the remaining
Openshift assets out into their own assets as well, which would allow
us to tighten down on the wildcard there.

There's also a bit of dancing to ensure our Machine filenames are
ordered by increasing index.  I'm padding the filenames with %02d (for
example) to get ...-01.yaml, etc. so they come back in the right order
on load (filepath.Glob sorts its output [2]).  To avoid hard-coding a
pad size, I format the largest index, measure its length, and use that
length to construct padFormat.

[1]: openshift#1205
[2]: https://github.com/golang/go/blob/go1.11.5/src/path/filepath/match.go#L325
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abhinavdahiya abhinavdahiya abhinavdahiya left review comments

@russellb russellb Awaiting requested review from russellb

@tomassedovic tomassedovic Awaiting requested review from tomassedovic

Assignees

@wking wking

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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.