Use cmp.Diff() replace reflect and diagnosis

[boenn]

What type of PR is this?

/kind design

What this PR does / why we need it:

Which issue(s) this PR fixes:

Part of #94696

Special notes for your reviewer:

Use cmp.Diff() replace reflect and diagnosis

Does this PR introduce a user-facing change?

none

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

/sig scheduling
/assign @ahg-g

[k8s-ci-robot]

@boenn: 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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[k8s-ci-robot]

Hi @boenn. 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.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[chendave]

/ok-to-test

[boenn]

/retest

[chendave]

/remove-kind design
/kind cleanup

[chendave]

changes looks good to me, @Huang-Wei do you aware of any negative impact of this change?

[ahg-g]

No need for the if statement

[boenn]

the code in here
so there will be no inconsistency between Status.failedPlugin and UnschedulablePlugins. If you manually modify all these fields like:

expected := framework.Diagnosis{
		NodeToStatusMap: framework.NodeToStatusMap{
			"1": framework.NewStatus(framework.Unschedulable, st.ErrReasonFake).WithFailedPlugin("AnotherFilter"),
			"2": framework.NewStatus(framework.Unschedulable, st.ErrReasonFake).WithFailedPlugin("AnotherFilter"),
			"3": framework.NewStatus(framework.Unschedulable, st.ErrReasonFake).WithFailedPlugin("AnotherFilter"),
		},
		UnschedulablePlugins: sets.NewString("AnotherFilter"),
	}

cmp.Diff is possible, and do not need to modify Equal.

@chendave

[Huang-Wei]

changes looks good to me, @Huang-Wei do you aware of any negative impact of this change?

when I reviewed this PR, this code wasn't there... And the latest code also removed it.

[chendave]

so there will be no inconsistency between Status.failedPlugin and UnschedulablePlugins. If you manually modify all these fields like:

Not follow here, shouldn't cmp.Diff expect to compare each of the fields here? Suppose diagnosis return struct where the field plugin is MatchFilter, and expected is defined as FakeFilter but the test pass, does that make sense?

[chendave]

@Huang-Wei , code is here, not sure why the code is reverted by boenn.

kubernetes/pkg/scheduler/framework/interface.go

Lines 214 to 225 in e326c00

	func (s *Status) Equal(x *Status) bool {
	if s == nil || x == nil {
	return s.IsSuccess() && x.IsSuccess()
	}
	if s.code != x.code {
	return false
	}
	if s.code == Error {
	return cmp.Equal(s.err, x.err, cmpopts.EquateErrors())
	}
	return cmp.Equal(s.reasons, x.reasons)
	}

[boenn]

Sorry I modified the code without explanation. I'm not sure that it is necessary to verify the failedPlugin, because from the code I said above, it will be locked when the failedPlugin is modified, and the result of manual modification may never appear. This part of the check is redundant.And..here is the code：

func (s *Status) Equal(x *Status) bool { 
  if s == nil || x == nil { 
   return s.IsSuccess() && x.IsSuccess() 
  } 
  if s.code != x.code { 
   return false 
  } 
  if s.code == Error { 
   return cmp.Equal(s.err, x.err, cmpopts.EquateErrors()) 
  } 
  if s.failedPlugin != "" || x.failedPlugin != "" {
  return s.failedPlugin == x.failedPlugin
  }
  return cmp.Equal(s.reasons, x.reasons) 
 } 

@Huang-Wei

[chendave]

/retest

[chendave]

Fixes #94696

should be part of the issue, or else it will close that entirely.

[Huang-Wei]

@boenn please squash the commits, and reword Fixes #94696 to Part of #94696 in the PR description.

[boenn]

@boenn please squash the commits, and reword Fixes #94696 to Part of #94696 in the PR description.

fine, thanks for your guidance..

[boenn]

/retest

[boenn]

/retest

[chendave]

https://github.com/kubernetes/kubernetes/pull/103508/files#r665007443 still stands, +1 to your last version where the method Equal of the Status should be updated.

[Huang-Wei]

/lgtm
/approve
/retest

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: boenn, Huang-Wei

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

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/scheduler/OWNERS [Huang-Wei]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[Huang-Wei]

/retest

[Huang-Wei]

/retest

[puerco]

/milestone v1.22

[boenn]

I understand what you mean, but here I think adding check failedPlugin is redundant, because findNodesThatFitPod doen't have this problem. Maybe it is better to add check failedPlugin and its corresponding test case in another PR.I will continue to think about this problem，thank you。 @chendave