Migrate to CRDS and Kube builder #33

marwanad · 2018-11-16T00:15:37Z

The diff is massive but going commit by commit should describe the migration flow.

Closes #8

azure-pipelines.yml

marwanad · 2018-11-16T00:21:23Z

Gopkg.toml

+  unused-packages = true
+
+[[prune.project]]


Disabling pruning for the cluster-api project to keep the sigs.k8s/cluster-api/config folder which is needed for applying the CRDs.

marwanad · 2018-11-16T00:24:51Z

cmd/clusterctl/examples/azure/cluster.yaml.template

+apiVersion: "cluster.k8s.io/v1alpha1"
+kind: Cluster
+metadata:
+  name: $CLUSTER_NAME


updated to use the generated cluster name

marwanad · 2018-11-16T04:23:25Z

config/crds/azureprovider_v1alpha1_azureclusterproviderconfig.yaml

@@ -0,0 +1,36 @@
+apiVersion: apiextensions.k8s.io/v1beta1


generated by kubebuilder

marwanad · 2018-11-16T04:23:45Z

cmd/manager/main.go

+		os.Exit(1)
+	}
+
+	clusterActuator, err := cluster.NewClusterActuator(cluster.ClusterActuatorParams{Client: mgr.GetClient()})


creates the actuator and registers them

marwanad · 2018-11-16T04:24:40Z

pkg/cloud/azure/actuators/machine/machine_status.go

+	if azure.client == nil {
+		return nil, nil
+	}
+	currentMachine, err := util.GetMachineIfExists(azure.client, m.ObjectMeta.Namespace, m.ObjectMeta.Name)


API changed upstream

marwanad · 2018-11-16T04:24:50Z

pkg/cloud/azure/actuators/machine/machine_status.go

+	if err != nil {
+		return err
+	}
+	return azure.client.Update(context.Background(), m)


API changed upstream

marwanad · 2018-11-16T04:27:50Z

pkg/cloud/azure/actuators/machine/machineactuator_test.go

+	}
+}
+
+// func TestUpdateNoSpecChange(t *testing.T) {


fake client was removed upstream so need to revisit unit testing this part.

marwanad · 2018-11-16T04:29:40Z

config/manager/manager.yaml

+  ports:
+  - port: 443
+---
+apiVersion: apps/v1


Logic is from the provider-components template with the un-used fields removed.

config/default/manager_image_patch.yaml

Comment out test cases that depend on the cluster v1 alpha1 client

…file

tariq1890 · 2018-12-05T00:15:15Z

Dockerfile

+# Build
+RUN CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -a -o manager github.com/platform9/azure-provider/cmd/manager
+
+# Copy the controller-manager into a thin image


Is alpine 3.8 possible?

yup - it should be. The file was generated using kube-builder and I think it defaulted to ubuntu.

tariq1890 · 2018-12-05T00:22:49Z

pkg/cloud/azure/actuators/machine/machine_status.go

