Keep PLEG interruptions in a separate interface #113825
Conversation
k8s-ci-robot
added
release-note-none
|
/sig node |
k8s-ci-robot
added
sig/node
|
/priority important-soon |
k8s-ci-robot
added
priority/important-soon
pkg/kubelet/pleg/pleg.go
Outdated
| Watch() chan *PodLifecycleEvent | ||
| Healthy() (bool, error) | ||
| PodLifecycleEventGeneratorHandler |
There was a problem hiding this comment.
@harche Thank you for your PR!
As far as I understood @derekwaynecarr's comment PodLifecycleEventGenerator should not include PodLifecycleEventGeneratorHandler methods as this is all the rest of the kubelet needs to know about. I guess you should do it other way around - PodLifecycleEventGeneratorHandler should embed PodLifecycleEventGenerator.
There was a problem hiding this comment.
@harche please, address this comment to proceed further with the PR, thanks.
There was a problem hiding this comment.
I will think through this and reply here. Thanks.
There was a problem hiding this comment.
GenericPLEG had been an implementation of the PodLifecycleEventGenerator interface and so is recently added EventedPLEG.. I am trying to accomodate @derekwaynecarr's request to seperate the newly added methods from existing methods without bringing in any major changes to how is it designed (especially GenericPLEG) as it stands today.
What do you think of following approach where we reduce the scope of podLifecycleEventGeneratorHandler by making it private?
type PodLifecycleEventGenerator interface {
Start()
Watch() chan *PodLifecycleEvent
Healthy() (bool, error)
podLifecycleEventGeneratorHandler
}
type podLifecycleEventGeneratorHandler interface {
Stop()
Update(relistDuration *RelistDuration)
Relist()
}There was a problem hiding this comment.
Although the moment you put podLifecycleEventGeneratorHandler inside PodLifecycleEventGenerator it is no longer private.
There was a problem hiding this comment.
@bart0sh Just occurred to me that I could have used type assertion to achieve what we want to do instead of interface composition. I just pushed the changes, please let me know WDYT.
There was a problem hiding this comment.
Nope, it's not right. The latest changes don't implement the interface correctly. Let me see if we can fix it.
There was a problem hiding this comment.
Well the interdepencies between those 6 methods is such that, if you try to truly separate them in two different interfaces it starts to mess around with the simple design of the existing Generic PLEG. I am not sure if this refactor is worth that.
But again, may be I am overlooking a simpler approach, so I am very much open to discussing the alternative approach.
There was a problem hiding this comment.
@harche FWIW, here is what I came up with. Not sure if this would work, but it compiles and passes all Kubelet unit tests. WDYT?
diff --git a/pkg/kubelet/pleg/evented.go b/pkg/kubelet/pleg/evented.go
index 1dd176489a5..704e9e5390a 100644
--- a/pkg/kubelet/pleg/evented.go
+++ b/pkg/kubelet/pleg/evented.go
@@ -71,7 +71,7 @@ type EventedPLEG struct {
// For testability.
clock clock.Clock
// GenericPLEG is used to force relist when required.
- genericPleg PodLifecycleEventGenerator
+ genericPleg podLifecycleEventGeneratorHandler
// The maximum number of retries when getting container events from the runtime.
eventedPlegMaxStreamRetries int
// Indicates relisting related parameters
@@ -93,7 +93,7 @@ func NewEventedPLEG(runtime kubecontainer.Runtime, runtimeService internalapi.Ru
runtimeService: runtimeService,
eventChannel: eventChannel,
cache: cache,
- genericPleg: genericPleg,
+ genericPleg: genericPleg.(podLifecycleEventGeneratorHandler),
eventedPlegMaxStreamRetries: eventedPlegMaxStreamRetries,
relistDuration: relistDuration,
clock: clock,
diff --git a/pkg/kubelet/pleg/pleg.go b/pkg/kubelet/pleg/pleg.go
index 2654f32d6fc..f972cc10f86 100644
--- a/pkg/kubelet/pleg/pleg.go
+++ b/pkg/kubelet/pleg/pleg.go
@@ -64,10 +64,14 @@ type PodLifecycleEvent struct {
// PodLifecycleEventGenerator contains functions for generating pod life cycle events.
type PodLifecycleEventGenerator interface {
Start()
- Stop()
- Update(relistDuration *RelistDuration)
Watch() chan *PodLifecycleEvent
Healthy() (bool, error)
- Relist()
UpdateCache(*kubecontainer.Pod, types.UID) (error, bool)
}
+
+type podLifecycleEventGeneratorHandler interface {
+ PodLifecycleEventGenerator
+ Stop()
+ Update(relistDuration *RelistDuration)
+ Relist()
+}
There was a problem hiding this comment.
@bart0sh Ah ha! I was trying to type cast while invoking the methods, e.genericPleg.(*GenericPLEG).Update(relistDuration) and got stuck in that. But your approach of casting Generic PLEG to podLifecycleEventGeneratorHandler works out better. Thanks for that pointer.
I verified that this change does not have any adverse effect on Evented or Generic PLEG and it does not export methods Stop(),Update(relistDuration *RelistDuration) and Relist() outside the pleg package.
PS: Considering type casting Generic PLEG to podLifecycleEventGeneratorHandler unblocked this PR, I feel it's a significant contribution and I have added you as a co-author in the commit. I hope that's alright by you.
pkg/kubelet/kubelet.go
Outdated
| eventedPlegRelistThreshold = time.Minute * 10 | ||
| eventedPlegMaxStreamRetries = 5 | ||
| genericPlegRelistPeriodWhenEventedPlegInUse = time.Second * 300 | ||
| genericPlegRelistThresholdWhenEventedPlegInUse = time.Minute * 10 |
There was a problem hiding this comment.
Can you point out where this renaming was requested?
|
/assign |
|
The Kubernetes project currently lacks enough contributors to adequately respond to all PRs. This bot triages PRs according to the following rules:
You can:
Please send feedback to sig-contributor-experience at kubernetes/community. /lifecycle stale |
k8s-ci-robot
added
the
lifecycle/stale
|
/assign @SergeyKanzhelev @derekwaynecarr |
harche
force-pushed
the
ep_comments
branch
4 times, most recently
from
a6b98fc
to
b3e1693
Compare
pkg/kubelet/pleg/evented.go
Outdated
| @@ -93,7 +93,7 @@ func NewEventedPLEG(runtime kubecontainer.Runtime, runtimeService internalapi.Ru | |||
| runtimeService: runtimeService, | |||
| eventChannel: eventChannel, | |||
| cache: cache, | |||
| genericPleg: genericPleg, | |||
| genericPleg: genericPleg.(podLifecycleEventGeneratorHandler), | |||
There was a problem hiding this comment.
Just for the sake of safety I'd suggest to add a casting check, e.g:
--- a/pkg/kubelet/pleg/evented.go
+++ b/pkg/kubelet/pleg/evented.go
@@ -87,17 +87,21 @@ type EventedPLEG struct {
// NewEventedPLEG instantiates a new EventedPLEG object and return it.
func NewEventedPLEG(runtime kubecontainer.Runtime, runtimeService internalapi.RuntimeService, eventChannel chan *PodLifecycleEvent,
cache kubecontainer.Cache, genericPleg PodLifecycleEventGenerator, eventedPlegMaxStreamRetries int,
- relistDuration *RelistDuration, clock clock.Clock) PodLifecycleEventGenerator {
+ relistDuration *RelistDuration, clock clock.Clock) (PodLifecycleEventGenerator, error) {
+ handler, ok := genericPleg.(podLifecycleEventGeneratorHandler)
+ if !ok {
+ return nil, fmt.Errorf("%v doesn't implement podLifecycleEventGeneratorHandler interface", genericPleg)
+ }
return &EventedPLEG{
runtime: runtime,
runtimeService: runtimeService,
eventChannel: eventChannel,
cache: cache,
- genericPleg: genericPleg,
+ genericPleg: handler,
eventedPlegMaxStreamRetries: eventedPlegMaxStreamRetries,
relistDuration: relistDuration,
clock: clock,
- }
+ }, nil
}
--- a/pkg/kubelet/kubelet.go
+++ b/pkg/kubelet/kubelet.go
@@ -734,8 +734,11 @@ func NewMainKubelet(kubeCfg *kubeletconfiginternal.KubeletConfiguration,
RelistPeriod: genericPlegRelistPeriod,
RelistThreshold: genericPlegRelistThreshold,
}
- klet.eventedPleg = pleg.NewEventedPLEG(klet.containerRuntime, klet.runtimeService, eventChannel,
+ klet.eventedPleg, err = pleg.NewEventedPLEG(klet.containerRuntime, klet.runtimeService, eventChannel,
klet.podCache, klet.pleg, eventedPlegMaxStreamRetries, eventedRelistDuration, clock.RealClock{})
+ if err != nil {
+ return nil, err
+ }
} else {
genericRelistDuration := &pleg.RelistDuration{
RelistPeriod: genericPlegRelistPeriod,
There was a problem hiding this comment.
@bart0sh updated. Thanks.
|
/test |
|
@harche: The
The following commands are available to trigger optional jobs:
Use
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. |
|
/test pull-kubernetes-node-crio-cgrpv1-evented-pleg-e2e |
Signed-off-by: Harshal Patil <harpatil@redhat.com> Co-authored-by: Ed Bartosh <eduard.bartosh@intel.com>
|
/test pull-kubernetes-node-crio-cgrpv1-evented-pleg-e2e |
|
/remove-lifecycle stale |
k8s-ci-robot
removed
the
lifecycle/stale
k8s-ci-robot
added
the
lgtm
|
LGTM label has been added. Git tree hash: 7529e3827a88cc5df6a2e67f6a572c732ce09426
|
|
/assign @mrunalp |
|
Oh, this is still pending approval! /assign @mrunalp @dchen1107 |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: harche, mrunalp 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 |
k8s-ci-robot
added
the
approved
Signed-off-by: Harshal Patil harpatil@redhat.com
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
Addresses some of the minor comments from the Evented PLEG PR, #111384, especially #111384 (comment)
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Mainly trying to address this comment, #111384 (comment)
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: