KEP-1977: Add KEP for ContainerNotifier

[xing-yang]

Enhancement issue: #1977

[xing-yang]

CC @yuxiangqian

[xing-yang]

/assign @thockin @saad-ali @liggitt
/assgin @sjenning @derekwaynecarr @dchen1107

[xing-yang]

/assgin @sjenning @derekwaynecarr @dchen1107

[thockin]

Nesting-wise this is a bit weird.

Pod {
  Spec: {
    Containers: [{
      Notifiers: [{
        Name: foo
        Action: {
          Handler: {
            Exec: {
              Command: bar
            }
          TimeoutSeconds: 10
        }
      }]
    }]
  }
}

I think we can get rid of one level by moving timeout to ContainerNotifier:

Pod {
  Spec: {
    Containers: [{
      Notifiers: [{
        Name: foo
        TimeoutSeconds: 10
        Action: {
          Exec: {
            Command: bar
          }
        }
      }]
    }]
  }
}

I know this is trying to look like Probe, but that embeds Handler, which is sort of problematic (@apelisse re one-of) I think.

[xing-yang]

Sure.

[thockin]

This is going to be a problem. Imagine a notifier that selects 1000 pods. That's not implausible. Even 5000 is believable. That leaves you just a few hundred bytes per pod for status, before it starts failing (too big). Worse, it will fail in non-deterministic ways in practice.

I think we have a few choices here, and we should seek a pattern precedent (@liggitt for consideration)

1. Notification.status is a "summary". The real status is stored in another new resource NotificationResult which is selected by something like "notification.kubernetes.io/uid=". Each NotificationResult holds the status for one single pod-notification. Need to define the lifecycle management of this so we don't leave them around for too long.

2. Notification.status is a "summary". The real status is stored in each pod, e.g. in status.notifications[] which would be a list of "recent" notifications and the per-container statuses

3. other ideas?

TL;DR: we can't have (practically) unbounded lists in a single resource.

[xing-yang]

Ok, I'll wait for more suggestions.

[xing-yang]

Are there any existing examples that I could follow?

[thockin]

EndpointSlice is similar - I am not sure there's a closer example.

[liggitt]

NotificationStatus represents the current state of a Notification. It has a list of PodNotificationStatuses.

when we talked about this in doc form, I thought notification was scoped to a single pod in order to allow notifying in waves and avoid unbounded status accumulation ... did that approach get revised since then?

[xing-yang]

So that means we can't have a PodSelector in the NotificationSpec. It will just be a PodName instead.

[xing-yang]

@thockin would you consider having one to one mapping between a Notification and a Pod (having a PodName in NotificationSpec)? That would simplify status update.

[krmayankk]

Just read the discussion in the doc, I think the notification per pod makes most sense and seems safer. Also the idea of building on top using a notification controller for safe rollout of notification is even better

[thockin]

If we go to 1:1, do we need a higher-level API that selects pods and expands a notification for each of them?or should we add that to kubectl? or should we just say "someone else's problem"?

If we go 1:1 does a subresource make more sense? It gives even finer RBAC control, but we need the result to be a new resource (for status). E.g. POST to /pods/foobar/notify -> creates a PodNotifier; kubelet watches PodNotifiers. Any precedent for that? Any need to get finer-grained than per-namespace?

[krmayankk]

I like the subresource idea which i was hinting here https://github.com/kubernetes/enhancements/pull/1995/files#r493239279 . I guess i don't follow why it should result in a new PodNotifier being created? For e.g. does the POST on a log or the exec sub resource result in a new object ? The main reason i like this is its less number of new objects to manage , less strain on etcd, since the notifications are an array. It does make the RBAC more fine grained. Thinking out loud, if i am not mistaken, you can say a service account can or cannot exec(subresource: pods/exec) to any pods in a namespace by specifying a RoleBinding referencing a Role, So similar should be possible with this notify subresource

[thockin]

We need to define what happens in this case:

time 0: pods A, B, C are running, labelled foo=bar
time 1: create Notification selecting foo=bar
time 2: all notifiers in A, B, C are done
time 3: pod D is created, labelled foo=bar
*** Do we run the notifier on D? If so, for how long does that apply? If not, how do we prevent it?

[xing-yang]

We have said that Kubelet (container notifier controller) will not do retries. So if that is the case, Kubelet (container notifier controller) will only process the pods matching the label when the Notification object is first created? If so, Pod D will not be processed?

[krmayankk]

Is this a namespaced object ? Once its created, it should apply to everything until the api object is deleted .
Is this more of an imperative api or we want it to be declarative (i guess the latter )?
i am thinking what does the declaration mean ?

• notify all containers that match this selector at this time t
• let there be notification for all pods matching this selector as long this api exists
• i want all containers of the pods matching a selector to be quiesced (this seems more natural)

its a bit confusing to think since the lifecycle hooks are tied to the lifecycle of the container/pod and are ok to be imperative, but this api is not tied to the lifecycle but more of a command to the containers.

[thockin]

@krmayankk is on the issue.

This is a pseudo-imperative operation.

@xing-yang

Kubelet (container notifier controller) will only process the pods matching the label when the Notification object is first created

That seems reasonable at first blush, but what is the mechanism for that? Is the rule "only applies to pods whose metadata.creationTimestamp is less than the Notification's metadata.creationTimestamp ? If so, let's document it. Also - less-than or less-than-or-equal?

Does this satisfy the higher-order controller's needs? Is it plausible that a quiesce notification could be created, a snapshot started, then a new pod shows up which is not quiesced? Even if Notifications applied to it, that would be async, so it still seems bad?

[yuxiangqian]

@thockin using CreationTimestamp can certainly be one option to rule out newly created pods, it does not cover the case when a pod is evicted/died during hook execution. From quiescing requirements perspective, its critical to have a point-in-time "fixed" scope for snapshotting, thus if a pod died during the notification, we might want to fail the the whole notification.
we have a couple of other potential options here:

1. make notification to pod one-to-one by replacing podSelector with a pod definition. higher level controller like application snapshot controller then can have a pre-selected list and create one notification to each desired pod.
   Advantage:

• simpler API
• ordering of notification is possible and more convenient to achieve for higher level controllers
  disadvantages:
  a. probably does not fit well with the signal case where multiple pods need to be notified without a higher level controller

2. introducing a list of resolved pod in Notification.Spec which will be immutable once resolved. Pod selection is done prior to any real execution of notification. And that list would be the source of truth for the scope of execution. This might cause issue when the resolved list is too big(~k).
   WDYT?

[thockin]

This is an interesting problem. If a pod dies mid-snapshot, do you fail the whole snapshot? If a new pod a starts mid-snapshot (scale-up?) does it need to quiesce? Maybe that just "doesn't happen" ?

Your (1) happens to ALSO solve the status cardinality problem, so is attractive in that regard. It means the scale of things kubelet has to watch just got much bigger, though, unless we set labels to scope it (e.g. set the node-name as a label, let kubelet watch only that).

But it is ugly for humans.

Another approach would be to define Notifications as applying to all pods. In the example above it would apply to D. And E, F, G etc until the Notification is removed. All the controller (or kubelet) could guarantee would be that every Notification that selects a given pod would be attempted once, and if there were multiple notifications, they would be attempted in some deterministic order (e.g. creationTimestamp or RV).

So to expand the example:

t0: pods A, B, C are running, labelled foo=bar
t1: create Notification N1 selecting foo=bar
t2: N1 notifiers in A, B, C are done
t3: pod D is created, labelled foo=bar
t4: N1 notifier in D is done
t5: create Notification N2 selecting foo=bar
t6: N2 notifiers in A, B, C, D are done
t7: pod E is created, labelled foo=bar
t8: N1 notifier in E is done
t9: N2 notifier in E is done
t10: delete Notification N1
t11: pod F is created, labelled foo=bar
t12: N2 notifier in F is done ## note N1 was not executed
t13: delete Notification N2

This seems maybe workable, but I need you to look at it from your use-cases. Bear in mind that rapid sequences can be observed out of order by watchers, so if your plan would be to delete N1 then create N2 you need some time in between to know that all kubelets have observed the deletion (or ensure that it is "safe" to trigger N1 on new pods). Similarly, if you want to clean up N1 and N2 you must delete them in order with sufficient time between to ensure no out-of-order deletion problems.

We don't have a "serializing barrier" operation.

Regarding "ugly for humans", maybe we need both? Individual pod-notifications (and status) which controllers can orchestrate PLUS a selector-notification which a built-in controller would expand into pod-notifications? This represents a broader API change, but is the more flexible model.

Thoughts?

Regarding your (2) - does that satisfy requirements? Let me see if I get it right:

t0: pods A, B, C are running, labelled foo=bar
t1: create Notification N1 selecting foo=bar
t2: a controller evaluates foo=bar one time and notes the result (A, B, C)
t3: N1 notifiers in A, B, C are done
t4: pod D is created, labelled foo=bar
t5: N1 notifier in D is NOT executed

Is that right? How does that map to the use-cases?

[krmayankk]

Applying to all pods for as long as the notification exists makes more sense from an api point of view. Also including the pod definition would be messy.

[krmayankk]

@krmayankk is on the issue.

??

This is a pseudo-imperative operation.

Is that ok to have ? Do we need to update any apiconvention guidelines on when a pseudo imperative api makes more sense ?

[xing-yang]

For the quiesce use case, a higher level backup controller needs to decide what pods are included. When a backup is started, the backup controller needs to figure out all the pods that need to get the quiesce command. It can retrieve this information using a PodSelector. After that it is a fixed list of Pods that need to be quiesced for this particular backup. Therefore, we can’t have PodSelector in Notification. It should be either a List of Pods that is immutable or a Pod name for a single pod, basically 2 and 1 suggested by Xiangqian.

If we decide to have a PodList in NotificationSpec, the steps would be like this:

1. An external Backup controller determines what pods need to receive the notification by using a PodSelector (this depends on backup controller logic).
2. Backup controller creates a Notification object with a PodList with the selected pods.

This way we have a concrete list of Pods. If quiesce fails for any pod/container, it should be treated as failure for the whole quiesce operation. Typically the backup software gives user an option to complete a backup that is not quiesced successfully or fail it if any quiesce action fails. If user chooses to do a backup even if quiesce fails, the backup controller will trigger a un-quiesce command to undo anything that is already quiesced but skip doing quiesce for the remaining pod/container. Then the backup controller will take a snapshot and mark that snapshot as not-quiesced. User can decide what to do with a snapshot that is not quiesced.

[thockin]

we can’t have PodSelector in Notification

I'm not sure I buy that. This API needs to be general enough to use by humans. We need to be clear about what happens if a new pod is created that satisfies the criteria. Don't over-focus on just quiesce

[krmayankk]

Do we need to start with signal in phase 1 and then add exec later if that is necessary ?

[xing-yang]

Support for signal requires changes to CRI. That means it can only happen in Kubelet. Since we are starting with a separate controller, we can't start with signal.

Also support for exec is an important use case (the quiesce use case for application consistent snapshot). So support for exec should be added in phase 1.

[thockin]

We should not go GA (and maybe not beta) without phase 2, but it is phased for a reason.

[krmayankk]

its not clear from this proposal why the controller is starting with a separate controller(also not clear if its separate or part of one of the existing controllers) and then later moving to kubelet

[xing-yang]

This will start as a separate controller in a new repo, and later on the logic will be moved to kubelet.
This is because there are concerns from sig-node on potential impact on kubelet.

[thockin]

An abundance of caution

[krmayankk]

so a separate external controller running per node (daemon set) ? In order to run the exec commands , will it exec into the pods ?

[xing-yang]

Yes.

[thockin]

not needed per-node, just per-cluster

[xing-yang]

CC @andrewsykim

[thockin]

I was proposing that we get rid of this layer of nesting:

Pod {
  Spec: {
    Containers: [{
      Notifiers: [{
        Name: foo
        TimeoutSeconds: 10
        Action: {   # note no "handler"
          Exec: {
            Command: bar
          }
        }
      }]
    }]
  }
}

[sjenning]

yes, agreed

[krmayankk]

+1

[thockin]

ping

[thockin]

@krmayankk is on the issue.

This is a pseudo-imperative operation.

@xing-yang

Kubelet (container notifier controller) will only process the pods matching the label when the Notification object is first created

That seems reasonable at first blush, but what is the mechanism for that? Is the rule "only applies to pods whose metadata.creationTimestamp is less than the Notification's metadata.creationTimestamp ? If so, let's document it. Also - less-than or less-than-or-equal?

Does this satisfy the higher-order controller's needs? Is it plausible that a quiesce notification could be created, a snapshot started, then a new pod shows up which is not quiesced? Even if Notifications applied to it, that would be async, so it still seems bad?

[thockin]

minor: This is a needlessly long name. Just Notifier or NotifierName perhaps?

[krmayankk]

It would be good to think about who can send notifications ? What permissions they need on a pod ?. Since the controller is doing exec, does it mean as long as exec is opened for the controller, anyone can create and run the commands using controller as a proxy?

Another idea is should we make notifier a subresource ?

[thockin]

RBAC governs who can create Notifications in a namepace - isn't that enough?

[krmayankk]

see here https://github.com/kubernetes/enhancements/pull/1995/files#r555640516

[thockin]

EndpointSlice is similar - I am not sure there's a closer example.

[thockin]

We should not go GA (and maybe not beta) without phase 2, but it is phased for a reason.

[thockin]

An abundance of caution

[thockin]

not needed per-node, just per-cluster

[sjenning]

I would like to see example pod specs (at least the container blocks) for the examples and use cases mentioned.

I understand I should be able to think about what it would look like from the API definition, but just to make it clearer.

It exposes issues like what @thockin noticed here

[yuxiangqian]

I think this is too aggressive given that the amount of work.

[xing-yang]

These milestones need to be updated.

[thockin]

Corner cases are so much fun

[thockin]

This is an interesting problem. If a pod dies mid-snapshot, do you fail the whole snapshot? If a new pod a starts mid-snapshot (scale-up?) does it need to quiesce? Maybe that just "doesn't happen" ?

Your (1) happens to ALSO solve the status cardinality problem, so is attractive in that regard. It means the scale of things kubelet has to watch just got much bigger, though, unless we set labels to scope it (e.g. set the node-name as a label, let kubelet watch only that).

But it is ugly for humans.

Another approach would be to define Notifications as applying to all pods. In the example above it would apply to D. And E, F, G etc until the Notification is removed. All the controller (or kubelet) could guarantee would be that every Notification that selects a given pod would be attempted once, and if there were multiple notifications, they would be attempted in some deterministic order (e.g. creationTimestamp or RV).

So to expand the example:

t0: pods A, B, C are running, labelled foo=bar
t1: create Notification N1 selecting foo=bar
t2: N1 notifiers in A, B, C are done
t3: pod D is created, labelled foo=bar
t4: N1 notifier in D is done
t5: create Notification N2 selecting foo=bar
t6: N2 notifiers in A, B, C, D are done
t7: pod E is created, labelled foo=bar
t8: N1 notifier in E is done
t9: N2 notifier in E is done
t10: delete Notification N1
t11: pod F is created, labelled foo=bar
t12: N2 notifier in F is done ## note N1 was not executed
t13: delete Notification N2

This seems maybe workable, but I need you to look at it from your use-cases. Bear in mind that rapid sequences can be observed out of order by watchers, so if your plan would be to delete N1 then create N2 you need some time in between to know that all kubelets have observed the deletion (or ensure that it is "safe" to trigger N1 on new pods). Similarly, if you want to clean up N1 and N2 you must delete them in order with sufficient time between to ensure no out-of-order deletion problems.

We don't have a "serializing barrier" operation.

Regarding "ugly for humans", maybe we need both? Individual pod-notifications (and status) which controllers can orchestrate PLUS a selector-notification which a built-in controller would expand into pod-notifications? This represents a broader API change, but is the more flexible model.

Thoughts?

Regarding your (2) - does that satisfy requirements? Let me see if I get it right:

t0: pods A, B, C are running, labelled foo=bar
t1: create Notification N1 selecting foo=bar
t2: a controller evaluates foo=bar one time and notes the result (A, B, C)
t3: N1 notifiers in A, B, C are done
t4: pod D is created, labelled foo=bar
t5: N1 notifier in D is NOT executed

Is that right? How does that map to the use-cases?

[xing-yang]

CC @ashish-amarnath

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle stale
• Mark this issue or PR as rotten with /lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[sftim]

BTW, kubernetes/kubernetes#24957 has some relevant, related discussion.

[thockin]

That's a good idea - @xing-yang any objections?

[xing-yang]

That's a good idea - @xing-yang any objections?

@thockin Sure, let's try that. Thanks.

[thockin]

Can you make the proposed change and drop the PRR file? Then we can merge as provisional.

[xing-yang]

Can you make the proposed change and drop the PRR file? Then we can merge as provisional.

Hi @thockin, I've dropped the PRR file and changed the status to "provisional".

[thockin]

Thanks!

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ruiwen-zhao, thockin, xing-yang
Once this PR has been reviewed and has the lgtm label, please ask for approval from dchen1107 by writing /assign @dchen1107 in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• keps/sig-node/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[thockin]

@dchen1107 can you approve this as "provisional" ? Nobody is working on it but I don't want to lose state.

[k8s-triage-robot]

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

[thockin]

I'm just going to close this and if anyone ever gets time, we can reopen