✨ Minimal MachineHealthCheck Reconciler skeleton #2101

JoelSpeed · 2020-01-20T11:42:22Z

Note: This PR is currently containing the commits #2049 and is blocked on that. I've separated the two so that the new controller and functionality can be reviewed in smaller chunks. If reviewing, ignore anything outside of the controllers folder.

What this PR does / why we need it:

This adds the controller structure for the Machine HealthChecking controller. It is not currently registered in main.go so will not be active even after this is merged, that will be done once all business logic is implemented.

So far, all that this does is setup the controller, fetch the instance, check the cluster it refers to exists and isn't paused and labels the MachineHealthCheck with the cluster name as per the other types within v1alpha3.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Skeleton reconcile logic from tracking issue #1990

JoelSpeed · 2020-01-20T11:56:53Z

/hold

#2049 Should be merged before this, I will rebase once that is done

vincepri · 2020-01-21T16:42:11Z

@JoelSpeed Can you please merge the two PRs so they can be reviewed together?

JoelSpeed · 2020-01-21T16:43:51Z

@vincepri I assumed it would be easier to review if it were two separate PRs, reviewing the first before merging the second?

If you'd rather review them together, then this PR contains all commits from the previous one, so I could close the previous in favour of this? (Though I noticed my last commit needs a fixup)

vincepri · 2020-01-21T16:45:19Z

Closing the other one seems fine to me, let's keep the commits separate until we're ready to merge the PR in, usually we require to squash at the end.

JoelSpeed · 2020-01-22T10:14:02Z

/unhold

Ready for review whenever people have time

api/v1alpha3/machinehealthcheck_types.go

api/v1alpha3/machinehealthcheck_webhook_test.go

controllers/machinehealthcheck_controller.go

ncdc · 2020-01-28T17:14:34Z

controllers/machinehealthcheck_controller_test.go

+		By("Ensuring the namespace exists")
+		if err := k8sClient.Get(ctx, client.ObjectKey{Name: namespace.GetName()}, namespace.DeepCopy()); apierrors.IsNotFound(err) {
+			Expect(k8sClient.Create(ctx, namespace.DeepCopy())).To(Succeed())
+		}
+		By("Creating the Cluster")
+		Expect(k8sClient.Create(ctx, testCluster.DeepCopy())).To(Succeed())
+	})
+
+	AfterEach(func() {
+		By("Deleting any MachineHealthChecks")
+		Expect(cleanupTestMachineHealthChecks(ctx, k8sClient)).To(Succeed())
+		By("Deleting the Cluster")
+		Expect(k8sClient.Delete(ctx, testCluster.DeepCopy())).To(Succeed())


Why all the DeepCopy() calls?

I was trying to avoid modifying the namespace and testCluster variables so they would be clean for each test, and they would be populated correctly when Ginkgo is building it's test graph, but I've managed to refactor to avoid the deepcopy's

ncdc · 2020-01-28T17:17:02Z

controllers/machinehealthcheck_controller_test.go

+		return err
+	}
+	for _, mhc := range mhcList.Items {
+		err = c.Delete(ctx, mhc.DeepCopy())


Why DeepCopy?

This one is because I needed a pointer and mhc is a range variable, I've declared a second variable in the scope and that works, don't know if that's better or not?

controllers/machinehealthcheck_controller_test.go

JoelSpeed

@ncdc thanks for the review, I've addressed your feedback

api/v1alpha3/machinehealthcheck_webhook_test.go

controllers/machinehealthcheck_controller.go

JoelSpeed · 2020-01-28T17:52:26Z

controllers/machinehealthcheck_controller_test.go

+		By("Ensuring the namespace exists")
+		if err := k8sClient.Get(ctx, client.ObjectKey{Name: namespace.GetName()}, namespace.DeepCopy()); apierrors.IsNotFound(err) {
+			Expect(k8sClient.Create(ctx, namespace.DeepCopy())).To(Succeed())
+		}
+		By("Creating the Cluster")
+		Expect(k8sClient.Create(ctx, testCluster.DeepCopy())).To(Succeed())
+	})
+
+	AfterEach(func() {
+		By("Deleting any MachineHealthChecks")
+		Expect(cleanupTestMachineHealthChecks(ctx, k8sClient)).To(Succeed())
+		By("Deleting the Cluster")
+		Expect(k8sClient.Delete(ctx, testCluster.DeepCopy())).To(Succeed())


I was trying to avoid modifying the namespace and testCluster variables so they would be clean for each test, and they would be populated correctly when Ginkgo is building it's test graph, but I've managed to refactor to avoid the deepcopy's

JoelSpeed · 2020-01-28T17:53:02Z

controllers/machinehealthcheck_controller_test.go

+		return err
+	}
+	for _, mhc := range mhcList.Items {
+		err = c.Delete(ctx, mhc.DeepCopy())


This one is because I needed a pointer and mhc is a range variable, I've declared a second variable in the scope and that works, don't know if that's better or not?

api/v1alpha3/machinehealthcheck_types.go

controllers/machinehealthcheck_controller.go

api/v1alpha3/machinehealthcheck_webhook.go

controllers/machinehealthcheck_controller.go

controllers/machinehealthcheck_controller_test.go

ncdc · 2020-01-30T20:54:19Z

api/v1alpha3/machinehealthcheck_webhook.go

+}
+
+// PopulateDefaultsMachineHealthCheck fills in default field values.
+func PopulateDefaultsMachineHealthCheck(m *MachineHealthCheck) {


Please delete this function and move its contents into Default() above.

ncdc · 2020-01-30T20:54:38Z

api/v1alpha3/machinehealthcheck_webhook.go

+
+// PopulateDefaultsMachineHealthCheck fills in default field values.
+func PopulateDefaultsMachineHealthCheck(m *MachineHealthCheck) {
+	defaultMaxUnhealthy := intstr.FromString("100%")


Do this inside the if check

ncdc · 2020-01-30T20:56:25Z

api/v1alpha3/machinehealthcheck_webhook.go

+func (m *MachineHealthCheck) ValidateUpdate(old runtime.Object) error {
+	mhc, ok := old.(*MachineHealthCheck)
+	if !ok {
+		return fmt.Errorf("expected a MachineHealthCheck but got a %T", old)


This should return apierrors.NewInvalid(...)

ncdc · 2020-01-30T21:05:30Z

controllers/machinehealthcheck_controller.go

+)
+
+const (
+	mhcClusterNameIndex = "mhcClusterNameIndex"


Suggested change

mhcClusterNameIndex = "mhcClusterNameIndex"

mhcClusterNameIndex = "spec.clusterName"

controllers/machinehealthcheck_controller.go

controllers/machinehealthcheck_controller_test.go

ncdc · 2020-01-30T21:12:48Z

controllers/machinehealthcheck_controller_test.go

+		Scheme:             scheme.Scheme,
+		MetricsBindAddress: "0",
+		NewCache: func(config *rest.Config, opts cache.Options) (cache.Cache, error) {
+			syncPeriod := 1 * time.Second


What's the motivation for having a sync period?

Honestly, I copied the manager set up over from suite_test assuming there was some good reason for it 😅 I don't think it will make a difference to these tests and will get rid of it

controllers/machinehealthcheck_controller_test.go

JoelSpeed

@ncdc I've fixed up all of your suggestions :)

JoelSpeed · 2020-01-31T13:19:13Z

controllers/machinehealthcheck_controller_test.go

+		Scheme:             scheme.Scheme,
+		MetricsBindAddress: "0",
+		NewCache: func(config *rest.Config, opts cache.Options) (cache.Cache, error) {
+			syncPeriod := 1 * time.Second


Honestly, I copied the manager set up over from suite_test assuming there was some good reason for it 😅 I don't think it will make a difference to these tests and will get rid of it

ncdc · 2020-01-31T15:17:47Z

api/v1alpha3/machinehealthcheck_webhook.go

+func (m *MachineHealthCheck) ValidateUpdate(old runtime.Object) error {
+	mhc, ok := old.(*MachineHealthCheck)
+	if !ok {
+		return fmt.Errorf("expected a MachineHealthCheck but got a %T", old)


ncdc · 2020-01-31T15:18:12Z

controllers/machinehealthcheck_controller.go

+)
+
+const (
+	mhcClusterNameIndex = "spec.ClusterName"


Suggested change

mhcClusterNameIndex = "spec.ClusterName"

mhcClusterNameIndex = "spec.clusterName"

controllers/machinehealthcheck_controller.go

ncdc · 2020-01-31T16:50:20Z

I think I just have 2 small outstanding items and then I say we merge & iterate in future PRs

JoelSpeed · 2020-01-31T16:55:45Z

@ncdc Apologies, I somehow missed commiting what I had done on the apierrors.NewInvalid(...), I'm not sure I've done it right though, can you advise?

ncdc · 2020-01-31T18:01:44Z

api/v1alpha3/machinehealthcheck_webhook.go

+	mhc, ok := old.(*MachineHealthCheck)
+	if !ok {
+		err := field.Invalid(&field.Path{}, old, fmt.Sprintf("expected a MachineHealthCheck but got a %T", old))
+		return apierrors.NewInvalid(GroupVersion.WithKind("MachineHealthCheck").GroupKind(), "", field.ErrorList{err})


Hmm yeah... maybe we should use NewBadRequest(reason) instead?

That is much cleaner! Fixed

enxebre · 2020-02-03T10:36:31Z

controllers/machinehealthcheck_controller.go

+// clusterToMachineHealthCheck maps events from Cluster objects to
+// MachineHealthCheck objects that belong to the Cluster
+func (r *MachineHealthCheckReconciler) clusterToMachineHealthCheck(o handler.MapObject) []reconcile.Request {
+	m, ok := o.Object.(*clusterv1.Cluster)


nit: replace m with c

controllers/machinehealthcheck_controller.go

ncdc · 2020-02-03T16:43:44Z

LGTM, please squash!!!

JoelSpeed · 2020-02-03T16:46:41Z

Should I do one commit for the types and another for the reconciler or just all in one?

ncdc · 2020-02-03T16:47:48Z

I'm fine with 1 commit

JoelSpeed · 2020-02-03T16:50:05Z

Great, squashed :)

ncdc · 2020-02-03T16:50:31Z

/lgtm

🎉🎉🎉🎉

JoelSpeed · 2020-02-03T17:05:23Z

controllers/machinehealthcheck_controller.go

+func (r *MachineHealthCheckReconciler) clusterToMachineHealthCheck(o handler.MapObject) []reconcile.Request {
+	c, ok := o.Object.(*clusterv1.Cluster)
+	if !ok {
+		r.Log.Error(errors.New("incorrect type"), "expected a MachineHealthCheck", "type", fmt.Sprintf("%T", o))


@ncdc Sorry, I just realised I've made a typo here, will fix-up and re-push

Suggested change

r.Log.Error(errors.New("incorrect type"), "expected a MachineHealthCheck", "type", fmt.Sprintf("%T", o))

r.Log.Error(errors.New("incorrect type"), "expected a Cluster", "type", fmt.Sprintf("%T", o))

ncdc · 2020-02-03T17:20:53Z

/lgtm

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 20, 2020

k8s-ci-robot requested review from chuckha and ncdc January 20, 2020 11:42

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 20, 2020

JoelSpeed mentioned this pull request Jan 22, 2020

✨ Add MachineHealthCheck types #2049

Closed

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from 91ac792 to 730bca2 Compare January 22, 2020 10:11

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2020

detiber reviewed Jan 22, 2020

View reviewed changes

api/v1alpha3/machinehealthcheck_types.go Outdated Show resolved Hide resolved

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from 730bca2 to 57a637c Compare January 24, 2020 10:51

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jan 24, 2020

jzhoucliqr reviewed Jan 27, 2020

View reviewed changes

api/v1alpha3/machinehealthcheck_types.go Outdated Show resolved Hide resolved

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 28, 2020

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from 8f46a38 to 19cac02 Compare January 28, 2020 11:08

ncdc reviewed Jan 28, 2020

View reviewed changes

JoelSpeed commented Jan 28, 2020

View reviewed changes

detiber reviewed Jan 28, 2020

View reviewed changes

ncdc added this to the v0.3.0 milestone Jan 28, 2020

ncdc reviewed Jan 28, 2020

View reviewed changes

controllers/machinehealthcheck_controller.go Show resolved Hide resolved

controllers/machinehealthcheck_controller_test.go Outdated Show resolved Hide resolved

controllers/machinehealthcheck_controller_test.go Show resolved Hide resolved

ncdc mentioned this pull request Jan 28, 2020

⚠️ MachinePool API Controller Implementation #1952

Merged

JoelSpeed force-pushed the machinehealthcheck-reconcile branch 3 times, most recently from a89504f to 93400e3 Compare January 29, 2020 14:22

ncdc reviewed Jan 30, 2020

View reviewed changes

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from 93623f0 to 7e354eb Compare January 31, 2020 13:32

JoelSpeed commented Jan 31, 2020

View reviewed changes

ncdc reviewed Jan 31, 2020

View reviewed changes

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from 7e354eb to 10a7dc1 Compare January 31, 2020 16:54

ncdc reviewed Jan 31, 2020

View reviewed changes

ncdc mentioned this pull request Jan 31, 2020

✨ ec2: Add support for userdata privacy kubernetes-sigs/cluster-api-provider-aws#1490

Merged

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from 10a7dc1 to ac1954c Compare February 3, 2020 10:05

enxebre reviewed Feb 3, 2020

View reviewed changes

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from ac1954c to 1b7bfbb Compare February 3, 2020 11:04

ncdc reviewed Feb 3, 2020

View reviewed changes

controllers/machinehealthcheck_controller.go Outdated Show resolved Hide resolved

controllers/machinehealthcheck_controller.go Outdated Show resolved Hide resolved

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from 1b7bfbb to c5634e9 Compare February 3, 2020 16:41

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from c5634e9 to 5a8da83 Compare February 3, 2020 16:49

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2020

JoelSpeed commented Feb 3, 2020

View reviewed changes

Add MachineHealthCheck Reconciler Skeleton

6ad9dfa

JoelSpeed force-pushed the machinehealthcheck-reconcile branch from 5a8da83 to 6ad9dfa Compare February 3, 2020 17:06

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 3, 2020

k8s-ci-robot merged commit 8fb847f into kubernetes-sigs:master Feb 3, 2020

enxebre mentioned this pull request Feb 5, 2020

REQUEST: New membership for enxebre kubernetes/org#1614

Closed

6 tasks

JoelSpeed deleted the machinehealthcheck-reconcile branch May 5, 2020 16:27

	mhcClusterNameIndex = "mhcClusterNameIndex"
	mhcClusterNameIndex = "spec.clusterName"

	r.Log.Error(errors.New("incorrect type"), "expected a MachineHealthCheck", "type", fmt.Sprintf("%T", o))
	r.Log.Error(errors.New("incorrect type"), "expected a Cluster", "type", fmt.Sprintf("%T", o))

✨ Minimal MachineHealthCheck Reconciler skeleton #2101

✨ Minimal MachineHealthCheck Reconciler skeleton #2101

Conversation

JoelSpeed commented Jan 20, 2020

JoelSpeed commented Jan 20, 2020

vincepri commented Jan 21, 2020

JoelSpeed commented Jan 21, 2020

vincepri commented Jan 21, 2020

JoelSpeed commented Jan 22, 2020

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

JoelSpeed 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

JoelSpeed 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

ncdc commented Jan 31, 2020

JoelSpeed commented Jan 31, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncdc commented Feb 3, 2020

JoelSpeed commented Feb 3, 2020

ncdc commented Feb 3, 2020

JoelSpeed commented Feb 3, 2020

ncdc commented Feb 3, 2020

Choose a reason for hiding this comment

ncdc commented Feb 3, 2020