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
4 changes: 2 additions & 2 deletions cmd/machine-api-operator/client_builder.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@ func (cb *ClientBuilder) KubeClientOrDie(name string) kubernetes.Interface {
return kubernetes.NewForConfigOrDie(rest.AddUserAgent(cb.config, name))
}

// NewForConfigOrDie returns the kubernetes client interface for dynamic objects.
func (cb *ClientBuilder) dynamicClientOrDie(name string) dynamic.Interface {
// DynamicClientOrDie returns a dynamic client interface.
func (cb *ClientBuilder) DynamicClientOrDie(name string) dynamic.Interface {
return dynamic.NewForConfigOrDie(rest.AddUserAgent(cb.config, name))
}

Expand Down
2 changes: 1 addition & 1 deletion cmd/machine-api-operator/start.go
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ func startControllers(ctx *ControllerContext) {
ctx.ConfigInformerFactory.Config().V1().FeatureGates(),
ctx.ClientBuilder.KubeClientOrDie(componentName),
ctx.ClientBuilder.OpenshiftClientOrDie(componentName),
ctx.ClientBuilder.dynamicClientOrDie(componentName),
ctx.ClientBuilder.DynamicClientOrDie(componentName),
recorder,
).Run(1, ctx.Stop)
}
Expand Down
67 changes: 67 additions & 0 deletions pkg/operator/baremetal_config.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package operator

import (
"github.com/golang/glog"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime/schema"
dynamic "k8s.io/client-go/dynamic"
)

var provisioningGVR = schema.GroupVersionResource{Group: "metal3.io", Resource: "provisionings", Version: "v1alpha1"}
var baremetalProvisioningCR = "cluster"

// Provisioning Config needed to deploy Metal3 pod
type BaremetalProvisioningConfig struct {
ProvisioningInterface string
ProvisioningIp string
ProvisioningNetworkCIDR string
ProvisioningDHCPExternal bool
ProvisioningDHCPRange string
}

func getBaremetalProvisioningConfig(dc dynamic.Interface, configName string) (BaremetalProvisioningConfig, error) {
provisioningClient := dc.Resource(provisioningGVR)
provisioningConfig, err := provisioningClient.Get(configName, metav1.GetOptions{})
if err != nil {
glog.Errorf("Error getting config from Baremetal provisioning CR %s", configName)
return BaremetalProvisioningConfig{}, err
}
provisioningSpec, found, err := unstructured.NestedMap(provisioningConfig.UnstructuredContent(), "spec")
if !found {
glog.Errorf("Nested Spec not found in Baremetal provisioning CR %s", configName)
return BaremetalProvisioningConfig{}, err
}
provisioningInterface, found, err := unstructured.NestedString(provisioningSpec, "provisioningInterface")
if !found {
glog.Errorf("provisioningInterface not found in Baremetal provisioning CR %s", configName)
return BaremetalProvisioningConfig{}, err
}
provisioningIP, found, err := unstructured.NestedString(provisioningSpec, "provisioningIP")
if !found {
glog.Errorf("provisioningIP not found in Baremetal provisioning CR %s", configName)
return BaremetalProvisioningConfig{}, err
}
provisioningNetworkCIDR, found, err := unstructured.NestedString(provisioningSpec, "provisioningNetworkCIDR")
if !found {
glog.Errorf("provisioningNetworkCIDR not found in Baremetal provisioning CR %s", configName)
return BaremetalProvisioningConfig{}, err
}
provisioningDHCPExternal, found, err := unstructured.NestedBool(provisioningSpec, "provisioningDHCPExternal")
if !found {
glog.Errorf("provisioningDHCPExternal not found in Baremetal provisioning CR %s", configName)
return BaremetalProvisioningConfig{}, err
}
provisioningDHCPRange, found, err := unstructured.NestedString(provisioningSpec, "provisioningDHCPRange")
if !found {
glog.Errorf("provisioningDHCPRange not found in Baremetal provisioning CR %s", configName)
return BaremetalProvisioningConfig{}, err
}
return BaremetalProvisioningConfig{
ProvisioningInterface: provisioningInterface,
ProvisioningIp: provisioningIP,
ProvisioningNetworkCIDR: provisioningNetworkCIDR,
ProvisioningDHCPExternal: provisioningDHCPExternal,
ProvisioningDHCPRange: provisioningDHCPRange,
}, nil
}
3 changes: 2 additions & 1 deletion pkg/operator/baremetal_pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,8 @@ func createMariadbPasswordSecret(client coreclientv1.SecretsGetter, config *Oper
return err
}

