Skip to content

Set clusterID label on machine #608

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jun 11, 2020
Merged

Set clusterID label on machine #608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 1 addition & 8 deletions pkg/apis/machine/v1beta1/machine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,6 @@ limitations under the License.
package v1beta1

import (
"fmt"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -196,13 +194,8 @@ type LastOperation struct {
func (m *Machine) Validate() field.ErrorList {
errors := field.ErrorList{}

// validate spec.labels
fldPath := field.NewPath("spec")
if m.Labels[MachineClusterIDLabel] == "" {
errors = append(errors, field.Invalid(fldPath.Child("labels"), m.Labels, fmt.Sprintf("missing %v label.", MachineClusterIDLabel)))
}

// validate provider config is set
fldPath := field.NewPath("spec")
if m.Spec.ProviderSpec.Value == nil {
errors = append(errors, field.Invalid(fldPath.Child("spec").Child("providerspec"), m.Spec.ProviderSpec, "value field must be set"))
}
Expand Down
27 changes: 27 additions & 0 deletions pkg/controller/machine/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"fmt"
"time"

configv1 "github.com/openshift/api/config/v1"
machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
"github.com/openshift/machine-api-operator/pkg/util"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -91,6 +92,8 @@ const (
unknownInstanceState = "Unknown"

skipWaitForDeleteTimeoutSeconds = 60 * 5

globalInfrastuctureName = "cluster"
Copy link

Choose a reason for hiding this comment

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

Same here, VSphere util already exposes this variable:

machine-api-operator/pkg/controller/vsphere/util.go

Line 18 in 60eb822

globalInfrastuctureName = "cluster"

)

var DefaultActuator Actuator
Expand Down Expand Up @@ -175,6 +178,11 @@ func (r *ReconcileMachine) Reconcile(request reconcile.Request) (reconcile.Resul
return reconcile.Result{}, err
}

// Add clusterID label
if err := r.setClusterIDLabel(ctx, m); err != nil {
return reconcile.Result{}, err
}

// If object hasn't been deleted and doesn't have a finalizer, add one
// Add a finalizer to newly created objects.
if m.ObjectMeta.DeletionTimestamp.IsZero() {
Expand Down Expand Up @@ -462,6 +470,25 @@ func (r *ReconcileMachine) patchFailedMachineInstanceAnnotation(machine *machine
return nil
}

func (r *ReconcileMachine) setClusterIDLabel(ctx context.Context, m *machinev1.Machine) error {
infra := &configv1.Infrastructure{}
infraName := client.ObjectKey{Name: globalInfrastuctureName}

if err := r.Client.Get(ctx, infraName, infra); err != nil {
return err
}
Comment on lines +474 to +479
Copy link

Choose a reason for hiding this comment

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

This could be substituted with

machine-api-operator/pkg/controller/vsphere/util.go

Lines 51 to 65 in 60eb822

func getInfrastructure(c runtimeclient.Reader) (*configv1.Infrastructure, error) {
if c == nil {
return nil, errors.New("no API reader -- will not fetch infrastructure config")
}
infra := &configv1.Infrastructure{}
infraName := runtimeclient.ObjectKey{Name: globalInfrastuctureName}
if err := c.Get(context.Background(), infraName, infra); err != nil {
return nil, err
}
return infra, nil
}

With added tests from pkg/controller/machine/machine_controller_test.go you'll essentially cover both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to import a function from vsphere utils, but making a shared utils package in future makes sense. Anyway, it's not the scope of this PR


clusterID := infra.Status.InfrastructureName

if m.Labels == nil {
m.Labels = make(map[string]string)
}

m.Labels[machinev1.MachineClusterIDLabel] = clusterID

return nil
}

func machineIsProvisioned(machine *machinev1.Machine) bool {
return len(machine.Status.Addresses) > 0 || stringPointerDeref(machine.Spec.ProviderID) != ""
}
Expand Down
43 changes: 43 additions & 0 deletions pkg/controller/machine/controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (
"time"

. "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1"
machinev1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand All @@ -40,7 +41,20 @@ var (
_ reconcile.Reconciler = &ReconcileMachine{}
)

func init() {
configv1.AddToScheme(scheme.Scheme)
}

func TestReconcileRequest(t *testing.T) {
infra := configv1.Infrastructure{
ObjectMeta: metav1.ObjectMeta{
Name: globalInfrastuctureName,
},
Status: configv1.InfrastructureStatus{
InfrastructureName: "test-id",
},
}

machineProvisioning := machinev1.Machine{
TypeMeta: metav1.TypeMeta{
Kind: "Machine",
Expand Down Expand Up @@ -278,6 +292,7 @@ func TestReconcileRequest(t *testing.T) {
&machineDeleting,
&machineFailed,
&machineRunning,
&infra,
),
scheme: scheme.Scheme,
actuator: act,
Expand Down Expand Up @@ -714,3 +729,31 @@ func TestDelayIfRequeueAfterError(t *testing.T) {
})
}
}

func TestSetClusterIDLabel(t *testing.T) {
clusterID := "test-id"
infra := &configv1.Infrastructure{
ObjectMeta: metav1.ObjectMeta{
Name: globalInfrastuctureName,
},
Status: configv1.InfrastructureStatus{
InfrastructureName: clusterID,
},
}

machine := &machinev1.Machine{}

reconciler := &ReconcileMachine{
Client: fake.NewFakeClientWithScheme(scheme.Scheme, infra),
scheme: scheme.Scheme,
}

if err := reconciler.setClusterIDLabel(ctx, machine); err != nil {
t.Fatal(err)
}

actualClusterID := machine.Labels[machinev1.MachineClusterIDLabel]
if actualClusterID != clusterID {
t.Fatalf("Expected clusterID %s, got %s", clusterID, actualClusterID)
}
}
6 changes: 5 additions & 1 deletion pkg/controller/machine/machine_controller_suite_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"path/filepath"
"testing"

configv1 "github.com/openshift/api/config/v1"
"github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
"k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/rest"
Expand All @@ -37,9 +38,12 @@ var (

func TestMain(m *testing.M) {
t := &envtest.Environment{
CRDDirectoryPaths: []string{filepath.Join("..", "..", "..", "install")},
CRDDirectoryPaths: []string{
filepath.Join("..", "..", "..", "install"),
filepath.Join("..", "..", "..", "vendor", "github.com", "openshift", "api", "config", "v1")},
}
v1beta1.AddToScheme(scheme.Scheme)
configv1.AddToScheme(scheme.Scheme)

var err error
if cfg, err = t.Start(); err != nil {
Expand Down
15 changes: 15 additions & 0 deletions pkg/controller/machine/machine_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"time"

. "github.com/onsi/gomega"
configv1 "github.com/openshift/api/config/v1"
machinev1beta1 "github.com/openshift/machine-api-operator/pkg/apis/machine/v1beta1"
"golang.org/x/net/context"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
Expand Down Expand Up @@ -52,6 +53,15 @@ func TestReconcile(t *testing.T) {
},
}

infra := &configv1.Infrastructure{
ObjectMeta: metav1.ObjectMeta{
Name: globalInfrastuctureName,
},
Status: configv1.InfrastructureStatus{
InfrastructureName: "test-id",
},
}

// Setup the Manager and Controller. Wrap the Controller Reconcile function so it writes each request to a
// channel when it is finished.
mgr, err := manager.New(cfg, manager.Options{MetricsBindAddress: "0"})
Expand All @@ -74,6 +84,11 @@ func TestReconcile(t *testing.T) {
}
}()

if err := c.Create(context.TODO(), infra); err != nil {
t.Fatalf("error creating instance: %v", err)
}
defer c.Delete(context.TODO(), infra)

// Create the Machine object and expect Reconcile and the actuator to be called
if err := c.Create(context.TODO(), instance); err != nil {
t.Fatalf("error creating instance: %v", err)
Expand Down