Skip to content

🐛 Remove direct use of logf.Log #1494

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

Open
bryanv wants to merge 1 commit into
base: main
Choose a base branch
from
Open

🐛 Remove direct use of logf.Log #1494

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
7 changes: 3 additions & 4 deletions pkg/providers/vsphere/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,12 @@ import (
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
logf "sigs.k8s.io/controller-runtime/pkg/log"

pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config"
pkglog "github.com/vmware-tanzu/vm-operator/pkg/log"
"github.com/vmware-tanzu/vm-operator/pkg/providers/vsphere/credentials"
)

var log = logf.Log.WithName("vsphere").WithName("config")

// VSphereVMProviderConfig represents the configuration for a Vsphere VM Provider instance.
// Contains information enabling integration with a backend vSphere instance for VM management.
type VSphereVMProviderConfig struct {
Expand Down Expand Up @@ -242,7 +240,8 @@ func UpdateVcInConfigMap(ctx context.Context, client ctrlclient.Client, vcPNID,

err = client.Patch(ctx, configMap, ctrlclient.MergeFrom(origConfigMap))
if err != nil {
log.Error(err, "Failed to update provider ConfigMap", "configMapName", configMap.Name)
pkglog.FromContextOrDefault(ctx).Error(err, "Failed to update provider ConfigMap",
"configMapName", configMap.Name)
return false, err
}

Expand Down
8 changes: 1 addition & 7 deletions pkg/providers/vsphere/contentlibrary/content_library.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,5 @@
// © Broadcom. All Rights Reserved.
// The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries.
// The term "Broadcom" refers to Broadcom Inc. and/or its subsidiaries.
// SPDX-License-Identifier: Apache-2.0

package contentlibrary

import (
logf "sigs.k8s.io/controller-runtime/pkg/log"
)

var log = logf.Log.WithName("vsphere").WithName("contentlibrary")
31 changes: 17 additions & 14 deletions pkg/providers/vsphere/contentlibrary/content_library_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,18 +13,18 @@ import (
"path/filepath"
"time"

apierrorsutil "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/wait"

"github.com/go-logr/logr"
"github.com/vmware/govmomi/object"
"github.com/vmware/govmomi/ovf"
"github.com/vmware/govmomi/vapi/library"
"github.com/vmware/govmomi/vapi/library/finder"
"github.com/vmware/govmomi/vapi/rest"
"github.com/vmware/govmomi/vim25/soap"
apierrorsutil "k8s.io/apimachinery/pkg/util/errors"
"k8s.io/apimachinery/pkg/util/wait"

pkgcfg "github.com/vmware-tanzu/vm-operator/pkg/config"
pkglog "github.com/vmware-tanzu/vm-operator/pkg/log"
"github.com/vmware-tanzu/vm-operator/pkg/util"
)

Expand Down Expand Up @@ -83,7 +83,7 @@ func NewProviderWithWaitSec(restClient *rest.Client, waitSeconds int) Provider {
}

func (cs *provider) ListLibraryItems(ctx context.Context, libraryUUID string) ([]string, error) {
logger := log.WithValues("libraryUUID", libraryUUID)
logger := pkglog.FromContextOrDefault(ctx).WithValues("libraryUUID", libraryUUID)
itemList, err := cs.libMgr.ListLibraryItems(ctx, libraryUUID)
if err != nil {
if util.IsNotFoundError(err) {
Expand All @@ -96,7 +96,7 @@ func (cs *provider) ListLibraryItems(ctx context.Context, libraryUUID string) ([
}

func (cs *provider) GetLibraryItems(ctx context.Context, libraryUUID string) ([]library.Item, error) {
logger := log.WithValues("libraryUUID", libraryUUID)
logger := pkglog.FromContextOrDefault(ctx).WithValues("libraryUUID", libraryUUID)
itemList, err := cs.libMgr.ListLibraryItems(ctx, libraryUUID)
if err != nil {
if util.IsNotFoundError(err) {
Expand Down Expand Up @@ -164,7 +164,7 @@ func (cs *provider) RetrieveOvfEnvelopeByLibraryItemID(ctx context.Context, item
}

if libItem == nil || libItem.Type != library.ItemTypeOVF {
log.Error(nil, "empty or non OVF library item type, skipping", "itemID", itemID)
pkglog.FromContextOrDefault(ctx).Error(nil, "empty or non OVF library item type, skipping", "itemID", itemID)
// No need to return the error here to avoid unnecessary reconciliation.
return nil, nil
}
Expand All @@ -177,7 +177,7 @@ func readerFromURL(ctx context.Context, c *rest.Client, url *url.URL) (io.ReadCl
readerStream, _, err := c.Download(ctx, url, &p)
if err != nil {
// Log message used by VMC LINT. Refer to before making changes
log.Error(err, "Error occurred when downloading file", "url", url)
pkglog.FromContextOrDefault(ctx).Error(err, "Error occurred when downloading file", "url", url)
return nil, err
}

Expand All @@ -193,7 +193,8 @@ func (cs *provider) RetrieveOvfEnvelopeFromLibraryItem(ctx context.Context, item
return nil, err
}

logger := log.WithValues("sessionID", sessionID, "itemID", item.ID, "itemName", item.Name)
logger := pkglog.FromContextOrDefault(ctx).WithValues("sessionID", sessionID, "itemID", item.ID, "itemName", item.Name)
ctx = logr.NewContext(ctx, logger)
logger.V(4).Info("download session for item created")

defer func() {
Expand All @@ -203,7 +204,7 @@ func (cs *provider) RetrieveOvfEnvelopeFromLibraryItem(ctx context.Context, item
}()

// Download ovf from the library item.
fileURL, err := cs.generateDownloadURLForLibraryItem(ctx, logger, sessionID, item)
fileURL, err := cs.generateDownloadURLForLibraryItem(ctx, sessionID, item)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -231,12 +232,13 @@ func (cs *provider) RetrieveOvfEnvelopeFromLibraryItem(ctx context.Context, item

// UpdateLibraryItem updates the content library item's name and description.
func (cs *provider) UpdateLibraryItem(ctx context.Context, itemID, newName string, newDescription *string) error {
log.Info("Updating Library Item", "itemID", itemID,
logger := pkglog.FromContextOrDefault(ctx)
logger.Info("Updating Library Item", "itemID", itemID,
"newName", newName, "newDescription", newDescription)

item, err := cs.libMgr.GetLibraryItem(ctx, itemID)
if err != nil {
log.Error(err, "error getting library item")
logger.Error(err, "error getting library item")
return err
}

Expand All @@ -262,7 +264,7 @@ func (cs *provider) SyncLibraryItem(

// Only used in testing.
func (cs *provider) CreateLibraryItem(ctx context.Context, libraryItem library.Item, path string) error {
log.Info("Creating Library Item", "item", libraryItem, "path", path)
pkglog.FromContextOrDefault(ctx).Info("Creating Library Item", "item", libraryItem, "path", path)

itemID, err := cs.libMgr.CreateLibraryItem(ctx, libraryItem)
if err != nil {
Expand Down Expand Up @@ -324,10 +326,11 @@ func (cs *provider) CreateLibraryItem(ctx context.Context, libraryItem library.I
// 3. download the file.
func (cs *provider) generateDownloadURLForLibraryItem(
ctx context.Context,
logger logr.Logger,
sessionID string,
item *library.Item) (*url.URL, error) {

logger := pkglog.FromContextOrDefault(ctx).WithValues("sessionID", sessionID)

// List the files available for download in the library item.
files, err := cs.libMgr.ListLibraryItemDownloadSessionFile(ctx, sessionID)
if err != nil {
Expand Down Expand Up @@ -385,7 +388,7 @@ func (cs *provider) generateDownloadURLForLibraryItem(
}

fileURL = info.DownloadEndpoint.URI
log.V(4).Info("Downloaded file", "fileURL", fileURL)
logger.V(4).Info("Downloaded file", "fileURL", fileURL)
return true, nil
})

Expand Down
67 changes: 15 additions & 52 deletions pkg/providers/vsphere/resources/vm.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,9 @@ import (
"context"
"fmt"

"github.com/go-logr/logr"
"github.com/vmware/govmomi/object"
"github.com/vmware/govmomi/vim25/mo"
vimtypes "github.com/vmware/govmomi/vim25/types"
logf "sigs.k8s.io/controller-runtime/pkg/log"

vmopv1 "github.com/vmware-tanzu/vm-operator/api/v1alpha6"
ctxop "github.com/vmware-tanzu/vm-operator/pkg/context/operation"
Expand All @@ -26,16 +24,12 @@ import (
type VirtualMachine struct {
Name string
vcVirtualMachine *object.VirtualMachine
logger logr.Logger
}

var log = logf.Log.WithName("vmresource")

func NewVMFromObject(objVM *object.VirtualMachine) *VirtualMachine {
return &VirtualMachine{
Name: objVM.Name(),
vcVirtualMachine: objVM,
logger: log.WithValues("name", objVM.Name()),
}
}

Expand All @@ -44,7 +38,7 @@ func (vm *VirtualMachine) VcVM() *object.VirtualMachine {
}

func (vm *VirtualMachine) Create(ctx context.Context, folder *object.Folder, pool *object.ResourcePool, vmSpec *vimtypes.VirtualMachineConfigSpec) error {
vm.logger.V(5).Info("Create VM")
pkglog.FromContextOrDefault(ctx).V(5).Info("Create VM")

if vm.vcVirtualMachine != nil {
return fmt.Errorf("failed to create VM %q because the VM object is already set", vm.Name)
Expand All @@ -65,7 +59,7 @@ func (vm *VirtualMachine) Create(ctx context.Context, folder *object.Folder, poo
}

func (vm *VirtualMachine) Clone(ctx context.Context, folder *object.Folder, cloneSpec *vimtypes.VirtualMachineCloneSpec) (*vimtypes.ManagedObjectReference, error) {
vm.logger.V(5).Info("Clone VM")
pkglog.FromContextOrDefault(ctx).V(5).Info("Clone VM")

cloneTask, err := vm.vcVirtualMachine.Clone(ctx, folder, cloneSpec.Config.Name, *cloneSpec)
if err != nil {
Expand Down Expand Up @@ -107,48 +101,13 @@ func (vm *VirtualMachine) GetProperties(ctx context.Context, properties []string
var o mo.VirtualMachine
err := vm.vcVirtualMachine.Properties(ctx, vm.vcVirtualMachine.Reference(), properties, &o)
if err != nil {
vm.logger.Error(err, "Error getting VM properties")
pkglog.FromContextOrDefault(ctx).Error(err, "Error getting VM properties")
return nil, err
}

return &o, nil
}

func (vm *VirtualMachine) ReferenceValue() string {
vm.logger.V(5).Info("Get ReferenceValue")
return vm.vcVirtualMachine.Reference().Value
}

func (vm *VirtualMachine) MoRef() vimtypes.ManagedObjectReference {
vm.logger.V(5).Info("Get MoRef")
return vm.vcVirtualMachine.Reference()
}

func (vm *VirtualMachine) UniqueID(ctx context.Context) (string, error) {
// Notes from Alkesh Shah regarding MoIDs in VC as of 7.0
//
// MoRef IDs are unique within the scope of a single VC. Since Clusters are entities in VCs, the MoRef IDs will be unique across clusters
//
// Identity in VC is derived from a sequence. This ID is used in generating the MoId (or MoRef ID) for the entity in VC. Sequence is monotonically
// increasing and so during regular operation there are no dupes
//
// Backup-Restore: We now make sure that our sequence does not go back in time when restoring from a backup
// ( ) So this
// ensures that after restore we get new MoIds which are never used before… (we advance the sequence counter based on time)
//
// Discovery of VMs: We only use moids from the VM store during restore from a backup. In the unlikely event
// that there are two VMs which are presenting the same MoId, we will regenerate a new MoId based on the current
// sequence. Keep in mind, Ideally the unlikely scenario should not occur as we attempt to tamper proof the MoId
// stored in the VM store ( )
// so two VMs having the same MoId should not happen because they cannot have the same VMX path and we use VMX path
// for ensuring this tamper proof behavior.
//
// Removing from VC and Re-adding the VM to same VC: VM will be given a new MoId (even if the VM is added using
// RegisterVM operation from VC)
// Basically, lifetime of the identity is tied to VC’s knowledge of it’s existence in it’s inventory
return vm.ReferenceValue(), nil
}

var ErrSetPowerState = pkgerr.NoRequeueNoErr("updated power state")

func (vm *VirtualMachine) SetPowerState(
Expand Down Expand Up @@ -190,10 +149,11 @@ func (vm *VirtualMachine) SetPowerState(

// GetVirtualDevices returns the VMs VirtualDeviceList.
func (vm *VirtualMachine) GetVirtualDevices(ctx context.Context) (object.VirtualDeviceList, error) {
vm.logger.V(5).Info("GetVirtualDevices")
logger := pkglog.FromContextOrDefault(ctx)
logger.V(5).Info("GetVirtualDevices")
deviceList, err := vm.vcVirtualMachine.Device(ctx)
if err != nil {
vm.logger.Error(err, "Failed to get devices for VM")
logger.Error(err, "Failed to get devices for VM")
return nil, err
}

Expand All @@ -202,35 +162,38 @@ func (vm *VirtualMachine) GetVirtualDevices(ctx context.Context) (object.Virtual

// GetVirtualDisks returns the list of VMs vmdks.
func (vm *VirtualMachine) GetVirtualDisks(ctx context.Context) (object.VirtualDeviceList, error) {
vm.logger.V(5).Info("GetVirtualDisks")
logger := pkglog.FromContextOrDefault(ctx)
logger.V(5).Info("GetVirtualDisks")
deviceList, err := vm.vcVirtualMachine.Device(ctx)
if err != nil {
vm.logger.Error(err, "Failed to get devices for VM")
logger.Error(err, "Failed to get devices for VM")
return nil, err
}

return deviceList.SelectByType((*vimtypes.VirtualDisk)(nil)), nil
}

func (vm *VirtualMachine) GetNetworkDevices(ctx context.Context) (object.VirtualDeviceList, error) {
vm.logger.V(4).Info("GetNetworkDevices")
logger := pkglog.FromContextOrDefault(ctx)
logger.V(4).Info("GetNetworkDevices")
devices, err := vm.vcVirtualMachine.Device(ctx)
if err != nil {
vm.logger.Error(err, "Failed to get devices for VM")
logger.Error(err, "Failed to get devices for VM")
return nil, err
}

return devices.SelectByType((*vimtypes.VirtualEthernetCard)(nil)), nil
}

func (vm *VirtualMachine) Customize(ctx context.Context, spec vimtypes.CustomizationSpec) error {
vm.logger.V(5).Info("Customize", "spec", spec)
logger := pkglog.FromContextOrDefault(ctx)
logger.V(5).Info("Customize", "spec", spec)

ctxop.MarkUpdate(ctx)

customizeTask, err := vm.vcVirtualMachine.Customize(ctx, spec)
if err != nil {
vm.logger.Error(err, "Failed to customize VM")
logger.Error(err, "Failed to customize VM")
return err
}

Expand Down
Loading