Skip to content
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.

Sign up for GitHub

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

Jump to bottom

patch service status instead of update in service controller #77984

Merged
merged 3 commits into from
May 17, 2019
Merged

patch service status instead of update in service controller #77984

merged 3 commits into from
May 17, 2019

Conversation

MrHohn
Copy link
Member

@MrHohn MrHohn commented May 16, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Split from #54569. This PR updates service controller to use patch when updating service's status. As #38349 (comment) mentioned, this will help with the conflicting update situation.

Which issue(s) this PR fixes:

Fixes #38349

Special notes for your reviewer:
/cc @bowei @andrewsykim @jhorwit2 @liggitt

Does this PR introduce a user-facing change?:

NONE

@k8s-ci-robot k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label May 16, 2019
@k8s-ci-robot k8s-ci-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label May 16, 2019
@k8s-ci-robot k8s-ci-robot requested a review from liggitt May 16, 2019 17:06
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 16, 2019
@bowei
Copy link
Member

bowei commented May 16, 2019

code looks good

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2019
@MrHohn
Copy link
Member Author

MrHohn commented May 16, 2019

Humm, need to fix the unit test:

--- FAIL: TestProcessServiceUpdate (0.00s)
    service_controller_test.go:410: If updating a valid service processServiceUpdate() failed to patch status for service validKey: failed to patch "{\"status\":{\"loadBalancer\":{\"ingress\":[{\"ip\":\"\\u003cnil\\u003e\"}]}}}" for svc default/external-balancer: services "external-balancer" not found
    service_controller_test.go:410: If Updating Loadbalancer IP processServiceUpdate() failed to patch status for service default/sync-test-name: failed to patch "{\"status\":{\"loadBalancer\":{\"ingress\":[{\"ip\":\"\\u003cnil\\u003e\"}]}}}" for svc default/sync-test-name: services "sync-test-name" not found

andrewsykim
andrewsykim reviewed May 16, 2019
View reviewed changes
pkg/controller/service/util/util_test.go Outdated
"k8s.io/client-go/kubernetes/fake"
)

func TestPatch(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we have a test case where we attempt a separate update before the merge patch?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah would be great to verify that. Added now.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2019
@MrHohn
Copy link
Member Author

MrHohn commented May 16, 2019

Humm, need to fix the unit test:

Did a minimal fix for now. I'm planing to cleanup the test when we remove the cache layer with finalizer support.

andrewsykim
andrewsykim reviewed May 16, 2019
View reviewed changes
pkg/controller/service/service_controller_test.go Outdated
@@ -404,6 +417,10 @@ func TestProcessServiceUpdate(t *testing.T) {

for _, tc := range testCases {
newSvc := tc.updateFn(tc.svc)
if _, err := client.CoreV1().Services(tc.svc.Namespace).Create(tc.svc); err != nil {
t.Errorf("Failed to prepare service %s for testing: %v", tc.key, err)
continue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

t.Fatalf and get rid of continue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, fixed now.

pkg/controller/service/util/util_test.go Outdated

func TestPatch(t *testing.T) {
svcOrigin := &v1.Service{
ObjectMeta: metav1.ObjectMeta{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should at least start with a name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added a name.

pkg/controller/service/service_controller.go Outdated
}
klog.V(2).Infof("Successfully patch status for service %s", key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd say that 2 is too verbose for this thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, given that we don't even log this before. Bumped it to 4.

@MrHohn MrHohn force-pushed the svc-finalizer-alpha branch 2 times, most recently from 59561d1 to 45cf9d5 Compare May 16, 2019 20:55
andrewsykim
andrewsykim reviewed May 16, 2019
View reviewed changes
pkg/controller/service/service_controller.go Outdated
}
klog.V(4).Infof("Successfully patch status for service %s", key)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Successfully patched

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, fixed now. Thanks for the quick review!

@andrewsykim
Copy link
Member

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2019
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 16, 2019
@MrHohn
Copy link
Member Author

MrHohn commented May 16, 2019

/assign @liggitt
for the controller_policy part, thanks!

andrewsykim
andrewsykim reviewed May 16, 2019
View reviewed changes
pkg/controller/service/util/util.go Outdated
limitations under the License.
*/

package util
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more nit (more of a personal preference): I don't think a util package is necessary for patch, can just have a patch.go go file in pkg/controller/service/

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree. Fewer public methods to accumulate callers, the better

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good point. Put it into patch.go instead.

@liggitt
Copy link
Member

liggitt commented May 16, 2019

Rbac change looks good, let me know when the rest is reviewed

andrewsykim
andrewsykim reviewed May 16, 2019
View reviewed changes
pkg/controller/service/patch.go Outdated
)

// patch patches service given the origin and updated ones.
// Caller is free to decide what (e.g. metadata, spec or status) is
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this comment accurate?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not anymore. This patch should only update status (for LB IP) and metadata (for finalizer) in service controller's use case. Let me update that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. PTAL

MrHohn and others added 3 commits May 16, 2019 17:30
@MrHohn @jhorwit2
Patch service instead of update in service controller
a1f0704 
Co-authored-by: Josh Horwitz <horwitzja@gmail.com>
@MrHohn @jhorwit2
Allow service controller role to patch service status
bff5f08 
Co-authored-by: Josh Horwitz <horwitzja@gmail.com>
@MrHohn @jhorwit2
Fixes service controller unit test for patching
cda73eb 
Co-authored-by: Josh Horwitz <horwitzja@gmail.com>
@andrewsykim
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 17, 2019
@liggitt
Copy link
Member

liggitt commented May 17, 2019

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, liggitt, MrHohn

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 17, 2019
@MrHohn
Copy link
Member Author

MrHohn commented May 17, 2019

/retest

@MrHohn
Copy link
Member Author

MrHohn commented May 17, 2019

/test pull-kubernetes-e2e-gce-100-performance

@k8s-ci-robot k8s-ci-robot merged commit 9cbbf59 into kubernetes:master May 17, 2019
@wallrj wallrj mentioned this pull request Dec 19, 2019
Merged
@liggitt liggitt removed the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewsykim andrewsykim andrewsykim left review comments

@bowei bowei Awaiting requested review from bowei

@jhorwit2 jhorwit2 Awaiting requested review from jhorwit2

@liggitt liggitt Awaiting requested review from liggitt

Assignees

@bowei bowei

@liggitt liggitt

@andrewsykim andrewsykim

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note-none Denotes a PR that doesn't merit a release note. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

servicecontroller is not retrying on service status update
5 participants
@MrHohn @bowei @andrewsykim @liggitt @k8s-ci-robot