Check provisioner ready status when applying reconcile logic #574

andfasano · 2020-07-06T11:45:06Z

This PR aims to improve the reconcile loop by checking the status of the Provisioner dependencies before trying to reconcile the state: if the provisioner is not ready, the current request is requeued.
In particular, it checks the availability of the Ironic and Ironic Inspector services (thanks to @stbenjam for specific check logic, extracted from https://github.com/openshift-metal3/terraform-provider-ironic/blob/master/ironic/provider.go).
This approach should help especially during the bootstrap of those deployment scenarios where the Ironic services are launched at the same time of the operator - where the reconcile loop could start to operate before Ironic services were effectively up and running.

metal3-io-bot · 2020-07-06T11:45:22Z

Hi @andfasano. Thanks for your PR.

I'm waiting for a metal3-io 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.

dhellmann · 2020-07-06T13:27:51Z

/ok-to-test

dhellmann · 2020-07-06T13:27:59Z

/test-integration

pkg/provisioner/ironic/ironic.go

pkg/provisioner/demo/demo.go

pkg/controller/baremetalhost/baremetalhost_controller.go

pkg/provisioner/ironic/ironic.go

andfasano · 2020-07-07T18:02:50Z

/test-integration

dhellmann

This still feels like a lot of extra code to be able to make 2 HTTP calls. I know you mentioned doing it in parallel for performance, but I'm not sure the baremetal-operator is ever going to be the bottleneck in the system. Ironic can take ages to actually provision a host, for example. Maybe we can talk through the approach on a call Wednesday?

pkg/controller/baremetalhost/baremetalhost_controller.go

pkg/provisioner/demo/demo.go

pkg/provisioner/ironic/ironicdependencies.go

pkg/provisioner/provisionerdependencies.go

stbenjam · 2020-07-08T11:46:58Z

pkg/provisioner/ironic/ironic_test.go

+	}
+
+	m.server = httptest.NewUnstartedServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		m.requests += r.Host + r.RequestURI + ";"


This doesn't feel like very idiomatic go to me - can we use slices instead?

It was done by design in reality to improve test readability / handling: with this testing technique events are accumulated in a string (using a separator), so that it would be easier to setup the expected string (and to read in case of failure, also the sequence is naturally represented). It's an approach that works well especially when there a few events.

stbenjam · 2020-07-08T11:51:29Z

pkg/provisioner/ironic/ironic_test.go

+		{
+			name:      "IsReady",
+			ironic:    newMockServer(6385).addDrivers(),
+			inspector: newMockServer(5050),


Are the ports hard coded so much that we have to do this? I'd feel better if we let go choose unused system ports which httptest can do.

If I'm running tests on my local dev box, I may very well have my own Ironic already running on these ports.

I think there's an impediment here:

baremetal-operator/pkg/provisioner/ironic/ironic.go

Lines 50 to 73 in 231ba34

func init() {

// NOTE(dhellmann): Use Fprintf() to report errors instead of

// logging, because logging is not configured yet in init().

deployKernelURL = os.Getenv("DEPLOY_KERNEL_URL")

if deployKernelURL == "" {

fmt.Fprintf(os.Stderr, "Cannot start: No DEPLOY_KERNEL_URL variable set\n")

os.Exit(1)

}

deployRamdiskURL = os.Getenv("DEPLOY_RAMDISK_URL")

if deployRamdiskURL == "" {

fmt.Fprintf(os.Stderr, "Cannot start: No DEPLOY_RAMDISK_URL variable set\n")

os.Exit(1)

}

ironicEndpoint = os.Getenv("IRONIC_ENDPOINT")

if ironicEndpoint == "" {

fmt.Fprintf(os.Stderr, "Cannot start: No IRONIC_ENDPOINT variable set\n")

os.Exit(1)

}

inspectorEndpoint = os.Getenv("IRONIC_INSPECTOR_ENDPOINT")

if inspectorEndpoint == "" {

fmt.Fprintf(os.Stderr, "Cannot start: No IRONIC_INSPECTOR_ENDPOINT variable set")

os.Exit(1)

}

}

. The init() method gets triggered before the test, and thus the related (not accessible) package variables fetching the env vars are already set once the newProvisioner call is made (and I think it's the reason why those env vars are set in the Makefile and also in hack/unit.sh). It could help to refactor the configuration initialization, maybe with a lazy setup.

I'm not really convinced that a unit test should be making network calls at all.

pkg/provisioner/provisionerdependencies.go

andfasano · 2020-07-13T10:37:39Z

/test-integration

pkg/provisioner/ironic/dependencies.go

pkg/provisioner/ironic/ironic_test.go

pkg/provisioner/ironic/dependencies.go

andfasano · 2020-07-14T16:56:17Z

/test unit

dhellmann · 2020-07-14T17:03:48Z

pkg/controller/baremetalhost/baremetalhost_controller.go

+	provStatus, err := prov.IsReady()
+	if err != nil {
+		return reconcile.Result{}, errors.Wrap(err,
+			fmt.Sprintf("failed to check services availability"))


This could just be an inline string, since there are no additional variables in the Sprintf(). That can be fixed in a follow-up PR unless there are other changes needed for this one.

zaneb

/approve

zaneb · 2020-07-14T17:39:18Z

pkg/provisioner/fixture/fixture.go

@@ -17,7 +17,7 @@ var provisionRequeueDelay = time.Second * 10

 // Provisioner implements the provisioning.Provisioner interface
 // and uses Ironic to manage the host.
-type fixtureProvisioner struct {
+type Provisioner struct {


What's the point of renaming this?

To allow setting the ready flag from the test

The type is private and only accessible via an interface that doesn't have the SetIsReady() function, so renaming it makes it public but still requires a type coercion in the test. What if we move it up into pkg/provisioner instead? That would let us keep it private, and the tests could instantiate one directly to avoid the type coercion.

If it is not a problem to move it then I think it sounds fine

Renamed the struct, see comment below

zaneb · 2020-07-14T17:50:33Z

pkg/provisioner/provisioner.go

+
+	// IsReady checks if the provisioning backend is available to accept
+	// all the incoming requests.
+	IsReady() (result Result, err error)


I'm sceptical about returning a provisioner.Result here. It's even less obvious than usual what the 'Dirty' flag means. A function named something like IsReady should return a bool.
I realise this is done so that we can return a RequeueAfter value, but I'm not sure that we really need that to be customisable by the provisioner.

I suggested the provisioner.Result in part to be consistent with the other methods, but I see your point.

I reverted back the IsReady() signature

metal3-io-bot · 2020-07-14T17:59:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, dhellmann, 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~~ [dhellmann,zaneb]

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

zaneb · 2020-07-28T14:26:22Z

pkg/provisioner/fixture/fixture.go

@@ -28,22 +28,25 @@ type fixtureProvisioner struct {
 	publisher provisioner.EventPublisher
 	// state to manage the two-step adopt process
 	adopted bool
+	// status of the provisioner
+	ready bool
 }

 // New returns a new Ironic Provisioner
 func New(host *metal3v1alpha1.BareMetalHost, bmcCreds bmc.Credentials, publisher provisioner.EventPublisher) (provisioner.Provisioner, error) {


This function is public and you can add whatever arguments you like, so maybe rather than making the type public you could allow the caller to pass something here (e.g. number of times to return false before returning true from IsReady(), or a user-supplied function to call from IsReady() to get the result).

Unfortunately the New function must match the provisioner.Factory type so I don't think it's recommended to modify its signature. Alternatively I could add another factory method (ie NewExt or some other better name), to be used only from the test, with the required additional params, and keep the type private.

I've added a new factory to allow injecting test parameters. With this approach, the fixture remains private and no need to move the file from its original location

dhellmann · 2020-07-29T11:56:49Z

/test-integration

This version looks OK to me. I'll leave it open for @zaneb to give the final approval since he also had some comments on earlier drafts.

andfasano · 2020-07-30T08:45:46Z

/test-integration

zaneb · 2020-07-30T15:08:25Z

I have some reservations about @stbenjam's comments, but we can always resolve that later; I think it's better not to hold up this work.
/lgtm

metal3-io-bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 6, 2020

metal3-io-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 6, 2020

andfasano mentioned this pull request Jul 6, 2020

[WIP] baremetal-operator: wait for Ironic services openshift/baremetal-operator#78

Closed

metal3-io-bot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 6, 2020

sadasu reviewed Jul 6, 2020

View reviewed changes

pkg/provisioner/ironic/ironic.go Outdated Show resolved Hide resolved

sadasu reviewed Jul 6, 2020

View reviewed changes

pkg/provisioner/ironic/ironic.go Outdated Show resolved Hide resolved

andfasano force-pushed the wait-for-ironic-upstream branch from cac72af to 958f654 Compare July 6, 2020 14:14

dhellmann reviewed Jul 6, 2020

View reviewed changes

andfasano force-pushed the wait-for-ironic-upstream branch from 1c47a5e to 9a3e71f Compare July 7, 2020 17:55

metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 7, 2020

andfasano requested review from dhellmann and sadasu July 7, 2020 18:01

dhellmann reviewed Jul 7, 2020

View reviewed changes

pkg/controller/baremetalhost/baremetalhost_controller.go Outdated Show resolved Hide resolved

pkg/provisioner/demo/demo.go Outdated Show resolved Hide resolved

stbenjam reviewed Jul 8, 2020

View reviewed changes

metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 8, 2020

andfasano requested a review from dhellmann July 8, 2020 16:28

andfasano force-pushed the wait-for-ironic-upstream branch from 65d4ce8 to 45d89c2 Compare July 9, 2020 09:39

metal3-io-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 10, 2020

dhellmann reviewed Jul 13, 2020

View reviewed changes

pkg/provisioner/ironic/dependencies.go Outdated Show resolved Hide resolved

pkg/provisioner/ironic/dependencies.go Outdated Show resolved Hide resolved

pkg/provisioner/ironic/ironic_test.go Show resolved Hide resolved

dhellmann reviewed Jul 14, 2020

View reviewed changes

pkg/provisioner/ironic/dependencies.go Outdated Show resolved Hide resolved

pkg/provisioner/ironic/dependencies.go Outdated Show resolved Hide resolved

dhellmann approved these changes Jul 14, 2020

View reviewed changes

metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2020

zaneb reviewed Jul 14, 2020

View reviewed changes

zaneb reviewed Jul 28, 2020

View reviewed changes

andfasano force-pushed the wait-for-ironic-upstream branch from 1809bc7 to 09b32a5 Compare July 29, 2020 11:16

metal3-io-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 29, 2020

Check provisioner ready status when applying reconcile logic

3f71294

andfasano force-pushed the wait-for-ironic-upstream branch from 09b32a5 to 3f71294 Compare July 30, 2020 07:32

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 30, 2020

metal3-io-bot merged commit d6eaf67 into metal3-io:master Jul 30, 2020

dhellmann mentioned this pull request Jul 30, 2020

Check provisioner ready status when applying reconcile logic openshift/baremetal-operator#89

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check provisioner ready status when applying reconcile logic #574

Check provisioner ready status when applying reconcile logic #574

andfasano commented Jul 6, 2020

metal3-io-bot commented Jul 6, 2020

dhellmann commented Jul 6, 2020

dhellmann commented Jul 6, 2020

andfasano commented Jul 7, 2020

dhellmann left a comment

stbenjam Jul 8, 2020

andfasano Jul 8, 2020

stbenjam Jul 8, 2020

andfasano Jul 8, 2020

zaneb Jul 30, 2020

andfasano commented Jul 13, 2020

andfasano commented Jul 14, 2020

dhellmann Jul 14, 2020

andfasano Jul 29, 2020

zaneb left a comment

zaneb Jul 14, 2020

andfasano Jul 15, 2020

dhellmann Jul 15, 2020

andfasano Jul 15, 2020

andfasano Jul 29, 2020

zaneb Jul 14, 2020

dhellmann Jul 15, 2020

andfasano Jul 29, 2020

metal3-io-bot commented Jul 14, 2020

zaneb Jul 28, 2020

andfasano Jul 28, 2020

andfasano Jul 29, 2020

dhellmann commented Jul 29, 2020

andfasano commented Jul 30, 2020

zaneb commented Jul 30, 2020

	func init() {
	// NOTE(dhellmann): Use Fprintf() to report errors instead of
	// logging, because logging is not configured yet in init().
	deployKernelURL = os.Getenv("DEPLOY_KERNEL_URL")
	if deployKernelURL == "" {
	fmt.Fprintf(os.Stderr, "Cannot start: No DEPLOY_KERNEL_URL variable set\n")
	os.Exit(1)
	}
	deployRamdiskURL = os.Getenv("DEPLOY_RAMDISK_URL")
	if deployRamdiskURL == "" {
	fmt.Fprintf(os.Stderr, "Cannot start: No DEPLOY_RAMDISK_URL variable set\n")
	os.Exit(1)
	}
	ironicEndpoint = os.Getenv("IRONIC_ENDPOINT")
	if ironicEndpoint == "" {
	fmt.Fprintf(os.Stderr, "Cannot start: No IRONIC_ENDPOINT variable set\n")
	os.Exit(1)
	}
	inspectorEndpoint = os.Getenv("IRONIC_INSPECTOR_ENDPOINT")
	if inspectorEndpoint == "" {
	fmt.Fprintf(os.Stderr, "Cannot start: No IRONIC_INSPECTOR_ENDPOINT variable set")
	os.Exit(1)
	}
	}

Check provisioner ready status when applying reconcile logic #574

Check provisioner ready status when applying reconcile logic #574

Conversation

andfasano commented Jul 6, 2020

metal3-io-bot commented Jul 6, 2020

dhellmann commented Jul 6, 2020

dhellmann commented Jul 6, 2020

andfasano commented Jul 7, 2020

dhellmann 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andfasano commented Jul 13, 2020

andfasano commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zaneb 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

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

Choose a reason for hiding this comment

metal3-io-bot commented Jul 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhellmann commented Jul 29, 2020

andfasano commented Jul 30, 2020

zaneb commented Jul 30, 2020