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

Create skeleton code for degraded mode procedure #1952

Merged
merged 2 commits into from
Mar 8, 2023

Conversation

sawsa307
Copy link
Contributor

@sawsa307 sawsa307 commented Feb 14, 2023

Create skeleton code for degraded mode procedure. Implementation for filling missing information and filter invalid pod will come in separate PRs.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 14, 2023
@k8s-ci-robot
Copy link
Contributor

Hi @sawsa307. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 14, 2023
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from 3bce024 to e8e77a4 Compare February 14, 2023 22:22
@sawsa307
Copy link
Contributor Author

/assign @swetharepakula

@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch 4 times, most recently from 43b7959 to 5a9c63b Compare February 16, 2023 01:11
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2023
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from 5a9c63b to e6ea78d Compare February 17, 2023 08:16
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 17, 2023
@sawsa307 sawsa307 changed the title Create skeleton for degraded mode procedure Create skeleton code for degraded mode procedure Feb 21, 2023
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 25, 2023
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from e6ea78d to 75a2c95 Compare February 28, 2023 01:24
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 28, 2023
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from 75a2c95 to 12c8597 Compare March 6, 2023 19:01
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 6, 2023
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch 2 times, most recently from 037b7c0 to 1fcc53a Compare March 6, 2023 19:56
endpointPodMap := negtypes.EndpointPodMap{}
for _, ed := range eds {
// skip custom endpoint slice so it won't block the neg api call from succeeding
if val, ok := ed.Meta.Labels[discovery.LabelManagedBy]; !ok || val != "endpointslice-controller.k8s.io" {
Copy link
Member

Choose a reason for hiding this comment

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

We should still validate custom endpoints. We should add them if they are valid pods.

Copy link
Member

Choose a reason for hiding this comment

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

this also means we won't be adding the customer endpoints to the NEG?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this part. Thanks!

@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from 1fcc53a to 0a32eb7 Compare March 6, 2023 21:38
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch 2 times, most recently from 344d3db to e92dc66 Compare March 7, 2023 22:33
t.Run(tc.desc, func(t *testing.T) {
testEndpointSlices := getTestEndpointSlices(testService, testNamespace)

testEndpointSlices[2].ObjectMeta.Labels[discovery.LabelManagedBy] = tc.managedByLabel
Copy link
Member

Choose a reason for hiding this comment

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

add this as a testcase instead of modifying the EPS after the fact.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. Removing this since the part that filter custom endpoint slice is deleted #1952 (comment).

Comment on lines 364 to 367
// validatePod checks if this pod is a valid pod resource
func validatePod(pod *apiv1.Pod) bool {
return true
}
Copy link
Member

Choose a reason for hiding this comment

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

Add this check in only when it is ready.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. Thanks!

pkg/neg/syncers/utils.go Show resolved Hide resolved
@sawsa307 sawsa307 requested review from swetharepakula and removed request for aojea and panslava March 7, 2023 23:21
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from e92dc66 to 6658372 Compare March 7, 2023 23:22
Copy link
Member

@swetharepakula swetharepakula left a comment

Choose a reason for hiding this comment

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

Last remaining comments are about logging and negative test cases. Otherwise the functionality looks good to me.

}
for _, endpointAddress := range ed.Addresses {
if endpointAddress.TargetRef == nil {
continue
Copy link
Member

Choose a reason for hiding this comment

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

add a log to indicate no matching targetRef

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added. Thanks!

pkg/neg/syncers/utils.go Show resolved Hide resolved
pkg/neg/syncers/utils.go Show resolved Hide resolved
pkg/neg/syncers/utils.go Show resolved Hide resolved
pkg/neg/syncers/utils_test.go Show resolved Hide resolved
@@ -1250,6 +1536,7 @@ func getTestEndpointSlices(name, namespace string) []*discovery.EndpointSlice {
notReady := false
emptyNamedPort := ""
testNamedPort := testNamedPort
managedByController := "endpointslice-controller.k8s.io"
Copy link
Member

Choose a reason for hiding this comment

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

why is this change necessary?

@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from 6658372 to 28b435a Compare March 8, 2023 01:07
@sawsa307 sawsa307 requested a review from swetharepakula March 8, 2023 01:08
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from 28b435a to 58410c5 Compare March 8, 2023 18:59
Copy link
Member

@swetharepakula swetharepakula left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Mar 8, 2023
@swetharepakula
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 8, 2023
Create skeleton for degraded mode procedure. It will be enabled with
enableDegradedMode flag, and they will be created in a different PR when
the implementation is done.
@sawsa307 sawsa307 force-pushed the degraded-mode-skeleton branch from 58410c5 to 155f260 Compare March 8, 2023 21:30
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 8, 2023
@swetharepakula
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 Mar 8, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sawsa307, swetharepakula

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 merged commit 6a840ae into kubernetes:master Mar 8, 2023
@sawsa307 sawsa307 deleted the degraded-mode-skeleton branch September 2, 2023 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. lgtm "Looks good to me", indicates that a PR is ready to be merged. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants