Integrate baremetal-operator with machine-api-operator #302

sadasu · 2019-04-26T18:47:53Z

Changes required for the MAO to also perform the Baremetal deployment.

openshift-ci-robot · 2019-04-26T18:48:07Z

Hi @sadasu. Thanks for your PR.

I'm waiting for a openshift member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

install/0000_30_machine-api-operator_01_images.configmap.yaml

install/image-references

pkg/operator/config.go

owned-manifests/machine-api-controllers.yaml

install/0000_30_machine-api-operator_01_images.configmap.yaml

russellb

Thanks for working on this!

install/image-references

install/0000_30_machine-api-operator_01_images.configmap.yaml

install/image-references

owned-manifests/baremetal_operator.yaml

dhellmann

This looks like it will install and start the operator, but not define the CRD it uses. Should the operator do that for itself? Or is that supposed to happen some other way? (Or maybe I missed a detail here)

sadasu · 2019-06-04T20:52:22Z

This looks like it will install and start the operator, but not define the CRD it uses. Should the operator do that for itself? Or is that supposed to happen some other way? (Or maybe I missed a detail here)

Yes, currently the changes proposed here does not define the CRD that the baremetal-operator uses. Code has changes quite a bit since these changes were proposed. Investigating to see if there is now a mechanism to provide baremetal-operator with the CRD at the time of deployment.

dhellmann · 2019-06-04T23:51:10Z

This looks like it will install and start the operator, but not define the CRD it uses. Should the operator do that for itself? Or is that supposed to happen some other way? (Or maybe I missed a detail here)

Yes, currently the changes proposed here does not define the CRD that the baremetal-operator uses. Code has changes quite a bit since these changes were proposed. Investigating to see if there is now a mechanism to provide baremetal-operator with the CRD at the time of deployment.

OK, that was an honest question on my part. I have no idea how it's supposed to work, but if the operator needs to do it let me know and we can make that happen.

russellb · 2019-06-05T02:08:26Z

@sadasu @dhellmann Take a look at the contents of the machine-api-operator/install/ directory. The image-references file specifies which images the machine-api-operator depends on that need to get included in the release payload. The rest are a bunch of manifests. Those manifests will get included in the release payload as well. Note that the Machine and MachineSet CRDs are in there, for example.

It seems one option would be to drop the BareMetalHost CRD in there. That would work, but I don't see any way to make that baremetal platform specific. The CRD would be defined for all clusters, and that's not exactly ideal, but also harmless, and maybe an acceptable starting point?

A second option would be to define the CRD via the API in go code in this patch, and ensure that's done before the baremetal-operator gets launched.

A third would be to have the baremetal-operator define its own CRD at runtime if not already defined, I suppose.

Any other ideas?

Maybe some of the other members of the cloud team that maintains machine-api-operator would have input, too. @enxebre any thoughts? or recommendations for who might have an opinion?

dhellmann · 2019-06-10T14:30:44Z

@sadasu @dhellmann Take a look at the contents of the machine-api-operator/install/ directory. The image-references file specifies which images the machine-api-operator depends on that need to get included in the release payload. The rest are a bunch of manifests. Those manifests will get included in the release payload as well. Note that the Machine and MachineSet CRDs are in there, for example.

It seems one option would be to drop the BareMetalHost CRD in there. That would work, but I don't see any way to make that baremetal platform specific. The CRD would be defined for all clusters, and that's not exactly ideal, but also harmless, and maybe an acceptable starting point?

I think that's a good starting point to unblock several other pieces of work to integrate the baremetal operator with the installer.

A second option would be to define the CRD via the API in go code in this patch, and ensure that's done before the baremetal-operator gets launched.

This seems like a better long term approach, once we are unblocked.

A third would be to have the baremetal-operator define its own CRD at runtime if not already defined, I suppose.

I expect we'll have issues giving the operator permissions to define CRDs, so while it makes sense from the perspective of only having the CRD when needed I don't think it's a viable approach.

Any other ideas?

Maybe some of the other members of the cloud team that maintains machine-api-operator would have input, too. @enxebre any thoughts? or recommendations for who might have an opinion?

pkg/operator/operator.go

pkg/operator/config.go

pkg/operator/operator.go

michaelgugino · 2019-06-19T16:53:10Z

pkg/operator/operator.go

 		},
