feat: Support daemonset target kind #455
Conversation
There was a problem hiding this comment.
@mathetake thank you for this. Please change the Canary CRDs and add DaemonSet to the list of accepted values here and run make crd.
pkg/canary/daemonset_controller.go
Outdated
| } | ||
|
|
||
| if cd.GetProgressDeadlineSeconds() > 0 { | ||
| // (@mathetake): should we? |
There was a problem hiding this comment.
Could we enforce the deadline by calculating the time span for DaemonSetCondition.LastTransitionTime?
There was a problem hiding this comment.
as far as i know, there seems no DaemonSetCondition to registered indatemonset.status.Conditions by kubernetes master as i mentioned in the description of this PR (even though type DaemonSetCondition is defined).
Therefore we can not use DaemonSetCondition.LastTransitionTime for deadline calculation.
There was a problem hiding this comment.
I see two options:
- use the canary condition
LastTransitionTime - use the DaemonSet pods status
LastTransitionTime
There was a problem hiding this comment.
please check out 9c8b887#diff-22b8047311f2556d87d745beff11924aR53
where i used canary.Status.LastTransitionTime for deadline calculation
and ignore daemonSetScaleDownNodeSelector in target spec change detection
and run goimports on several files
resolves #345
this PR adds support for daemonset target
Although there is still much of codes that can be shared among Deployments and DaemonSets (for example, test fixtures have lines of duplicated codes), i intentionally left as it is and just copied and pasted for daemonset support so that there would be less influence on existing code bases for deployment/service.
notes
ignore progressDeadlineSeconds for daemonset since currently there's noDaemonSetConditiontypes in k8s to check the progress as Deployment does.anyway, thanks for maintaining this awesome project 👍