+		return nil, nil
+	}
+	currentMachine, err := util.GetMachineIfExists(azure.client, m.ObjectMeta.Namespace, m.ObjectMeta.Name)
+	if err != nil {


Suggested change

if err != nil {

if err != nil || currentMachine == nil {

Can we merge the two if loops using that?

I left the two checks to differentiate between the cases when the machine is actually because it has been deleted and just erroring on attempting to call the query the clients to check for machine existence.

Can we merge them in a way without losing that info?

tariq1890 · 2018-12-05T00:23:05Z

pkg/cloud/azure/actuators/machine/machine_status.go

+		return nil
+	}
+	currentMachine, err := util.GetMachineIfExists(azure.client, machine.ObjectMeta.Namespace, machine.ObjectMeta.Name)
+	if err != nil {


See previous comment

tariq1890 · 2018-12-05T00:25:36Z

pkg/cloud/azure/actuators/machine/machine_status.go

+}
+
+func (azure *AzureClient) updateAnnotations(cluster *clusterv1.Cluster, machine *clusterv1.Machine) error {
+	if azure.client == nil {


Still not sure how I feel about these nil pre-checks at the beginning of a method.I would avoid this by having a nil check / validation much earlier in the call stack so that we don't do them over here. I may not have the complete context here of course. Open to discussing more.

Was mostly left there for testing. But we can just initialize the actuator in the tests using the fake client. I'll get rid of those checks.

So this change would require us to update the controller-runtime revision to pickup this change: https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/fake/client.go#L57 for the unit tests to pass.

I'd say we wait on this to reduce the scope of this PR.

tariq1890 · 2018-12-05T00:26:41Z

pkg/cloud/azure/services/mock_interfaces.go

@@ -0,0 +1,235 @@
+/*


Mocks !!! YAY!. Great going @marwanad :)

tariq1890 · 2018-12-05T00:28:29Z

pkg/cloud/azure/actuators/cluster/clusteractuator.go

+	if err != nil {
+		return fmt.Errorf("error creating or updating network security group: %v", err)
+	}
+	err = azure.network().WaitForNetworkSGsCreateOrUpdateFuture(*networkSGFuture)


can we move to azure.network() to a variable and reuse that?

The method network() is just a wrapper that returns azure.services.Network. It doesn't recreate the clients or anything but I can switch it over to local variables if you think that's better for readability.

tariq1890 · 2018-12-05T00:29:20Z

pkg/cloud/azure/actuators/cluster/clusteractuator.go

+	if err != nil {
+		return fmt.Errorf("error loading cluster provider config: %v", err)
+	}
+	resp, err := azure.resourcemanagement().CheckGroupExistence(clusterConfig.ResourceGroup)


can we move azure.resourcemanagement() and reuse that?

Suggested change

resp, err := azure.resourcemanagement().CheckGroupExistence(clusterConfig.ResourceGroup)

azResourceClient := azure.resourcemanagement()

ok-to-test

Looks good to me overall. I apologize for the not keeping up with the updates here. Thanks for keeping the tests up to date. I'll get this merged asap to prevent this PR from getting bigger.

vannrt · 2018-12-10T20:12:55Z

@ok-to-test Is an account I made for testing some webhooks.

vannrt

Thanks @marwanad

* added auto-scaling support for azure * changes to allow creationg of autoscaling machine pool during creation of AKS cluster

[ES-569654] Remove agent pool profiles from managed cluster update request

marwanad commented Nov 16, 2018

View reviewed changes

azure-pipelines.yml Outdated Show resolved Hide resolved

marwanad commented Nov 16, 2018

View reviewed changes

config/default/manager_image_patch.yaml Outdated Show resolved Hide resolved

marwanad changed the base branch from machine-actuator-update to master November 27, 2018 02:25

marwanad added 19 commits November 26, 2018 18:45

initial machine actuator update implementation

befff6d

update kubelet and control plane version

0fbb848

init kubebuilder project

3ca0378

add API types for machine and cluster provider config

0b4a88d

prepare for cluster api vendor upgrade

5e2c20b

vendor: update cluster-api and vendor controller-runtime dependencies

746cf33

move cloud directory under pkg

2c83deb

update cluster actuator to use controller manager

7288998

update machine actuator and machine status

b245b91

Comment out test cases that depend on the cluster v1 alpha1 client

remove unused providerconfig

1023e46

update manager to reigster acutators properly

32fa826

fix package import for azureconfigv1

75e319e

re-run codegen

085d8bd

remove unused controller

32bb521

make it compile

14f5fbe

move arm templates under resourcemanagement package and update Docker…

16bb34e

…file

update kustomize config files

8906a00

use generated cluster name in the template file

4539635

fix one machine update test case that doesnt utilize cluster api client

4d32b96

marwanad added 3 commits November 26, 2018 18:55

change docker image placeholder

ead34fa

export service principal values to a file

d914b2c

update README with the new setup instructions

129dfde

marwanad force-pushed the migrate-to-crd-kubebuilder branch from 90c93e5 to 129dfde Compare November 27, 2018 02:55

marwanad added 6 commits November 26, 2018 19:05

vendor: update packages

fa55115

add skipping creating service principal if flag is set

afe761c

update e2e tests

fe3957b

fix machine annotation update for first creation

9b8fd56

only update the status once for node recreation

a5b920d

install specific control plane version through kubeadm

9b3a268

tariq1890 reviewed Dec 5, 2018

View reviewed changes

marwanad added 2 commits December 5, 2018 16:09

upgrade kubectl client version with the control plane

aa97db8

switch to alpine 3.8

ab655ce

marwanad force-pushed the migrate-to-crd-kubebuilder branch from 2dd6a73 to ab655ce Compare December 6, 2018 00:10

use explicit argument indices for formatting strings

06cb84b

ok-to-test approved these changes Dec 10, 2018

View reviewed changes

vannrt approved these changes Dec 10, 2018

View reviewed changes

vannrt merged commit 9906ecd into master Dec 10, 2018

cblecker deleted the migrate-to-crd-kubebuilder branch January 10, 2019 00:29

tariq1890 mentioned this pull request Jan 15, 2019

REQUEST: New membership for tariq1890 kubernetes/org#362

Closed

6 tasks

marwanad mentioned this pull request Jan 22, 2019

REQUEST: New membership for marwanad kubernetes/org#396

Closed

6 tasks

satviksr-db pushed a commit to databricks/cluster-api-provider-azure-1 that referenced this pull request Nov 10, 2023

Merge pull request kubernetes-sigs#33 from databricks/es-569654

720720a

[ES-569654] Remove agent pool profiles from managed cluster update request

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to CRDS and Kube builder #33

Migrate to CRDS and Kube builder #33

marwanad commented Nov 16, 2018 •

edited

Loading

marwanad Nov 16, 2018 •

edited

Loading

marwanad Nov 16, 2018

marwanad Nov 16, 2018

marwanad Nov 16, 2018

marwanad Nov 16, 2018

marwanad Nov 16, 2018

marwanad Nov 16, 2018

marwanad Nov 16, 2018

tariq1890 Dec 5, 2018

marwanad Dec 5, 2018

tariq1890 Dec 5, 2018

marwanad Dec 5, 2018

tariq1890 Dec 5, 2018

tariq1890 Dec 5, 2018

marwanad Dec 5, 2018

marwanad Dec 5, 2018

tariq1890 Dec 5, 2018

tariq1890 Dec 5, 2018

marwanad Dec 5, 2018

tariq1890 Dec 5, 2018

ok-to-test left a comment

vannrt commented Dec 10, 2018

vannrt left a comment

	resp, err := azure.resourcemanagement().CheckGroupExistence(clusterConfig.ResourceGroup)
	azResourceClient := azure.resourcemanagement()

Migrate to CRDS and Kube builder #33

Migrate to CRDS and Kube builder #33

Conversation

marwanad commented Nov 16, 2018 • edited Loading

marwanad Nov 16, 2018 • edited Loading

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

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

ok-to-test left a comment

Choose a reason for hiding this comment

vannrt commented Dec 10, 2018

vannrt left a comment

Choose a reason for hiding this comment

marwanad commented Nov 16, 2018 •

edited

Loading

marwanad Nov 16, 2018 •

edited

Loading