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

Refactor kubelet node status setters, add test coverage #65660

Merged
merged 19 commits into from Jul 20, 2018

Conversation

@mtaufen
Copy link
Contributor

mtaufen commented Jun 30, 2018

This internal refactor moves the node status setters to a new package, explicitly injects dependencies to facilitate unit testing, and adds individual unit tests for the setters.

I gave each setter a distinct commit to facilitate review.

Non-goals:

  • I intentionally excluded the class of setters that return a "modified" boolean, as I want to think more carefully about how to cleanly handle the behavior, and this PR is already rather large.
  • I would like to clean up the status update control loops as well, but that belongs in a separate PR.
NONE

@mtaufen mtaufen added this to the v1.12 milestone Jun 30, 2018

@k8s-ci-robot k8s-ci-robot requested review from jeffvance and vishh Jun 30, 2018

@mtaufen mtaufen force-pushed the mtaufen:incremental-refactor-kubelet-node-status branch from 476c1dd to 0ad76ed Jun 30, 2018

@mtaufen mtaufen force-pushed the mtaufen:incremental-refactor-kubelet-node-status branch from 0ad76ed to 7838f76 Jun 30, 2018

@mtaufen mtaufen force-pushed the mtaufen:incremental-refactor-kubelet-node-status branch from 7838f76 to 59c8c9e Jun 30, 2018

@mtaufen mtaufen force-pushed the mtaufen:incremental-refactor-kubelet-node-status branch 4 times, most recently from bbe1a8b to 2e83e6e Jul 9, 2018

@dashpole
Copy link
Contributor

dashpole left a comment

Love the changes in general. I generally prefer passing interfaces as arguments rather than functions. Mostly because it makes it easier to figure out what function underlying function is being called. I can grep for IsUnderDiskPressure, but not pressureFunc. I'm also not sure if the comments by each argument to Condition functions are useful.

@@ -1081,8 +970,13 @@ func (kl *Kubelet) defaultNodeStatusFuncs() []func(*v1.Node) error {
return nil
}
}
// if cloud is not nil, we expect the cloud resource sync manager to exist
var nodeAddressesFunc func() ([]v1.NodeAddress, error)
if kl.cloud != nil {

This comment has been minimized.

@dashpole

dashpole Jul 11, 2018

Contributor

almost seems like cloudResourceSyncManager should be a sub-structure of cloud. I.E. kl.cloud.resourceSyncManager.NodeStatusFunctions() returns a list of functions (initially just NodeAddresses)

@@ -946,7 +890,7 @@ func (kl *Kubelet) defaultNodeStatusFuncs() []func(*v1.Node) error {
nodestatus.NodeAddress(kl.nodeIP, kl.nodeIPValidator, kl.hostname, kl.externalCloudProvider, kl.cloud, nodeAddressesFunc),
withoutError(kl.setNodeStatusInfo),
nodestatus.OutOfDiskCondition(kl.clock.Now, kl.recordNodeStatusEvent),
withoutError(kl.setNodeMemoryPressureCondition),
nodestatus.MemoryPressureCondition(kl.clock.Now, kl.evictionManager.IsUnderMemoryPressure, kl.recordNodeStatusEvent),

This comment has been minimized.

@dashpole

dashpole Jul 11, 2018

Contributor

rather than defining each pressure as a func, can we define an interface NodeResourcePressureProvider (or whatever you want to name it), which includes all the IsUnderXPressure functions, and pass kl.evictionManager as the implementation of this interface. I prefer passing interfaces to functions whenever possible.

@@ -213,6 +213,67 @@ func MemoryPressureCondition(nowFunc func() time.Time, // typically Kubelet.cloc
}
}

// PIDPressureCondition returns a Setter that updates the v1.NodePIDPressure condition on the node.
func PIDPressureCondition(nowFunc func() time.Time, // typically Kubelet.clock.Now

This comment has been minimized.

@dashpole

dashpole Jul 11, 2018

Contributor

Pass a clock.Clock in instead? Prefer interface arguments to explicit functions. (although if you disagree, i'm not as sold on this one).

@@ -467,3 +467,16 @@ func OutOfDiskCondition(nowFunc func() time.Time, // typically Kubelet.clock.Now
return nil
}
}

// VolumesInUse returns a Setter that updates the volumes in use on the node.
func VolumesInUse(syncedFunc func() bool, // typically Kubelet.volumeManager.ReconcilerStatesHasBeenSynced

This comment has been minimized.

@dashpole

dashpole Jul 11, 2018

Contributor

would prefer to have an interface which is a subset of the volume manager interface here as well that includes these two functions.

maxPods int,
podsPerCore int,
machineInfoFunc func() (*cadvisorapiv1.MachineInfo, error), // typically Kubelet.GetCachedMachineInfo
capacityFunc func() v1.ResourceList, // typically Kubelet.containerManager.GetCapacity

This comment has been minimized.

@dashpole

dashpole Jul 11, 2018

Contributor

an interface which is a subset of containerManager would be useful here too.

@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented Jul 12, 2018

One other reason using interfaces is helpful is because we can re-use the mocks and fakes associated with the implementation of the interface. For example, instead of defining a new testing function for the nowFunc(), we can re-use the fake clock.

@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Jul 13, 2018

That's a fair point. I need to go look at the existing mocks and fakes and see how painful they are to construct - after seeing how complex the Kubelet mock was, I just tried to avoid them altogether to minimize the dependencies of the test.

@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented Jul 13, 2018

I'm also fine leaving that for the future if it ends up being a larger change. This PR is big enough as is :)

remove incorrect comment referencing removed functionality
The cbr0 configuration behavior this comment references was removed in #34906
@mtaufen

This comment has been minimized.

Copy link
Contributor Author

mtaufen commented Jul 16, 2018

I'd prefer to revisit it in a future PR, as you said, this one is already big enough.

mtaufen added some commits Jun 25, 2018

port setNodeAddress to Setter abstraction, port test
also put cloud_request_manager.go in its own package
lift node-info setters into defaultNodeStatusFuncs
Instead of hiding these behind a helper, we just register them in a
uniform way. We are careful to keep the call-order of the setters the
same, though we can consider re-ordering in a future PR to achieve
fewer appends.

@mtaufen mtaufen force-pushed the mtaufen:incremental-refactor-kubelet-node-status branch from 2e83e6e to 0f8976b Jul 16, 2018

@dashpole

This comment has been minimized.

Copy link
Contributor

dashpole commented Jul 19, 2018

/lgtm
I think the added testing this provides, and the fact that it compartmentalizes some of the behavior previously in package kubelet makes this a worthwhile change.

@k8s-ci-robot k8s-ci-robot added the lgtm label Jul 19, 2018

@yujuhong

This comment has been minimized.

Copy link
Member

yujuhong commented Jul 19, 2018

This is a massive cleanup! Thanks!
/approve

@saad-ali

This comment has been minimized.

Copy link
Member

saad-ali commented Jul 20, 2018

/lgtm
/approve

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 20, 2018

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@dashpole @hanxiaoshuai @mtaufen @saad-ali @yujuhong

Pull Request Labels
  • sig/node: Pull Request will be escalated to these SIGs if needed.
  • priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
  • kind/cleanup: Adding tests, refactoring, fixing old bugs.
Help
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 20, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dashpole, mtaufen, saad-ali, yujuhong

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-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 20, 2018

Automatic merge from submit-queue (batch tested with PRs 66152, 66406, 66218, 66278, 65660). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 53ee0c8 into kubernetes:master Jul 20, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation mtaufen authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
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.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.