rebuild using kubebuilder 2.3.1 by dhellmann · Pull Request #650 · metal3-io/baremetal-operator

dhellmann · 2020-09-24T14:18:15Z

This implements the work described in
https://github.com/metal3-io/metal3-docs/blob/master/design/baremetal-operator/kubebuilder-migration.md

It deviates a bit from the work list, because it turns out not to be
necessary to fork controller-tools now that they have added a flag to
support "unsafe" types like our float64 field.

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

$ kubebuilder version && kubebuilder init --domain "" Version: version.Version{KubeBuilderVersion:"2.3.1", KubernetesVendor:"1.16.4", GitCommit:"8b53abeb4280186e494b726edf8f54ca7aa64a49", BuildDate:"2020-03-26T16:42:00Z", GoOs:"unknown", GoArch:"unknown"} Writing scaffold for you to edit... Get controller runtime: $ go get sigs.k8s.io/controller-runtime@v0.5.0 Update go.mod: $ go mod tidy Running make: $ make /home/dhellmann/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..." go fmt ./... go vet ./... go build -o bin/manager main.go Next: define a resource with: $ kubebuilder create api Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

$ kubebuilder edit --multigroup=true Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

$ kubebuilder create api --group metal3.io --version v1alpha1 --kind BareMetalHost Create Resource [y/n] y Create Controller [y/n] y Writing scaffold for you to edit... apis/metal3.io/v1alpha1/baremetalhost_types.go controllers/metal3.io/baremetalhost_controller.go Running make: $ make /home/dhellmann/go/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..." go fmt ./... go vet ./... go build -o bin/manager main.go make manifests Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Use the latest release and pass the arguments we need to allow us to use the float64 field for clock speed and to generate output consistent with what we had from the operator-sdk. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Move the old implementation to the new location. Update Makefile to only run controller-gen over our api package. Run 'make manifests' and 'make generate' and check in the output. Fix source of api version details for baremetalhost. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Move the environment variable check from init() to the point where the value of the variable would be used. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Filter reconcile events for hosts where the error count is what changed. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

* Ensure every provisioner created in the tests has a logger. * Use the right request and result types. * Create a separate test scheme and create the fake clients using that scheme. * Change old r.client.* calls to r.* Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Update the Makefile to include the targets from before the kubebuilder conversion. Fix the hack scripts used by some of those targets to handle the new layout. Add makefile rules for programs in cmds Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Update go.mod and go.sum based on the values used before the conversion. Run `make mod`. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

metal3-io-bot · 2020-09-24T14:19:05Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dhellmann

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dhellmann]

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

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann · 2020-09-24T14:23:05Z

/test-integration

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Match the go version in the module file. Fix source copy instructions in the build phase. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

metal3-dev-env expects this repo to have build/Dockerfile for building the image, since that's what the operator-sdk generated. The new kubebuilder Dockerfile is in the root of the repo. In order to be able to land this patch series, we need either to work. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

The event filter may be given references to owned objects, like the secrets used for passwords. It needs to accept those instead of panicking, so if the type assertion fails then we treat the case as needing reconciliation. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

asalkeld · 2020-09-25T02:42:48Z

+    spec:
+      containers:
+      - name: kube-rbac-proxy
+        image: gcr.io/kubebuilder/kube-rbac-proxy:v0.5.0


is the image version not going to be needing updating? is it worth kustomizing

This was produced by the code generator. @maelk and I are going to collaborate to fix up all of the kustomize inputs after this PR has landed and we can unblock the other PRs in this repo.

asalkeld · 2020-09-25T02:45:29Z

 	"math/rand"

-	metal3 "github.com/metal3-io/baremetal-operator/pkg/apis/metal3/v1alpha1"
+	metal3 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"


put "time" at the top? what import ordering/grouping are we following?

I've fixed up the imports in all of the files in this directory to use a consistent order.

asalkeld · 2020-09-25T02:47:12Z

+		}
+	}()
+
+	_ = context.Background()


is this ^ doing anything?

No, that's debris from the code generator output. Removed.

asalkeld · 2020-09-25T03:07:37Z

+		ironic.LogStartup()
+	}
+
+	if err = (&metal3iocontroller.BareMetalHostReconciler{


personally not a fan of this making the Reconciler struct public, you could just make a metal3iocontroller.New()

This is what kubebuilder generates. I don't know if the controller runtime expects these members to be public or not, but didn't want to deviate too far from what the code generator does in this PR.

asalkeld · 2020-09-25T03:18:51Z

+CONTROLLER_GEN=$(GOBIN)/controller-gen
+else
+CONTROLLER_GEN=$(shell which controller-gen)
+endif


you could put this at the top

ifeq (, $(shell which controller-gen)) CONTROLLER_GEN=$(GOBIN)/controller-gen else CONTROLLER_GEN=$(shell which controller-gen) endif

then

$(CONTROLLER_GEN): #install stuff

just to avoid the phony target and repeated installs

actually isn't it better to vendor? if the generated code varies across versions that's going to be a pain

The install logic uses the specific version we need. It's based on what kubebuilder generates. We could enhance it to install into a local directory, like with kustomize, in another PR if we think that's important.

The old images used /baremetal-operator as the entry point, so let's use that for the new version to maintain compatibility. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Group by stdlib, non-k8s, k8s, then metal3 Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Update the version of go used in the baremetal-operator jobs in preparation for metal3-io/baremetal-operator#650 Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann · 2020-09-25T17:44:20Z

We may need metal3-io/project-infra#151 to make the jobs work.

dhellmann · 2020-09-25T18:04:57Z

/test-integration

zaneb

There's a lot here. The only way I've found to review it is to basically recreate the migration myself and then do diffs against this branch to find out what I missed. I'm only part way through, but I did have some questions so I'll post the ones I've found now.

I also have an idea for how to structure the history so that e.g. git blame is not lost on important files like the bmh controller. It should make rebasing a bit easier as well, so we might be able to unblock other changes. I'm still working my way through all the diffs though (with a goal of ending up in roughly the same place via a different history graph). So I'll have to come back to this on Monday.

zaneb · 2020-09-25T20:12:33Z

@@ -2,7 +2,14 @@
 # Image URL to use all building/pushing image targets
 IMG ?= controller:latest
 # Produce CRDs that work back to Kubernetes 1.11 (no version conversion)


Should delete this comment.

zaneb · 2020-09-26T02:35:34Z

+#     eventually allowing conversion webhooks
+# allowDangerousTypes=true lets use the float64 field for clock speeds
+# crdVersions=v1 generates the v1 version of the CRD type
+# preserveUnknownFields=false causes the API server to discard "extra" data instead of storing it


I'm pretty sure this is unnecessary when crdVersions=v1 is set

I think you're right. I figured being explicit wouldn't hurt, though.

zaneb · 2020-09-26T02:46:47Z

+	BootMode BootMode `json:"bootMode,omitempty"`
+}
+
+// +k8s:deepcopy-gen:interfaces=k8s.io/apimachinery/pkg/runtime.Object


It appears that kubebuilder uses:

// +kubebuilder:object:root=true

instead of this (several times in this file). I assume that's a replacement for the same thing?

Perhaps? I just moved the old file into the new location, and I probably should have done what I did with the controller where I copied the contents over by hand in pieces.

Further research reveals that they're probably not related, but the deepcopy-gen thing was to cause it to generate a DeepCopyObject() method, which was some ancient workaround that doesn't appear to be needed any more since we are generating this anyway.

zaneb · 2020-09-26T02:59:52Z

-	flag.StringVar(&metricsAddr, "metrics-addr", ":8080", "The address the metric endpoint binds to.")
-	flag.BoolVar(&enableLeaderElection, "enable-leader-election", false,
-		"Enable leader election for controller manager. "+
-			"Enabling this will ensure there is only one active controller manager.")


We don't want to keep the enableLeaderElection flag?

zaneb · 2020-09-26T04:22:11Z

+	// is for multi-tenancy, then the BMO should watch only the provided
+	// namespace.
+	flag.StringVar(&watchNamespace, "namespace", "",
+		"Namespace that the controller watches to reconcile BMO objects.")


Is it safe to add this without adding the code that we used to have to look up the current namespace if it wasn't supplied?

The old lookup code was part of the operator-sdk library that has been dropped. AFAICT, we always set the namespace explicitly. We may want to add some logic to let us use an environment variable to do that more easily than a command line flag, for when we're running inside the cluster.

That's fine by me as long as we comply with the CAPI requirements, that the namespace should be providable by command line, and if unset (both in command line and environment variable in that case), default to all namespaces.

dhellmann · 2020-09-26T14:43:24Z

There's a lot here. The only way I've found to review it is to basically recreate the migration myself and then do diffs against this branch to find out what I missed. I'm only part way through, but I did have some questions so I'll post the ones I've found now.

I also have an idea for how to structure the history so that e.g. git blame is not lost on important files like the bmh controller. It should make rebasing a bit easier as well, so we might be able to unblock other changes. I'm still working my way through all the diffs though (with a goal of ending up in roughly the same place via a different history graph). So I'll have to come back to this on Monday.

Perhaps I should remove the holds on other PRs and redo this PR with that new approach. Let's talk Monday.

maelk · 2020-09-27T14:49:26Z

Perhaps I should remove the holds on other PRs and redo this PR with that new approach. Let's talk Monday.

If so, then we'll need to revert the change of image for the generate job in project-infra. That effectively is blocking all other PRs for now.

dhellmann · 2020-09-27T15:17:27Z

Perhaps I should remove the holds on other PRs and redo this PR with that new approach. Let's talk Monday.

If so, then we'll need to revert the change of image for the generate job in project-infra. That effectively is blocking all other PRs for now.

Let's see if #655 is easier to review. If not, then I'll revert the image change in infra to unblock others while we figure out how to make the migration reviewable.

maelk · 2020-09-28T06:03:52Z

It was not possible to review either this or #655 on Github UI due to the file number restriction (max 3000). But I managed with vscode Github extension. Unfortunately it did not properly show the moved files (they were displayed as added) but overall could be reviewed without major issues.

dhellmann · 2020-09-28T16:33:15Z

The consensus seems to be that #655 is easier to follow, so I'm going to close this one.

dhellmann added 22 commits September 24, 2020 09:12

remove old operator-sdk files

0e66eca

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

remove unused test files

140db05

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

remove unused version file

afac59f

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

remove old manager main

2b7ec5e

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

turn on multi-group for kubebuilder

42c6d61

$ kubebuilder edit --multigroup=true Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

update controller tools version and arguments

cf1459f

Use the latest release and pass the arguments we need to allow us to use the float64 field for clock speed and to generate output consistent with what we had from the operator-sdk. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

add command line arguments to main.go

9a2c6c2

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

update imports for baremetalhost api

7d3bd7b

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

update host reconciler to take a provisioner factory argument

3537ac8

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

configure the concurrency option for the controller

9b15772

Move the environment variable check from init() to the point where the value of the variable would be used. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

watch for secrets owned by hosts

37ac7c6

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

add the watch filter for error count

324b089

Filter reconcile events for hosts where the error count is what changed. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

import the logic from the old controller to the new one

f903ebe

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

remove remains of old controller

c7003e5

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

include version details in build and log on startup

d545f89

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

restore extra Makefile targets

e30d7fa

Update the Makefile to include the targets from before the kubebuilder conversion. Fix the hack scripts used by some of those targets to handle the new layout. Add makefile rules for programs in cmds Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

add 'mod' make target to clean up go modules files

04ab546

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

restore module list

a80cd59

Update go.mod and go.sum based on the values used before the conversion. Run `make mod`. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

metal3-io-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 24, 2020

add unit-cover make target needed by CI job

9b12428

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann added 3 commits September 24, 2020 10:29

add 'docker' makefile target

65887fe

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

fix up dockerfile

4eeda97

Match the go version in the module file. Fix source copy instructions in the build phase. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann added 3 commits September 24, 2020 15:43

use a valid leader election id

42ec1c0

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

apply consistency to logger names

e8bc531

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann force-pushed the kubebuilder-rewrite branch from f61be06 to efff9d2 Compare September 24, 2020 20:23

asalkeld reviewed Sep 25, 2020

View reviewed changes

dhellmann added 5 commits September 25, 2020 13:20

make entry point in dockerfile baremetal-operator

e9387bf

The old images used /baremetal-operator as the entry point, so let's use that for the new version to maintain compatibility. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

makefile: use phony and comment formatting consistently

38a9e57

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

standardize import groupings in controller code

05c6fe4

Group by stdlib, non-k8s, k8s, then metal3 Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

restore makefile rule to install kustomize

af18f8c

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

remove unused context from code generator output

7487af6

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann force-pushed the kubebuilder-rewrite branch from bf44d9d to 7487af6 Compare September 25, 2020 17:25

update hack scripts to use go 1.14

00f8f4e

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann mentioned this pull request Sep 25, 2020

update go images for baremetal-operator jobs metal3-io/project-infra#151

Merged

dhellmann mentioned this pull request Sep 25, 2020

handle externally provisioned hosts without image settings #609

Closed

zaneb reviewed Sep 26, 2020

View reviewed changes

dhellmann mentioned this pull request Sep 27, 2020

migrate to kubebuilder 2.3.1 #655

Merged

dhellmann closed this Sep 28, 2020

Conversation

dhellmann commented Sep 24, 2020

Uh oh!

metal3-io-bot commented Sep 24, 2020

Uh oh!

dhellmann commented Sep 24, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhellmann commented Sep 25, 2020

Uh oh!

dhellmann commented Sep 25, 2020

Uh oh!

zaneb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhellmann commented Sep 26, 2020

Uh oh!

maelk commented Sep 27, 2020

Uh oh!

dhellmann commented Sep 27, 2020

Uh oh!

maelk commented Sep 28, 2020

Uh oh!

dhellmann commented Sep 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants