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

vhost-vdpa: fix env val not show #524

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

Conversation

makoto126
Copy link

func (rs *resourceServer) Allocate(ctx context.Context, rqt *pluginapi.AllocateRequest) (*pluginapi.AllocateResponse, error) {
	glog.Infof("Allocate() called with %+v", rqt)
	resp := new(pluginapi.AllocateResponse)

	for _, container := range rqt.ContainerRequests {
		containerResp := new(pluginapi.ContainerAllocateResponse)

		envs, err := rs.getEnvs(container.DevicesIDs)
		if err != nil {
			glog.Errorf("failed to get environment variables for device IDs %v: %v", container.DevicesIDs, err)
			return nil, err
		}

		if rs.useCdi {
			containerResp.Annotations, err = rs.cdi.CreateContainerAnnotations(
				container.DevicesIDs, rs.resourceNamePrefix, rs.resourcePool.GetCDIName())
			if err != nil {
				return nil, fmt.Errorf("can't create container annotation: %s", err)
			}
		} else {
			containerResp.Devices = rs.resourcePool.GetDeviceSpecs(container.DevicesIDs)
			containerResp.Mounts = rs.resourcePool.GetMounts(container.DevicesIDs)
		}

		containerResp.Envs = envs
		resp.ContainerResponses = append(resp.ContainerResponses, containerResp)
	}
	glog.Infof("AllocateResponse send: %+v", resp)
	return resp, nil
}

In the Allocate implementation, getEnvs comes first and GetDeviceSpecs comes second.
But the GetEnvVal implementation of vdpaInfoProvider has vdpaPath assigned after the GetDeviceSpecs call.

// GetDeviceSpecs returns the DeviceSpec slice
func (vip *vdpaInfoProvider) GetDeviceSpecs() []*pluginapi.DeviceSpec {
	if healthy, err := vip.isHealthy(); !healthy {
		glog.Errorf("GetDeviceSpecs(): vDPA is required in the configuration but device does not have a healthy vdpa device: %s",
			err)
		return nil
	}
	devSpecs := make([]*pluginapi.DeviceSpec, 0)

	// DeviceSpecs only required for vhost vdpa type as the
	if vip.vdpaType == types.VdpaVhostType {
		vdpaPath, err := vip.dev.GetPath()
		if err != nil {
			glog.Errorf("Unexpected error when fetching the vdpa device path: %s", err)
			return nil
		}
		devSpecs = append(devSpecs, &pluginapi.DeviceSpec{
			HostPath:      vdpaPath,
			ContainerPath: vdpaPath,
			Permissions:   "rw",
		})
		vip.vdpaPath = vdpaPath
	}
	return devSpecs
}

// GetEnvVal returns the environment variable value
func (vip *vdpaInfoProvider) GetEnvVal() types.AdditionalInfo {
	envs := make(map[string]string, 0)
	if vip.vdpaPath != "" {
		envs["mount"] = vip.vdpaPath
	}

	return envs
}

This causes the vDPA device to be missing this environment variable the first time it is assigned, as follows:
PCIDEVICE_JAGUARMICRO_COM_JAGUAR_INFO={"0000:68:00.1":{"generic":{"deviceID":"0000:68:00.1"},"vdpa":{}}}
This can sometimes occur if multiple vDPA devices are requested(0000:68:00.1 has already been allocated once and 0000:68:00.2 is the first allocation):
PCIDEVICE_JAGUARMICRO_COM_JAGUAR_INFO={"0000:68:00.1":{"generic":{"deviceID":"0000:68:00.1"},"vdpa":{"mount":"/dev/vhost-vdpa-0"}},"0000:68:00.2":{"generic":{"deviceID":"0000:68:00.2"},"vdpa":{}}}

This PR solves the above problem.

@SchSeba
Copy link
Collaborator

SchSeba commented Feb 12, 2024

@lmilleri can you please take a look on this one?

@lmilleri
Copy link
Contributor

Well spotted, it looks good to me

Copy link
Collaborator

@SchSeba SchSeba left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice work!

@makoto126
Copy link
Author

/retest

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 7953738506

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • -2 of 9 (77.78%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.03%) to 75.192%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pkg/infoprovider/vdpaInfoProvider.go 7 9 77.78%
Totals Coverage Status
Change from base Build 7947730362: 0.03%
Covered Lines: 2052
Relevant Lines: 2729

💛 - Coveralls

@makoto126
Copy link
Author

@SchSeba Mellanox CI shows that ERROR: Error cloning remote repo 'SRIOV_NETWORK_DEVICE_PLUGIN',Can you tell me what the problem is?
I have tested this one in my cluster. works well.

@SchSeba
Copy link
Collaborator

SchSeba commented Mar 10, 2024

That is fine I think the MLX CI is not working right now.

@e0ne can you please review this PR we need one more maintainer to approve before we can merge this PR
Thanks!

Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you modify unit-test to make sure getEnvVal does not depend on getDeviceSpecs ?[1]

[1]

It("should return object with the mounts", func() {

@makoto126
Copy link
Author

@adrianchiris thanks for your advice. Passed the test after removing dip.GetDeviceSpecs().

@makoto126 makoto126 closed this Jul 15, 2024
@makoto126 makoto126 reopened this Jul 15, 2024
@SchSeba
Copy link
Collaborator

SchSeba commented Aug 20, 2024

/test

@SchSeba
Copy link
Collaborator

SchSeba commented Aug 20, 2024

Hi @makoto126,
can you do a force push to trigger the CI system.

Thanks!

vdpaPath is now assigned a value in NewVdpaInfoProvider.

Signed-off-by: Ziteng Liu <ziteng.liu@jaguarmicro.com>
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 this pull request may close these issues.

None yet

5 participants