Repository: Azure/ARO-RP
Pull request #1950 (2022-02-07 20:48:35)

add worker only controller with operator for machine health check
### Which issue this PR addresses:
Work item: 10747624

### What this PR does / why we need it:

Reduce the number of node ready alerts by utilizing the machine health check controller

### Test plan for issue:

manual testing on a dev cluster, unit tests

### Is there any documentation that needs to be updated for this PR?

Docs updated in PR


karanmagdani: @s-amann Thank you so much for working on this, this will fix a huge pain point during oncall. The code looks good to me in general, but when i tried to create a cluster, i ran into some issues with the manifest not being applied correctly. 

Am i missing something?
s-amann: > Am i missing something?
Not that I know of, I will try the same and see if I need to make any additional changes. 

hawkowl: Since this updates a manifest, it should belong on the master -- the manifest will cause things to be deployed on the workers. The controller would only run on the worker if it was doing something that had to be done on a worker (e.g. internet checking), and as a consequence runs with lower permissions than the operator on the master. 
hawkowl: 
> Regarding rolling this out in a few clusters first / having the flag set to false by default, in the past we have rolled out stuff like this everywhere assuming we agree on the design, so I'm not sure if there are other plans.

Yes, we are now deploying different behavior off by default where possible. This will, in future, be paired with things that allow us to update the defaults for new clusters without an RP release. See https://docs.google.com/document/d/1NQrG3LmcdpwSzP_UfShfr9dawganlEKbRbeOhUiewj8/edit for the rationale.

> Unit test coverage is looking good, but I'd add a few extra test cases (maybe "resource is not present, operator should create it", or "resource is present but modified, operator should correct it"), if possible.

I believe that by using `DynamicHelper.Ensure`, these test cases are redundant, since `Ensure` should create or update. A unit test for "not present -> operator creates" is valid though (since it makes sure that the DH stuff is called), the tests in https://github.com/Azure/ARO-RP/pull/1845 may be a good model for this.
s-amann: @hawkowl / @cadenmarchese  I've updated the tests to cover a variety of cases. @karanmagdani there shouldn't be anything special for this operator to work locally. I was able to follow our docs on how to run an operator locally and see the mhc get created/reconciled on a dev cluster.
s-amann: /azp run e2e
azure-pipelines[bot]: <samp>
Azure Pipelines successfully started running 1 pipeline(s).<br>

</samp>
github-actions[bot]: Please rebase pull request.
s-amann: /azp run e2e
github-actions[bot]: Please rebase pull request.

Patch:
diff --git a/Makefile b/Makefile
index 9879be238..5d1e82d73 100644
--- a/Makefile
+++ b/Makefile
@@ -29,7 +29,7 @@ aro: generate
 	go build -tags aro,containers_image_openpgp,codec.safe -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./cmd/aro
 
 runlocal-rp:
-	go run -tags aro -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./cmd/aro rp
+	go run -tags aro,containers_image_openpgp -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./cmd/aro rp
 
 az: pyenv
 	. pyenv/bin/activate && \
@@ -50,7 +50,7 @@ client: generate
 # TODO: hard coding dev-config.yaml is clunky; it is also probably convenient to
 # override COMMIT.
 deploy:
-	go run -tags aro -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./cmd/aro deploy dev-config.yaml ${LOCATION}
+	go run -tags aro,containers_image_openpgp -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./cmd/aro deploy dev-config.yaml ${LOCATION}
 
 dev-config.yaml:
 	go run ./hack/gendevconfig >dev-config.yaml
@@ -109,7 +109,7 @@ proxy:
 	go build -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./hack/proxy
 
 run-portal:
-	go run -tags aro -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./cmd/aro portal
+	go run -tags aro,containers_image_openpgp -ldflags "-X github.com/Azure/ARO-RP/pkg/util/version.GitCommit=$(VERSION)" ./cmd/aro portal
 
 build-portal:
 	cd portal && npm install && npm run build
@@ -132,7 +132,7 @@ secrets:
 secrets-update:
 	@[ "${SECRET_SA_ACCOUNT_NAME}" ] || ( echo ">> SECRET_SA_ACCOUNT_NAME is not set"; exit 1 )
 	tar -czf secrets.tar.gz secrets
-	az storage blob upload -n secrets.tar.gz -c secrets -f secrets.tar.gz --account-name ${SECRET_SA_ACCOUNT_NAME} >/dev/null
+	az storage blob upload -n secrets.tar.gz -c secrets -f secrets.tar.gz --overwrite --account-name ${SECRET_SA_ACCOUNT_NAME} >/dev/null
 	rm secrets.tar.gz
 
 tunnel:
@@ -161,7 +161,7 @@ validate-fips:
 	hack/fips/validate-fips.sh
 
 unit-test-go:
-	go run ./vendor/gotest.tools/gotestsum/main.go --format pkgname --junitfile report.xml -- -tags=aro -coverprofile=cover.out ./...
+	go run ./vendor/gotest.tools/gotestsum/main.go --format pkgname --junitfile report.xml -- -tags=aro,containers_image_openpgp -coverprofile=cover.out ./...
 
 lint-go:
 	go run ./vendor/github.com/golangci/golangci-lint/cmd/golangci-lint run
diff --git a/README.md b/README.md
index 21e3ed8c4..c48997f22 100644
--- a/README.md
+++ b/README.md
@@ -116,6 +116,13 @@ questions or comments.
 
     * machineset: Ensures that a minimum of two worker replicas are met.
 
