Skip to content
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.

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

Need to add PVM instance id as part of node service #35

Closed
Madhan-SWE opened this issue Dec 8, 2021 · 1 comment · Fixed by #36
Closed

Need to add PVM instance id as part of node service #35

Madhan-SWE opened this issue Dec 8, 2021 · 1 comment · Fixed by #36
Assignees

Comments

@Madhan-SWE
Copy link

Madhan-SWE commented Dec 8, 2021

Metadata service should be called only when calling newNodeService.
But in our current implementation, metadata service is called in NodeGetInfo Method inorder to get the pvmInstanceId.

func (d *nodeService) NodeGetInfo(ctx context.Context, req *csi.NodeGetInfoRequest) (*csi.NodeGetInfoResponse, error) {
	klog.V(4).Infof("NodeGetInfo: called with args %+v", *req)
	metadata, err := cloud.NewMetadataService(cloud.DefaultKubernetesAPIClient)
	if err != nil {
		panic(err)
	}
	pvmInstanceId := metadata.GetPvmInstanceId()

	in, err := d.cloud.GetPVMInstanceByID(pvmInstanceId)
       ....
       ....
}

While unit testing NodeGetInfo method, the test case will fail as if it is not running inside a pod.

This can be optimised by adding pvmInstanceId as part of NodeService when it is initialised as below.

func newNodeService(driverOptions *Options) nodeService {
	klog.V(4).Infof("retrieving node info from metadata service")
	....
        ....
        ....
	return nodeService{
		cloud:         pvsCloud,
		mounter:       newNodeMounter(),
		driverOptions: driverOptions,
		pvmInstanceId: metadata.GetCloudInstanceId(),
	}
}

This also allows our unit tests to run anywhere(could be out side cluster, inside cluster node).
If the metadata is other than service initialisation then we can run the unit tests only inside the pod as the metadata service uses in cluster configuration.

Referred EFS csi driver code for following this approach.

@Madhan-SWE
Copy link
Author

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant