This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Hi @chuangw6. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

@tallclair do you mind stamping ok-to-test

/assign @tallclair

/ok-to-test

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chuangw6
Once this PR has been reviewed and has the lgtm label, please ask for approval from tallclair. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

@chuangw6 Thank you for your PR! Can you try to get rid of remaining context.Background and context.TODO calls where possible?

$ git grep 'context\.\(TODO\|Background\)' ./pkg/kubelet/cm/devicemanager/
pkg/kubelet/cm/devicemanager/endpoint.go:       return e.api.GetPreferredAllocation(context.Background(), &pluginapi.PreferredAllocationRequest{
pkg/kubelet/cm/devicemanager/endpoint.go:       return e.api.Allocate(context.Background(), &pluginapi.AllocateRequest{
pkg/kubelet/cm/devicemanager/endpoint.go:       ctx, cancel := context.WithTimeout(context.Background(), pluginapi.KubeletPreStartContainerRPCTimeoutInSecs*time.Second)
pkg/kubelet/cm/devicemanager/manager.go:        options, err := p.API().GetDevicePluginOptions(context.Background(), &pluginapi.Empty{})
pkg/kubelet/cm/devicemanager/plugin/v1beta1/client.go:  stream, err := c.client.ListAndWatch(context.Background(), &api.Empty{})
pkg/kubelet/cm/devicemanager/plugin/v1beta1/client.go:  ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
pkg/kubelet/cm/devicemanager/plugin/v1beta1/stub.go:                            err := wait.PollUntilContextTimeout(context.Background(), 10*time.Second, 2*time.Minute, false, func(context.Context) (done bool, err error) {
pkg/kubelet/cm/devicemanager/plugin/v1beta1/stub.go:    ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
pkg/kubelet/cm/devicemanager/plugin/v1beta1/stub.go:    _, err = client.Register(context.Background(), reqt)
pkg/kubelet/cm/devicemanager/topology_hints.go: logger := klog.FromContext(context.TODO())
pkg/kubelet/cm/devicemanager/topology_hints.go: logger := klog.FromContext(context.TODO())

@bart0sh Thanks for review.

This PR is focused on pkg/kubelet/cm/devicemanager/topology_hints.go. I can try to do it in separate PR for other files under device manager.

Also note context.TODO() is explicitly used in topology_hints.go to avoid plumbing through ctx all over the places outside devicemanager because of the use of interface.

pkg/kubelet/cm/devicemanager/topology_hints.go: logger := klog.FromContext(context.TODO())
pkg/kubelet/cm/devicemanager/topology_hints.go: logger := klog.FromContext(context.TODO())

/test pull-kubernetes-unit-windows-master

This PR is focused on pkg/kubelet/cm/devicemanager/topology_hints.go. I can try to do it in separate PR for other files under device manager.

That would complicate review and approval and cause conflicts even in your future PRs for devicemanager. I'd suggest expanding the scope to at least one component. If you think device manager is too big for one PR, that's OK. You can start from it's sub-components, for example from pkg/kubelet/cm/devicemanager/plugin.

This PR is focused on pkg/kubelet/cm/devicemanager/topology_hints.go. I can try to do it in separate PR for other files under device manager.

That would complicate review and approval and cause conflicts even in your future PRs for devicemanager. I'd suggest expanding the scope to at least one component. If you think device manager is too big for one PR, that's OK. You can start from it's sub-components, for example from pkg/kubelet/cm/devicemanager/plugin.

I understand the struggle to balance the scope and to keep the manageable size. I'm also concerned about cascading changes to the generic plugin manager (see 1/5 in the series)