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.

Sign up for GitHub

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

Jump to bottom

KEP-3386: Graduate Evented PLEG to Beta #3817

Merged
merged 1 commit into from
Feb 8, 2023
Merged

KEP-3386: Graduate Evented PLEG to Beta #3817

merged 1 commit into from
Feb 8, 2023

Conversation

harche
Copy link
Contributor

@harche harche commented Feb 1, 2023

Signed-off-by: Harshal Patil harpatil@redhat.com

  • One-line PR description: Graduate Evented PLEG to Beta
  • Other comments:

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory labels Feb 1, 2023
@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 1, 2023
@harche harche mentioned this pull request Feb 1, 2023
Open
25 tasks
@harche harche force-pushed the evented_pleg_beta branch from 8e9018d to 74fad2b Compare February 1, 2023 18:59
@harche harche changed the title Update Evented PLEG to Beta KEP-3386: Update Evented PLEG to Beta Feb 1, 2023
@harche harche force-pushed the evented_pleg_beta branch 3 times, most recently from ff61a1d to 90014a3 Compare February 1, 2023 19:06
@harche harche changed the title KEP-3386: Update Evented PLEG to Beta KEP-3386: Graduate Evented PLEG to Beta Feb 1, 2023
@mrunalp
Copy link
Contributor

mrunalp commented Feb 1, 2023

/lgtm

@harche harche force-pushed the evented_pleg_beta branch from 90014a3 to f7ffdb5 Compare February 1, 2023 19:09
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 1, 2023
@mrunalp
Copy link
Contributor

mrunalp commented Feb 1, 2023

@derekwaynecarr ptal

@mrunalp
Copy link
Contributor

mrunalp commented Feb 1, 2023

/label lead-opted-in

@mrunalp
Copy link
Contributor

mrunalp commented Feb 1, 2023

/milestone v1.27

@k8s-ci-robot k8s-ci-robot added this to the v1.27 milestone Feb 1, 2023
@k8s-ci-robot k8s-ci-robot added the lead-opted-in Denotes that an issue has been opted in to a release label Feb 1, 2023
@harche harche force-pushed the evented_pleg_beta branch from f7ffdb5 to 428912c Compare February 2, 2023 15:43
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2023
@rphillips
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 2, 2023
@derekwaynecarr
Copy link
Member

/approve

ruiwen-zhao
ruiwen-zhao reviewed Feb 3, 2023
View reviewed changes
keps/sig-node/3386-kubelet-evented-pleg/README.md
@@ -341,6 +344,10 @@ We expect no non-infra related flakes in the last month as a GA graduation crite
- Feature implemented behind a feature flag
- Existing `node e2e` tests around pod lifecycle must pass

#### Beta
- Add E2E Node Conformance presubmit job in CI
- Add E2E Node Conformance periodic job in CI
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this would require containerd support I believe. Evented PLEG related support is in containerd 1.7, but I think we have a testgrid testing against main branch of containerd, which should be sufficient.

jeremyrickard
jeremyrickard reviewed Feb 3, 2023
View reviewed changes
Copy link
Contributor

@jeremyrickard jeremyrickard left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there, I'm doing a first pass of some PRR reviews this cycle and I see this one hasn't been updated for beta. I think it looks pretty good, but it would be useful to include a little more context around testing rollback and upgrade. In the current PRR you mentioned N/A for alpha release, but as you're targeting beta now it would be nice to include some information about upgrade and rollback testing since this will be enabled.

It would also be nice to reference any enablement/disablement unit tests that were added during the alpha work (the PRR template mentions they would be added, linking to them now assuming they were would be helpful).

There is a new question in the scalability section. Could you add that to this PR and answer the question (see 78200cb for the change).

@harche harche force-pushed the evented_pleg_beta branch from 428912c to fb49693 Compare February 7, 2023 15:11
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 7, 2023
@harche harche force-pushed the evented_pleg_beta branch from fb49693 to 569427b Compare February 7, 2023 15:14
@harche
Copy link
Contributor Author

harche commented Feb 7, 2023

Hey there, I'm doing a first pass of some PRR reviews this cycle and I see this one hasn't been updated for beta. I think it looks pretty good, but it would be useful to include a little more context around testing rollback and upgrade. In the current PRR you mentioned N/A for alpha release, but as you're targeting beta now it would be nice to include some information about upgrade and rollback testing since this will be enabled.

It would also be nice to reference any enablement/disablement unit tests that were added during the alpha work (the PRR template mentions they would be added, linking to them now assuming they were would be helpful).

There is a new question in the scalability section. Could you add that to this PR and answer the question (see 78200cb for the change).

Thanks for your review. I have addressed those points in latest changes.

@deads2k
Copy link
Contributor

deads2k commented Feb 7, 2023

The What specific metrics should inform a rollback? looks light to me. For alpha, it was ok to say, "check this value on the host", but for beta we should have metrics to assess the health of the evented PLEG connection. Metrics for error counts, successful connections, latency, and notifications received all seem plausible to expect and could inform cluster-admins about the health of their nodes using this feature.

@harche
KEP-3386: Graduate Evented PLEG to Beta
0a85b19 
Signed-off-by: Harshal Patil <harpatil@redhat.com>
@harche harche force-pushed the evented_pleg_beta branch from 569427b to 0a85b19 Compare February 7, 2023 21:45
@harche
Copy link
Contributor Author

harche commented Feb 7, 2023

The What specific metrics should inform a rollback? looks light to me. For alpha, it was ok to say, "check this value on the host", but for beta we should have metrics to assess the health of the evented PLEG connection. Metrics for error counts, successful connections, latency, and notifications received all seem plausible to expect and could inform cluster-admins about the health of their nodes using this feature.

Thanks @deads2k. I updated the PR with metrics for evented PLEG's connection with the CRI Runtime.

@deads2k
Copy link
Contributor

deads2k commented Feb 8, 2023

Thanks @deads2k. I updated the PR with metrics for evented PLEG's connection with the CRI Runtime.

Thanks, PRR looks good.

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, derekwaynecarr, harche

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2023
@rphillips
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2023
@k8s-ci-robot k8s-ci-robot merged commit d084a97 into kubernetes:master Feb 8, 2023
@harche harche deleted the evented_pleg_beta branch February 8, 2023 17:40
@harche harche mentioned this pull request Mar 8, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeremyrickard jeremyrickard jeremyrickard left review comments

@ruiwen-zhao ruiwen-zhao ruiwen-zhao left review comments

@pacoxu pacoxu pacoxu left review comments

@dchen1107 dchen1107 Awaiting requested review from dchen1107

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@rphillips rphillips

@mrunalp mrunalp

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lead-opted-in Denotes that an issue has been opted in to a release lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
SIG Node: code and documentation PRs
Archived in project
Milestone
v1.27
Development

Successfully merging this pull request may close these issues.
9 participants
@harche @mrunalp @rphillips @derekwaynecarr @deads2k @k8s-ci-robot @pacoxu @ruiwen-zhao @jeremyrickard