Skip to content

fix: Dynamic GPU partitioning lacks single-GPU-level granularity. (#1… - #1061

Merged
archlitchi merged 1 commit into
Project-HAMi:masterfrom
Goend:master
May 26, 2025
Merged

fix: Dynamic GPU partitioning lacks single-GPU-level granularity. (#1…#1061
archlitchi merged 1 commit into
Project-HAMi:masterfrom
Goend:master

Conversation

@Goend

@Goend Goend commented May 19, 2025

Copy link
Copy Markdown
Contributor

…054)

What type of PR is this?

What this PR does / why we need it:
fix: Dynamic GPU partitioning lacks single-GPU-level granularity. (#1

Which issue(s) this PR fixes:
Fixes #1054

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

@github-actions github-actions Bot added the kind/bug Something isn't working label May 19, 2025
@archlitchi

Copy link
Copy Markdown
Member

/assign @ouyangluwei163


if len(migConfigs["current"]) == 1 && len(migConfigs["current"][0].Devices) == 0 {
for i := 0; i < deviceCount; i++ {
config := migConfigs["current"][0]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is the intention here to perform a deep copy of the map type instead of using a reference?

@ouyangluwei163 ouyangluwei163 May 20, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

yes, if multiple cards in the machine are one flavor.

version: v1
mig-configs:
  current:
  - devices: [0, 1, 2]
    mig-enabled: true
    mig-devices:
      1g.5gb: 1
      2g.10gb: 3

and if not deep copy, when device 0 is modified, devices 1/2 will also be modified

@Goend Goend May 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done.the deepCopyMigConfig function has been used to perform a deep copy

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

great, I will test it later

return result, nil
}

for i := 0; i < deviceCount; i++ {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Here you can simplify, 692-715

		for i := 0; i < deviceCount; i++ {
			var found bool
			for idx, config := range migConfigs["current"] {
				if len(config.Devices) == 0 || containsDevice(i, config.Devices) {
					newConfig := migConfigs["current"][idx]
					newConfig.MigDevices = make(map[string]int32)
					for k, v := range migConfigs["current"][idx].MigDevices {
						newConfig.MigDevices[k] = v
					}
					newConfig.Devices = []int32{int32(i)}
					result = append(result, newConfig)
					found = true
					break
				}
			}
			if !found {
				return nil, fmt.Errorf("device %d does not match any MIG configuration", i)
			}
		}

@Goend Goend May 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done,use additional auxiliary structures

deviceToConfig := make(map[int32]*nvidia.MigConfigSpec)

to reduce the number of for loop layers

deviceIndex := int32(i)
config, exists := deviceToConfig[deviceIndex]
if !exists {
return nil, fmt.Errorf("device %d does not match any MIG configuration", i)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The error here does not need to be handled. If the corresponding device configuration is not found, simply continue and issue a warning. There is no need for
return nil, fmt.Errorf("device %d does not match any MIG configuration", i)
instead, just use continue. and return other config
such as return config,fmt.Errorf("device %d does not match any MIG configuration", i)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ouyangluwei163 What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's OK

@archlitchi

Copy link
Copy Markdown
Member

@ouyangluwei163 ready to integrate?

@Goend

Goend commented May 23, 2025

Copy link
Copy Markdown
Contributor Author

when signal gpu in node,the generate config is ok

I0523 07:36:58.771205   58171 util.go:204] Applying data= version: v1
mig-configs:
    current:
        - devices: [0]
          mig-enabled: true
          mig-devices:
            1g.5gb: 7

F0523 07:36:59.384536   58171 util.go:212] nvidia-mig-parted failed with exit status 1
[root@node-37 ~]# kubectl  -n hami  logs hami-device-plugin-42wt2 --previous|grep test
Defaulted container "device-plugin" out of: device-plugin, vgpu-monitor
I0523 07:36:52.418380   58171 server.go:241] test

@ouyangluwei163

Copy link
Copy Markdown
Contributor

/lgtm

@archlitchi

Copy link
Copy Markdown
Member

/lgtm

@codecov

codecov Bot commented May 26, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Flag Coverage Δ
unittests 61.14% <ø> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

see 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Something isn't working lgtm

Projects

None yet

Development

Successfully merging this pull request may close these issues.

the division of mig-gpus is confusing when using dynamic-mig

3 participants