Update to v1alpha3 #172

bboreham · 2020-04-28T12:17:42Z

Fixes #170

Since upstream CAPI has changed so much this is an extremely breaking change: there is no automated upgrade from v1alpha1.
The v1alpha1 types are left in place for anyone who wants to run programs containing both.
We now run two controllers - upstream CAPI controller for Machine and Cluster, and wks controller for BareMetalMachine and BareMetalCluster.
Minimum Kubernetes version is now 1.16.0, as required by upstream CAPI.
Examples are not updated yet

richardcase

I've not looked at the parts that do the actual ssh / kubeadm stuff. I've just looked at the CAPI specific parts.

pkg/baremetal/v1alpha3/types.go

richardcase · 2020-04-29T07:22:03Z

cmd/controller/main.go

+	if err != nil {
+		log.Fatal(err)
+	}
+	if err = clusterReconciler.SetupWithManager(mgr, controller.Options{MaxConcurrentReconciles: 1}); err != nil {


Can the reconcile occur in parallel or does it have to be a singleton? A lot of the other provider expose a concurrency flag.

pkg/apis/wksprovider/controller/wksctl/cluster_controller.go

richardcase · 2020-04-29T07:30:26Z

pkg/apis/wksprovider/controller/wksctl/machine_controller.go

@@ -342,22 +393,11 @@ func (a *MachineActuator) installNewBootstrapToken(ns string) (*corev1.Secret, e
 }

 // Delete the machine. If no error is returned, it is assumed that all dependent resources have been cleaned up.
-func (a *MachineActuator) Delete(ctx context.Context, cluster *clusterv1.Cluster, machine *clusterv1.Machine) error {
-	contextLog := log.WithFields(log.Fields{"machine": machine.Name, "cluster": cluster.Name})
+func (a *MachineController) delete(ctx context.Context, c *baremetalspecv1.BareMetalCluster, machine *clusterv1.Machine, bmm *baremetalspecv1.BareMetalMachine) error {


Could we name this reconcileDelete instead to have consistency with other providers?

richardcase · 2020-04-29T07:31:36Z

pkg/apis/wksprovider/controller/wksctl/machine_controller.go

+	return ctrl.Result{}, nil
+}
+
+func (a *MachineController) create(ctx context.Context, c *baremetalspecv1.BareMetalCluster, machine *clusterv1.Machine, bmm *baremetalspecv1.BareMetalMachine) error {


Could this be called reconcileNormal or reconcile instead? This will be consistent with other providers.

pkg/baremetal/v1alpha3/types.go

richardcase · 2020-04-29T07:49:10Z

pkg/cluster/machine/machine.go

-func FirstMaster(machines []*clusterv1.Machine) *clusterv1.Machine {
-	for _, machine := range machines {
+// Machines and BareMetalMachines must be in the same order
+func FirstMaster(machines []*clusterv1.Machine, bl []*baremetalspecv1.BareMetalMachine) (*clusterv1.Machine, *baremetalspecv1.BareMetalMachine) {


I wonder if we should rethink how we denote master nodes. I'm not sure a label on Machine is the right place. I think we should probably implement a capi control plane instead to make it explicit. https://cluster-api.sigs.k8s.io/developer/architecture/controllers/control-plane.html

Assuming there is no way to reuse the existing kubeadm control plane. Or perhaps support both?

pkg/kubernetes/config/kubeconfig.go

luxas

Awesome work @bboreham 👏!
I think we shall merge this now, and fix the comments I spot here in smaller follow-up PRs.

luxas · 2020-06-24T08:28:21Z

cmd/controller/main.go

-	"github.com/weaveworks/wksctl/pkg/apis"
-	"github.com/weaveworks/wksctl/pkg/apis/wksprovider/controller/wksctl"
+	wks "github.com/weaveworks/wksctl/pkg/apis/wksprovider/controller/wksctl"
+	baremetalv1 "github.com/weaveworks/wksctl/pkg/baremetal/v1alpha3"


nit: I'd either name this baremetalapi, baremetalext (as in external version), just baremetal or fully-qualified baremetalv1alpha3, but to me v1 suffix means stable

I think we are following the precedent of upstream, e.g.:
https://github.com/kubernetes-sigs/cluster-api-provider-aws/blob/4831a1d5347b00a2523cf7b02845eee56fbac1f7/controllers/awsmachine_controller.go#L28

luxas · 2020-06-24T08:28:37Z

cmd/controller/main.go

-	clustercommon "sigs.k8s.io/cluster-api/pkg/apis/cluster/common"
-	capicluster "sigs.k8s.io/cluster-api/pkg/controller/cluster"
-	capimachine "sigs.k8s.io/cluster-api/pkg/controller/machine"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1alpha3"


Same here. Maybe just clusterapi?

luxas · 2020-06-24T08:30:04Z

cmd/controller/main.go

-	machineActuator, err := wks.NewMachineActuator(wks.MachineActuatorParams{
-		EventRecorder:       mgr.GetRecorder(wks.ProviderName + "-controller"),
+
+	machineController, err := wks.NewMachineController(wks.MachineControllerParams{


As discussed, maybe rename Controller to Reconciler

luxas · 2020-06-24T08:31:36Z

cmd/controller/main.go

+	}
+
+	var ctlrNamespace string
+	{


are you creating a new scope here? maybe add a comment why

To limit where client created on the line below can be used.
I suspect the correct fix is to not care about namespaces.

luxas · 2020-06-24T08:32:50Z

cmd/controller/main.go

+		if err != nil {
+			log.Fatalf("failed to create client: %s", err)
+		}
+		ctlrNamespace, err = initializeControllerNamespace(client)


what is this doing? machineutil.GetKubernetesNamespaceFromMachines that is called by this fn doesn't seem to change state, but this name suggests it does

Good question, but the name of that function did not change in this PR.

luxas · 2020-06-24T09:12:24Z

pkg/baremetal/v1alpha3/register.go

@@ -0,0 +1,46 @@
+package v1alpha3


A comment for later, but I'd maybe keep the v1alpha1 packages here for a grace period for being able to convert objects over eventually.

A comment for now: this is an api, it should exist under pkg/apis

luxas · 2020-06-24T09:12:50Z

pkg/baremetal/v1alpha3/register.go

+	return nil
+}
+
+func NewScheme() (*runtime.Scheme, error) {


This function seems unused?

luxas · 2020-06-24T09:14:29Z

pkg/cluster/machine/machine.go

-		return nil, err
-	}
+// Parse parses the provided machines io.Reader.
+func Parse(r io.ReadCloser) (ml []*clusterv1.Machine, bl []*baremetalspecv1.BareMetalMachine, err error) {


FYI: I've coded a pretty cool serializer that can do these things in a generic manner, so we can use that later to avoid writing the same code over and over again. But for a future PR.

luxas · 2020-06-24T09:15:44Z

pkg/cluster/machine/machine.go

 	}
 	Populate(machines)

-	errors := Validate(machines)
-	return errorsHandler(machines, errors)
+	errors := Validate(machines, bl)


I'd put validation of the API objects in pkg/apis/baremetal/validation

luxas · 2020-06-24T09:19:19Z

pkg/utilities/manifest/manifest.go

@@ -38,14 +38,14 @@ var AddToScheme = SchemeBuilder.AddToScheme

 func addKnownTypes(scheme *runtime.Scheme) error {


This function seems weird. I'd use the AddToScheme methods instead to get what's actually needed without duplicating code.

Many things have changed: - Where we had a List of Machine, we now have a YAML stream, i.e. multiple documents separated by --- - Where we had a single struct, we must deal with a ClusterAPI struct and a provider-specific struct Fix up ParseAndDefaultAndValidate to return both cluster structs

(renamed from "actuator")

(renamed from 'actuator')

To fit with upstream requirements: https://cluster-api.sigs.k8s.io/developer/architecture/controllers/cluster.html#infrastructure-provider

Blank means use the default, but when ImageRepository is set the current code is failing.

Almost everywhere uses Public.Address or Private.Address, so these other ones are just a source of confusion.

Signed-off-by: Dennis Marttinen <dennis@weave.works>

so it can be used by other packages

It's awkward having to do this every time the controller does anything.

This means the controller will see it as unchanged when it fires up.

bboreham force-pushed the update-to-v1alpha3 branch 2 times, most recently from 0347619 to 26d795b Compare April 28, 2020 15:04

richardcase reviewed Apr 29, 2020

View reviewed changes

bboreham force-pushed the update-to-v1alpha3 branch from d1649f2 to 4a08ebf Compare May 7, 2020 09:59

D3nn force-pushed the master branch from 84a21a1 to 6add7d4 Compare May 8, 2020 00:50

bboreham force-pushed the update-to-v1alpha3 branch 5 times, most recently from 851730f to 20d6bf6 Compare May 15, 2020 11:53

bboreham force-pushed the update-to-v1alpha3 branch 2 times, most recently from c434da7 to 2c65b3d Compare June 9, 2020 16:20

bboreham force-pushed the update-to-v1alpha3 branch from 6fc3ed9 to e6a282f Compare June 19, 2020 17:37

bboreham mentioned this pull request Jun 19, 2020

Use the correct "installer" to associate a standard node plan with the initial master node #207

Merged

bboreham force-pushed the update-to-v1alpha3 branch from e6a282f to fb82932 Compare June 23, 2020 16:15

luxas approved these changes Jun 24, 2020

View reviewed changes

bboreham added 14 commits June 24, 2020 14:43

Update ClusterAPI from v1alpha1 to v1alpha3: basic types

6769317

Update install_plans for v1alpha3

ee1264f

Update runners/ssh for v1alpha3

ac67cd8

Deal with changes in upstream Sealed Secrets API

be9f1bd

Deal with upstream change in kube-proxy config

37ec1de

Deal with upstream changes in cordon helper

0db4f18

Update os handling for extra 'spec' struct

02065fd

Update 'specs' package for v1alpha3

f07049b

Fetch fallback namespace from manifest, not from cluster

d680644

For v1alpha3 reuse cluster parsing from specs package

1feb337

BareMetal machine controller updated to v1alpha3

e5949cd

(renamed from "actuator")

BareMetal cluster controller updated to v1alpha3

9b202a0

(renamed from 'actuator')

Update controller main to v1alpha3

cdd097b

bboreham and others added 24 commits June 24, 2020 14:43

Split out function to write manifest from machine lists

da5779e

Further v1alpha3 updates for cluster.yaml

54a6c39

Update apply_test for v1alpha3

2a84ec8

Fail test early if setup returns an error

212ff95

fix up function to write manifest from machine lists

0d6db22

Update 'badmachines' part of test for v1alpha3

6be71a0

Set clusterName on machines in test

60f30c7

Rename field from ExternalLoadBalancer to ControlPlaneEndpoint

8adf3f6

To fit with upstream requirements: https://cluster-api.sigs.k8s.io/developer/architecture/controllers/cluster.html#infrastructure-provider

Update generated files after rebases

bdeb394

Add a 'ready' field to BareMetalCluster status

111fd21

Update docunmentation for v1alpha3

5ddd31b

Fix compile error in apply_test

574a8a6

Avoid failure when controllerImage is blank

aa74436

Blank means use the default, but when ImageRepository is set the current code is failing.

Fix up incorrect use of PrivateAddress in test

b1b2406

Remove top-level address and port fields

8408805

Almost everywhere uses Public.Address or Private.Address, so these other ones are just a source of confusion.

Fail parsing if manifest lacks Cluster or BareMetalCluster definition

7123c81

Signed-off-by: Dennis Marttinen <dennis@weave.works>

Clarify parsing error message

b090579

Add 'bootstrap' field to Machine; it's mandatory

1687425

refactor: move annotation key for plans

b6df6a6

so it can be used by other packages

Remove 'initializeMasterPlanIfNecessary' step

cee0d70

It's awkward having to do this every time the controller does anything.

Annotate initial master node with its plan

f93f176

This means the controller will see it as unchanged when it fires up.

Put a note in the README

cd91132

Update vfsdata files

e967587

Update go.sum

2b644aa

bboreham force-pushed the update-to-v1alpha3 branch from 550d9da to 2b644aa Compare June 24, 2020 14:49

bboreham marked this pull request as ready for review June 24, 2020 16:04

bboreham merged commit 0a81eff into master Jun 24, 2020

bboreham deleted the update-to-v1alpha3 branch June 24, 2020 16:21

twelho mentioned this pull request Jun 25, 2020

Add golangci-lint linting using Github Actions and fix errors #242

Merged

luxas mentioned this pull request Jul 2, 2020

Use the generic Serializer in libgitops, and fix some API machinery nits #257

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to v1alpha3 #172

Update to v1alpha3 #172

bboreham commented Apr 28, 2020 •

edited

richardcase left a comment

richardcase Apr 29, 2020

richardcase Apr 29, 2020

richardcase Apr 29, 2020

richardcase Apr 29, 2020 •

edited

richardcase Apr 29, 2020

luxas left a comment

luxas Jun 24, 2020

bboreham Jun 24, 2020

luxas Jun 24, 2020

luxas Jun 24, 2020

luxas Jun 24, 2020

bboreham Jun 24, 2020

luxas Jun 24, 2020

bboreham Jun 24, 2020

luxas Jun 24, 2020

luxas Jun 24, 2020

bboreham Jun 24, 2020

luxas Jun 24, 2020

luxas Jun 24, 2020

luxas Jun 24, 2020

		@@ -38,14 +38,14 @@ var AddToScheme = SchemeBuilder.AddToScheme

		func addKnownTypes(scheme *runtime.Scheme) error {

Update to v1alpha3 #172

Update to v1alpha3 #172

Conversation

bboreham commented Apr 28, 2020 • edited

richardcase 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

richardcase Apr 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas 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

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

bboreham commented Apr 28, 2020 •

edited

richardcase Apr 29, 2020 •

edited