Bug 1804812: fix(deployment): deployment spec hash #1301
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
ecordell
force-pushed
the
depspec-hash
branch
from
204c450
to
b70102b
Compare
ecordell
changed the title
openshift-ci-robot
added
bugzilla/invalid-bug
|
@ecordell: This pull request references Bugzilla bug 1751903, which is invalid:
Comment 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. |
ecordell
changed the title
openshift-ci-robot
added
bugzilla/valid-bug
|
@ecordell: This pull request references Bugzilla bug 1804812, 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. 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. |
ecordell
force-pushed
the
depspec-hash
branch
from
b70102b
to
0cd9010
Compare
|
/retest |
1 similar comment
|
/retest |
ecordell
force-pushed
the
depspec-hash
branch
from
0cd9010
to
447be43
Compare
ecordell
changed the title
|
/retest |
1 similar comment
|
/retest |
pkg/controller/install/deployment.go
Outdated
| @@ -104,6 +107,11 @@ func (i *StrategyDeploymentInstaller) deploymentForSpec(name string, spec appsv1 | |||
| return | |||
| } | |||
|
|
|||
| // Calculate hash without template annotations | |||
There was a problem hiding this comment.
Interesting - why do we calculate the deployment hash without the annotations?
|
/retest |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
ecordell
force-pushed
the
depspec-hash
branch
from
447be43
to
2ae94ba
Compare
|
/retest |
1 similar comment
|
/retest |
| _, err = crc.OperatorsV1alpha1().ClusterServiceVersions(otherNamespaceName).Update(fetchedCSV) | ||
| return err | ||
| } | ||
| require.NoError(t, retry.RetryOnConflict(retry.DefaultBackoff, updateCSV)) |
There was a problem hiding this comment.
cool -- common source of flakes?
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/lgtm Thanks for answering my questions. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: awgreene, ecordell, exdx, njhale 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@ecordell: All pull requests linked via external trackers have merged. Bugzilla bug 1804812 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. |
|
/cherry-pick release-4.4 |
|
@ecordell: new pull request created: #1333 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. |
Description of the change:
Use deployment spec hash for determining if change is needed on cluster
DeepDerivative will miss cases where values are removed, because an
empty value is treated as unchanged.
This adds some duplication to the apiservice handling, but I avoided doing any serious refactoring to ensure this cherry picks cleanly.
Motivation for the change:
We had been using deep derivative to compare the operator's view of what the deployment should be to the cluster's. But that has a lot of edge cases (zero values for ints, unsetting previously set values), so instead we just store a hash of the deployment spec on the resulting deployment and recalculate it when we need to compare.
Reviewer Checklist
/docs