Custom deploy procedure support #156

kirankt · 2021-07-13T02:55:23Z

Add support for custom-deploy procedure. See:
metal3-io/baremetal-operator#884

kirankt · 2021-07-13T02:55:43Z

/assign @dtantsur

kirankt · 2021-07-13T12:33:46Z

/retest

kirankt · 2021-07-13T13:30:50Z

/retest

pkg/apis/baremetal/v1alpha1/baremetalmachineproviderspec_types.go

dtantsur · 2021-07-13T14:27:25Z

I wonder if we need to update CAPM3 in a similar way...

kirankt · 2021-07-13T19:20:18Z

/retest

kirankt · 2021-07-14T15:12:10Z

/retest

kirankt · 2021-07-15T13:05:05Z

/retest

Update Dockerfile Keep Dockerfile.rhel

kirankt · 2021-07-19T02:49:38Z

/retest

hardys · 2021-07-19T09:16:38Z

/test e2e-metal-ipi-virtualmedia

hardys · 2021-07-19T09:17:28Z

The virtualmedia job has never passed so I'm wondering if there's a real issue - in the latest run I noticed BMO was failing to start due to a port conflict, but that doesn't seem to be related to this change so retesting to confirm if that's repeatable

openshift-ci · 2021-07-19T11:40:42Z

@kirankt: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-metal-ipi-virtualmedia	`1ea2b23`	link	`/test e2e-metal-ipi-virtualmedia`

Full PR test history. Your PR dashboard.

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.

dtantsur · 2021-07-19T11:48:24Z

At least some tests have passed, so it's not necessarily a problem with this patch.

dtantsur · 2021-07-19T11:52:55Z

pkg/cloud/baremetal/actuators/machine/actuator.go

@@ -552,17 +554,23 @@ func (a *Actuator) provisionHost(ctx context.Context, host *bmh.BareMetalHost,
 	config *bmv1alpha1.BareMetalMachineProviderSpec) error {
 	originalHost := host.DeepCopy()

-	// We only want to update the image setting if the host does not


Changing this is potentially breaking as it will cause re-deploys on upgrade. Maybe skip everything if host.Spec.Image != nil || host.Spec.CustomDeploy != nil?

This comment was at best out of date and at worst nonsense. Kiran was right to delete it.
Hosts that are already provisioned/ing are not going to be selected anyway, so we will never get to this point.

dtantsur · 2021-07-19T11:53:30Z

pkg/apis/baremetal/v1alpha1/baremetalmachineproviderspec_types.go

 // IsValid returns an error if the object is not valid, otherwise nil. The
 // string representation of the error is suitable for human consumption.
 func (s *BareMetalMachineProviderSpec) IsValid() error {
 	missing := []string{}
-	if s.Image.URL == "" {
+	if s.CustomDeploy.Method == "" && s.Image.URL == "" {


nit:

if s.CustomDeploy.Method == "" { return nil }

TBH, I think the whole IsValid function is now defunct. I thought about removing it since Image.URL/Checksum are now optional unlike before.

kirankt · 2021-07-19T12:20:34Z

The virtualmedia job has never passed so I'm wondering if there's a real issue - in the latest run I noticed BMO was failing to start due to a port conflict, but that doesn't seem to be related to this change so retesting to confirm if that's repeatable

I don't think the failure is relate but I did notice that BMO was failing a few times in my test failing to bind to 9440 which is the liveness probe port for both BMO and CAPBM. Not sure why they are the same. My best guess is that when these two competing pods end up scheduled on the same control plane host, BMO fails because its the last to start.

https://github.com/openshift/baremetal-operator/blob/c9a3f263854478328765208f3e8109eafae8e5bd/main.go#L102

cluster-api-provider-baremetal/cmd/manager/main.go

Line 60 in fbb6f1b

healthAddr := flag.String(

zaneb · 2021-07-19T17:49:45Z

I did notice that BMO was failing a few times in my test failing to bind to 9440 which is the liveness probe port for both BMO and CAPBM. Not sure why they are the same.

That's only a problem if CAPBM is also running with --net=host. I can't think of a reason it should, but maybe it does?

zaneb

/lgtm

zaneb · 2021-07-19T18:02:03Z

pkg/cloud/baremetal/actuators/machine/actuator.go

@@ -552,17 +554,23 @@ func (a *Actuator) provisionHost(ctx context.Context, host *bmh.BareMetalHost,
 	config *bmv1alpha1.BareMetalMachineProviderSpec) error {
 	originalHost := host.DeepCopy()

-	// We only want to update the image setting if the host does not


This comment was at best out of date and at worst nonsense. Kiran was right to delete it.
Hosts that are already provisioned/ing are not going to be selected anyway, so we will never get to this point.

zaneb · 2021-07-19T18:03:39Z

pkg/apis/baremetal/v1alpha1/baremetalmachineproviderspec_types.go

 		missing = append(missing, "Image.URL")
 	}
-	if s.Image.Checksum == "" {
+	if s.CustomDeploy.Method == "" && s.Image.Checksum == "" {


Arguably we should always check for a checksum when there is an image, not just if there is no custom deploy method. But for now there's no difference.

openshift-ci · 2021-07-19T18:04:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kirankt, zaneb

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

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

zaneb · 2021-07-20T02:40:22Z

I did notice that BMO was failing a few times in my test failing to bind to 9440 which is the liveness probe port for both BMO and CAPBM. Not sure why they are the same.

That's only a problem if CAPBM is also running with --net=host. I can't think of a reason it should, but maybe it does?

I checked and AFAICT it doesn't, but maybe something else does and uses the same default port.
Nobody, including BMO, owns this port, so at least 2 things are Doing It Wrong.

kirankt · 2021-07-20T02:47:12Z

I did notice that BMO was failing a few times in my test failing to bind to 9440 which is the liveness probe port for both BMO and CAPBM. Not sure why they are the same.

That's only a problem if CAPBM is also running with --net=host. I can't think of a reason it should, but maybe it does?

I checked and AFAICT it doesn't, but maybe something else does and uses the same default port.
Nobody, including BMO, owns this port, so at least 2 things are Doing It Wrong.

You are right, but there are processes on master-0 that use ports 9440 and 9441 and AFAIK, only MAO and the its providers use these ports for healtz. I haven't been able to definitely locate the process/pod that runs on 9440, so I assumed it was CAPBM.. As long as CBO gets scheduled to the other control plane nodes, it works.

openshift-ci bot requested review from hardys and russellb July 13, 2021 02:55

openshift-ci bot assigned dtantsur Jul 13, 2021

dtantsur reviewed Jul 13, 2021

View reviewed changes

pkg/apis/baremetal/v1alpha1/baremetalmachineproviderspec_types.go Outdated Show resolved Hide resolved

kirankt added 2 commits July 17, 2021 18:21

Add DeploySteps support

9c035fe

Update Dockerfile Keep Dockerfile.rhel

Vendor support for DeploySteps

1ea2b23

dtantsur reviewed Jul 19, 2021

View reviewed changes

zaneb reviewed Jul 19, 2021

View reviewed changes

openshift-ci bot assigned zaneb Jul 19, 2021

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2021

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2021

openshift-merge-robot merged commit 94eff91 into openshift:master Jul 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom deploy procedure support #156

Custom deploy procedure support #156

kirankt commented Jul 13, 2021

kirankt commented Jul 13, 2021

kirankt commented Jul 13, 2021

kirankt commented Jul 13, 2021

dtantsur commented Jul 13, 2021

kirankt commented Jul 13, 2021

kirankt commented Jul 14, 2021

kirankt commented Jul 15, 2021

kirankt commented Jul 19, 2021

hardys commented Jul 19, 2021

hardys commented Jul 19, 2021

openshift-ci bot commented Jul 19, 2021

dtantsur commented Jul 19, 2021

dtantsur Jul 19, 2021

zaneb Jul 19, 2021

dtantsur Jul 19, 2021

kirankt Jul 19, 2021

kirankt commented Jul 19, 2021 •

edited

zaneb commented Jul 19, 2021

zaneb left a comment

zaneb Jul 19, 2021

zaneb Jul 19, 2021

openshift-ci bot commented Jul 19, 2021

zaneb commented Jul 20, 2021

kirankt commented Jul 20, 2021

Custom deploy procedure support #156

Custom deploy procedure support #156

Conversation

kirankt commented Jul 13, 2021

kirankt commented Jul 13, 2021

kirankt commented Jul 13, 2021

kirankt commented Jul 13, 2021

dtantsur commented Jul 13, 2021

kirankt commented Jul 13, 2021

kirankt commented Jul 14, 2021

kirankt commented Jul 15, 2021

kirankt commented Jul 19, 2021

hardys commented Jul 19, 2021

hardys commented Jul 19, 2021

openshift-ci bot commented Jul 19, 2021

dtantsur commented Jul 19, 2021

dtantsur Jul 19, 2021

Choose a reason for hiding this comment

zaneb Jul 19, 2021

Choose a reason for hiding this comment

dtantsur Jul 19, 2021

Choose a reason for hiding this comment

kirankt Jul 19, 2021

Choose a reason for hiding this comment

kirankt commented Jul 19, 2021 • edited

zaneb commented Jul 19, 2021

zaneb left a comment

Choose a reason for hiding this comment

zaneb Jul 19, 2021

Choose a reason for hiding this comment

zaneb Jul 19, 2021

Choose a reason for hiding this comment

openshift-ci bot commented Jul 19, 2021

zaneb commented Jul 20, 2021

kirankt commented Jul 20, 2021

kirankt commented Jul 19, 2021 •

edited