Skip to content

Commit

Permalink
resolve comment
Browse files Browse the repository at this point in the history
Signed-off-by: Xudong Liu <xudongl@vmware.com>
  • Loading branch information
Xudong Liu committed Jan 31, 2024
1 parent 1071d14 commit d861bcd
Show file tree
Hide file tree
Showing 14 changed files with 76 additions and 44 deletions.
2 changes: 1 addition & 1 deletion pkg/cloudprovider/vsphereparavirtual/instances.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import (
)

type instances struct {
vmClient vmop.V1alpha1Interface
vmClient vmop.Interface
namespace string
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/cloudprovider/vsphereparavirtual/instances_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -93,12 +93,12 @@ func initTest(testVM *vmopv1alpha1.VirtualMachine) (*instances, *dynamicfake.Fak
scheme := runtime.NewScheme()
_ = vmopv1alpha1.AddToScheme(scheme)
fc := dynamicfake.NewSimpleDynamicClient(scheme)
fcw := vmopclient.NewFakeClient(fc)
fcw := vmopclient.NewFakeClientSet(fc)
instance := &instances{
vmClient: fcw,
namespace: testClusterNameSpace,
}
_, err := fcw.VirtualMachines(testVM.Namespace).Create(context.TODO(), testVM, metav1.CreateOptions{})
_, err := fcw.V1alpha1().VirtualMachines(testVM.Namespace).Create(context.TODO(), testVM, metav1.CreateOptions{})
return instance, fc, err
}

Expand Down
26 changes: 13 additions & 13 deletions pkg/cloudprovider/vsphereparavirtual/loadbalancer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,14 +53,14 @@ var (
}
)

func newTestLoadBalancer() (cloudprovider.LoadBalancer, *vmopclient.FakeClient) {
func newTestLoadBalancer() (cloudprovider.LoadBalancer, *dynamicfake.FakeDynamicClient) {
scheme := runtime.NewScheme()
_ = vmopv1alpha1.AddToScheme(scheme)
fc := dynamicfake.NewSimpleDynamicClient(scheme)
fcw := vmopclient.NewFakeClient(fc)
fcw := vmopclient.NewFakeClientSet(fc)

vms := vmservice.NewVMService(fcw, testClusterNameSpace, &testOwnerReference)
return &loadBalancer{vmService: vms}, fcw
return &loadBalancer{vmService: vms}, fc
}

func TestNewLoadBalancer(t *testing.T) {
Expand Down Expand Up @@ -150,7 +150,7 @@ func TestUpdateLoadBalancer(t *testing.T) {

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
lb, fcw := newTestLoadBalancer()
lb, fc := newTestLoadBalancer()
testK8sService := &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: testK8sServiceName,
Expand All @@ -176,7 +176,7 @@ func TestUpdateLoadBalancer(t *testing.T) {

if testCase.expectErr {
// Ensure that the client Update call returns an error on update
fcw.DynamicClient.PrependReactor("update", "virtualmachineservices", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
fc.PrependReactor("update", "virtualmachineservices", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
return true, nil, fmt.Errorf("Some undefined update error")
})
err = lb.UpdateLoadBalancer(context.Background(), testClustername, testK8sService, []*v1.Node{})
Expand All @@ -190,7 +190,7 @@ func TestUpdateLoadBalancer(t *testing.T) {
}

func TestEnsureLoadBalancer_VMServiceExternalTrafficPolicyLocal(t *testing.T) {
lb, fcw := newTestLoadBalancer()
lb, fc := newTestLoadBalancer()
testK8sService := &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: testK8sServiceName,
Expand All @@ -201,7 +201,7 @@ func TestEnsureLoadBalancer_VMServiceExternalTrafficPolicyLocal(t *testing.T) {
},
}

fcw.DynamicClient.PrependReactor("create", "virtualmachineservices", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
fc.PrependReactor("create", "virtualmachineservices", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
unstructuredObj, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(&vmopv1alpha1.VirtualMachineService{
Status: vmopv1alpha1.VirtualMachineServiceStatus{
LoadBalancer: vmopv1alpha1.LoadBalancerStatus{
Expand Down Expand Up @@ -247,14 +247,14 @@ func TestEnsureLoadBalancer(t *testing.T) {

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
lb, fcw := newTestLoadBalancer()
lb, fc := newTestLoadBalancer()
testK8sService := &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: testK8sServiceName,
Namespace: testK8sServiceNameSpace,
},
}
fcw.DynamicClient.PrependReactor("create", "virtualmachineservices", testCase.createFunc)
fc.PrependReactor("create", "virtualmachineservices", testCase.createFunc)

_, ensureErr := lb.EnsureLoadBalancer(context.Background(), testClustername, testK8sService, []*v1.Node{})
assert.Equal(t, ensureErr.Error(), testCase.expectErr.Error())
Expand All @@ -266,15 +266,15 @@ func TestEnsureLoadBalancer(t *testing.T) {
}

func TestEnsureLoadBalancer_VMServiceCreatedIPFound(t *testing.T) {
lb, fcw := newTestLoadBalancer()
lb, fc := newTestLoadBalancer()
testK8sService := &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: testK8sServiceName,
Namespace: testK8sServiceNameSpace,
},
}
// Ensure that the client Create call returns a VMService with a valid IP
fcw.DynamicClient.PrependReactor("create", "virtualmachineservices", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
fc.PrependReactor("create", "virtualmachineservices", func(action clientgotesting.Action) (handled bool, ret runtime.Object, err error) {
unstructuredObj, _ := runtime.DefaultUnstructuredConverter.ToUnstructured(&vmopv1alpha1.VirtualMachineService{
Status: vmopv1alpha1.VirtualMachineServiceStatus{
LoadBalancer: vmopv1alpha1.LoadBalancerStatus{
Expand Down Expand Up @@ -342,7 +342,7 @@ func TestEnsureLoadBalancer_DeleteLB(t *testing.T) {

for _, testCase := range testCases {
t.Run(testCase.name, func(t *testing.T) {
lb, fcw := newTestLoadBalancer()
lb, fc := newTestLoadBalancer()
testK8sService := &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Name: testK8sServiceName,
Expand All @@ -354,7 +354,7 @@ func TestEnsureLoadBalancer_DeleteLB(t *testing.T) {
err := lb.EnsureLoadBalancerDeleted(context.Background(), testClustername, testK8sService)
assert.NoError(t, err)

fcw.DynamicClient.PrependReactor("delete", "virtualmachineservices", testCase.deleteFunc)
fc.PrependReactor("delete", "virtualmachineservices", testCase.deleteFunc)

err = lb.EnsureLoadBalancerDeleted(context.Background(), "test", testK8sService)
if err != nil {
Expand Down
8 changes: 4 additions & 4 deletions pkg/cloudprovider/vsphereparavirtual/vmoperator.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (

// discoverNodeByProviderID takes a ProviderID and returns a VirtualMachine if one exists, or nil otherwise
// VirtualMachine not found is not an error
func discoverNodeByProviderID(ctx context.Context, providerID string, namespace string, vmClient vmop.V1alpha1Interface) (*vmopv1alpha1.VirtualMachine, error) {
func discoverNodeByProviderID(ctx context.Context, providerID string, namespace string, vmClient vmop.Interface) (*vmopv1alpha1.VirtualMachine, error) {
var discoveredNode *vmopv1alpha1.VirtualMachine = nil

// Adding Retry here because there is no retry in caller from node controller
Expand All @@ -24,7 +24,7 @@ func discoverNodeByProviderID(ctx context.Context, providerID string, namespace
checkError,
func() error {
uuid := GetUUIDFromProviderID(providerID)
vms, err := vmClient.VirtualMachines(namespace).List(ctx, metav1.ListOptions{})
vms, err := vmClient.V1alpha1().VirtualMachines(namespace).List(ctx, metav1.ListOptions{})
if err != nil {
return err
}
Expand All @@ -44,7 +44,7 @@ func discoverNodeByProviderID(ctx context.Context, providerID string, namespace

// discoverNodeByName takes a node name and returns a VirtualMachine if one exists, or nil otherwise
// VirtualMachine not found is not an error
func discoverNodeByName(ctx context.Context, name types.NodeName, namespace string, vmClient vmop.V1alpha1Interface) (*vmopv1alpha1.VirtualMachine, error) {
func discoverNodeByName(ctx context.Context, name types.NodeName, namespace string, vmClient vmop.Interface) (*vmopv1alpha1.VirtualMachine, error) {
var discoveredNode *vmopv1alpha1.VirtualMachine = nil

// Adding Retry here because there is no retry in caller from node controller
Expand All @@ -53,7 +53,7 @@ func discoverNodeByName(ctx context.Context, name types.NodeName, namespace stri
DiscoverNodeBackoff,
checkError,
func() error {
vm, err := vmClient.VirtualMachines(namespace).Get(ctx, string(name), metav1.GetOptions{})
vm, err := vmClient.V1alpha1().VirtualMachines(namespace).Get(ctx, string(name), metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
return nil
Expand Down
21 changes: 17 additions & 4 deletions pkg/cloudprovider/vsphereparavirtual/vmoperator/client/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,17 @@ var (
}
)

// Clientset contains the clients for groups. Each group has exactly one
// version included in a Clientset.
type Clientset struct {
vmopv1alpha1 *VmoperatorV1alpha1Client
}

// V1alpha1 retrieves the VmoperatorV1alpha1Client
func (c *Clientset) V1alpha1() vmoperator.V1alpha1Interface {
return c.vmopv1alpha1
}

// VmoperatorV1alpha1Client contains the dynamic client for vm operator group
type VmoperatorV1alpha1Client struct {
dynamicClient *dynamic.DynamicClient
Expand All @@ -50,7 +61,7 @@ func (c *VmoperatorV1alpha1Client) Client() dynamic.Interface {
}

// NewForConfig creates a new client for the given config.
func NewForConfig(c *rest.Config) (*VmoperatorV1alpha1Client, error) {
func NewForConfig(c *rest.Config) (*Clientset, error) {
scheme := runtime.NewScheme()
_ = vmopv1alpha1.AddToScheme(scheme)

Expand All @@ -59,8 +70,10 @@ func NewForConfig(c *rest.Config) (*VmoperatorV1alpha1Client, error) {
return nil, err
}

client := &VmoperatorV1alpha1Client{
dynamicClient: dynamicClient,
clientSet := &Clientset{
vmopv1alpha1: &VmoperatorV1alpha1Client{
dynamicClient: dynamicClient,
},
}
return client, nil
return clientSet, nil
}
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,32 @@ import (
"k8s.io/cloud-provider-vsphere/pkg/cloudprovider/vsphereparavirtual/vmoperator"
)

// FakeClientSet contains the fake clients for groups. Each group has exactly one
// version included in a Clientset.
type FakeClientSet struct {
FakeClient *FakeClient
}

// V1alpha1 retrieves the fake VmoperatorV1alpha1Client
func (c *FakeClientSet) V1alpha1() vmoperator.V1alpha1Interface {
return c.FakeClient
}

// NewFakeClientSet creates a FakeClientWrapper
func NewFakeClientSet(fakeClient *dynamicfake.FakeDynamicClient) *FakeClientSet {
fcw := &FakeClientSet{
FakeClient: &FakeClient{
DynamicClient: fakeClient,
},
}
return fcw
}

// FakeClient contains the fake dynamic client for vm operator group
type FakeClient struct {
DynamicClient *dynamicfake.FakeDynamicClient
}

// NewFakeClient creates a FakeClientWrapper
func NewFakeClient(fakeClient *dynamicfake.FakeDynamicClient) *FakeClient {
fcw := FakeClient{}
fcw.DynamicClient = fakeClient
return &fcw
}

// VirtualMachines retrieves the virtualmachine client
func (c *FakeClient) VirtualMachines(namespace string) vmoperator.VirtualMachineInterface {
return newVirtualMachines(c, namespace)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func initVMTest() (*virtualMachines, *dynamicfake.FakeDynamicClient) {
scheme := runtime.NewScheme()
_ = vmopv1alpha1.AddToScheme(scheme)
fc := dynamicfake.NewSimpleDynamicClient(scheme)
vms := newVirtualMachines(NewFakeClient(fc), "test-ns")
vms := newVirtualMachines(NewFakeClientSet(fc).V1alpha1(), "test-ns")
return vms, fc
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ func initVMServiceTest() (*virtualMachineServices, *dynamicfake.FakeDynamicClien
scheme := runtime.NewScheme()
_ = vmopv1alpha1.AddToScheme(scheme)
fc := dynamicfake.NewSimpleDynamicClient(scheme)
vms := newVirtualMachineServices(NewFakeClient(fc), "test-ns")
vms := newVirtualMachineServices(NewFakeClientSet(fc).V1alpha1(), "test-ns")
return vms, fc
}

Expand Down
5 changes: 5 additions & 0 deletions pkg/cloudprovider/vsphereparavirtual/vmoperator/interface.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ import (
vmopv1alpha1 "github.com/vmware-tanzu/vm-operator-api/api/v1alpha1"
)

// Interface has methods to work with Vmoperator resources.
type Interface interface {
V1alpha1() V1alpha1Interface
}

// V1alpha1Interface has methods to work with Vmoperator V1alpha1 resources.
type V1alpha1Interface interface {
Client() dynamic.Interface
Expand Down
2 changes: 1 addition & 1 deletion pkg/cloudprovider/vsphereparavirtual/vmservice/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ type VMService interface {

// vmService takes care of mapping of LB type of service to VM service in supervisor cluster
type vmService struct {
vmClient vmop.V1alpha1Interface
vmClient vmop.Interface
namespace string
ownerReference *metav1.OwnerReference
}
12 changes: 6 additions & 6 deletions pkg/cloudprovider/vsphereparavirtual/vmservice/vmservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,12 @@ var (

// GetVmopClient gets a vm-operator-api client
// This is separate from NewVMService so that a fake client can be injected for testing
func GetVmopClient(config *rest.Config) (vmop.V1alpha1Interface, error) {
func GetVmopClient(config *rest.Config) (vmop.Interface, error) {
return vmopclient.NewForConfig(config)
}

// NewVMService creates a vmService object
func NewVMService(vmClient vmop.V1alpha1Interface, ns string, ownerRef *metav1.OwnerReference) VMService {
func NewVMService(vmClient vmop.Interface, ns string, ownerRef *metav1.OwnerReference) VMService {
return &vmService{
vmClient: vmClient,
namespace: ns,
Expand Down Expand Up @@ -129,7 +129,7 @@ func (s *vmService) Get(ctx context.Context, service *v1.Service, clusterName st
logger := log.WithValues("name", service.Name, "namespace", service.Namespace)
logger.V(2).Info("Attempting to get VirtualMachineService")

vmService, err := s.vmClient.VirtualMachineServices(s.namespace).Get(ctx, s.GetVMServiceName(service, clusterName), metav1.GetOptions{})
vmService, err := s.vmClient.V1alpha1().VirtualMachineServices(s.namespace).Get(ctx, s.GetVMServiceName(service, clusterName), metav1.GetOptions{})
if err != nil {
if apierrors.IsNotFound(err) {
return nil, nil
Expand All @@ -152,7 +152,7 @@ func (s *vmService) Create(ctx context.Context, service *v1.Service, clusterName
return nil, err
}

vmService, err = s.vmClient.VirtualMachineServices(s.namespace).Create(ctx, vmService, metav1.CreateOptions{})
vmService, err = s.vmClient.V1alpha1().VirtualMachineServices(s.namespace).Create(ctx, vmService, metav1.CreateOptions{})
if err != nil {
logger.Error(ErrCreateVMService, fmt.Sprintf("%v", err))
return nil, err
Expand Down Expand Up @@ -250,7 +250,7 @@ func (s *vmService) Update(ctx context.Context, service *v1.Service, clusterName
}

if needsUpdate {
newVMService, err = s.vmClient.VirtualMachineServices(s.namespace).Update(ctx, newVMService, metav1.UpdateOptions{})
newVMService, err = s.vmClient.V1alpha1().VirtualMachineServices(s.namespace).Update(ctx, newVMService, metav1.UpdateOptions{})
if err != nil {
logger.Error(ErrUpdateVMService, fmt.Sprintf("%v", err))
return nil, err
Expand All @@ -268,7 +268,7 @@ func (s *vmService) Delete(ctx context.Context, service *v1.Service, clusterName
logger := log.WithValues("name", service.Name, "namespace", service.Namespace)
logger.V(2).Info("Attempting to delete VirtualMachineService")

err := s.vmClient.VirtualMachineServices(s.namespace).Delete(ctx, s.GetVMServiceName(service, clusterName), metav1.DeleteOptions{})
err := s.vmClient.V1alpha1().VirtualMachineServices(s.namespace).Delete(ctx, s.GetVMServiceName(service, clusterName), metav1.DeleteOptions{})
if err != nil {
logger.Error(ErrDeleteVMService, fmt.Sprintf("%v", err))
return err
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func initTest() (*v1.Service, VMService, *dynamicfake.FakeDynamicClient) {
scheme := runtime.NewScheme()
_ = vmopv1alpha1.AddToScheme(scheme)
fc := dynamicfake.NewSimpleDynamicClient(scheme)
vms = NewVMService(vmopclient.NewFakeClient(fc), testClusterNameSpace, &testOwnerReference)
vms = NewVMService(vmopclient.NewFakeClientSet(fc), testClusterNameSpace, &testOwnerReference)
return testK8sService, vms, fc
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/cloudprovider/vsphereparavirtual/zone.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
)

type zones struct {
vmClient vmop.V1alpha1Interface
vmClient vmop.Interface
namespace string
}

Expand Down
4 changes: 2 additions & 2 deletions pkg/cloudprovider/vsphereparavirtual/zone_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,12 @@ func initVMopClient(testVM *vmopv1alpha1.VirtualMachine) (zones, *dynamicfake.Fa
scheme := runtime.NewScheme()
_ = vmopv1alpha1.AddToScheme(scheme)
fc := dynamicfake.NewSimpleDynamicClient(scheme)
fcw := vmopclient.NewFakeClient(fc)
fcw := vmopclient.NewFakeClientSet(fc)
zone := zones{
vmClient: fcw,
namespace: testClusterNameSpace,
}
_, err := fcw.VirtualMachines(testVM.Namespace).Create(context.TODO(), testVM, metav1.CreateOptions{})
_, err := fcw.V1alpha1().VirtualMachines(testVM.Namespace).Create(context.TODO(), testVM, metav1.CreateOptions{})
return zone, fc, err
}

Expand Down

0 comments on commit d861bcd

Please sign in to comment.