func newMetal3Deployment(config *OperatorConfig) *appsv1.Deployment {
func newMetal3Deployment(config *OperatorConfig, baremetalProvisioningConfig BaremetalProvisioningConfig) *appsv1.Deployment {
// TODO(sadasu): Use config from BaremetalProvisioningConfig to set env vars for containers.
replicas := int32(1)
template := newMetal3PodTemplateSpec(config)

Expand Down
61 changes: 61 additions & 0 deletions pkg/operator/baremetal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,31 @@ import (
"testing"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
"k8s.io/apimachinery/pkg/runtime"
fakedynamic "k8s.io/client-go/dynamic/fake"
fakekube "k8s.io/client-go/kubernetes/fake"
"sigs.k8s.io/yaml"
)

var yamlContent = `
apiVersion: metal3.io/v1alpha1
kind: Provisioning
metadata:
name: test
spec:
provisioningInterface: "ensp0"
provisioningIP: "172.30.20.3"
provisioningNetworkCIDR: "172.30.20.0/24"
provisioningDHCPExternal: false
provisioningDHCPRange: "172.30.20.10, 72.30.20.100"
`
var (
expectedProvisioningInterface = "ensp0"
expectedProvisioningIP = "172.30.20.3"
expectedProvisioningNetworkCIDR = "172.30.20.0/24"
expectedProvisioningDHCPExternal = false
expectedProvisioningDHCPRange = "172.30.20.10, 72.30.20.100"
)

func TestGenerateRandomPassword(t *testing.T) {
Expand Down Expand Up @@ -71,3 +95,40 @@ func TestCreateMariadbPasswordSecret(t *testing.T) {
t.Logf("First Mariadb password is being preserved over re-creation as expected.")
}
}

func TestGetBaremetalProvisioningConfig(t *testing.T) {
u := &unstructured.Unstructured{Object: map[string]interface{}{}}
if err := yaml.Unmarshal([]byte(yamlContent), &u); err != nil {
t.Errorf("failed to unmarshall input yaml content:%v", err)
}
dynamicClient := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme(), u)
baremetalConfig, err := getBaremetalProvisioningConfig(dynamicClient, "test")
if err != nil {
t.Logf("Unstructed Config: %+v", u)
t.Fatalf("Failed to get Baremetal Provisioning Interface from CR %s", "test")
}
if baremetalConfig.ProvisioningInterface != expectedProvisioningInterface ||
baremetalConfig.ProvisioningIp != expectedProvisioningIP ||
baremetalConfig.ProvisioningNetworkCIDR != expectedProvisioningNetworkCIDR ||
baremetalConfig.ProvisioningDHCPExternal != expectedProvisioningDHCPExternal ||
baremetalConfig.ProvisioningDHCPRange != expectedProvisioningDHCPRange {
t.Logf("Actual BaremetalProvisioningConfig: %+v", baremetalConfig)
t.Logf("Expected : ProvisioningInterface: %s, ProvisioningIP: %s, ProvisioningNetworkCIDR: %s, ProvisioningDHCPExternal: %t, expectedProvisioningDHCPRange: %s", expectedProvisioningInterface, expectedProvisioningIP, expectedProvisioningNetworkCIDR, expectedProvisioningDHCPExternal, expectedProvisioningDHCPRange)
t.Fatalf("failed getBaremetalProvisioningConfig. One or more BaremetalProvisioningConfig items do not match the expected config.")
}
}

func TestGetIncorrectBaremetalProvisioningCR(t *testing.T) {
u := &unstructured.Unstructured{Object: map[string]interface{}{}}
if err := yaml.Unmarshal([]byte(yamlContent), &u); err != nil {
t.Errorf("failed to unmarshall input yaml content:%v", err)
}
dynamicClient := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme(), u)
baremetalConfig, err := getBaremetalProvisioningConfig(dynamicClient, "test1")
if err != nil {
Copy link
Member

@enxebre enxebre Jan 22, 2020

Choose a reason for hiding this comment

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

wouldn't be the client expected to error here and so getBaremetalProvisioningConfig?

For unit testing we prefer to have one testTunction per function i.e TestGetBaremetalProvisioningConfig and loop over multiple test cases within it. e.g https://github.com/openshift/machine-api-operator/blob/master/pkg/controller/vsphere/machine_scope_test.go#L139-L261

t.Logf("Unable to get Baremetal Provisioning Config from CR %s as expected", "test1")
}
if baremetalConfig.ProvisioningInterface != "" {
t.Errorf("BaremetalProvisioningConfig is not expected to be set.")
}
}
3 changes: 3 additions & 0 deletions pkg/operator/operator_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/util/wait"
fakedynamic "k8s.io/client-go/dynamic/fake"
"k8s.io/client-go/informers"
fakekube "k8s.io/client-go/kubernetes/fake"
"k8s.io/client-go/tools/record"
Expand Down Expand Up @@ -42,6 +43,7 @@ func newOperatorConfig() *OperatorConfig {
func newFakeOperator(kubeObjects []runtime.Object, osObjects []runtime.Object, stopCh <-chan struct{}) *Operator {
kubeClient := fakekube.NewSimpleClientset(kubeObjects...)
osClient := fakeos.NewSimpleClientset(osObjects...)
dynamicClient := fakedynamic.NewSimpleDynamicClient(runtime.NewScheme(), kubeObjects...)
kubeNamespacedSharedInformer := informers.NewSharedInformerFactoryWithOptions(kubeClient, 2*time.Minute, informers.WithNamespace(targetNamespace))
configSharedInformer := configinformersv1.NewSharedInformerFactoryWithOptions(osClient, 2*time.Minute)
featureGateInformer := configSharedInformer.Config().V1().FeatureGates()
Expand All @@ -50,6 +52,7 @@ func newFakeOperator(kubeObjects []runtime.Object, osObjects []runtime.Object, s
optr := &Operator{
kubeClient: kubeClient,
osClient: osClient,
dynamicClient: dynamicClient,
featureGateLister: featureGateInformer.Lister(),
deployLister: deployInformer.Lister(),
imagesFile: "fixtures/images.json",
Expand Down
9 changes: 7 additions & 2 deletions pkg/operator/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,18 @@ func (optr *Operator) syncClusterAPIController(config *OperatorConfig) error {
}

func (optr *Operator) syncBaremetalControllers(config *OperatorConfig) error {
// First create a Secret needed for the Metal3 deployment
// Try to get baremetal provisioning config from a CR
baremetalProvisioningConfig, err := getBaremetalProvisioningConfig(optr.dynamicClient, baremetalProvisioningCR)
if err != nil {
glog.Infof("Unable to read Baremetal Provisioning config from CR %s.", baremetalProvisioningCR)
Copy link
Member

Choose a reason for hiding this comment

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

I find it weird not returning err here, at minimum this should log.error, and make clear in the message the function will continue nevertheless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this release, we cannot return error and stop deploying Metal3 because during discussions around upgrade strategy, we had decided to have one release where the config could be obtained from either sources, the ConfigMap or the newly created Config resource.
In the current implementation, we are prioritizing reading config from the new Config Resource. When that fails, (could be for a simple reason that the user did not want to provide a CR and instead still wanted to use a configmap), then we should be able to proceed with the deployment.
The other part of the comment talks about logging an error. Again, I worry that this might be interpreted as something went wrong. If you think seeing an error msg in the logs will not be interpreted as I think it will, I am fine with making this Info statement into an Error msg.

}
// Create a Secret needed for the Metal3 deployment
if err := createMariadbPasswordSecret(optr.kubeClient.CoreV1(), config); err != nil {
glog.Error("Not proceeding with Metal3 deployment. Failed to create Mariadb password.")
return err
}

metal3Deployment := newMetal3Deployment(config)
metal3Deployment := newMetal3Deployment(config, baremetalProvisioningConfig)
_, updated, err := resourceapply.ApplyDeployment(optr.kubeClient.AppsV1(), metal3Deployment)
if err != nil {
return err
Expand Down
Loading