pkg/asset: Add asset for Master machines #491

csrwng · 2018-10-18T21:05:54Z

Generate resources for master machines that can be adopted by the Cluster API machine controller.

csrwng · 2018-10-18T21:06:44Z

@abhinavdahiya as promised
Needs testing, particularly the libvirt and openstack bits

pkg/asset/manifests/tectonic.go

wking · 2018-10-18T22:09:54Z

pkg/asset/machines/aws/master.go

+	ClusterName string
+	AMIID       string
+	Tags        map[string]string
+	Region      string


I think populating this will be more straightforward if we just add an AWSPlatform property. You could even use DefaultMachinePlatform instead of your current Machine.

You could even use DefaultMachinePlatform instead of your current Machine

that opens up the api to vpc, vpccidr block, which might not be ideal

You could even use DefaultMachinePlatform instead of your current Machine.

I don't think that would be correct, we need to account for 3 sources to create the machine.

installer defaults for unfilled values in install config

defaultmachineplatform

machinepoolplatform

that opens up the api to vpc, vpccidr block, which might not be ideal

So create a new type for the shared fields (region and tags, maybe also the AMI?)? You could also push tags and the AMI down into AWSMachinePoolPlatform, although you'd want to leave a tags entry on AWSPlatform for tagging non-machine resources. But I'd like to minimize the number of slightly-different phrasings of types in this space if possible.

You could even use DefaultMachinePlatform instead of your current Machine.

I don't think that would be correct, we need to account for 3 sources to create the machine.

installer defaults for unfilled values in install config

defaultmachineplatform

machinepoolplatform

Sorry, I was trying to suggest using the DefaultMachinePlatform property on a local instance of the AWSPlatform type. You could fill in that information using all of the usual install-config sources. I agree you don't want to use the install-config AWSPlatform.DefaultMachinePlatform instance directly.

pkg/asset/machines/aws/master.go

wking · 2018-10-18T22:13:34Z

pkg/asset/machines/aws/zones.go

+	"github.com/aws/aws-sdk-go/service/ec2"
+)
+
+// AvailabilityZones retrieves a list of availability zones for the given region


nit: this is missing a trailing period (docs).

abhinavdahiya · 2018-10-18T22:21:17Z

pkg/asset/machines/aws/master.go

+  resourceVersion: ""
+  selfLink: ""
+items:
+{{- range $index,$instance := .Instances}}


$c.Instances

wking · 2018-10-18T22:21:43Z

pkg/asset/machines/aws/zones.go

+
+func ec2Client(region string) (*ec2.EC2, error) {
+	awsConfig := &aws.Config{Region: aws.String(region)}
+	s, err := session.NewSession(awsConfig)


I think we should follow this pattern to create AWS sessions. For example, I can't see the govcloud-us region or its availability zones.

+1 for session.

pkg/asset/machines/userdata.go

abhinavdahiya · 2018-10-18T22:25:56Z

/cc @flaper87
can you see if the Machine object for Openstack is correct..?

tomassedovic · 2018-10-19T08:00:41Z

@abhinavdahiya @flaper87 I've checked this for OpenStack and it looks good to me. The deployment works just as it did before too so there's no breakage as far as I can see.

csrwng · 2018-10-19T16:45:35Z

/test golint

csrwng · 2018-10-19T17:17:21Z

@abhinavdahiya @wking thank you for the review. Comments addressed.

abhinavdahiya · 2018-10-19T18:29:36Z

/lgtm

openshift-ci-robot · 2018-10-19T18:29:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, csrwng

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]

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

Adds ClusterK8sIO from e2dc955 (pkg/asset: add ClusterK8sIO, machines.Worker assets, 2018-10-15, openshift#468) and Master from 586ad45 (pkg/asset: Add asset for Master machines, 2018-10-18, openshift#491). Removes KubeCoreOperator from c9b0e2f (manifests: stop using kube core operator, 2018-10-08, openshift#420). Generated with: $ openshift-install graph | dot -Tsvg >docs/design/resource_dep.svg using: $ dot -V dot - graphviz version 2.30.1 (20170916.1124)

Master machine assets were introduced by openshift#491 to adopt instances generated by tf. There was never a follow up to leverage adoption for fatrther addon features. Unless there's a good reason to keep the adoption, this PR removes the machine API master awareness and assets generation

wking reviewed Oct 18, 2018

View reviewed changes

pkg/asset/manifests/tectonic.go Show resolved Hide resolved

wking reviewed Oct 18, 2018

View reviewed changes

abhinavdahiya reviewed Oct 18, 2018

View reviewed changes

pkg/asset/machines/aws/master.go Show resolved Hide resolved

wking reviewed Oct 18, 2018

View reviewed changes

abhinavdahiya reviewed Oct 18, 2018

View reviewed changes

wking reviewed Oct 18, 2018

View reviewed changes

abhinavdahiya reviewed Oct 18, 2018

View reviewed changes

pkg/asset/machines/userdata.go Show resolved Hide resolved

openshift-ci-robot requested a review from flaper87 October 18, 2018 22:25

csrwng force-pushed the master_machines branch from 6e9f774 to 9d913d2 Compare October 19, 2018 16:31

openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 19, 2018

csrwng force-pushed the master_machines branch from 9d913d2 to c9bafcb Compare October 19, 2018 16:40

pkg/asset: Add asset for Master machines

586ad45

csrwng force-pushed the master_machines branch from c9bafcb to 586ad45 Compare October 19, 2018 16:52

csrwng changed the title ~~WIP: pkg/asset: Add asset for Master machines~~ pkg/asset: Add asset for Master machines Oct 19, 2018

openshift-ci-robot assigned abhinavdahiya Oct 19, 2018

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Oct 19, 2018

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2018

openshift-merge-robot merged commit f6477cc into openshift:master Oct 19, 2018

wking mentioned this pull request Oct 20, 2018

docs/design/resource_dep: Rebuild after kube-core removal #508

Merged

enxebre mentioned this pull request Apr 29, 2019

Stop adopting masters with machine API #1687

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/asset: Add asset for Master machines #491

pkg/asset: Add asset for Master machines #491

csrwng commented Oct 18, 2018

csrwng commented Oct 18, 2018

wking Oct 18, 2018 •

edited

Loading

abhinavdahiya Oct 18, 2018

wking Oct 18, 2018

wking Oct 18, 2018

abhinavdahiya Oct 18, 2018

wking Oct 18, 2018

abhinavdahiya Oct 18, 2018 •

edited

Loading

abhinavdahiya commented Oct 18, 2018

tomassedovic commented Oct 19, 2018

csrwng commented Oct 19, 2018

csrwng commented Oct 19, 2018

abhinavdahiya commented Oct 19, 2018

openshift-ci-robot commented Oct 19, 2018

pkg/asset: Add asset for Master machines #491

pkg/asset: Add asset for Master machines #491

Conversation

csrwng commented Oct 18, 2018

csrwng commented Oct 18, 2018

wking Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

abhinavdahiya Oct 18, 2018

Choose a reason for hiding this comment

wking Oct 18, 2018

Choose a reason for hiding this comment

wking Oct 18, 2018

Choose a reason for hiding this comment

abhinavdahiya Oct 18, 2018

Choose a reason for hiding this comment

wking Oct 18, 2018

Choose a reason for hiding this comment

abhinavdahiya Oct 18, 2018 • edited Loading

Choose a reason for hiding this comment

abhinavdahiya commented Oct 18, 2018

tomassedovic commented Oct 19, 2018

csrwng commented Oct 19, 2018

csrwng commented Oct 19, 2018

abhinavdahiya commented Oct 19, 2018

openshift-ci-robot commented Oct 19, 2018

wking Oct 18, 2018 •

edited

Loading

abhinavdahiya Oct 18, 2018 •

edited

Loading