Skip to content

Commit

Permalink
Fixup code review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
DanielXiao committed Aug 26, 2024
1 parent fd7ae02 commit ccb1474
Show file tree
Hide file tree
Showing 4 changed files with 32 additions and 30 deletions.
4 changes: 2 additions & 2 deletions pkg/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import (

"github.com/pkg/errors"
netopv1 "github.com/vmware-tanzu/net-operator-api/api/v1alpha1"
vpcapisv1 "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
nsxvpcv1 "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
vmoprv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha2"
ncpv1 "github.com/vmware-tanzu/vm-operator/external/ncp/api/v1alpha1"
"gopkg.in/fsnotify.v1"
Expand Down Expand Up @@ -65,7 +65,7 @@ func New(ctx context.Context, opts Options) (Manager, error) {
_ = vmoprv1.AddToScheme(opts.Scheme)
_ = ncpv1.AddToScheme(opts.Scheme)
_ = netopv1.AddToScheme(opts.Scheme)
_ = vpcapisv1.AddToScheme(opts.Scheme)
_ = nsxvpcv1.AddToScheme(opts.Scheme)
_ = topologyv1.AddToScheme(opts.Scheme)
_ = ipamv1.AddToScheme(opts.Scheme)

Expand Down
4 changes: 2 additions & 2 deletions pkg/services/network/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ package network

import (
netopv1 "github.com/vmware-tanzu/net-operator-api/api/v1alpha1"
vpcapisv1 "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
nsxvpcv1 "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
ncpv1 "github.com/vmware-tanzu/vm-operator/external/ncp/api/v1alpha1"
)

Expand Down Expand Up @@ -49,5 +49,5 @@ var (
NetworkGVKNSXT = ncpv1.SchemeGroupVersion.WithKind("VirtualNetwork")

// NetworkGVKNSXTVPC is the GVK used for networks in NSX-T VPC mode.
NetworkGVKNSXTVPC = vpcapisv1.SchemeGroupVersion.WithKind("SubnetSet")
NetworkGVKNSXTVPC = nsxvpcv1.SchemeGroupVersion.WithKind("SubnetSet")
)
32 changes: 16 additions & 16 deletions pkg/services/network/network_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (
. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"
netopv1 "github.com/vmware-tanzu/net-operator-api/api/v1alpha1"
vpcapisv1 "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
nsxvpcv1 "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
vmoprv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha2"
ncpv1 "github.com/vmware-tanzu/vm-operator/external/ncp/api/v1alpha1"
corev1 "k8s.io/api/core/v1"
Expand Down Expand Up @@ -270,7 +270,7 @@ var _ = Describe("Network provider", func() {
Expect(vm.Spec.Network.Interfaces).To(HaveLen(1))
Expect(vm.Spec.Network.Interfaces[0].Network.Name).To(Equal(vSphereCluster.Name))
Expect(vm.Spec.Network.Interfaces[0].Network.TypeMeta.Kind).To(Equal("SubnetSet"))
Expect(vm.Spec.Network.Interfaces[0].Network.TypeMeta.APIVersion).To(Equal(vpcapisv1.SchemeGroupVersion.String()))
Expect(vm.Spec.Network.Interfaces[0].Network.TypeMeta.APIVersion).To(Equal(nsxvpcv1.SchemeGroupVersion.String()))
})
})
})
Expand Down Expand Up @@ -336,7 +336,7 @@ var _ = Describe("Network provider", func() {
Expect(ncpv1.AddToScheme(scheme)).To(Succeed())
Expect(corev1.AddToScheme(scheme)).To(Succeed())
Expect(vmwarev1.AddToScheme(scheme)).To(Succeed())
Expect(vpcapisv1.AddToScheme(scheme)).To(Succeed())
Expect(nsxvpcv1.AddToScheme(scheme)).To(Succeed())
})

