Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cmd/machine-api-operator/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ func runStartCmd(cmd *cobra.Command, args []string) {
}

ctrlCtx.KubeNamespacedInformerFactory.Start(ctrlCtx.Stop)
ctrlCtx.ConfigInformerFactory.Start(ctrlCtx.Stop)
close(ctrlCtx.InformersStarted)

select {}
Expand Down
49 changes: 49 additions & 0 deletions config/0000_05_config-operator_01_featuregate.crd.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
name: featuregates.config.openshift.io
spec:
group: config.openshift.io
version: v1
scope: Cluster
names:
kind: FeatureGate
singular: featuregate
plural: featuregates
listKind: FeatureGateList
versions:
- name: v1
served: true
storage: true
validation:
openAPIV3Schema:
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#resources'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/value, and/value and

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copy-pasted the CRD, cant' fix that.

type: string
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/api-conventions.md#types-kinds'
type: string
metadata:
description: Standard object's metadata.
type: object
spec:
description: spec holds user settable values for configuration
properties:
featureSet:
description: featureSet changes the list of features in the cluster. The
default is empty. Be very careful adjusting this setting. Turning
on or off features may cause irreversible changes in your cluster
which cannot be undone.
type: string
type: object
status:
description: status holds observed values from the cluster. They may not
be overridden.
type: object
required:
- spec
10 changes: 10 additions & 0 deletions config/machine-api-operator-patch.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,16 @@ rules:
verbs:
- get

- apiGroups:
- config.openshift.io
resources:
- featuregates
- featuregates/status
verbs:
- get
- list
- watch

- apiGroups:
- metalkube.org
resources:
Expand Down
10 changes: 10 additions & 0 deletions install/0000_30_machine-api-operator_08_rbac.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,16 @@ rules:
verbs:
- get

- apiGroups:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in #295

- config.openshift.io
resources:
- featuregates
- featuregates/status
verbs:
- get
- list
- watch

