Merge pull request #307 from dtantsur/OCPBUGS-747

OCPBUGS-747: Use machines instead of nodes to detect masters

config/rbac/role.yaml

@@ -29,14 +29,6 @@ rules:
   - list
   - patch
   - watch
-- apiGroups:
-  - ""
-  resources:
-  - nodes
-  verbs:
-  - get
-  - list
-  - watch
 - apiGroups:
   - admissionregistration.k8s.io
   resources:
@@ -106,6 +98,14 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - machine.openshift.io
+  resources:
+  - machines
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - metal3.io
   resources:

controllers/provisioning_controller.go

@@ -19,8 +19,9 @@ import (
 	"context"
 	"fmt"
 	"net"
-	"net/url"
 	"os"
+	"reflect"
+	"sort"
 	"strings"
 	"time"
 
@@ -36,6 +37,7 @@ import (
 	"k8s.io/apimachinery/pkg/selection"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
+	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
 	utilnet "k8s.io/utils/net"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -44,6 +46,7 @@ import (
 
 	baremetalv1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	osconfigv1 "github.com/openshift/api/config/v1"
+	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	osclientset "github.com/openshift/client-go/config/clientset/versioned"
 	"github.com/openshift/cluster-baremetal-operator/api/v1alpha1"
 	metal3iov1alpha1 "github.com/openshift/cluster-baremetal-operator/api/v1alpha1"
@@ -60,6 +63,8 @@ const (
 	clusterConfigName      = "cluster-config-v1"
 	clusterConfigKey       = "install-config"
 	clusterConfigNamespace = "kube-system"
+	// Annotation linking a machine to a host
+	HostAnnotation = "metal3.io/BareMetalHost"
 )
 
 // ProvisioningReconciler reconciles a Provisioning object
@@ -93,8 +98,8 @@ type ensureFunc func(*provisioning.ProvisioningInfo) (bool, error)
 // +kubebuilder:rbac:groups=config.openshift.io,resources=infrastructures;infrastructures/status,verbs=get
 // +kubebuilder:rbac:groups="",resources=events,verbs=create;watch;list;patch
 // +kubebuilder:rbac:groups="",resources=configmaps;secrets;services,verbs=get;list;watch;create;update;patch;delete
-// +kubebuilder:rbac:groups="",resources=nodes,verbs=get;list;watch
 // +kubebuilder:rbac:groups=apps,resources=deployments;daemonsets,verbs=get;list;watch;create;update;patch;delete
+// +kubebuilder:rbac:groups=machine.openshift.io,resources=machines,verbs=get;list;watch
 // +kubebuilder:rbac:groups=metal3.io,resources=provisionings;provisionings/finalizers,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=metal3.io,resources=provisionings/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=metal3.io,resources=baremetalhosts,verbs=get;list;watch;update;patch
@@ -425,37 +430,44 @@ func (r *ProvisioningReconciler) networkStackFromServiceNetwork(ctx context.Cont
 	return ns, nil
 }
 
-func getHostByProviderId(provId string) string {
-	if provId == "" {
+func getHostByMachine(meta metav1.ObjectMeta) string {
+	annotations := meta.GetAnnotations()
+	annotation, ok := annotations[HostAnnotation]
+	if !ok {
+		klog.Warningf("Ignoring machine %s without annotation linking it to BareMetalHost", meta.Name)
 		return ""
 	}
 
-	provider, err := url.Parse(provId)
-	if err != nil || provider.Scheme != "baremetalhost" {
+	hostNamespace, hostName, err := cache.SplitMetaNamespaceKey(annotation)
+	if err != nil {
+		klog.Warningf("Ignoring machine %s with invalid BareMetalHost link %s", meta.Name, annotation)
 		return ""
 	}
-
-	path := strings.Split(strings.Trim(provider.Path, "/"), "/")
-	if len(path) < 2 || path[0] != ComponentNamespace {
+	if hostNamespace != ComponentNamespace {
+		klog.Warningf("Ignoring machine %s that is linked to a BareMetalHost in namespace %s", meta.Name, hostNamespace)
 		return ""
 	}
 
-	return path[1]
+	return hostName
 }
 
 func (r *ProvisioningReconciler) updateProvisioningMacAddresses(ctx context.Context, provConfig *metal3iov1alpha1.Provisioning) error {
 	if len(provConfig.Spec.ProvisioningMacAddresses) != 0 {
 		return nil
 	}
 
-	nodes := corev1.NodeList{}
+	machines := machinev1beta1.MachineList{}
 	bmhNames := []string{}
-	labelReq, _ := labels.NewRequirement("node-role.kubernetes.io/master", selection.Exists, nil)
-	if err := r.Client.List(ctx, &nodes, &client.ListOptions{LabelSelector: labels.NewSelector().Add(*labelReq)}); err != nil {
-		return errors.Wrap(err, "cannot list master nodes")
+	labelReq, _ := labels.NewRequirement("machine.openshift.io/cluster-api-machine-role", selection.Equals, []string{"master"})
+	if err := r.Client.List(ctx, &machines, &client.ListOptions{LabelSelector: labels.NewSelector().Add(*labelReq)}); err != nil {
+		return errors.Wrap(err, "cannot list master machines")
 	}
-	for _, node := range nodes.Items {
-		bmhName := getHostByProviderId(node.Spec.ProviderID)
+	if len(machines.Items) < 1 {
+		return errors.New("machines with cluster-api-machine-role=master not found")
+	}
+
+	for _, machine := range machines.Items {
+		bmhName := getHostByMachine(machine.ObjectMeta)
 		if bmhName != "" {
 			bmhNames = append(bmhNames, bmhName)
 		}
@@ -471,8 +483,16 @@ func (r *ProvisioningReconciler) updateProvisioningMacAddresses(ctx context.Cont
 			macs = append(macs, bmh.Spec.BootMACAddress)
 		}
 	}
-	provConfig.Spec.ProvisioningMacAddresses = macs
-	return r.Client.Update(ctx, provConfig)
+
+	sort.Strings(macs)
+	if !reflect.DeepEqual(provConfig.Spec.ProvisioningMacAddresses, macs) {
+		klog.InfoS("Updating provisioning MAC addresses",
+			"CurrentMacAddresses", provConfig.Spec.ProvisioningMacAddresses, "NewMacAddresses", macs)
+		provConfig.Spec.ProvisioningMacAddresses = macs
+		return r.Client.Update(ctx, provConfig)
+	} else {
+		return nil
+	}
 }
 
 // SetupWithManager configures the manager to run the controller
@@ -520,5 +540,6 @@ func (r *ProvisioningReconciler) SetupWithManager(mgr ctrl.Manager) error {
 		Owns(&appsv1.DaemonSet{}).
 		Owns(&osconfigv1.ClusterOperator{}).
 		Owns(&osconfigv1.Proxy{}).
+		Owns(&machinev1beta1.Machine{}).
 		Complete(r)
 }

controllers/provisioning_controller_test.go

@@ -13,6 +13,7 @@ import (
 
 	baremetalv1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	configv1 "github.com/openshift/api/config/v1"
+	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	fakeconfigclientset "github.com/openshift/client-go/config/clientset/versioned/fake"
 	metal3iov1alpha1 "github.com/openshift/cluster-baremetal-operator/api/v1alpha1"
 	"github.com/openshift/cluster-baremetal-operator/provisioning"
@@ -24,6 +25,7 @@ func setUpSchemeForReconciler() *runtime.Scheme {
 	// we need to add the openshift/api to the scheme to be able to read
 	// the infrastructure CR
 	utilruntime.Must(configv1.AddToScheme(scheme))
+	utilruntime.Must(machinev1beta1.AddToScheme(scheme))
 	utilruntime.Must(metal3iov1alpha1.AddToScheme(scheme))
 	utilruntime.Must(baremetalv1alpha1.AddToScheme(scheme))
 	return scheme
@@ -215,65 +217,52 @@ func TestUpdateProvisioningMacAddresses(t *testing.T) {
 				BootMACAddress: "00:3d:25:45:bf:e9",
 			},
 		},
-		&corev1.Node{
+		&machinev1beta1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:   "node-0",
-				Labels: map[string]string{"node-role.kubernetes.io/master": ""},
-			},
-			Spec: corev1.NodeSpec{
-				ProviderID: "baremetalhost:///openshift-machine-api/test-master-0/something",
+				Name:        "node-0",
+				Labels:      map[string]string{"machine.openshift.io/cluster-api-machine-role": "master"},
+				Annotations: map[string]string{"metal3.io/BareMetalHost": "openshift-machine-api/test-master-0"},
 			},
 		},
-		&corev1.Node{
+		&machinev1beta1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:   "node-1",
-				Labels: map[string]string{"node-role.kubernetes.io/master": ""},
-			},
-			Spec: corev1.NodeSpec{
-				ProviderID: "provider:///openshift-machine-api/test-worker-0/something",
+				Name:        "node-1",
+				Labels:      map[string]string{"machine.openshift.io/cluster-api-machine-role": "master"},
+				Annotations: map[string]string{"metal3.io/BareMetalHost": "another-ns/host"},
 			},
 		},
-		&corev1.Node{
+		&machinev1beta1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:   "node-2",
-				Labels: map[string]string{"node-role.kubernetes.io/master": ""},
-			},
-			Spec: corev1.NodeSpec{
-				ProviderID: "baremetalhost:///broken",
+				Name:        "node-2",
+				Labels:      map[string]string{"machine.openshift.io/cluster-api-machine-role": "master"},
+				Annotations: map[string]string{"metal3.io/BareMetalHost": "invalid"},
 			},
 		},
-		&corev1.Node{
+		&machinev1beta1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:   "node-3",
-				Labels: map[string]string{"node-role.kubernetes.io/master": ""},
-			},
-			Spec: corev1.NodeSpec{
-				ProviderID: "baremetalhost:///openshift-machine-api/test-controlplane-1/something",
+				Name:        "node-3",
+				Labels:      map[string]string{"machine.openshift.io/cluster-api-machine-role": "master"},
+				Annotations: map[string]string{"metal3.io/BareMetalHost": "openshift-machine-api/test-controlplane-1"},
 			},
 		},
-		&corev1.Node{
+		&machinev1beta1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
 				Name:   "node-4",
-				Labels: map[string]string{"node-role.kubernetes.io/master": ""},
+				Labels: map[string]string{"machine.openshift.io/cluster-api-machine-role": "master"},
 			},
-			Spec: corev1.NodeSpec{},
 		},
-		&corev1.Node{
+		&machinev1beta1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:   "node-5",
-				Labels: map[string]string{"node-role.kubernetes.io/master": ""},
-			},
-			Spec: corev1.NodeSpec{
-				ProviderID: "baremetalhost:///openshift-machine-api/test-master-2/something",
+				Name:        "node-5",
+				Labels:      map[string]string{"machine.openshift.io/cluster-api-machine-role": "master"},
+				Annotations: map[string]string{"metal3.io/BareMetalHost": "openshift-machine-api/test-master-2"},
 			},
 		},
-		&corev1.Node{
+		&machinev1beta1.Machine{
 			ObjectMeta: metav1.ObjectMeta{
-				Name:   "node-6",
-				Labels: map[string]string{"node-role.kubernetes.io/worker": ""},
-			},
-			Spec: corev1.NodeSpec{
-				ProviderID: "baremetalhost:///openshift-machine-api/test-worker-1/something",
+				Name:        "node-6",
+				Labels:      map[string]string{"machine.openshift.io/cluster-api-machine-role": "worker"},
+				Annotations: map[string]string{"metal3.io/BareMetalHost": "openshift-machine-api/test-worker-0"},
 			},
 		},
 	}

main.go

@@ -34,6 +34,7 @@ import (
 
 	baremetalv1alpha1 "github.com/metal3-io/baremetal-operator/apis/metal3.io/v1alpha1"
 	osconfigv1 "github.com/openshift/api/config/v1"
+	machinev1beta1 "github.com/openshift/api/machine/v1beta1"
 	osclientset "github.com/openshift/client-go/config/clientset/versioned"
 	metal3iov1alpha1 "github.com/openshift/cluster-baremetal-operator/api/v1alpha1"
 	"github.com/openshift/cluster-baremetal-operator/controllers"
@@ -49,6 +50,7 @@ func init() {
 	utilruntime.Must(clientgoscheme.AddToScheme(scheme))
 	utilruntime.Must(metal3iov1alpha1.AddToScheme(scheme))
 	utilruntime.Must(osconfigv1.AddToScheme(scheme))
+	utilruntime.Must(machinev1beta1.AddToScheme(scheme))
 	utilruntime.Must(baremetalv1alpha1.AddToScheme(scheme))
 
 	// +kubebuilder:scaffold:scheme

manifests/0000_31_cluster-baremetal-operator_05_rbac.yaml

@@ -94,14 +94,6 @@ rules:
   - list
   - patch
   - watch
-- apiGroups:
-  - ""
-  resources:
-  - nodes
-  verbs:
-  - get
-  - list
-  - watch
 - apiGroups:
   - admissionregistration.k8s.io
   resources:
@@ -171,6 +163,14 @@ rules:
   - get
   - list
   - watch
+- apiGroups:
+  - machine.openshift.io
+  resources:
+  - machines
+  verbs:
+  - get
+  - list
+  - watch
 - apiGroups:
   - metal3.io
   resources: