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.

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

Split field manager with interface #82553

Open
wants to merge 1 commit into
base: master
from

Conversation

@jennybuckley
Copy link
Contributor

commented Sep 10, 2019

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Make the field manager type more unit testable

Does this PR introduce a user-facing change?:

NONE

/cc @apelisse

/priority backlog
/wg apply

@k8s-ci-robot k8s-ci-robot requested a review from apelisse Sep 10, 2019

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-fieldmanager-interface branch from dfea467 to c5905d6 Sep 10, 2019

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-fieldmanager-interface branch 3 times, most recently from 1841152 to 8ebb748 Sep 10, 2019

@@ -826,6 +826,7 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
if err != nil {
return nil, err
}
reqScope.FieldManager = fieldmanager.NewLazyFieldManager(reqScope.FieldManager, reqScope.Creater, reqScope.Kind)

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 10, 2019

Member

I like that we have a different set of dependencies for the lazy one than for the non-lazy one. That's good. Avoids having an increasing number of members in the regular one.

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 11, 2019

Member

I find it a little confusing that you put the fieldmanager into reqScope and then replace it right away, maybe use a temporary instead?

if err != nil {
return nil, fmt.Errorf("failed to create empty object of type %v: %v", f.gvk, err)
}
liveObj, err = f.fieldManager.Update(emptyObj, liveObj, "before-first-apply")

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 10, 2019

Member

That's so much better


// if managed field is still empty, skip updating managed fields altogether
if len(managed.Fields) == 0 {
if len(liveObjAccessor.GetManagedFields()) == 0 && len(newObjAccessor.GetManagedFields()) == 0 {

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 10, 2019

Member

I think that's cool

@apelisse

This comment has been minimized.

Copy link
Member

commented Sep 11, 2019

#82175 is related since it was moving one of the for that feature.

srcs = ["fieldmanager.go"],
srcs = [
"fieldmanager.go",
"lazy_fieldmanager.go",

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 11, 2019

Member

I don't know if I'm a big fan of the name. "Lazy" doesn't really say why it's being lazy. What about: TrackAppliedFieldManager, that's pretty bad, but you get the idea.

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Sep 11, 2019

Author Contributor

I'll think about it

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Sep 11, 2019

Author Contributor

Maybe SkipNonAppliedFieldManager?

@@ -560,7 +560,7 @@ func (a *APIInstaller) registerResourceHandlers(path string, storage rest.Storag
if err != nil {
return nil, fmt.Errorf("failed to create field manager: %v", err)
}
reqScope.FieldManager = fm
reqScope.FieldManager = fieldmanager.NewLazyFieldManager(fm, a.group.Creater, fqKindToRegister)

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 11, 2019

Member

Why don't we create the lazyfieldmanager at the same time that we create the fieldmanager?

Good!

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-fieldmanager-interface branch 5 times, most recently from 9d3e690 to 9c43c7c Sep 11, 2019


err := f.Apply(appliedBytes, "fieldmanager_test_apply", false)
apiStatus, _ := err.(apierrors.APIStatus)
if err == nil || !apierrors.IsConflict(err) || len(apiStatus.Status().Details.Causes) != 1 {

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Sep 12, 2019

Author Contributor

isConflict makes sure that err will be an APIStatus

@@ -826,6 +826,8 @@ func (r *crdHandler) getOrCreateServingInfoFor(uid types.UID, name string) (*crd
if err != nil {
return nil, err
}
fm = fieldmanager.NewSkipNonAppliedManager(fm, reqScope.Creater, reqScope.Kind)
reqScope.FieldManager = fm

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 12, 2019

Member

This one is not necessary ;-)

f.fieldManager = fieldmanager.NewSkipNonAppliedManager(f.fieldManager, &fakeObjectCreater{}, schema.GroupVersionKind{})

updatedObj := &corev1.Pod{}
updatedObj.ObjectMeta.Labels = map[string]string{"k": "v"}

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 12, 2019

Member

This is not used anywhere?

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Sep 12, 2019

Author Contributor

It is just to make sure that there is a field that would have been considered owned, if we hadn't skipped tracking ownership
Wait that's just a typo

}
}

func TestUpateBeforeFirstApply(t *testing.T) {

This comment has been minimized.

Copy link
@apelisse

apelisse Sep 12, 2019

Member

We don't really have a test that shows that it skips?

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Sep 12, 2019

Author Contributor

I included the same checks after the first update in this test (TestUpateBeforeFirstApply) that I had in the old test which just showed it skipped

This comment has been minimized.

Copy link
@jennybuckley

jennybuckley Sep 12, 2019

Author Contributor
	updatedObj := &corev1.Pod{}
	updatedObj.ObjectMeta.Labels = map[string]string{"app": "nginx"}

	if err := f.Update(updatedObj, "fieldmanager_test_update"); err != nil {
		t.Fatalf("failed to update object: %v", err)
	}

	if m := f.ManagedFields(); len(m) != 0 {
		t.Fatalf("managedFields were tracked on update only: %v", m)
	}

This comment has been minimized.

Copy link
@apelisse

@jennybuckley jennybuckley force-pushed the jennybuckley:apply-fieldmanager-interface branch from 9c43c7c to 2c67bf4 Sep 12, 2019

@apelisse

This comment has been minimized.

Copy link
Member

commented Sep 16, 2019

Thank you Jenny!
/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Sep 16, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: apelisse, jennybuckley
To complete the pull request process, please assign deads2k
You can assign the PR to them by writing /assign @deads2k in a comment when ready.

The full list of commands accepted by this bot can be found 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.