Supplemental update for v1alpha2 #896

vincepri · 2019-07-12T18:31:31Z

Signed-off-by: Vince Prignano [email protected]

What this PR does / why we need it:
This PR is a supplemental update for #892 and adds the following:

Refactors the controller tests to follow CAPI.
Adds a MapFunc to trigger AWSMachine updates from Machines.
Updates manager/main.go to include the new controller and API types.
Add the AWSMachineTemplate type to be used with MachineSets.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Please confirm that if this PR changes any image versions, then that's the sole change this PR makes.

Release note:

k8s-ci-robot · 2019-07-12T18:31:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

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

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

chuckha

this looks fine to me, one q about the rbac though

chuckha · 2019-07-12T19:59:44Z

config/rbac/role.yaml

 - apiGroups:
-  - ""
+  - cluster.sigs.k8s.io


this is the v1a1 api group for clusters?

This is CAPI v1a2 group

no, it's not

i solved my rbac issue thanks to this 😂

oops sorry didn't see your response, i was responding to myself, not you!

vincepri · 2019-07-12T20:30:42Z

/test pull-cluster-api-provider-aws-bazel-integration

vincepri · 2019-07-13T16:33:53Z

/test pull-cluster-api-provider-aws-bazel-integration

Signed-off-by: Vince Prignano <[email protected]>

chuckha

b031db8

this commit looks fine. the code is removed as it should be in CABPK

Signed-off-by: Vince Prignano <[email protected]>

vincepri · 2019-07-16T17:55:04Z

/test pull-cluster-api-provider-aws-bazel-integration

detiber · 2019-07-16T17:13:28Z

cmd/manager/main.go

 		CoreClient:     coreClient,
-		Client:         cs.ClusterV1alpha2(),
+		ClusterClient:  cs.ClusterV1alpha2(),


Not needed currently, but as we continue to refine for v1alpha2, since we have access to the manager client, we should probably remove the typed clients.

Totally agree, once the Cluster actuator is also out, we might be able to get rid of them easily

detiber · 2019-07-16T17:16:12Z

config/crds/infrastructure.cluster.sigs.k8s.io_awsmachines.yaml

              items:
-                description: AWSMachineProviderCondition is a condition in a AWSMachineProviderStatus
+                description: NodeAddress contains information for the node's address.


s/NodeAddress/MachineAddresses or InstanceAddresses since we said that these do not need to align with the reported NodeAddresses from the Cloud Provider integration.

Do we need to fork/copy the NodeAddress struct?

I'm thinking it wouldn't hurt to create our own with inspiration from the NodeAddress struct, since we could very well end up with different requirements down the line.

I'll open a PR to CAPI to fork the type, so other providers can use it as well. I can follow-up in a new PR to fixup the CRD/dependency or fix it here, which do you prefer?

followup is good

detiber · 2019-07-16T17:23:35Z

pkg/apis/awsprovider/v1alpha1/types.go

@@ -25,6 +25,12 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )

+const (
+	AnnotationClusterInfrastructureReady = "aws.cluster.sigs.k8s.io/infrastructure-ready"


Should this be: aws.infrastructure.cluster.sigs.k8s.io instead?

(For v1alpha2)

pkg/apis/infrastructure/v1alpha2/awsmachine_types.go

pkg/apis/infrastructure/v1alpha2/types.go

pkg/controller/awsmachine/awsmachine_controller.go

detiber · 2019-07-16T18:18:13Z

pkg/controller/awsmachine/awsmachine_controller.go

+}
+
+// create creates a machine and is invoked by the machine controller.
+func (r *ReconcileAWSMachine) create(scope *actuators.MachineScope) error {


Since this is eventually calling CreateOrGetMachine should this be named createOrGet instead?

I don't think so, I copied the old actuator code. But this create call only happens if the exist call doesn't succeed

We probably need to clean this up quite a bit down the line, no need to do additional queries against the AWS api if we've already verified the existence.

Completely agree

detiber · 2019-07-16T18:19:06Z

pkg/controller/awsmachine/awsmachine_controller.go

+
+	scope.SetInstanceID(i.ID)
+	scope.SetInstanceState(i.State)
+	scope.SetAnnotation("cluster-api-provider-aws", "true")


Shouldn't this annotation be namespaced?

We can definitely namespace it, I copied it from the old codebase

detiber · 2019-07-16T18:20:16Z

pkg/controller/awsmachine/awsmachine_controller.go

+	case infrav1.InstanceStateRunning:
+		scope.Info("Machine instance is running", "instance-id", *scope.GetInstanceID())
+	case infrav1.InstanceStatePending:
+		scope.Info("Machine instance is pending", "instance-id", *scope.GetInstanceID())


We said that the Machine should enter an error state if the underlying instance is no longer available, should that be taken into account here?

I'll follow-up with more logic changes in later PRs, how does that sound? There is a bunch of things to take care of, e.g. Ready/Addresses/etc

Signed-off-by: Vince Prignano <[email protected]>

ncdc · 2019-07-16T19:22:35Z

pkg/apis/infrastructure/v1alpha2/awsmachine_types.go

 }

 // AWSMachineStatus defines the observed state of AWSMachine
 type AWSMachineStatus struct {
-	// InstanceID is the instance ID of the machine created in AWS
+	// Ready is true when the provider resource is ready.
+	Ready *bool `json:"ready,omitempty"`


Remind me why this is optional?

We went for *bool on Status.InfrastructureReady, didn't we want to keep all of them consistent? I honestly don't remember anymore why we decided to go that route, I'll need to dig some CAPI PR

pkg/apis/infrastructure/v1alpha2/awsmachinetemplate_types.go

detiber · 2019-07-16T20:53:15Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 12, 2019

k8s-ci-robot requested review from detiber and ingvagabund July 12, 2019 18:31

k8s-ci-robot 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 Jul 12, 2019

vincepri force-pushed the supplemental-v1alpha2 branch 5 times, most recently from 00340df to 2596d19 Compare July 12, 2019 19:00

chuckha reviewed Jul 12, 2019

View reviewed changes

vincepri force-pushed the supplemental-v1alpha2 branch 2 times, most recently from edb1cb2 to 5bdf1d1 Compare July 12, 2019 20:16

vincepri force-pushed the supplemental-v1alpha2 branch from 5bdf1d1 to ac34e79 Compare July 13, 2019 16:08

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2019

vincepri added 4 commits July 15, 2019 14:25

Supplemental update for v1alpha2

d082b20

Signed-off-by: Vince Prignano <[email protected]>

Check that bootstrap.data is populated and available

f5de5d4

Signed-off-by: Vince Prignano <[email protected]>

Add finalizer

a5ca9d5

Signed-off-by: Vince Prignano <[email protected]>

Migrate actuator logic

59ab908

Signed-off-by: Vince Prignano <[email protected]>

vincepri force-pushed the supplemental-v1alpha2 branch from 6a88e5a to 59ab908 Compare July 15, 2019 21:25

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 15, 2019

vincepri added 6 commits July 15, 2019 15:35

Move controlplane init locker, fix cluster actuator

6d2c47f

Signed-off-by: Vince Prignano <[email protected]>

Fixup actuator client and uncomment code

795a8ee

Signed-off-by: Vince Prignano <[email protected]>

Remove code that should be in bootstrap controller

b031db8

Signed-off-by: Vince Prignano <[email protected]>

Move const at the top of the file

1026799

Signed-off-by: Vince Prignano <[email protected]>

rename scope to cluster scope

5c4d210

Signed-off-by: Vince Prignano <[email protected]>

Add helper methods to scope

ec7fb9b

Signed-off-by: Vince Prignano <[email protected]>

chuckha reviewed Jul 16, 2019

View reviewed changes

update vendored version of capi

d0e1047

Signed-off-by: Vince Prignano <[email protected]>

vincepri force-pushed the supplemental-v1alpha2 branch from a5bf3c8 to d0e1047 Compare July 16, 2019 16:47

vincepri added 2 commits July 16, 2019 10:05

update controller to use map func

5adf1de

Signed-off-by: Vince Prignano <[email protected]>

Retrieve cluster on machine scope, update tests, update vendor

ad4d143

Signed-off-by: Vince Prignano <[email protected]>

detiber reviewed Jul 16, 2019

View reviewed changes

vincepri added 6 commits July 16, 2019 11:28

Update rbac

13412e6

Signed-off-by: Vince Prignano <[email protected]>

rename annotation group

1f34d42

Signed-off-by: Vince Prignano <[email protected]>

address actuator feedback

a3809ee

Signed-off-by: Vince Prignano <[email protected]>

Use utils to get owner Machine in scope

46f5fe9

Signed-off-by: Vince Prignano <[email protected]>

update vendor and add status.errors for awsmachine

74188ec

Signed-off-by: Vince Prignano <[email protected]>

fix tests

5783669

Signed-off-by: Vince Prignano <[email protected]>

ncdc reviewed Jul 16, 2019

View reviewed changes

pkg/apis/infrastructure/v1alpha2/awsmachinetemplate_types.go Show resolved Hide resolved

k8s-ci-robot assigned detiber Jul 16, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 16, 2019

k8s-ci-robot merged commit 8347a46 into kubernetes-sigs:master Jul 16, 2019

sbueringer mentioned this pull request Aug 16, 2019

[v1alpha2]: prepare for v1alpha2 adoption kubernetes-sigs/cluster-api-provider-openstack#380

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supplemental update for v1alpha2 #896

Supplemental update for v1alpha2 #896

vincepri commented Jul 12, 2019

k8s-ci-robot commented Jul 12, 2019

chuckha left a comment

chuckha Jul 12, 2019

vincepri Jul 12, 2019

chuckha Jul 12, 2019

chuckha Jul 12, 2019

vincepri commented Jul 12, 2019

vincepri commented Jul 13, 2019

chuckha left a comment

vincepri commented Jul 16, 2019

detiber Jul 16, 2019

vincepri Jul 16, 2019

detiber Jul 16, 2019

vincepri Jul 16, 2019

detiber Jul 16, 2019

vincepri Jul 16, 2019

detiber Jul 16, 2019

detiber Jul 16, 2019

vincepri Jul 16, 2019

vincepri Jul 16, 2019

detiber Jul 16, 2019

vincepri Jul 16, 2019

detiber Jul 16, 2019

vincepri Jul 16, 2019

detiber Jul 16, 2019

vincepri Jul 16, 2019

detiber Jul 16, 2019

vincepri Jul 16, 2019

ncdc Jul 16, 2019

vincepri Jul 16, 2019

detiber commented Jul 16, 2019

Supplemental update for v1alpha2 #896

Supplemental update for v1alpha2 #896

Conversation

vincepri commented Jul 12, 2019

k8s-ci-robot commented Jul 12, 2019

chuckha 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

vincepri commented Jul 12, 2019

vincepri commented Jul 13, 2019

chuckha left a comment

Choose a reason for hiding this comment

vincepri commented Jul 16, 2019

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

detiber commented Jul 16, 2019