+                BaremetalOperatorConfig: BaremetalConfig{


We should set this whole config to nil if baremetal is not enabled. Build BaremetalConfig object above if necesary and refer to it here.

Right before we define the BaremetalConfig object appears to be the only place we should check baremetalEnabled. All the other checks above are redundant. Make that determination here, if it's true, get the images and create BaremetalConfig; else BaremetalConfig := nil

pkg/operator/sync.go

pkg/operator/operator.go

pkg/operator/sync.go

owned-manifests/baremetal_operator.yaml

install/0000_30_machine-api-operator_01_images.configmap.yaml

This means we can keep the OCP specific pieces out of the upstream repo, and prototype the integration which will ultimately be handled via openshift/machine-api-operator#302 Also set the RHCOS image URL n the configmap based on the variable from common.sh

This means we can keep the OCP specific pieces out of the upstream repo, and prototype the integration which will ultimately be handled via openshift/machine-api-operator#302 Also set the RHCOS image URL n the configmap based on the variable from common.sh Co-Authored-By: Ian Main <[email protected]>

pkg/operator/baremetal_pod.go

michaelgugino · 2019-08-05T20:55:54Z

install/0000_30_machine-api-operator_12_baremetalhost.crd.yaml

+apiVersion: apiextensions.k8s.io/v1beta1
+kind: CustomResourceDefinition
+metadata:
+  name: baremetalhosts.metal3.io


We need to figure out if we want to ship in this namespace. @enxebre @derekwaynecarr @eparis

the resource is alpha, i would like to use the community project name for these resources in the interim. before graduating to GA in openshift, i want to understand how these API resources are evolved in community context.

michaelgugino

Just a few things, otherwise looks good to me.

pkg/operator/config.go

pkg/operator/operator.go

enxebre · 2019-08-06T15:58:04Z

install/0000_30_machine-api-operator_12_baremetalhost.crd.yaml

+     - bmh
+     - bmhost
+  scope: Namespaced
+  version: v1alpha1


are we good shipping with alpha commitment?

For this high touch beta, yes. We'll raise the version level in an upcoming release.

alpha is fine.

enxebre · 2019-08-06T15:59:30Z

pkg/operator/sync.go

+
+	// In addition, if the Provider is BareMetal, then bring up
+	// the baremetal-operator pod
+	if config.BaremetalControllers.BaremetalOperator != "" {


does the baremetal actuator depends on this components to run successfully? if so you'd need to ensure this gets deployed before syncClusterAPIController

The baremetal actuator does not depend on the metal3 pod to run successfully (but needs it to be running to be useful.) It is OK for the baremetal actuator if the metal3 pod is available later. Also, if the metal3 pod deployment wasn't successful we would still want to go ahead with the machine-api-controller pod deployment. For these reasons, would like to keep the ordering as is.

These changes allow the MAO to bring up the metal3 pod that contains the baremetal-operator and Ironic containers.

michaelgugino

This looks good to me, nice work!

sadasu · 2019-08-06T18:04:01Z

/retest

enxebre · 2019-08-07T15:42:45Z

/approve

openshift-ci-robot · 2019-08-07T15:42:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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~~ [enxebre]

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

michaelgugino · 2019-08-07T16:32:46Z

/lgtm

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 Apr 26, 2019

openshift-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 26, 2019

openshift-ci-robot requested review from frobware and michaelgugino April 26, 2019 18:48

dhellmann reviewed Apr 26, 2019

View reviewed changes

sadasu force-pushed the 297-dev branch from a2c355b to 02e0a81 Compare May 3, 2019 18:36

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 3, 2019

sadasu force-pushed the 297-dev branch 2 times, most recently from 2d90d12 to 6244ed2 Compare May 7, 2019 17:46

dhellmann reviewed May 7, 2019

View reviewed changes

install/0000_30_machine-api-operator_01_images.configmap.yaml Outdated Show resolved Hide resolved

russellb requested changes May 7, 2019

View reviewed changes

sadasu force-pushed the 297-dev branch from 6244ed2 to a8d4d0b Compare May 8, 2019 17:29

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 28, 2019

dhellmann reviewed Jun 4, 2019

View reviewed changes

michaelgugino suggested changes Jun 19, 2019

View reviewed changes

markmc reviewed Jun 24, 2019

View reviewed changes

owned-manifests/baremetal_operator.yaml Outdated Show resolved Hide resolved

install/0000_30_machine-api-operator_01_images.configmap.yaml Outdated Show resolved Hide resolved

russellb mentioned this pull request Jun 27, 2019

ServiceMonitor for ironic-prometheus-exporter metal3-io/baremetal-operator#226

Closed

sadasu force-pushed the 297-dev branch from a8d4d0b to b0a81b3 Compare July 2, 2019 18:54

iurygregory mentioned this pull request Jul 3, 2019

Monitoring for ironic-prometheus-exporter #340

Closed

sadasu force-pushed the 297-dev branch from b0a81b3 to 7a090bb Compare July 9, 2019 17:59

openshift-ci-robot removed the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 9, 2019

sadasu mentioned this pull request Jul 30, 2019

Add images required for bringing up a baremetal deployment. #367

Merged

sadasu force-pushed the 297-dev branch 2 times, most recently from 85e1610 to 8fbd9e4 Compare August 1, 2019 21:49

russellb reviewed Aug 2, 2019

View reviewed changes

pkg/operator/baremetal_pod.go Outdated Show resolved Hide resolved

sadasu force-pushed the 297-dev branch from 8fbd9e4 to f9e08e1 Compare August 5, 2019 20:12

michaelgugino reviewed Aug 5, 2019

View reviewed changes

michaelgugino suggested changes Aug 5, 2019

View reviewed changes

pkg/operator/config.go Outdated Show resolved Hide resolved

pkg/operator/config.go Outdated Show resolved Hide resolved

pkg/operator/operator.go Outdated Show resolved Hide resolved

pkg/operator/operator.go Outdated Show resolved Hide resolved

enxebre reviewed Aug 6, 2019

View reviewed changes

Integrate baremetal-operator with machine-api-operator

43dd52d

These changes allow the MAO to bring up the metal3 pod that contains the baremetal-operator and Ironic containers.

sadasu force-pushed the 297-dev branch from cc20e78 to 43dd52d Compare August 6, 2019 16:37

sadasu changed the title ~~WIP: Integrate baremetal-operator with machine-api-operator~~ Integrate baremetal-operator with machine-api-operator Aug 6, 2019

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 Aug 6, 2019

michaelgugino approved these changes Aug 6, 2019

View reviewed changes

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 7, 2019

openshift-ci-robot assigned michaelgugino Aug 7, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 7, 2019

openshift-merge-robot merged commit 2afc612 into openshift:master Aug 7, 2019

sadasu mentioned this pull request Aug 7, 2019

Rename bmo-ironic-configmap to metal3-config. openshift-metal3/dev-scripts#729

Merged

hardys mentioned this pull request Oct 30, 2019

Proposing enhancement to BareMetalPlatformStatus openshift/enhancements#90

Closed

sadasu deleted the 297-dev branch February 4, 2020 18:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate baremetal-operator with machine-api-operator #302

Integrate baremetal-operator with machine-api-operator #302

sadasu commented Apr 26, 2019 •

edited

Loading

openshift-ci-robot commented Apr 26, 2019

russellb left a comment

dhellmann left a comment

sadasu commented Jun 4, 2019

dhellmann commented Jun 4, 2019

russellb commented Jun 5, 2019

dhellmann commented Jun 10, 2019

michaelgugino Jun 19, 2019

michaelgugino Jun 19, 2019

michaelgugino Aug 5, 2019

derekwaynecarr Aug 7, 2019

michaelgugino left a comment

enxebre Aug 6, 2019

dhellmann Aug 6, 2019

derekwaynecarr Aug 7, 2019

enxebre Aug 6, 2019 •

edited

Loading

sadasu Aug 6, 2019

michaelgugino left a comment

sadasu commented Aug 6, 2019

enxebre commented Aug 7, 2019

openshift-ci-robot commented Aug 7, 2019

michaelgugino commented Aug 7, 2019

Integrate baremetal-operator with machine-api-operator #302

Integrate baremetal-operator with machine-api-operator #302

Conversation

sadasu commented Apr 26, 2019 • edited Loading

openshift-ci-robot commented Apr 26, 2019

russellb left a comment

Choose a reason for hiding this comment

dhellmann left a comment

Choose a reason for hiding this comment

sadasu commented Jun 4, 2019

dhellmann commented Jun 4, 2019

russellb commented Jun 5, 2019

dhellmann commented Jun 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelgugino left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Aug 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelgugino left a comment

Choose a reason for hiding this comment

sadasu commented Aug 6, 2019

enxebre commented Aug 7, 2019

openshift-ci-robot commented Aug 7, 2019

michaelgugino commented Aug 7, 2019

sadasu commented Apr 26, 2019 •

edited

Loading

enxebre Aug 6, 2019 •

edited

Loading