Bug 1842741: Fix serviceChanged and daemonsetConfigChanged #174
Conversation
openshift-ci-robot
added
the
bugzilla/severity-medium
|
@Miciah: This pull request references Bugzilla bug 1842741, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
In response to this:
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. |
openshift-ci-robot
added
the
bugzilla/valid-bug
openshift-ci-robot
added
the
approved
Miciah
force-pushed
the
BZ1842741-serviceChanged-fix-session-affinity-and-type-daemonsetConfigChanged-fix-volume-default-mode
branch
from
c74be28
to
266a252
Compare
| if !cmp.Equal(a.Optional, b.Optional, cmpopts.EquateEmpty()) { | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
nit: could condense lines 253-256 to return cmp.Equal(a.Optional, b.Optional, cmpopts.EquateEmpty())
There was a problem hiding this comment.
Stylistically, I prefer to follow the pattern consistently:
if thing1 doesn't match {
return false
}
if thing2 doesn't match {
return false
}
// ...
if thingn doesn't match {
return false
}
return trueThere is nothing special about thingn (in concrete terms, the Optional field), so I prefer not to deviate from the established pattern. Does that seem reasonable?
There was a problem hiding this comment.
yes, it does. I see what you mean now!
| if !cmp.Equal(a.Items, b.Items, cmpopts.EquateEmpty()) { | ||
| return false | ||
| } | ||
| aDefaultMode := int32(420) |
There was a problem hiding this comment.
Would it make sense to define a defaultMode := int32(420) literal somewhere, in the event the default mode were to change?
There was a problem hiding this comment.
I think it is extremely unlikely that the default mode would change because that seems like it would be a breaking API change. I could introduce a constant though. Would using the same constant for both secret volume sources and configmap volume sources make sense?
There was a problem hiding this comment.
Ah, my mistake. I think using a constant as you mentioned would make sense.
Miciah
force-pushed
the
BZ1842741-serviceChanged-fix-session-affinity-and-type-daemonsetConfigChanged-fix-volume-default-mode
branch
from
266a252
to
d7e886e
Compare
|
@Miciah: This pull request references Bugzilla bug 1842741, which is valid. 3 validation(s) were run on this bug
In response to this:
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. |
openshift-ci-robot
added
the
needs-rebase
When comparing services to determine whether an update is required, treat implicit and explicit default values as equal. Before this commit, the update logic would keep trying to revert the default value that the API set for the service's session affinity or type. This commit fixes part of bug 1842741. https://bugzilla.redhat.com/show_bug.cgi?id=1842741 * pkg/operator/controller/controller_dns_service.go (serviceChanged): Use the new cmpServiceAffinity and cmpServiceType functions so that two services are considered equal if the only difference between them is that one does not specify the session affinity or type and the other service has the default values. (cmpServiceAffinity, cmpServiceType): New function. * pkg/operator/controller/controller_dns_service_test.go (TestDNSServiceChanged): Verify that serviceChanged returns false if the only mutation is that the session affinity or type has been updated from empty to the default value.
|
Rebased. |
Miciah
force-pushed
the
BZ1842741-serviceChanged-fix-session-affinity-and-type-daemonsetConfigChanged-fix-volume-default-mode
branch
from
d7e886e
to
eb482f6
Compare
openshift-ci-robot
removed
the
needs-rebase
| newVal := int32(0) | ||
| daemonset.Spec.Template.Spec.Volumes[1].Secret.DefaultMode = &newVal | ||
| }, | ||
| expect: true, |
There was a problem hiding this comment.
These 2 test cases cover the cmpSecretVolumeSource(...) function. Perhaps there should be 2 similar test cases for cmpConfigMapVolumeSource(...)?
There was a problem hiding this comment.
Added. I also noticed that the index was wrong (it should be 0 rather than 1 for the config-volume volume), so I fixed the index on the existing test and added a host-path volume to make the list of volumes consistent with the actual manifest. Thanks!
When comparing daemonsets to determine whether an update is required, treat implicit and explicit default values as equal. Before this commit, the update logic would keep trying to revert the default value that the API set for a volume's default mode. This commit fixes part of bug 1842741. https://bugzilla.redhat.com/show_bug.cgi?id=1842741 * pkg/operator/controller/controller_dns_daemonset.go (daemonsetConfigChanged): Use the new cmpConfigMapVolumeSource and cmpSecretVolumeSource functions so that two daemonsets are considered equal if the only difference between them is that one does not specify a default mode value and the other daemonset specifies the default value. (volumeDefaultMode): New constant with the default mode that the API uses for configmap and secret volume sources. (cmpConfigMapVolumeSource, cmpSecretVolumeSource): New functions. * pkg/operator/controller/controller_dns_daemonset_test.go (TestDaemonsetConfigChanged): Verify that daemonsetConfigChanged returns false if the only mutation is that a volume's default mode value has been updated from empty to the default value.
Miciah
force-pushed
the
BZ1842741-serviceChanged-fix-session-affinity-and-type-daemonsetConfigChanged-fix-volume-default-mode
branch
from
eb482f6
to
147d79a
Compare
|
/test e2e-aws |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah, sgreene570 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:
Approvers can indicate their approval by writing |
|
@Miciah: All pull requests linked via external trackers have merged: openshift/cluster-dns-operator#174. Bugzilla bug 1842741 has been moved to the MODIFIED state. In response to this:
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. |
serviceChanged: Fix session affinity, typeWhen comparing services to determine whether an update is required, treat implicit and explicit default values as equal.
Before this commit, the update logic would keep trying to revert the default value that the API set for the service's session affinity or type.
pkg/operator/controller/controller_dns_service.go(serviceChanged): Use the newcmpServiceAffinityandcmpServiceTypefunctions so that two services are considered equal if the only difference between them is that one does not specify the session affinity or type and the other service has the default values.(
cmpServiceAffinity,cmpServiceType): New function.pkg/operator/controller/controller_dns_service_test.go: (TestDNSServiceChanged): Verify thatserviceChangedreturns false if the only mutation is that the session affinity or type has been updated from empty to the default value.daemonsetConfigChanged: Fix for volume default modeWhen comparing daemonsets to determine whether an update is required, treat implicit and explicit default values as equal.
Before this commit, the update logic would keep trying to revert the default value that the API set for a volume's default mode.
pkg/operator/controller/controller_dns_daemonset.go(daemonsetConfigChanged): Use the newcmpConfigMapVolumeSourceandcmpSecretVolumeSourcefunctions so that two daemonsets are considered equal if the only difference between them is that one does not specify a default mode value and the other daemonset specifies the default value.(
volumeDefaultMode): New constant with the default mode that the API uses for configmap and secret volume sources.(
cmpConfigMapVolumeSource,cmpSecretVolumeSource): New functions.pkg/operator/controller/controller_dns_daemonset_test.go: (TestDaemonsetConfigChanged): Verify thatdaemonsetConfigChangedreturns false if the only mutation is that a volume's default mode value has been updated from empty to the default value.