Context("with dummy network provider", func() {
Expand Down Expand Up @@ -599,14 +599,14 @@ var _ = Describe("Network provider", func() {

It("should not update subnetset", func() {
// Fetch the SubnetSet before the operation
initialSubnetSet := &vpcapisv1.SubnetSet{}
initialSubnetSet := &nsxvpcv1.SubnetSet{}
err = client.Get(ctx, apitypes.NamespacedName{
Name: dummyCluster,
Namespace: dummyNs,
}, initialSubnetSet)
Expect(err).NotTo(HaveOccurred())
status := vpcapisv1.SubnetSetStatus{
Conditions: []vpcapisv1.Condition{
status := nsxvpcv1.SubnetSetStatus{
Conditions: []nsxvpcv1.Condition{
{
Type: "Ready",
Status: "True",
Expand All @@ -620,7 +620,7 @@ var _ = Describe("Network provider", func() {
Expect(err).ToNot(HaveOccurred())
Expect(subnetset).To(Equal(clusterCtx.VSphereCluster.Name))

createdSubnetSet := &vpcapisv1.SubnetSet{}
createdSubnetSet := &nsxvpcv1.SubnetSet{}
err = client.Get(ctx, apitypes.NamespacedName{
Name: dummyCluster,
Namespace: dummyNs,
Expand All @@ -633,7 +633,7 @@ var _ = Describe("Network provider", func() {
It("should successfully retrieve VM service annotations, including the annotation to enable LB healthcheck", func() {
annotations, err := np.GetVMServiceAnnotations(ctx, clusterCtx)
Expect(err).ToNot(HaveOccurred())
Expect(annotations).To(HaveKey("lb.iaas.vmware.com/enable-endpoint-health-check"))
Expect(annotations).To(HaveKey(AnnotationEnableEndpointHealthCheckKey))
})

})
Expand All @@ -658,7 +658,7 @@ var _ = Describe("Network provider", func() {
Expect(err).ToNot(HaveOccurred())
Expect(subnetset).To(Equal(clusterCtx.VSphereCluster.Name))

createdSubnetSet := &vpcapisv1.SubnetSet{}
createdSubnetSet := &nsxvpcv1.SubnetSet{}
err = client.Get(ctx, apitypes.NamespacedName{
Name: dummyCluster,
Namespace: dummyNs,
Expand Down Expand Up @@ -700,14 +700,14 @@ var _ = Describe("Network provider", func() {

BeforeEach(func() {
scheme = runtime.NewScheme()
Expect(vpcapisv1.AddToScheme(scheme)).To(Succeed())
Expect(nsxvpcv1.AddToScheme(scheme)).To(Succeed())
nsxvpcNp, _ = NSXTVpcNetworkProvider(client).(*nsxtVPCNetworkProvider)
np = nsxvpcNp
})

It("should return error when subnetset ready status is false", func() {
status := vpcapisv1.SubnetSetStatus{
Conditions: []vpcapisv1.Condition{
status := nsxvpcv1.SubnetSetStatus{
Conditions: []nsxvpcv1.Condition{
{
Type: "Ready",
Status: "False",
Expand All @@ -716,7 +716,7 @@ var _ = Describe("Network provider", func() {
},
},
}
subnetsetObj = &vpcapisv1.SubnetSet{
subnetsetObj = &nsxvpcv1.SubnetSet{
ObjectMeta: metav1.ObjectMeta{
Namespace: cluster.Namespace,
Name: cluster.Name,
Expand All @@ -732,10 +732,10 @@ var _ = Describe("Network provider", func() {
})

It("should return error when subnetset ready status is not set", func() {
status := vpcapisv1.SubnetSetStatus{
Conditions: []vpcapisv1.Condition{},
status := nsxvpcv1.SubnetSetStatus{
Conditions: []nsxvpcv1.Condition{},
}
subnetsetObj = &vpcapisv1.SubnetSet{
subnetsetObj = &nsxvpcv1.SubnetSet{
ObjectMeta: metav1.ObjectMeta{
Namespace: cluster.Namespace,
Name: cluster.Name,
Expand Down
22 changes: 12 additions & 10 deletions pkg/services/network/nsxt_vpc_provider.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
"fmt"

"github.com/pkg/errors"
vpcapisv1 "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
nsxvpcv1 "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
vmoprv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha2"
vmoprv1common "github.com/vmware-tanzu/vm-operator/api/v1alpha2/common"
corev1 "k8s.io/api/core/v1"
Expand All @@ -41,7 +41,9 @@ import (
)

const (
// AnnotationEnableEndpointHealthCheckKey is the key of the annotation that is used to enable health check on the VMService endpoint port.
// AnnotationEnableEndpointHealthCheckKey is the key of the annotation that is used to enable health check on the
// Service endpoint port. vm-operator propagates annotations in VMService to Service and LB providers like NSX-T
// will enable health check on the endpoint target port when this annotation is present on the Service.
AnnotationEnableEndpointHealthCheckKey = "lb.iaas.vmware.com/enable-endpoint-health-check"
)

Expand Down Expand Up @@ -71,13 +73,13 @@ func (vp *nsxtVPCNetworkProvider) SupportsVMReadinessProbe() bool {
// verifyNsxtVpcSubnetSetStatus checks the status conditions of a given SubnetSet within a cluster context.
// If the subnet isn't ready, it is marked as false, and the function returns an error.
// If the subnet is ready, the function updates the VSphereCluster with a "true" status and returns nil.
func (vp *nsxtVPCNetworkProvider) verifyNsxtVpcSubnetSetStatus(vspherecluster *vmwarev1.VSphereCluster, subnetset *vpcapisv1.SubnetSet) error {
func (vp *nsxtVPCNetworkProvider) verifyNsxtVpcSubnetSetStatus(vspherecluster *vmwarev1.VSphereCluster, subnetset *nsxvpcv1.SubnetSet) error {
clusterName := vspherecluster.Name
namespace := vspherecluster.Namespace
hasReadyCondition := false

for _, condition := range subnetset.Status.Conditions {
if condition.Type != vpcapisv1.Ready {
if condition.Type != nsxvpcv1.Ready {
continue
}
hasReadyCondition = true
Expand All @@ -101,7 +103,7 @@ func (vp *nsxtVPCNetworkProvider) verifyNsxtVpcSubnetSetStatus(vspherecluster *v
// If it is, then it calls verifyNsxVpcSubnetSetStatus with the SubnetSet to verify its status.
// If it's not, it returns an error.
func (vp *nsxtVPCNetworkProvider) VerifyNetworkStatus(_ context.Context, clusterCtx *vmware.ClusterContext, obj runtime.Object) error {
subnetset, ok := obj.(*vpcapisv1.SubnetSet)
subnetset, ok := obj.(*nsxvpcv1.SubnetSet)
if !ok {
return fmt.Errorf("expected NSX VPC SubnetSet but got %T", obj)
}
Expand All @@ -124,14 +126,14 @@ func (vp *nsxtVPCNetworkProvider) ProvisionClusterNetwork(ctx context.Context, c
log.Info("Provisioning ")
defer log.Info("Finished provisioning")

subnetset := &vpcapisv1.SubnetSet{
subnetset := &nsxvpcv1.SubnetSet{
ObjectMeta: metav1.ObjectMeta{
Namespace: networkNamespace,
Name: networkName,
},
Spec: vpcapisv1.SubnetSetSpec{
AdvancedConfig: vpcapisv1.AdvancedConfig{
StaticIPAllocation: vpcapisv1.StaticIPAllocation{
Spec: nsxvpcv1.SubnetSetSpec{
AdvancedConfig: nsxvpcv1.AdvancedConfig{
StaticIPAllocation: nsxvpcv1.StaticIPAllocation{
Enable: true,
},
},
Expand Down Expand Up @@ -159,7 +161,7 @@ func (vp *nsxtVPCNetworkProvider) ProvisionClusterNetwork(ctx context.Context, c

// GetClusterNetworkName returns the name of a valid cluster network if one exists.
func (vp *nsxtVPCNetworkProvider) GetClusterNetworkName(ctx context.Context, clusterCtx *vmware.ClusterContext) (string, error) {
subnetset := &vpcapisv1.SubnetSet{}
subnetset := &nsxvpcv1.SubnetSet{}
cluster := clusterCtx.VSphereCluster
namespacedName := types.NamespacedName{
Namespace: cluster.Namespace,
Expand Down

0 comments on commit ccb1474

Please sign in to comment.