From 51a270dfca7c818ca196e1e0d021422c92888f85 Mon Sep 17 00:00:00 2001 From: Qian Sun Date: Fri, 9 Feb 2024 07:13:05 +0000 Subject: [PATCH] Move SubnetPort.spec.AttachmentRef to Annotations Testing done: 1. kubectl apply -f test/e2e/manifest/testSubnetPort/vm_without_labels.yaml 2. kubectl apply -f test/e2e/manifest/testSubnetPort/port_with_attachment_ref.yaml 3. add/update/delete the labels on VM and check the port tags --- .../yaml/crd/nsx.vmware.com_subnetports.yaml | 38 ------------ .../v1alpha1/subnetport_types.go | 3 - .../v1alpha1/zz_generated.deepcopy.go | 1 - pkg/apis/v1alpha1/subnetport_types.go | 3 - pkg/apis/v1alpha1/zz_generated.deepcopy.go | 1 - pkg/controllers/common/utils.go | 17 ++++++ pkg/controllers/common/utils_test.go | 59 +++++++++++++++++++ .../subnetport/subnetport_controller.go | 26 ++++---- pkg/nsx/services/common/types.go | 1 + .../port_with_attachment_ref.yaml | 9 +++ .../testSubnetPort/vm_without_labels.yaml | 6 ++ 11 files changed, 106 insertions(+), 58 deletions(-) create mode 100644 pkg/controllers/common/utils_test.go create mode 100644 test/e2e/manifest/testSubnetPort/port_with_attachment_ref.yaml create mode 100644 test/e2e/manifest/testSubnetPort/vm_without_labels.yaml diff --git a/build/yaml/crd/nsx.vmware.com_subnetports.yaml b/build/yaml/crd/nsx.vmware.com_subnetports.yaml index 9835d95af..6afab7e2c 100644 --- a/build/yaml/crd/nsx.vmware.com_subnetports.yaml +++ b/build/yaml/crd/nsx.vmware.com_subnetports.yaml @@ -48,44 +48,6 @@ spec: spec: description: SubnetPortSpec defines the desired state of SubnetPort. properties: - attachmentRef: - description: AttachmentRef refers to the virtual machine which the - SubnetPort is attached. - properties: - apiVersion: - description: API version of the referent. - type: string - fieldPath: - description: 'If referring to a piece of an object instead of - an entire object, this string should contain a valid JSON/Go - field access statement, such as desiredState.manifest.containers[2]. - For example, if the object reference is to a container within - a pod, this would take on a value like: "spec.containers{name}" - (where "name" refers to the name of the container that triggered - the event) or if no container name is specified "spec.containers[2]" - (container with index 2 in this pod). This syntax is chosen - only to have some well-defined way of referencing a part of - an object. TODO: this design is not final and this field is - subject to change in the future.' - type: string - kind: - description: 'Kind of the referent. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds' - type: string - name: - description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names' - type: string - namespace: - description: 'Namespace of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/namespaces/' - type: string - resourceVersion: - description: 'Specific resourceVersion to which this reference - is made, if any. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#concurrency-control-and-consistency' - type: string - uid: - description: 'UID of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#uids' - type: string - type: object - x-kubernetes-map-type: atomic subnet: description: Subnet defines the parent Subnet name of the SubnetPort. type: string diff --git a/pkg/apis/nsx.vmware.com/v1alpha1/subnetport_types.go b/pkg/apis/nsx.vmware.com/v1alpha1/subnetport_types.go index d395692f7..ba6d96c0f 100644 --- a/pkg/apis/nsx.vmware.com/v1alpha1/subnetport_types.go +++ b/pkg/apis/nsx.vmware.com/v1alpha1/subnetport_types.go @@ -4,7 +4,6 @@ package v1alpha1 import ( - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -14,8 +13,6 @@ type SubnetPortSpec struct { Subnet string `json:"subnet,omitempty"` // SubnetSet defines the parent SubnetSet name of the SubnetPort. SubnetSet string `json:"subnetSet,omitempty"` - // AttachmentRef refers to the virtual machine which the SubnetPort is attached. - AttachmentRef corev1.ObjectReference `json:"attachmentRef,omitempty"` } // SubnetPortStatus defines the observed state of SubnetPort. diff --git a/pkg/apis/nsx.vmware.com/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/nsx.vmware.com/v1alpha1/zz_generated.deepcopy.go index 2d5e262e6..ce15e9e60 100644 --- a/pkg/apis/nsx.vmware.com/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/nsx.vmware.com/v1alpha1/zz_generated.deepcopy.go @@ -919,7 +919,6 @@ func (in *SubnetPortList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SubnetPortSpec) DeepCopyInto(out *SubnetPortSpec) { *out = *in - out.AttachmentRef = in.AttachmentRef } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SubnetPortSpec. diff --git a/pkg/apis/v1alpha1/subnetport_types.go b/pkg/apis/v1alpha1/subnetport_types.go index d395692f7..ba6d96c0f 100644 --- a/pkg/apis/v1alpha1/subnetport_types.go +++ b/pkg/apis/v1alpha1/subnetport_types.go @@ -4,7 +4,6 @@ package v1alpha1 import ( - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" ) @@ -14,8 +13,6 @@ type SubnetPortSpec struct { Subnet string `json:"subnet,omitempty"` // SubnetSet defines the parent SubnetSet name of the SubnetPort. SubnetSet string `json:"subnetSet,omitempty"` - // AttachmentRef refers to the virtual machine which the SubnetPort is attached. - AttachmentRef corev1.ObjectReference `json:"attachmentRef,omitempty"` } // SubnetPortStatus defines the observed state of SubnetPort. diff --git a/pkg/apis/v1alpha1/zz_generated.deepcopy.go b/pkg/apis/v1alpha1/zz_generated.deepcopy.go index 2d5e262e6..ce15e9e60 100644 --- a/pkg/apis/v1alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/v1alpha1/zz_generated.deepcopy.go @@ -919,7 +919,6 @@ func (in *SubnetPortList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *SubnetPortSpec) DeepCopyInto(out *SubnetPortSpec) { *out = *in - out.AttachmentRef = in.AttachmentRef } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new SubnetPortSpec. diff --git a/pkg/controllers/common/utils.go b/pkg/controllers/common/utils.go index 46ab8cc69..d098937ca 100644 --- a/pkg/controllers/common/utils.go +++ b/pkg/controllers/common/utils.go @@ -125,3 +125,20 @@ func NodeIsMaster(node *v1.Node) bool { } return false } + +func GetVirtualMachineNameForSubnetPort(subnetPort *v1alpha1.SubnetPort) (string, error) { + annotations := subnetPort.GetAnnotations() + if annotations == nil { + return "", nil + } + attachmentRef, exist := annotations[servicecommon.AnnotationAttachmentRef] + if !exist { + return "", nil + } + array := strings.Split(attachmentRef, "/") + if len(array) != 2 || !strings.EqualFold(array[0], servicecommon.ResourceTypeVirtualMachine) { + err := fmt.Errorf("invalid annotation value of '%s': %s", servicecommon.AnnotationAttachmentRef, attachmentRef) + return "", err + } + return array[1], nil +} diff --git a/pkg/controllers/common/utils_test.go b/pkg/controllers/common/utils_test.go new file mode 100644 index 000000000..a2b19fca0 --- /dev/null +++ b/pkg/controllers/common/utils_test.go @@ -0,0 +1,59 @@ +package common + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/vmware-tanzu/nsx-operator/pkg/apis/v1alpha1" +) + +func TestGetVirtualMachineNameForSubnetPort(t *testing.T) { + type args struct { + subnetPort *v1alpha1.SubnetPort + } + type want struct { + vm string + err error + } + tests := []struct { + name string + args args + want want + }{ + { + "port_with_annotation", + args{&v1alpha1.SubnetPort{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "nsx.vmware.com/attachment_ref": "virtualmachine/abc", + }, + }}}, + want{vm: "abc", err: nil}, + }, + { + "port_without_annotation", + args{&v1alpha1.SubnetPort{}}, + want{vm: "", err: nil}, + }, + { + "port_with_invalid_annotation", + args{&v1alpha1.SubnetPort{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{ + "nsx.vmware.com/attachment_ref": "invalid/abc", + }, + }}}, + want{vm: "", err: fmt.Errorf("invalid annotation value of 'nsx.vmware.com/attachment_ref': invalid/abc")}, + }, + } + for _, tt := range tests { + got, err := GetVirtualMachineNameForSubnetPort(tt.args.subnetPort) + assert.Equal(t, err, tt.want.err) + if got != tt.want.vm { + t.Errorf("%s failed: got %s, want %s", tt.name, got, tt.want.vm) + } + } +} diff --git a/pkg/controllers/subnetport/subnetport_controller.go b/pkg/controllers/subnetport/subnetport_controller.go index fe433cf95..bf5e8a3cc 100644 --- a/pkg/controllers/subnetport/subnetport_controller.go +++ b/pkg/controllers/subnetport/subnetport_controller.go @@ -94,10 +94,9 @@ func (r *SubnetPortReconciler) Reconcile(ctx context.Context, req ctrl.Request) } labels, err := r.getLabelsFromVirtualMachine(ctx, subnetPort) if err != nil { - log.Error(err, "failed to get labels from virtualmachine", "subnetPort.Name", subnetPort.Name, "subnetPort.UID", subnetPort.UID, "subnetPort.Spec.AttachmentRef", subnetPort.Spec.AttachmentRef) + log.Error(err, "failed to get labels from virtualmachine", "subnetPort.Name", subnetPort.Name, "subnetPort.UID", subnetPort.UID) return common.ResultRequeue, err } - log.Info("got labels from virtualmachine for subnetport", "subnetPort.UID", subnetPort.UID, "virtualmachine name", subnetPort.Spec.AttachmentRef.Name, "labels", labels) nsxSubnetPortState, err := r.SubnetPortService.CreateOrUpdateSubnetPort(subnetPort, nsxSubnetPath, "", labels) if err != nil { log.Error(err, "failed to create or update NSX subnet port, would retry exponentially", "subnetport", req.NamespacedName) @@ -180,8 +179,13 @@ func (r *SubnetPortReconciler) vmMapFunc(_ context.Context, vm client.Object) [] return requests } for _, subnetPort := range subnetPortList.Items { - if subnetPort.Spec.AttachmentRef.Name == vm.GetName() && (subnetPort.Spec.AttachmentRef.Namespace == vm.GetNamespace() || - (subnetPort.Spec.AttachmentRef.Namespace == "" && subnetPort.Namespace == vm.GetNamespace())) { + port := subnetPort + vmName, err := common.GetVirtualMachineNameForSubnetPort(&port) + if err != nil { + // not block the subnetport visiting because of invalid annotations + log.Error(err, "failed to get virtualmachine name from subnetport", "subnetPort.UID", subnetPort.UID) + } + if vmName == vm.GetName() && subnetPort.Namespace == vm.GetNamespace() { requests = append(requests, reconcile.Request{ NamespacedName: types.NamespacedName{ Name: subnetPort.Name, @@ -424,20 +428,18 @@ func (r *SubnetPortReconciler) updateSubnetStatusOnSubnetPort(subnetPort *v1alph } func (r *SubnetPortReconciler) getLabelsFromVirtualMachine(ctx context.Context, subnetPort *v1alpha1.SubnetPort) (*map[string]string, error) { - if subnetPort.Spec.AttachmentRef.Name == "" { - return nil, nil + vmName, err := common.GetVirtualMachineNameForSubnetPort(subnetPort) + if vmName == "" { + return nil, err } vm := &vmv1alpha1.VirtualMachine{} - namespace := subnetPort.Spec.AttachmentRef.Namespace - if len(namespace) == 0 { - namespace = subnetPort.Namespace - } namespacedName := types.NamespacedName{ - Name: subnetPort.Spec.AttachmentRef.Name, - Namespace: namespace, + Name: vmName, + Namespace: subnetPort.Namespace, } if err := r.Client.Get(ctx, namespacedName, vm); err != nil { return nil, err } + log.Info("got labels from virtualmachine for subnetport", "subnetPort.UID", subnetPort.UID, "vmName", vmName, "labels", vm.ObjectMeta.Labels) return &vm.ObjectMeta.Labels, nil } diff --git a/pkg/nsx/services/common/types.go b/pkg/nsx/services/common/types.go index 799cbcb2f..0931ca466 100644 --- a/pkg/nsx/services/common/types.go +++ b/pkg/nsx/services/common/types.go @@ -73,6 +73,7 @@ const ( AnnotationVPCNetworkConfig string = "nsx.vmware.com/vpc_network_config" AnnotationVPCName string = "nsx.vmware.com/vpc_name" AnnotationDefaultNetworkConfig string = "nsx.vmware.com/default" + AnnotationAttachmentRef string = "nsx.vmware.com/attachment_ref" AnnotationPodMAC string = "nsx.vmware.com/mac" AnnotationPodAttachment string = "nsx.vmware.com/attachment" TagScopePodName string = "nsx-op/pod_name" diff --git a/test/e2e/manifest/testSubnetPort/port_with_attachment_ref.yaml b/test/e2e/manifest/testSubnetPort/port_with_attachment_ref.yaml new file mode 100644 index 000000000..c798e4055 --- /dev/null +++ b/test/e2e/manifest/testSubnetPort/port_with_attachment_ref.yaml @@ -0,0 +1,9 @@ +apiVersion: nsx.vmware.com/v1alpha1 +kind: SubnetPort +metadata: + name: vm-port + namespace: subnetport-e2e + annotations: + nsx.vmware.com/attachment_ref: virtualmachine/vm1 +spec: + subnetSet: vm-default diff --git a/test/e2e/manifest/testSubnetPort/vm_without_labels.yaml b/test/e2e/manifest/testSubnetPort/vm_without_labels.yaml new file mode 100644 index 000000000..82c47dc01 --- /dev/null +++ b/test/e2e/manifest/testSubnetPort/vm_without_labels.yaml @@ -0,0 +1,6 @@ +apiVersion: vmoperator.vmware.com/v1alpha1 +kind: VirtualMachine +metadata: + name: vm1 + namespace: subnetport-e2e +spec: