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

[WIP] add a fallback for kube-scheduler when events.k8s.io is disabled #83692

Open
wants to merge 1 commit into
base: master
from

Conversation

@yastij
Copy link
Member

commented Oct 9, 2019

Signed-off-by: Yassine TIJANI ytijani@vmware.com

What type of PR is this?

/kind bug

What this PR does / why we need it: This adds a fallback to kube-scheduler when events.k8s.io v1beta1 is disabled. scheduler will fallback to core/v1

Which issue(s) this PR fixes: Fixes #83203

Special notes for your reviewer:

this is a WIP. There are two approaches:

  • embedding two recorders in the scheduler
  • reworking the events/ package to implement a fallback mechanism

this PR implements the first option. The second one would require reworking extensively the new implementation, but open for suggestions.

/assign @liggitt @wojtek-t

Does this PR introduce a user-facing change?:

kube-scheduler now fallbacks to emitting events using core/v1 Events when events.k8s.io/v1beta1 is disabled.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:


Signed-off-by: Yassine TIJANI <ytijani@vmware.com>
@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: yastij
To complete the pull request process, please assign k82cn
You can assign the PR to them by writing /assign @k82cn 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

recorder := eventBroadcaster.NewRecorder(scheme.Scheme, c.ComponentConfig.SchedulerName)
leaderElectionBroadcaster := record.NewBroadcaster()
leaderElectionRecorder := leaderElectionBroadcaster.NewRecorder(scheme.Scheme, corev1.EventSource{Component: c.ComponentConfig.SchedulerName})
if _, err := client.Discovery().ServerResourcesForGroupVersion(eventsv1beta1.SchemeGroupVersion.String()); err == nil {

This comment has been minimized.

Copy link
@liggitt

liggitt Oct 9, 2019

Member

were we making other API calls to the server in Config() previously?

This comment has been minimized.

Copy link
@yastij

yastij Oct 9, 2019

Author Member

no we were not IIRC. If there's an error the v1beta1 related fields would be nil -> fallback. Are you concerned about adding an API call for building sheduler's config ?

This comment has been minimized.

Copy link
@wojtek-t

wojtek-t Oct 10, 2019

Member

@liggitt - can you explain why do you think it's a problem?

This comment has been minimized.

Copy link
@liggitt

liggitt Oct 11, 2019

Member

It removes our ability to construct Config without a live server present. It's normal to make config, then start the process, and expect API calls to not start happening or being required until Run is called

if _, err := client.Discovery().ServerResourcesForGroupVersion(eventsv1beta1.SchemeGroupVersion.String()); err == nil {
c.Broadcaster = events.NewBroadcaster(&events.EventSinkImpl{Interface: eventClient.EventsV1beta1().Events("")})
c.Recorder = c.Broadcaster.NewRecorder(scheme.Scheme, c.ComponentConfig.SchedulerName)
c.EventClient = eventClient.EventsV1beta1()

This comment has been minimized.

Copy link
@liggitt

liggitt Oct 9, 2019

Member

do all call sites tolerate these three fields being nil?

This comment has been minimized.

Copy link
@yastij

yastij Oct 9, 2019

Author Member

not yet, but idea is to do so

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2019

@yastij: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-bazel-test d22e6e3 link /test pull-kubernetes-bazel-test
pull-kubernetes-typecheck d22e6e3 link /test pull-kubernetes-typecheck
pull-kubernetes-integration d22e6e3 link /test pull-kubernetes-integration
pull-kubernetes-verify d22e6e3 link /test pull-kubernetes-verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

@danielqsj

This comment has been minimized.

Copy link
Member

commented Oct 12, 2019

/cc

@k8s-ci-robot k8s-ci-robot requested a review from danielqsj Oct 12, 2019
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.