- apiGroups:
- metalkube.org
resources:
Expand Down
3 changes: 3 additions & 0 deletions kustomization.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ resources:
# Pulled from https://github.com/openshift/cluster-version-operator/blob/master/install/0000_00_cluster-version-operator_01_clusteroperator.crd.yaml
# and updated (delete depricated clusteroperators.operatorstatus.openshift.io CRD)
- config/0000_00_cluster-version-operator_01_clusteroperator.crd.yaml
# Install feature gate CRD reguired by mao (so it can enable/disable features)
# Pulled from https://raw.githubusercontent.com/openshift/cluster-config-operator/master/manifests/0000_05_config-operator_01_featuregate.crd.yaml
- config/0000_05_config-operator_01_featuregate.crd.yaml
# Install mao namespaces, CRDS and other resources to properly deploy machine API stack
- install/0000_30_machine-api-operator_00_namespace.yaml
- install/0000_30_machine-api-operator_01_images.configmap.yaml
Expand Down
16 changes: 8 additions & 8 deletions pkg/operator/operator.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ type Operator struct {
deployLister appslisterv1.DeploymentLister
deployListerSynced cache.InformerSynced

featureGateLister configlistersv1.FeatureGateLister
featureGateCacheSync cache.InformerSynced
featureGateLister configlistersv1.FeatureGateLister
featureGateCacheSynced cache.InformerSynced

// queue only ever has one item, but it has nice error handling backoff/retry semantics
queue workqueue.RateLimitingInterface
Expand Down Expand Up @@ -108,7 +108,7 @@ func New(
optr.deployListerSynced = deployInformer.Informer().HasSynced

optr.featureGateLister = featureGateInformer.Lister()
optr.featureGateCacheSync = featureGateInformer.Informer().HasSynced
optr.featureGateCacheSynced = featureGateInformer.Informer().HasSynced

return optr
}
Expand All @@ -122,7 +122,8 @@ func (optr *Operator) Run(workers int, stopCh <-chan struct{}) {
defer glog.Info("Shutting down Machine API Operator")

if !cache.WaitForCacheSync(stopCh,
optr.deployListerSynced) {
optr.deployListerSynced,
optr.featureGateCacheSynced) {
glog.Error("Failed to sync caches")
return
}
Expand Down Expand Up @@ -223,10 +224,9 @@ func (optr *Operator) maoConfigFromInfrastructure() (*OperatorConfig, error) {
return &OperatorConfig{
TargetNamespace: optr.namespace,
Controllers: Controllers{
Provider: providerControllerImage,
NodeLink: machineAPIOperatorImage,
MachineHealthCheck: machineAPIOperatorImage,
MachineHealthCheckEnabled: true,
Provider: providerControllerImage,
NodeLink: machineAPIOperatorImage,
MachineHealthCheck: machineAPIOperatorImage,
},
}, nil
}
106 changes: 42 additions & 64 deletions pkg/operator/operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -181,21 +181,34 @@ func deploymentHasContainer(d *appsv1.Deployment, containerName string) bool {
return false
}

func TestOperatorSyncClusterAPIControllerHealthCheckControllerNotDeployed(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put the tests under two separete functions so it's obvious which case is tested. Though, if you feel it's better this way, I am fine with that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

table driven tests is the recommended approach i guess, https://github.com/golang/go/wiki/TableDrivenTests

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find that it depends. Quite often table driven tests end up adding complexity and sometimes I find it easier to do:

func TestFooDoesX

func TestFooErrorsWithBar

func TestFooSucceedsWithKnownInputs

fg := &v1.FeatureGate{
ObjectMeta: metav1.ObjectMeta{
Name: MachineAPIFeatureGateName,
func TestOperatorSyncClusterAPIControllerHealthCheckController(t *testing.T) {
tests := []struct {
featureGate *v1.FeatureGate
expectedMachineHealthCheckController bool
}{{
featureGate: &v1.FeatureGate{
ObjectMeta: metav1.ObjectMeta{
Name: MachineAPIFeatureGateName,
},
Spec: v1.FeatureGateSpec{
FeatureSet: configv1.Default,
},
},
Spec: v1.FeatureGateSpec{
FeatureSet: configv1.Default,
expectedMachineHealthCheckController: false,
}, {
featureGate: &v1.FeatureGate{},
expectedMachineHealthCheckController: false,
}, {
featureGate: &v1.FeatureGate{
ObjectMeta: metav1.ObjectMeta{
Name: MachineAPIFeatureGateName,
},
Spec: v1.FeatureGateSpec{
FeatureSet: configv1.TechPreviewNoUpgrade,
},
},
}

kubeclientSet := fakekube.NewSimpleClientset()
op, err := initOperator(fg, kubeclientSet)
if err != nil {
t.Errorf("Unable to init operator: %v", err)
}
expectedMachineHealthCheckController: true,
}}

oc := OperatorConfig{
TargetNamespace: targetNamespace,
Expand All @@ -206,59 +219,24 @@ func TestOperatorSyncClusterAPIControllerHealthCheckControllerNotDeployed(t *tes
},
}

if err := op.syncClusterAPIController(oc); err != nil {
t.Errorf("Failed to sync machine API controller: %v", err)
}

d, err := kubeclientSet.AppsV1().Deployments(targetNamespace).Get(deploymentName, metav1.GetOptions{})
if err != nil {
t.Errorf("Failed to get %q deployment: %v", deploymentName, err)
}

if deploymentHasContainer(d, hcControllerName) {
t.Errorf("Did not expect to find %q container", hcControllerName)
} else {
t.Logf("%q container not found as expected", hcControllerName)
}
}

func TestOperatorSyncClusterAPIControllerHealthCheckControllerDeployed(t *testing.T) {
fg := &v1.FeatureGate{
ObjectMeta: metav1.ObjectMeta{
Name: MachineAPIFeatureGateName,
},
Spec: v1.FeatureGateSpec{
FeatureSet: configv1.TechPreviewNoUpgrade,
},
}

kubeclientSet := fakekube.NewSimpleClientset()
op, err := initOperator(fg, kubeclientSet)
if err != nil {
t.Errorf("Unable to init operator: %v", err)
}

oc := OperatorConfig{
TargetNamespace: targetNamespace,
Controllers: Controllers{
Provider: "controllers-provider",
NodeLink: "controllers-nodelink",
MachineHealthCheck: "controllers-machinehealthcheck",
},
}
for _, tc := range tests {
kubeclientSet := fakekube.NewSimpleClientset()
op, err := initOperator(tc.featureGate, kubeclientSet)
if err != nil {
t.Fatalf("Unable to init operator: %v", err)
}

if err := op.syncClusterAPIController(oc); err != nil {
t.Errorf("Failed to sync machine API controller: %v", err)
}
if err := op.syncClusterAPIController(oc); err != nil {
t.Errorf("Failed to sync machine API controller: %v", err)
}

d, err := op.deployLister.Deployments(targetNamespace).Get(deploymentName)
if err != nil {
t.Errorf("Failed to get %q deployment: %v", deploymentName, err)
}
d, err := op.deployLister.Deployments(targetNamespace).Get(deploymentName)
if err != nil {
t.Errorf("Failed to get %q deployment: %v", deploymentName, err)
}

if deploymentHasContainer(d, hcControllerName) {
t.Logf("%q container found as expected", hcControllerName)
} else {
t.Errorf("Failed to find find %q container", hcControllerName)
if deploymentHasContainer(d, hcControllerName) != tc.expectedMachineHealthCheckController {
t.Errorf("Expected deploymentHasContainer for %q container to be %t", hcControllerName, tc.expectedMachineHealthCheckController)
}
}
}
3 changes: 2 additions & 1 deletion pkg/operator/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ func (optr *Operator) syncClusterAPIController(config OperatorConfig) error {
if !errors.IsNotFound(err) {
return err
}
glog.V(2).Infof("failed to find feature gate %s, will use default feature set", MachineAPIFeatureGateName)
glog.V(2).Infof("Failed to find feature gate %q, will use default feature set", MachineAPIFeatureGateName)
featureSet = osev1.Default
} else {
featureSet = featureGate.Spec.FeatureSet
Expand All @@ -70,6 +70,7 @@ func (optr *Operator) syncClusterAPIController(config OperatorConfig) error {

// add machine-health-check controller container if it exists and enabled under feature gates
if enabled, ok := features[FeatureGateMachineHealthCheck]; ok && enabled {
glog.V(2).Infof("Feature %q is enabled", FeatureGateMachineHealthCheck)
config.Controllers.MachineHealthCheckEnabled = true
}

Expand Down