Fix Config Daemon node selector

[wizhaoredhat]

When node selectors are added then removed via "configDaemonNodeSelector" via the sriovoperatorconfigs CRD, certain combinations will not trigger an update due to the ordering of arguments into "DeepDerivative" from the reflect library.

This is an example combination of setting "configDaemonNodeSelector" that would make it such that the sriov-config-daemon daemon set's nodeSelector to become out of sync with the original "DeepDerivative" argument order:

Step 1) Create 3 labels in node selector:
configDaemonNodeSelector = {"labelA": "", "labelB": "", "labelC": ""}
config-daemon DS NodeSelector = {"labelA": "", "labelB": "", "labelC": ""}

Step 2) Remove 1 label in node selector without making changes to the other labels:
configDaemonNodeSelector = {"labelA": "", "labelB": ""}
config-daemon DS NodeSelector = {"labelA": "", "labelB": "", "labelC": ""} (Out of Sync)

"DeepDerivative" assumes that the left argument is the original (v1) and the right argument is the updated (v2). For maps it only checks if v1 > v2 in length:

    case reflect.Map:
        ...
        if v1.Len() > v2.Len() {
            return false
        }
        ...

This commit just reverts changes from commit:
"661a65b8e1aee6339037948732f75d06ceb91611"
Use DeepDerivative to compare the kube object content
Such that we react to changes to node selectors properly.

[github-actions]

Thanks for your PR,
To run vendors CIs use one of:

• /test-all: To run all tests for all vendors.
• /test-e2e-all: To run all E2E tests for all vendors.
• /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

• /skip-all: To skip all tests for all vendors.
• /skip-e2e-all: To skip all E2E tests for all vendors.
• /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
  Best regards.

[wizhaoredhat]

/cc @SchSeba
/cc @zeeke
/cc @e0ne
/cc @adrianchiris

PTAL

[coveralls]

Pull Request Test Coverage Report for Build 5731589116

• 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.05%) to 24.5%

---

Totals
Change from base Build 5645891211:	0.05%
Covered Lines:	2081
Relevant Lines:	8494

---

💛 - Coveralls

[wizhaoredhat]

/cc @zshi-redhat

[wizhaoredhat]

@bn222

[SchSeba]

this looks good can you please add a quick unit test to cover this in the future please

[adrianchiris]

maybe the entire use of DeepDerivative is wrong here and we should use different logic to compare objects based on the type.

if you swap the order then:

-we would generally fail to compare resources since resourceVersion will not be ignored (since they exist in existing obj but not in obj from manifest)

• we would fail if exsiting service account have secrets attached by k8s (since object from manifest do not have these secrets)
• we would fail if labels are added to desired object

now specifically for maps, looking a few lines of code down from what you referenced:

	case reflect.Map:
		if v1.IsNil() || v1.Len() == 0 {
			return true
		}
		if v1.Len() > v2.Len() {
			return false
		}
		if v1.Pointer() == v2.Pointer() {
			return true
		}
		for _, k := range v1.MapKeys() {
			if !e.deepValueDerive(v1.MapIndex(k), v2.MapIndex(k), visited, depth+1) {
				return false
			}
		}

we can see it will only compare keys from v1 which after your change is the existing object
that means new labels will be ignored in the comparison

[wizhaoredhat]

maybe the entire use of DeepDerivative is wrong here and we should use different logic to compare objects based on the type.

@adrianchiris

Perhaps we should do DeepDerivative both ways(DeepDerivative(exisiting,obj) || DeepDerivative(obj, existing))? Wondering if that solves the issue or introduces new ones.

[bn222]

I think DeepDerivate is actually meant for a different use case than what we want. Do we want DeepEqual from reflect instead?

Your approach of checking in a symmetrical way(in the mathematical sense) seems to be getting us close to DeepEqual but in a way that's twice as slow.

[wizhaoredhat]

I think DeepDerivate is actually meant for a different use case than what we want. Do we want DeepEqual from reflect instead?

Your approach of checking in a symmetrical way(in the mathematical sense) seems to be getting us close to DeepEqual but in a way that's twice as slow.

FYI For Reference: 661a65b

[bn222]

I've looked into this more deeply

I'm in favor of removing the "get + compare" portion of the update. We do not gain anything from that. If the argument is that "update" is causing a call to the api server, then the counterargument is that the "get" isn't for free either. In addition, as evidenced by this PR, the compare part is far from trivial.

[github-actions]

Thanks for your PR,
To run vendors CIs use one of:

• /test-all: To run all tests for all vendors.
• /test-e2e-all: To run all E2E tests for all vendors.
• /test-e2e-nvidia-all: To run all E2E tests for NVIDIA vendor.

To skip the vendors CIs use one of:

• /skip-all: To skip all tests for all vendors.
• /skip-e2e-all: To skip all E2E tests for all vendors.
• /skip-e2e-nvidia-all: To skip all E2E tests for NVIDIA vendor.
  Best regards.

[zeeke]

LGTM

[wizhaoredhat]

@adrianchiris @e0ne Please take a look when you have a chance. Thanks!

[SchSeba]

LGTM