New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Bug 1898500: Support InstallPlan steps upgrading existing ClusterIP Services. #1884
Bug 1898500: Support InstallPlan steps upgrading existing ClusterIP Services. #1884
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: benluddy 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 |
@benluddy: This pull request references Bugzilla bug 1898500, 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. |
service = service.DeepCopy() | ||
service.Spec.ClusterIP = "" | ||
} | ||
patchBytes, err := createPatch(normalized, service) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be an issue applying the patch in the case where the new service specifies a ClusterIP
whereas the old one does not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that should fail. My thinking was that there are two scenarios where a patch shouldn't touch ClusterIP
:
- v1 and v2 omit
ClusterIP
- v1 and v2 both set the same
ClusterIP
value
For the case where v1 omits ClusterIP
and v2 does not, I'm not convinced that there is a clear automatic decision to be made. We could go ahead and ignore the fact that v2 specifies a certain IP, but it's plausible that for some reason the operator depends on the v2 service having exactly that IP, so I felt that it was best in that case to force a person to step in and decide what to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that makes sense. I feel like the behavior could be somewhat dependent on the service Type
-- maybe a loadbalanced or external service would expect different upgrade behavior than a cluster-internal one. I think we can keep an eye on this and build out more robust service upgrades upstream (if it arises) or downstream.
6431d9e
to
f51b185
Compare
If a ClusterIP-type Service manifest omits the immutable spec field "ClusterIP", a value is selected automatically when the Service is created. The catalog operator can patch existing Service resources if the updated manifest either omits "ClusterIP" or sets "ClusterIP" to the same value as that of the existing Service.
f51b185
to
0f1f2dc
Compare
/lgtm |
/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. |
@benluddy: All pull requests linked via external trackers have merged: Bugzilla bug 1898500 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.6 |
@benluddy: new pull request created: #1977 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. |
If a ClusterIP-type Service manifest omits the immutable spec field
"ClusterIP", a value is selected automatically when the Service is
created. The catalog operator can patch existing Service resources if
the updated manifest either omits "ClusterIP" or sets "ClusterIP" to
the same value as that of the existing Service.