add updateStrategy to daemonSets

[hj-johannes-lee]

closes: #1500

[mythi]

Please review though it is a draft yet!

I'd be very careful adding this to the API. Do you think it's useful? Our priority is to make our operator 'full lifecycle' compliant and if adding safe defaults to deployments/*_plugin/base/intel-*-plugin.yaml is sufficient that's what we should start with.

[hj-johannes-lee]

@mythi I think I still have no idea what makes our operator 'full cycle'..

Umm, the point that I do not understand is:
How can deployments/*_plugin/base/intel-*-plugin.yaml which has nothing to do with operator be related to making our operator full cycle?
I thought what we need to change is the operator part...

[hj-johannes-lee]

Ah do you mean...
I did this

define user configurable updateStrategy settings and add to the operator (to be decided/defined based on the need)

but didn't do this?

enable default strategy in plugin daemonSets

[mythi]

How can deployments/*_plugin/base/intel-*-plugin.yaml which has nothing to do with operator

the operator uses these as the "skeleton" so changes there will add to operator automatically. See https://github.com/intel/intel-device-plugins-for-kubernetes/blob/main/deployments/daemonsets.go and

intel-device-plugins-for-kubernetes/pkg/controllers/qat/controller.go

Line 83 in 9764665

daemonSet := deployments.QATPluginDaemonSet()

[mythi]

Ah do you mean... I did this

define user configurable updateStrategy settings and add to the operator (to be decided/defined based on the need)

but didn't do this?

enable default strategy in plugin daemonSets

yes, and the first part needs more thinking if we really want/need to do it. we cannot let users to set rollingUpdate.maxSurge since only 0 works for the plugins so that'd leave rollingUpdate.maxUnavailable AFAUI.

[hj-johannes-lee]

True. maxSurge should be always 0. Now I see your point.!

Then, if we can make users set only maxUnavailable, would it be good? Or, would we not need it since device plugins have already maxUnavailable value as default value (1)?

[mythi]

Then, if we can make users set only maxUnavailable, would it be good?

my feedback is we don't need it configurable right now but let's prioritize the rest of the capability criteria

[hj-johannes-lee]

sure, then, let me just add other plugins' yamls with controller_test.go updated.

[hj-johannes-lee]

@mythi anything else to do?

[mythi]

The values look fine. I hope this is what the operator sdk capability expects. One minor suggestion.

One other thing: I think your repo fork is broken due to the repo incident we had. Maybe you need to create a new fork and re-create the PR from there... let me check on this

[mythi]

Suggested change

	maxUnavailable := &intstr.IntOrString{}
	maxUnavailable.IntVal = 1
	maxSurge := &intstr.IntOrString{}
	maxSurge.IntVal = 0
	maxUnavailable := intstr.FromInt(1)
	maxSurge := intstr.FromInt(0)

[mythi]

Suggested change

	MaxUnavailable: maxUnavailable,
	MaxSurge: maxSurge,
	MaxUnavailable: &maxUnavailable,
	MaxSurge: &maxSurge,