+    * machinehealthcheck: Ensures the MachineHealthCheck resource is running as configured so that at most one worker node at a time is automatically
+      reconciled when not ready for at least 5 minutes.
+        * The CR will only be applied when both `aro.machinehealthcheck.managed` and `aro.machinehealthcheck.enabled` are set to `"true"`.
+        * When `aro.machinehealthcheck.enabled` is `"false"` and `aro.machinehealthcheck.managed` is `"false"` the CR will be removed from the cluster.
+        * If `aro.machinehealthcheck.enabled` is `"false"` no actions will be taken to modify the CR.
+        * More information around the MHC CR can be found [in openshift documentation of MHC](https://docs.openshift.com/container-platform/4.9/machine_management/deploying-machine-health-checks.html)
+
     * monitoring: Ensures that the OpenShift monitoring configuration in the `openshift-monitoring` namespace is consistent and immutable.
 
     * node: Force deletes pods when a node fails to drain for 1 hour.  It should clear up any pods that refuse to be evicted on a drain due to violating a pod disruption budget.
diff --git a/cmd/aro/operator.go b/cmd/aro/operator.go
index 49701e892..31cad9598 100644
--- a/cmd/aro/operator.go
+++ b/cmd/aro/operator.go
@@ -31,6 +31,7 @@ import (
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/genevalogging"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/imageconfig"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/machine"
+	"github.com/Azure/ARO-RP/pkg/operator/controllers/machinehealthcheck"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/machineset"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/monitoring"
 	"github.com/Azure/ARO-RP/pkg/operator/controllers/muo"
@@ -216,6 +217,10 @@ func operator(ctx context.Context, log *logrus.Entry) error {
 			mgr)).SetupWithManager(mgr); err != nil {
 			return fmt.Errorf("unable to create controller %s: %v", autosizednodes.ControllerName, err)
 		}
+		if err = (machinehealthcheck.NewReconciler(
+			arocli, dh)).SetupWithManager(mgr); err != nil {
+			return fmt.Errorf("unable to create controller %s: %v", machinehealthcheck.ControllerName, err)
+		}
 	}
 
 	if err = (checker.NewReconciler(
diff --git a/pkg/api/defaults.go b/pkg/api/defaults.go
index 7adae0932..d82ccac55 100644
--- a/pkg/api/defaults.go
+++ b/pkg/api/defaults.go
@@ -68,6 +68,8 @@ func DefaultOperatorFlags() OperatorFlags {
 		"aro.imageconfig.enabled":                  flagTrue,
 		"aro.machine.enabled":                      flagTrue,
 		"aro.machineset.enabled":                   flagTrue,
+		"aro.machinehealthcheck.enabled":           flagFalse,
+		"aro.machinehealthcheck.managed":           flagFalse,
 		"aro.monitoring.enabled":                   flagTrue,
 		"aro.nodedrainer.enabled":                  flagTrue,
 		"aro.pullsecret.enabled":                   flagTrue,
diff --git a/pkg/operator/controllers/machinehealthcheck/bindata.go b/pkg/operator/controllers/machinehealthcheck/bindata.go
new file mode 100644
index 000000000..3925344ce
--- /dev/null
+++ b/pkg/operator/controllers/machinehealthcheck/bindata.go
@@ -0,0 +1,244 @@
+// Code generated for package machinehealthcheck by go-bindata DO NOT EDIT. (@generated)
+// sources:
+// staticresources/machinehealthcheck.yaml
+package machinehealthcheck
+
+import (
+	"bytes"
+	"compress/gzip"
+	"fmt"
+	"io"
+	"io/ioutil"
+	"os"
+	"path/filepath"
+	"strings"
+	"time"
+)
+
+func bindataRead(data []byte, name string) ([]byte, error) {
+	gz, err := gzip.NewReader(bytes.NewBuffer(data))
+	if err != nil {
+		return nil, fmt.Errorf("Read %q: %v", name, err)
+	}
+
+	var buf bytes.Buffer
+	_, err = io.Copy(&buf, gz)
+	clErr := gz.Close()
+
+	if err != nil {
+		return nil, fmt.Errorf("Read %q: %v", name, err)
+	}
+	if clErr != nil {
+		return nil, err
+	}
+
+	return buf.Bytes(), nil
+}
+
+type asset struct {
+	bytes []byte
+	info  os.FileInfo
+}
+
+type bindataFileInfo struct {
+	name    string
+	size    int64
+	mode    os.FileMode
+	modTime time.Time
+}
+
+// Name return file name
+func (fi bindataFileInfo) Name() string {
+	return fi.name
+}
+
+// Size return file size
+func (fi bindataFileInfo) Size() int64 {
+	return fi.size
+}
+
+// Mode return file mode
+func (fi bindataFileInfo) Mode() os.FileMode {
+	return fi.mode
+}
+
+// Mode return file modify time
+func (fi bindataFileInfo) ModTime() time.Time {
+	return fi.modTime
+}
+
+// IsDir return file whether a directory
+func (fi bindataFileInfo) IsDir() bool {
+	return fi.mode&os.ModeDir != 0
+}
+
+// Sys return file is sys mode
+func (fi bindataFileInfo) Sys() interface{} {
+	return nil
+}
+
+var _machinehealthcheckYaml = []byte("\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\xff\x9c\x90\xb1\x6e\xf3\x30\x0c\x84\x77\x3f\x05\xa1\xdd\xf9\x13\xfc\x9b\xd6\x20\x45\x3b\xb4\x43\x81\x74\x67\x65\x06\x12\x2c\x51\x82\x48\xa7\xf1\xdb\x17\xb2\x93\x74\x28\xba\x64\xb3\x4f\x77\xdf\x9d\x84\x25\x7c\x50\x95\x90\xd9\x42\x42\xe7\x03\xd3\x26\x17\x62\xf1\xe1\xa4\x9b\x90\xff\x9d\x77\x9f\xa4\xb8\xeb\xc6\xc0\x83\x85\xd7\xd5\xf2\x4c\x18\xd5\xef\x3d\xb9\xb1\x4b\xa4\x38\xa0\xa2\xed\x00\x18\x13\x59\xc0\x9a\xfb\x2b\xcb\x2f\x46\xb7\x18\xd7\x63\x29\xe8\xc8\xc2\xbd\xe3\xe6\xec\xb1\x84\x4e\x0a\xb9\xc6\x11\x8a\xe4\x34\xd7\xf6\x0d\x90\x50\x9d\x3f\x5c\x4a\x25\x69\x43\x65\x55\x7b\x18\x69\xfe\x63\xb4\x8b\x93\x28\xd5\xc6\xbc\xf3\x6b\x8e\xb4\x04\xa1\x95\x57\x6c\x78\x78\xcb\xfa\xc2\x57\xf5\x8c\x71\xa2\x2b\xbc\xe1\x4d\xe0\x53\x45\xf3\xf3\x9f\xb0\x41\xcd\x23\xed\x42\xfa\xab\xfb\x70\x09\xa2\xd2\x01\x4c\xbc\x3e\xd3\xbc\xcf\x3c\x04\xbd\x5d\xb1\x07\x9d\x0b\xd9\x16\x32\xef\x84\xc3\xbc\x36\x6b\x48\x94\x27\xb5\x60\xfe\x6f\xb7\x62\x60\x11\x45\x51\x27\xb1\x60\x9e\x30\x0a\x99\x47\xd3\x47\x1e\x39\x7f\x71\xb3\x26\xbc\x1c\x6f\xbb\x2c\x98\x9d\xf9\x0e\x00\x00\xff\xff\x9f\x0e\x45\xac\x2a\x02\x00\x00")
+
+func machinehealthcheckYamlBytes() ([]byte, error) {
+	return bindataRead(
+		_machinehealthcheckYaml,
+		"machinehealthcheck.yaml",
+	)
+}
+
+func machinehealthcheckYaml() (*asset, error) {
+	bytes, err := machinehealthcheckYamlBytes()
+	if err != nil {
+		return nil, err
+	}
+
+	info := bindataFileInfo{name: "machinehealthcheck.yaml", size: 0, mode: os.FileMode(0), modTime: time.Unix(0, 0)}
+	a := &asset{bytes: bytes, info: info}
+	return a, nil
+}
+
+// Asset loads and returns the asset for the given name.
+// It returns an error if the asset could not be found or
+// could not be loaded.
+func Asset(name string) ([]byte, error) {
+	cannonicalName := strings.Replace(name, "\\", "/", -1)
+	if f, ok := _bindata[cannonicalName]; ok {
+		a, err := f()
+		if err != nil {
+			return nil, fmt.Errorf("Asset %s can't read by error: %v", name, err)
+		}
+		return a.bytes, nil
+	}
+	return nil, fmt.Errorf("Asset %s not found", name)
+}
+
+// MustAsset is like Asset but panics when Asset would return an error.
+// It simplifies safe initialization of global variables.
+func MustAsset(name string) []byte {
+	a, err := Asset(name)
+	if err != nil {
+		panic("asset: Asset(" + name + "): " + err.Error())
+	}
+
+	return a
+}
+
+// AssetInfo loads and returns the asset info for the given name.
+// It returns an error if the asset could not be found or
+// could not be loaded.
+func AssetInfo(name string) (os.FileInfo, error) {
+	cannonicalName := strings.Replace(name, "\\", "/", -1)
+	if f, ok := _bindata[cannonicalName]; ok {
+		a, err := f()
+		if err != nil {
+			return nil, fmt.Errorf("AssetInfo %s can't read by error: %v", name, err)
+		}
+		return a.info, nil
+	}
+	return nil, fmt.Errorf("AssetInfo %s not found", name)
+}
+
+// AssetNames returns the names of the assets.
+func AssetNames() []string {
+	names := make([]string, 0, len(_bindata))
+	for name := range _bindata {
+		names = append(names, name)
+	}
+	return names
+}
+
+// _bindata is a table, holding each asset generator, mapped to its name.
+var _bindata = map[string]func() (*asset, error){
+	"machinehealthcheck.yaml": machinehealthcheckYaml,
+}
+
+// AssetDir returns the file names below a certain
+// directory embedded in the file by go-bindata.
+// For example if you run go-bindata on data/... and data contains the
+// following hierarchy:
+//     data/
+//       foo.txt
+//       img/
+//         a.png
+//         b.png
+// then AssetDir("data") would return []string{"foo.txt", "img"}
+// AssetDir("data/img") would return []string{"a.png", "b.png"}
+// AssetDir("foo.txt") and AssetDir("notexist") would return an error
+// AssetDir("") will return []string{"data"}.
+func AssetDir(name string) ([]string, error) {
+	node := _bintree
+	if len(name) != 0 {
+		cannonicalName := strings.Replace(name, "\\", "/", -1)
+		pathList := strings.Split(cannonicalName, "/")
+		for _, p := range pathList {
+			node = node.Children[p]
+			if node == nil {
+				return nil, fmt.Errorf("Asset %s not found", name)
+			}
+		}
+	}
+	if node.Func != nil {
+		return nil, fmt.Errorf("Asset %s not found", name)
+	}
+	rv := make([]string, 0, len(node.Children))
+	for childName := range node.Children {
+		rv = append(rv, childName)
+	}
+	return rv, nil
+}
+
+type bintree struct {
+	Func     func() (*asset, error)
+	Children map[string]*bintree
+}
+
+var _bintree = &bintree{nil, map[string]*bintree{
+	"machinehealthcheck.yaml": {machinehealthcheckYaml, map[string]*bintree{}},
+}}
+
+// RestoreAsset restores an asset under the given directory
+func RestoreAsset(dir, name string) error {
+	data, err := Asset(name)
+	if err != nil {
+		return err
+	}
+	info, err := AssetInfo(name)
+	if err != nil {
+		return err
+	}
+	err = os.MkdirAll(_filePath(dir, filepath.Dir(name)), os.FileMode(0755))
+	if err != nil {
+		return err
+	}
+	err = ioutil.WriteFile(_filePath(dir, name), data, info.Mode())
+	if err != nil {
+		return err
+	}
+	err = os.Chtimes(_filePath(dir, name), info.ModTime(), info.ModTime())
+	if err != nil {
+		return err
+	}
+	return nil
+}
+
+// RestoreAssets restores an asset under the given directory recursively
+func RestoreAssets(dir, name string) error {
+	children, err := AssetDir(name)
+	// File
+	if err != nil {
+		return RestoreAsset(dir, name)
+	}
+	// Dir
+	for _, child := range children {
+		err = RestoreAssets(dir, filepath.Join(name, child))
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+func _filePath(dir, name string) string {
+	cannonicalName := strings.Replace(name, "\\", "/", -1)
+	return filepath.Join(append([]string{dir}, strings.Split(cannonicalName, "/")...)...)
+}
diff --git a/pkg/operator/controllers/machinehealthcheck/doc.go b/pkg/operator/controllers/machinehealthcheck/doc.go
new file mode 100644
index 000000000..8af406d71
--- /dev/null
+++ b/pkg/operator/controllers/machinehealthcheck/doc.go
@@ -0,0 +1,27 @@
+package machinehealthcheck
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+/*
+
+The controller in this package aims to ensure that MachineHealthCheck objects
+exist and are correctly configured to automatically mitigate non-ready worker nodes.
+
+There are two flags which control the operations performed by the controller:
+
+aro.machinehealthcheck.enabled:
+- When set to false, the controller will noop and not perform any further action
+- When set to true, the controller continues on to check the managed flag
+
+aro.machinehealthcheck.managed
+- When set to false, the controller will attempt to remove the aro-machinehealthcheck CR from the cluster.
+  This should effectively disable the MHC we deploy and prevent the automatic reconciliation of nodes.
+- When set to true, the controller will deploy/overwrite the aro-machinehealthcheck CR to the cluster.
+  This enables the cluster to self heal when at most 1 worker node goes not ready for at least 5 minutes.
+
+The aro-machinehealth check is configured in a way that if 2 worker nodes go not ready it will not take any action.
+More information about how the MHC works can be found here:
+https://docs.openshift.com/container-platform/4.9/machine_management/deploying-machine-health-checks.html
+
+*/
diff --git a/pkg/operator/controllers/machinehealthcheck/generate.go b/pkg/operator/controllers/machinehealthcheck/generate.go
new file mode 100644
index 000000000..9fd716667
--- /dev/null
+++ b/pkg/operator/controllers/machinehealthcheck/generate.go
@@ -0,0 +1,7 @@
+package machinehealthcheck
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+//go:generate go run ../../../../vendor/github.com/go-bindata/go-bindata/go-bindata -nometadata -pkg $GOPACKAGE -prefix staticresources staticresources/...
+//go:generate gofmt -s -l -w bindata.go
diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
new file mode 100644
index 000000000..57bae9f59
--- /dev/null
+++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller.go
@@ -0,0 +1,102 @@
+package machinehealthcheck
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"time"
+
+	machinev1beta1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kruntime "k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/scheme"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
+	"github.com/Azure/ARO-RP/pkg/util/dynamichelper"
+)
+
+const (
+	ControllerName string = "MachineHealthCheck"
+	managed        string = "aro.machinehealthcheck.managed"
+	enabled        string = "aro.machinehealthcheck.enabled"
+)
+
+type Reconciler struct {
+	arocli aroclient.Interface
+	dh     dynamichelper.Interface
+}
+
+func NewReconciler(arocli aroclient.Interface, dh dynamichelper.Interface) *Reconciler {
+	return &Reconciler{
+		arocli: arocli,
+		dh:     dh,
+	}
+}
+
+func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.Result, error) {
+	instance, err := r.arocli.AroV1alpha1().Clusters().Get(ctx, arov1alpha1.SingletonClusterName, metav1.GetOptions{})
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	if !instance.Spec.OperatorFlags.GetSimpleBoolean(enabled) {
+		return reconcile.Result{}, nil
+	}
+
+	if !instance.Spec.OperatorFlags.GetSimpleBoolean(managed) {
+		err := r.dh.EnsureDeleted(ctx, "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck")
+		if err != nil {
+			return reconcile.Result{RequeueAfter: time.Hour}, err
+		}
+		return reconcile.Result{}, nil
+	}
+
+	var resources []kruntime.Object
+	// this loop prevents us from hard coding resource strings
+	// and ensures all static resources are accounted for.
+	for _, assetName := range AssetNames() {
+		b, err := Asset(assetName)
+		if err != nil {
+			return reconcile.Result{}, err
+		}
+
+		resource, _, err := scheme.Codecs.UniversalDeserializer().Decode(b, nil, nil)
+		if err != nil {
+			return reconcile.Result{}, err
+		}
+
+		resources = append(resources, resource)
+	}
+
+	// helps with garbage collection of the resources we are dealing with
+	err = dynamichelper.SetControllerReferences(resources, instance)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	// make sure we will be able to deploy a new resource into the cluster
+	err = dynamichelper.Prepare(resources)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	// create/update the MHC CR
+	err = r.dh.Ensure(ctx, resources...)
+	if err != nil {
+		return reconcile.Result{}, err
+	}
+
+	return reconcile.Result{}, nil
+}
+
+// SetupWithManager will manage only our MHC resource with our specific controller name
+func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
+	return ctrl.NewControllerManagedBy(mgr).
+		For(&machinev1beta1.MachineHealthCheck{}).
+		Named(ControllerName).
+		Complete(r)
+}
diff --git a/pkg/operator/controllers/machinehealthcheck/staticresources/machinehealthcheck.yaml b/pkg/operator/controllers/machinehealthcheck/staticresources/machinehealthcheck.yaml
new file mode 100644
index 000000000..9f8f8fc13
--- /dev/null
+++ b/pkg/operator/controllers/machinehealthcheck/staticresources/machinehealthcheck.yaml
@@ -0,0 +1,23 @@
+apiVersion: machine.openshift.io/v1beta1
+kind: MachineHealthCheck
+metadata:
+  name: aro-machinehealthcheck
+  namespace: openshift-machine-api
+spec:
+  selector:
+    matchExpressions:
+    - key: machine.openshift.io/cluster-api-machine-role
+      operator: NotIn
+      values:
+      - "infra"
+      - "master"
+    - key: machine.openshift.io/cluster-api-machineset
+      operator: Exists
+  unhealthyConditions:
+  - type:    "Ready"
+    timeout: "300s" 
+    status: "False"
+  - type:    "Ready"
+    timeout: "300s" 
+    status: "Unknown"
+  maxUnhealthy: "1"
\ No newline at end of file
diff --git a/pkg/proxy/proxy.go b/pkg/proxy/proxy.go
index dd9c7bef3..0730f24c7 100644
--- a/pkg/proxy/proxy.go
+++ b/pkg/proxy/proxy.go
@@ -10,6 +10,7 @@ import (
 	"io/ioutil"
 	"net"
 	"net/http"
+	"sync"
 
 	"github.com/sirupsen/logrus"
 
@@ -144,27 +145,27 @@ func Proxy(log *logrus.Entry, w http.ResponseWriter, r *http.Request, sz int) {
 	}
 
 	defer c1.Close()
-	ch := make(chan struct{})
+	var wg sync.WaitGroup
 
+	// Wait for the c1->c2 goroutine to complete before exiting.
+	//Then the deferred c1.Close() and c2.Close() will be called.
+	defer wg.Wait()
+
+	wg.Add(1)
 	go func() {
 		// use a goroutine to copy from c1->c2.  Call c2.CloseWrite() when done.
 		defer recover.Panic(log)
-		defer close(ch)
+		defer wg.Done()
 		defer func() {
 			_ = c2.(*net.TCPConn).CloseWrite()
 		}()
 		_, _ = io.Copy(c2, buf)
 	}()
 
-	func() {
-		// copy from c2->c1.  Call c1.CloseWrite() when done.
-		defer func() {
-			_ = c1.(interface{ CloseWrite() error }).CloseWrite()
-		}()
-		_, _ = io.Copy(c1, c2)
+	// copy from c2->c1.  Call c1.CloseWrite() when done.
+	defer func() {
+		_ = c1.(interface{ CloseWrite() error }).CloseWrite()
 	}()
+	_, _ = io.Copy(c1, c2)
 
-	// wait for the c1->c2 goroutine to complete.  Then the deferred c1.Close()
-	// and c2.Close() will be called.
-	<-ch
 }


Test patch:
diff --git a/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go
new file mode 100644
index 000000000..92146e784
--- /dev/null
+++ b/pkg/operator/controllers/machinehealthcheck/machinehealthcheck_controller_test.go
@@ -0,0 +1,164 @@
+package machinehealthcheck
+
+// Copyright (c) Microsoft Corporation.
+// Licensed under the Apache License 2.0.
+
+import (
+	"context"
+	"errors"
+	"strconv"
+	"testing"
+	"time"
+
+	"github.com/golang/mock/gomock"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	ctrl "sigs.k8s.io/controller-runtime"
+
+	arov1alpha1 "github.com/Azure/ARO-RP/pkg/operator/apis/aro.openshift.io/v1alpha1"
+	aroclient "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned"
+	arofake "github.com/Azure/ARO-RP/pkg/operator/clientset/versioned/fake"
+	mock_dynamichelper "github.com/Azure/ARO-RP/pkg/util/mocks/dynamichelper"
+)
+
+// Test reconcile function
+func TestReconciler(t *testing.T) {
+	type test struct {
+		name             string
+		arocli           aroclient.Interface
+		mocks            func(mdh *mock_dynamichelper.MockInterface)
+		wantErr          string
+		wantRequeueAfter time.Duration
+	}
+
+	for _, tt := range []*test{
+		{
+			name: "Failure to get instance",
+			arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "some other name",
+				},
+			}),
+			mocks:   func(mdh *mock_dynamichelper.MockInterface) {},
+			wantErr: `clusters.aro.openshift.io "cluster" not found`,
+		},
+		{
+			name: "Enabled Feature Flag is false",
+			arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					OperatorFlags: arov1alpha1.OperatorFlags{
+						enabled: strconv.FormatBool(false),
+					},
+				},
+			}),
+			mocks: func(mdh *mock_dynamichelper.MockInterface) {
+				mdh.EXPECT().EnsureDeleted(gomock.Any(), "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck").Times(0)
+				mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(nil).Times(0)
+			},
+			wantErr: "",
+		},
+		{
+			name: "Managed Feature Flag is false: ensure mhc is deleted",
+			arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					OperatorFlags: arov1alpha1.OperatorFlags{
+						enabled: strconv.FormatBool(true),
+						managed: strconv.FormatBool(false),
+					},
+				},
+			}),
+			mocks: func(mdh *mock_dynamichelper.MockInterface) {
+				mdh.EXPECT().EnsureDeleted(gomock.Any(), "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck").Times(1)
+			},
+			wantErr: "",
+		},
+		{
+			name: "Managed Feature Flag is false: mhc fails to delete, an error is returned",
+			arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					OperatorFlags: arov1alpha1.OperatorFlags{
+						enabled: strconv.FormatBool(true),
+						managed: strconv.FormatBool(false),
+					},
+				},
+			}),
+			mocks: func(mdh *mock_dynamichelper.MockInterface) {
+				mdh.EXPECT().EnsureDeleted(gomock.Any(), "MachineHealthCheck", "openshift-machine-api", "aro-machinehealthcheck").Return(errors.New("Could not delete mhc"))
+			},
+			wantErr:          "Could not delete mhc",
+			wantRequeueAfter: time.Hour,
+		},
+		{
+			name: "Managed Feature Flag is true: dynamic helper ensures resources",
+			arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					OperatorFlags: arov1alpha1.OperatorFlags{
+						enabled: strconv.FormatBool(true),
+						managed: strconv.FormatBool(true),
+					},
+				},
+			}),
+			mocks: func(mdh *mock_dynamichelper.MockInterface) {
+				mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(nil).Times(1)
+			},
+			wantErr: "",
+		},
+		{
+			name: "When ensuring resources fails, an error is returned",
+			arocli: arofake.NewSimpleClientset(&arov1alpha1.Cluster{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: arov1alpha1.SingletonClusterName,
+				},
+				Spec: arov1alpha1.ClusterSpec{
+					OperatorFlags: arov1alpha1.OperatorFlags{
+						enabled: strconv.FormatBool(true),
+						managed: strconv.FormatBool(true),
+					},
+				},
+			}),
+			mocks: func(mdh *mock_dynamichelper.MockInterface) {
+				mdh.EXPECT().Ensure(gomock.Any(), gomock.Any()).Return(errors.New("failed to ensure"))
+			},
+			wantErr: "failed to ensure",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			controller := gomock.NewController(t)
+			defer controller.Finish()
+
+			mdh := mock_dynamichelper.NewMockInterface(controller)
+
+			tt.mocks(mdh)
+
+			ctx := context.Background()
+			r := &Reconciler{
+				arocli: tt.arocli,
+				dh:     mdh,
+			}
+			request := ctrl.Request{}
+			request.Name = "cluster"
+
+			result, err := r.Reconcile(ctx, request)
+
+			if tt.wantRequeueAfter != result.RequeueAfter {
+				t.Errorf("Wanted to requeue after %v but was set to %v", tt.wantRequeueAfter, result.RequeueAfter)
+			}
+
+			if err != nil && err.Error() != tt.wantErr ||
+				err == nil && tt.wantErr != "" {
+				t.Error(err)
+			}
+		})
+	}
+}