drop deprecated mao config #518

enxebre · 2018-10-23T08:24:31Z

This config is not needed for the mao anymore since:
1 - We moved to CRDs
openshift/machine-api-operator#100
openshift/cluster-api-provider-aws#81
openshift/cluster-api-provider-libvirt#73

2 - machineSets objects are being generate on the installer now
openshift/machine-api-operator#101 so relevant config is no needed in the operator

enxebre · 2018-10-24T09:49:35Z

/test e2e-aws

enxebre · 2018-10-24T13:27:44Z

cc @abhinavdahiya

abhinavdahiya · 2018-10-24T17:25:37Z

/lgtm

enxebre · 2018-10-24T18:16:48Z

2018/10/24 14:59:02 Container test in pod e2e-aws completed successfully
2018/10/24 17:38:48 Copying artifacts from e2e-aws into /logs/artifacts/e2e-aws
2018/10/24 17:38:48 error: unable to signal to artifacts container to terminate in pod e2e-aws, triggering deletion: could not run remote command: unable to upgrade connection: container not found ("artifacts")
2018/10/24 17:38:48 error: unable to retrieve artifacts from pod e2e-aws: could not read gzipped artifacts: unable to upgrade connection: container not found ("artifacts")
E1024 17:38:53.774984      11 event.go:200] Server rejected event '&v1.Event{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, ObjectMeta:v1.ObjectMeta{Name:".15609c1ffcf191a4", GenerateName:"", Namespace:"ci-op-nnbjxjq9", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:v1.Time{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, DeletionTimestamp:(*v1.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil), Annotations:map[string]string(nil), OwnerReferences:[]v1.OwnerReference(nil), Initializers:(*v1.Initializers)(nil), Finalizers:[]string(nil), ClusterName:""}, InvolvedObject:v1.ObjectReference{Kind:"", Namespace:"ci-op-nnbjxjq9", Name:"", UID:"", APIVersion:"", ResourceVersion:"", FieldPath:""}, Reason:"CiJobFailed", Message:"Running job pull-ci-openshift-installer-master-e2e-aws for PR https://github.com/openshift/installer/pull/518 in namespace ci-op-nnbjxjq9 from author enxebre", Source:v1.EventSource{Component:"ci-op-nnbjxjq9", Host:""}, FirstTimestamp:v1.Time{Time:time.Time{wall:0xbeec496b6d700fa4, ext:10988947084642, loc:(*time.Location)(0x1973400)}}, LastTimestamp:v1.Time{Time:time.Time{wall:0xbeec496b6d700fa4, ext:10988947084642, loc:(*time.Location)(0x1973400)}}, Count:1, Type:"Warning", EventTime:v1.MicroTime{Time:time.Time{wall:0x0, ext:0, loc:(*time.Location)(nil)}}, Series:(*v1.EventSeries)(nil), Action:"", Related:(*v1.ObjectReference)(nil), ReportingController:"", ReportingInstance:""}': 'events ".15609c1ffcf191a4" is forbidden: unable to create new content in namespace ci-op-nnbjxjq9 because it is being terminated' (will not retry!)
2018/10/24 17:38:54 Ran for 3h3m9s
error: could not run steps: could not wait for pod to complete: could not wait for pod completion: pod e2e-aws was already deleted

/test e2e-aws

wking · 2018-10-24T20:11:03Z

pkg/asset/manifests/machine-api-operator.go

-	Region      string `json:"region"`
-	Replicas    int    `json:"replicas"`
+	TargetNamespace string `json:"targetNamespace"`
+	Provider        string `json:"provider"`


Do we need these two? Can MAO install them on its own? It already has namespace opinions here. And the provider information is already included in the worker machine-set:

$ openshift-install --dir testing create manifests $ cat testing/tectonic/99_openshift-cluster-api_worker-machineset.yaml apiVersion: cluster.k8s.io/v1alpha1 kind: MachineSet metadata: ... spec: replicas: 1 selector: ... template: metadata: ... spec: providerConfig: value: apiVersion: libvirtproviderconfig.k8s.io/v1alpha1 kind: LibvirtMachineProviderConfig ... versions: ...

And also in the master machine entry:

$ cat testing/tectonic/99_openshift-cluster-api_master-machines.yaml kind: List apiVersion: v1 metadata: resourceVersion: "" selfLink: "" items: - apiVersion: cluster.k8s.io/v1alpha1 kind: Machine metadata: ... spec: providerConfig: value: apiVersion: libvirtproviderconfig.k8s.io/v1alpha1 kind: LibvirtMachineProviderConfig ... versions: ...

Hey @wking we can possibly drop the namespace, the provider is needed so the operator knows which provider controller to run to react to this objects with the right implementation

... the provider is needed so the operator knows which provider controller to run....

Don't both .spec.providerConfig.value.apiVersion and .spec.providerConfig.value.kind contain the provider information you need (see my examples above, where they are libvirtproviderconfig.k8s.io/v1alpha1 and LibvirtMachineProviderConfig respectively)?

I'm not sure how you mean here . The operator needs to know the provider so it runs a different provider controller depending on it. Then that controller watches machines and understand only its specific kind. There's no global controller which decode the kind and choose an implementation based on it at runtime; currently the cluster api requires to choose a provider controller ahead of time. Let me know if it does not make sense

The operator needs to know the provider so it runs a different provider controller depending on it. Then that controller watches machines and understand only its specific kind. There's no global controller which decode the kind and choose an implementation based on it at runtime; currently the cluster api requires to choose a provider controller ahead of time.

Ah, maybe we should grow this ;). As it stands, can't we do the Go equivalent of:

oc get -o jsonpath --template '{.items[0].spec.template.spec.providerConfig.value.kind}' -n openshift-cluster-api machinesets

from the operator to figure out which controller to launch?

@wking do you want the operator to deploy controllers based on the machinesets that were pushed to api? :P I don't think that's a good idea.

Yeah that's what I want. But I'm not very familiar with this space, maybe it's crazy. Is there a big difference between the operator choosing based on a machineAPIOperatorConfig and the operator choosing based on the worker MachineSet? They seem similar to me, except the latter would be more DRY. And it also sets us up for "this is mostly an AWS cluster, but toss some workers up on GCP" at some point in the distant future.

@enxebre almost all operators are using the kube-system/cluster-config-v1 configmap's key install-config to discover configuration like the platform.

But weren't we planning on dropping that? I'll see if I can find a reference.

And it also sets us up for "this is mostly an AWS cluster, but toss some workers up on GCP"

I think we are getting too ahead of ourselves :P. Our machines only talk on internal load balancers, this will not work. also none of the other components are multi cloud aware and i don't think they should be... due to simpler decision boundaries..

But weren't we planning on dropping that? I'll see if I can find a reference.

We might drop it in favor of something else but there will always be some discovery available for operator.

hey @wking @abhinavdahiya There's too many moving pieces yet (downstream and upstream) that makes me hesitant on thinking about enabling a discovery which requires to run at least one object before the controller which watches it. I can't see any effective immediate benefit whereas it would get as to a path with loads of open questions and a bunch of new cases that we'll have to contemplate and which are difficult to reason about at this point (what happen if the operator see two objects for different clouds? use the first one? do they map to the same cluster object? or do we support simultaneous clouds? if so that's a whole different story, etc, etc... ). The cluster api itself might support a more dynamic system for running different controller impl based on different machine classes eventually or we could think of it when the time comes but it's just too soon. I think for now using https://github.com/openshift/installer/blob/master/pkg/types/installconfig.go#L34 make sense. I'll update the operator to consume the installconfig so totally disconnect it from this configmap, then I'll update this PR to drop it all

I'll update the operator to consume the installconfig...

I dunno how future-proof this will be, but I'm completely fine with it as a short-term solution. And who knows, maybe it will last longer than I think ;).

@enxebre almost all operators are using the kube-system/cluster-config-v1 configmap's key install-config to discover configuration like the platform.

But weren't we planning on dropping that? I'll see if I can find a reference.

Here's the reference I was thinking of.

wking · 2018-10-24T20:33:58Z

/hold

Can we drop the namespace?

enxebre · 2018-10-29T09:58:17Z

Dropping all config now. See openshift/machine-api-operator@35a6e3a for reference

wking

/lgtm

openshift-ci-robot · 2018-10-29T11:20:34Z

[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 · 2018-10-29T13:36:15Z

need to drop do-not-merge/hold label

wking · 2018-10-29T15:06:33Z

/hold cancel

openshift-ci-robot requested review from aaronlevy and rajatchopra October 23, 2018 08:24

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

enxebre changed the title ~~drop deprecated mao config~~ WIP: DO NOT MERGE drop deprecated mao config Oct 23, 2018

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

enxebre mentioned this pull request Oct 23, 2018

Remove machineset and cluster objects openshift/machine-api-operator#101

Merged

enxebre force-pushed the maoconfig branch from 01a1d30 to eb59385 Compare October 23, 2018 08:33

enxebre changed the title ~~WIP: DO NOT MERGE drop deprecated mao config~~ drop deprecated mao config Oct 24, 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 24, 2018

enxebre mentioned this pull request Oct 24, 2018

Rename ClusterName to ClusterID openshift/machine-api-operator#103

Closed

enxebre force-pushed the maoconfig branch from eb59385 to 0099214 Compare October 24, 2018 14:35

openshift-ci-robot assigned abhinavdahiya Oct 24, 2018

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Oct 24, 2018

wking reviewed Oct 24, 2018

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 24, 2018

enxebre force-pushed the maoconfig branch from 0099214 to b0f684e Compare October 24, 2018 20:47

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

drop deprecated mao config

29fd3f7

enxebre force-pushed the maoconfig branch from b0f684e to 29fd3f7 Compare October 29, 2018 09:57

openshift-ci-robot assigned wking Oct 29, 2018

wking approved these changes Oct 29, 2018

View reviewed changes

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

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 29, 2018

openshift-merge-robot merged commit 5ddb3b7 into openshift:master Oct 29, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drop deprecated mao config #518

drop deprecated mao config #518

enxebre commented Oct 23, 2018 •

edited

Loading

enxebre commented Oct 24, 2018

enxebre commented Oct 24, 2018

abhinavdahiya commented Oct 24, 2018

enxebre commented Oct 24, 2018 •

edited

Loading

wking Oct 24, 2018 •

edited

Loading

enxebre Oct 24, 2018 •

edited

Loading

wking Oct 24, 2018

enxebre Oct 24, 2018 •

edited

Loading

wking Oct 24, 2018

wking Oct 24, 2018

abhinavdahiya Oct 24, 2018

enxebre Oct 25, 2018 •

edited

Loading

wking Oct 25, 2018

wking Oct 25, 2018

wking commented Oct 24, 2018

enxebre commented Oct 29, 2018 •

edited

Loading

wking left a comment

openshift-ci-robot commented Oct 29, 2018

enxebre commented Oct 29, 2018

wking commented Oct 29, 2018

drop deprecated mao config #518

drop deprecated mao config #518

Conversation

enxebre commented Oct 23, 2018 • edited Loading

enxebre commented Oct 24, 2018

enxebre commented Oct 24, 2018

abhinavdahiya commented Oct 24, 2018

enxebre commented Oct 24, 2018 • edited Loading

wking Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

enxebre Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

wking Oct 24, 2018

Choose a reason for hiding this comment

enxebre Oct 24, 2018 • edited Loading

Choose a reason for hiding this comment

wking Oct 24, 2018

Choose a reason for hiding this comment

wking Oct 24, 2018

Choose a reason for hiding this comment

abhinavdahiya Oct 24, 2018

Choose a reason for hiding this comment

enxebre Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

wking Oct 25, 2018

Choose a reason for hiding this comment

wking Oct 25, 2018

Choose a reason for hiding this comment

wking commented Oct 24, 2018

enxebre commented Oct 29, 2018 • edited Loading

wking left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Oct 29, 2018

enxebre commented Oct 29, 2018

wking commented Oct 29, 2018

enxebre commented Oct 23, 2018 •

edited

Loading

enxebre commented Oct 24, 2018 •

edited

Loading

wking Oct 24, 2018 •

edited

Loading

enxebre Oct 24, 2018 •

edited

Loading

enxebre Oct 24, 2018 •

edited

Loading

enxebre Oct 25, 2018 •

edited

Loading

enxebre commented Oct 29, 2018 •

edited

Loading