UPSTREAM: <carry>: pods in openshift-* namespace can be marked critical

[derekwaynecarr]

If we pursue the static pod based deployment topology, we should extend the critical pod support to openshift-* namespaces pending priority/preemption graduation in kubernetes.

input from @smarterclayton @liggitt @sjenning @aveshagarwal appreciated.

[derekwaynecarr]

/hold pending review.

[aveshagarwal]

@derekwaynecarr where are we using scheduler.alpha.kubernetes.io/critical-pod in openshift?

[aveshagarwal]

@derekwaynecarr is it for avoiding eviction by kubelet when eviction threshold is reached? Or is there any other reason? just trying to understand.

[aveshagarwal]

Using scheduler.alpha.kubernetes.io/critical-pod which never came out of alpha and is being deprecated by priority/preemption, does not seem right way to go at this stage. May be we can find some other ways for time being than using scheduler.alpha.kubernetes.io/critical-pod?

[aveshagarwal]

@derekwaynecarr it is not clear to me why we need this for static pod based deployment topology?

[derekwaynecarr]

@aveshagarwal see details here: kubernetes/kubernetes#40573

static pods once evicted are never restarted, so we need to ensure they are never evicted.

[aveshagarwal]

@aveshagarwal see details here: kubernetes/kubernetes#40573

static pods once evicted are never restarted, so we need to ensure they are never evicted.

Wondering what would evicts those pods in the context we are discussing here? If the eviction happens due to reaching any eviction thresholds on a machine, we should make sure such conditions don't occur on those machines (should not be difficult if we dont allow any other pods on the machines), where infra static pods are running. If its about drain, drain by default does not evict static pods unless --force is used.

[smarterclayton]

There will be lots of workloads on masters with varying CPU load. It is not a safe assumption to assume that eviction will not happen on masters.

…

On Mon, Mar 26, 2018 at 4:46 PM, Avesh Agarwal ***@***.***> wrote: @aveshagarwal <https://github.com/aveshagarwal> see details here: kubernetes/kubernetes#40573 <kubernetes/kubernetes#40573> static pods once evicted are never restarted, so we need to ensure they are never evicted. Wondering what would evicts those pods in the context we are discussing here? If the eviction happens due to reaching any eviction thresholds on a machine, we should make sure such conditions don't occur on those machines (should not be difficult if we dont allow any other pods on the machines), where infra static pods are running. If its about drain, drain by default does not evict static pods unless --force is used. — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#19104 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABG_p4ZqW7nrewL08mpxPPAEvUoAj927ks5tiVOkgaJpZM4S7voT> .

[derekwaynecarr]

@aveshagarwal independent of masters, openshift-logging would benefit as well.

[aveshagarwal]

@aveshagarwal independent of masters, openshift-logging would benefit as well.

I agree with that.

Was just wondering if there is an option to not use scheduler.alpha.kubernetes.io/critical-pod and disable eviction on kubelet for static pod deployment strategy (assuming only critical work loads are run and not any non-critical work loads).

For openshift-logging which needs to run on all nodes, right now it seems only scheduler.alpha.kubernetes.io/critical-podcan help to avoid eviction. Still thinking/wondering if there is any possibility to use priority/pre-emption alpha feature than this critical annotation based alpha feature.

[sjenning]

This doesn't build right now

vendor/k8s.io/kubernetes/pkg/kubelet/types/pod_update.go:155:56: not enough arguments in call to strings.HasPrefix

[derekwaynecarr]

current list of critical pods:

1. sdn
2. ovs
3. sync

should be critical (but do not appear to be as of 4/20)

1. master-api
2. master-controllers
3. master-etcd
4. logging fluentd ds

up for debate:

1. router (in default namespace, so not covered by this pr, i dont think its crtical)
2. docker-registry (^)
3. web-console (dont think its critical)

fyi @smarterclayton

[derekwaynecarr]

i think this should pass all checkers now...

[openshift-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr

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:

• vendor/k8s.io/kubernetes/pkg/OWNERS [derekwaynecarr]

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

[liggitt]

/lgtm

will drop once we get priority in 1.11

[derekwaynecarr]

/test gcp
/test extended_conformance_install

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[openshift-bot]

/retest

Please review the full test history for this PR and help us cut down flakes.

[ravisantoshgudimetla]

I know that I am responding late but yesterday, I realized that a side-effect of enabling this feature is kubelet will start preempting pods(https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/preemption/preemption.go) when there is a resource crunch on the node. This is when kubelet starts or when static pods are admitted onto the node but there is very good chance that some 'high priority'(not critical ones) pods are evicted from the node as preemption logic there is not priority aware.(except for critical pods).