add an API for machinepool -> machinesets #573

abhinavdahiya · 2018-10-30T19:51:15Z

https://github.com/openshift/hive/ recommended that installer expose some api so that the machine and machineset generation for machinepool can be reused.

/cc @dgoodwin @rajatchopra

@dgoodwin

do you want a platform agnostic API?
the API assumes completely valid machinepool definition, no defaulting is applied. (@dgoodwin wanted the api to not consume client for cloud)

builds on #567

pkg/asset/machines/aws/master.go

pkg/asset/machines/aws/worker.go

pkg/asset/machines/master.go

abhinavdahiya · 2018-10-31T14:20:45Z

@dgoodwin
if the all hive does is use install-config give it to the installer to create stuff, all the defaulting is handled by the installer

if hive wants to create new machinsets/machines, later on, this API requires defaulting for that to be handled explicitly. I don't think installer's defaults make sense then?

dgoodwin · 2018-10-31T14:47:37Z

The problem is Hive would expose an API with fields that are optional and would need defaults, we would convert to InstallConfig and pass to you, but we have no way to know what defaults were applied.

As the cluster lives on and we maintain machine sets, we're still just operating off that config the user specified which may not have all fields set.

Is there a reason this API can't apply the same defaults as it did during install?

Or do we need shared APIs to calculate defaults and apply to an install config?

dgoodwin · 2018-10-31T14:54:40Z

Thinking about this some more, the AMI specifically can't be a live lookup, it would change over time which we don't want. I think we may need to have Hive explicitly apply defaults that match yours?

wking · 2018-10-31T16:21:57Z

The problem is Hive would expose an API with fields that are optional and would need defaults, we would convert to InstallConfig and pass to you, but we have no way to know what defaults were applied.

Can you use sentinel values for "I don't care, let the installer choose"? Then just leave those unset in your install-config (related: #263), and we'll fill in our defaults for the unset fields. I'm not sure if we do that at the moment, but it shouldn't be too hard.

abhinavdahiya · 2018-10-31T16:27:39Z

@wking

Can you use sentinel values for "I don't care, let the installer choose"? Then just leave those unset in your install-config

we already default.

As the cluster lives on and we maintain machine sets, we're still just operating off that config the user specified which may not have all fields set.

@dgoodwin installer cannot choose sane default after installation. hive will have to create its own merge/update logic.

I think we may need to have Hive explicitly apply defaults that match yours?

thinking hive as an operator; you should fetch the live machineset, use this api with values to create expected state and then merge them based on some logic...

dgoodwin · 2018-10-31T17:13:12Z

Yes talking about this with @csrwng I think we're going to want to apply our own defaults, possibly with re-used code between our two projects again. The AMI specifically can't change so we can't hand off to have a default reapplied.

abhinavdahiya · 2018-10-31T18:19:48Z

Yes talking about this with @csrwng I think we're going to want to apply our own defaults, possibly with re-used code between our two projects again. The AMI specifically can't change so we can't hand off to have a default reapplied.

so this is good to go as-is ?

dgoodwin · 2018-10-31T18:37:37Z

I think so yes, thanks.

abhinavdahiya · 2018-10-31T21:00:15Z

I think so yes, thanks.

an /lgtm would be appreciated 😇

wking · 2018-10-31T23:14:48Z

Gopkg.toml

@@ -49,6 +49,17 @@ ignored = ["github.com/openshift/installer/tests*"]
  name = "k8s.io/client-go"
  version = "6.0.0"

+[[constraint]]
+  name = "sigs.k8s.io/cluster-api"
+  branch = "master"


Do we really want these floating with master? Can we pin to their current tips and then manually bump when they make changes?

master is fine

pkg/asset/machines/aws/master.go

pkg/asset/machines/libvirt/worker.go

wking · 2018-10-31T23:36:44Z

pkg/asset/machines/aws/master.go

+				},
+				Versions: clusterapi.MachineVersionInfo{
+					Kubelet:      "",
+					ControlPlane: "",


I'd still rather leave Versions unset and file issues or set a FIXME about these two parameters. Setting them to empty strings doesn't make sense to me.

This is explicitly saying don't care about version for these, we have different mechanism for controlling them.

This is explicitly saying don't care about version for these we have diff mechanism for controlling them.

But you can use zero-values for that, no?

$ cat test.go package main import ( "fmt" clusterapi "sigs.k8s.io/cluster-api/pkg/apis/cluster/v1alpha1" ) func main() { spec := clusterapi.MachineSpec{} fmt.Printf("%q %t\n", spec.Versions.Kubelet, spec.Versions.Kubelet == "") } $ go run test.go "" true

pkg/asset/machines/aws/worker.go

dgoodwin · 2018-11-01T11:02:44Z

Sorry @abhinavdahiya didn't know we could, or should, be lgtm'ing in your repo. Going to hold off now though and let @wking

abhinavdahiya · 2018-11-01T20:11:57Z

@wking wants the api to move to this:

$ godoc ./pkg/asset/machines/aws
PACKAGE DOCUMENTATION

package aws
    import "./pkg/asset/machines/aws"

    Package aws generates Machine objects for aws.

FUNCTIONS

func AvailabilityZones(region string) ([]string, error)
    AvailabilityZones retrieves a list of availability zones for the given
    region.

func MachineSets(config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.MachineSet, error)
    MachineSets returns a list of machinesets for a machinepool.

func Machines(config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.Machine, error)
    Machines returns a list of machines for a machinepool.


$ godoc ./pkg/asset/machines/libvirt
PACKAGE DOCUMENTATION

package libvirt
    import "./pkg/asset/machines/libvirt"

    Package libvirt generates Machine objects for libvirt.

FUNCTIONS

func MachineSets(config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.MachineSet, error)
    MachineSets returns a list of machinesets for a machinepool.

func Machines(config *types.InstallConfig, pool *types.MachinePool, role, userDataSecret string) ([]clusterapi.Machine, error)
    Machines returns a list of machines for a machinepool.

pushed commits to match that.

pkg/asset/machines/aws/machines.go

pkg/asset/machines/worker.go

pkg/types/installconfig.go

pkg/asset/machines/aws/machines.go

wking · 2018-11-01T22:05:12Z

pkg/asset/machines/aws/machines.go

+				ProviderConfig: clusterapi.ProviderConfig{
+					Value: &runtime.RawExtension{Object: &provider},
+				},
+				Versions: clusterapi.MachineVersionInfo{


Did you not find my zero-value demonstration convincing? ;)

these values have a meaning upstream, but set them to "" so anybody that sees this code understands that these values to set to "" because we have different mechanics to control them.
From the consumer end it makes no difference.

... anybody that sees this code understands that these values to set to "" because we have different mechanics to control them...

This is not obvious to me from reading the code ;). How about:

Spec: clusterapi.MachineSpec{ ProviderConfig: clusterapi.ProviderConfig{ Value: &runtime.RawExtension{Object: &provider}, }, // we don't need to set Versions, because we control those via {reference our different mechanism} }

This is not obvious to me from reading the code ;).

Well in that case, hopefully // we don't need to set Versions, because we control those via {reference our different mechanism} does the job

done with d562522 -> 476be07

k8s.io/utils/pointer provider various type to *type converts which are helpful

Currently the Machine and MachineSet objects are go templates. Moving them to vendored code allows other consumers like openshift/hive to use these public helpers effectively. openstack platform still uses go templates because of vendoring problems.

abhinavdahiya · 2018-11-02T18:53:35Z

@wking tests are green :)

wking

/lgtm

openshift-ci-robot · 2018-11-02T18:56:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, 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

Empty machinepool is a valid for libvirt because we don't use it. Fixes the error ```console FATAL Error executing openshift-install: non-Libvirt machine-pool: "" `` introduced in openshift#573

openshift#573 had wrong calculation for machine objects in AWS.

…orm}.Name The old *PlatformType are from cccbb37 (Generate installation assets via a dependency graph, 2018-08-10, openshift#120), but since 476be07 (pkg/asset: use vendored cluster-api instead of go templates, 2018-10-30, openshift#573), we've had variables for the name strings in the more central pkg/types. With this commit, we drop the more peripheral forms. I've also pushed the types.PlatformName{Platform} variables down into types.{platform}.Name at Ahbinav's suggestion [1]. I've added a unit test to enforce sorting in PlatformNames, because the order is required by sort.SearchStrings in queryUserForPlatform. [1]: openshift#659 (comment)

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 Oct 30, 2018

openshift-ci-robot requested review from dgoodwin and rajatchopra October 30, 2018 19:51

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 30, 2018

abhinavdahiya mentioned this pull request Oct 30, 2018

vendor: Bump k8s.io/client-go to 9.0.0 and apimachinery to 1.12.2 #574

Closed

dgoodwin reviewed Oct 31, 2018

View reviewed changes

abhinavdahiya mentioned this pull request Oct 31, 2018

cmd/openshift-install/create: Destroy bootstrap on bootstrap-complete #579

Merged

abhinavdahiya force-pushed the mp_to_ms branch from 18423f2 to a07e0c2 Compare October 31, 2018 18:19

abhinavdahiya changed the title ~~WIP: add an API for machinepool -> machinesets~~ add an API for machinepool -> machinesets Oct 31, 2018

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 31, 2018

abhinavdahiya mentioned this pull request Oct 31, 2018

[WIP] Convert machineset templates to actual objects #567

Closed

5 tasks