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

sync Service API status rest storage #120623

Merged
merged 1 commit into from
Sep 14, 2023
Merged

sync Service API status rest storage #120623

merged 1 commit into from
Sep 14, 2023

Conversation

aojea
Copy link
Member

@aojea aojea commented Sep 13, 2023 •
edited

The Service API Rest implementation is complex and has to use different hooks on the REST storage. The status store was making a shallow copy of the storage before adding the hooks, so it was not inheriting the hooks.

The status store must have the same hooks as the rest store to be able to handle correctly the allocation and deallocation of ClusterIPs and nodePorts.

/kind bug
Fixes #120536

Fixes a bug where Services using finalizers may hold onto ClusterIP and/or NodePort allocated resources for longer than expected if the finalizer is removed using the status subresource

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 13, 2023
@k8s-ci-robot
Copy link
Contributor

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Sep 13, 2023
@aojea
Copy link
Member Author

aojea commented Sep 13, 2023

/assign @thockin
/cc @liggitt

you are going to love this one :)

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 13, 2023
@aojea
Copy link
Member Author

aojea commented Sep 13, 2023

/sig network

@k8s-ci-robot k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. area/test sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 13, 2023
@aojea aojea mentioned this pull request Sep 13, 2023
Closed
@aojea
sync Service API status rest storage
21e2648 
The Service API Rest implementation is complex and has to use different
hooks on the REST storage. The status store was making a shallow copy of
the storage before adding the hooks, so it was not inheriting the hooks.

The status store must have the same hooks as the rest store to be able
to handle correctly the allocation and deallocation of ClusterIPs and
nodePorts.

Change-Id: I44be21468d36017f0ec41a8f912b8490f8f13f55
Signed-off-by: Antonio Ojea <[email protected]>
@aojea
Copy link
Member Author

aojea commented Sep 13, 2023 

[FAILED] pod: "initcontinar-oomkill-target-pod", container: "oomkill-target-init-container" has unexpected reason: "Error"
Expected
    <string>: Error
to equal
    <string>: OOMKilled

unrelated
/test pull-kubernetes-node-e2e-containerd

@aojea aojea mentioned this pull request Sep 13, 2023
Closed
@thockin
Copy link
Member

thockin commented Sep 13, 2023

hmm, I wonder if anyone else hits this..

@thockin
Copy link
Member

thockin commented Sep 13, 2023

Seems not. Yet again, Service out here, doing weird stuff on our own. It makes me question the design of subresources being wholly independent from the main resource. Seems like a trap.

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2023
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: e988b25a4315c46e2dc4777853746bca1f83ad3c

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: aojea, thockin

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

@lojies
Copy link
Contributor

lojies commented Sep 14, 2023

/test pull-kubernetes-e2e-kind

@k8s-ci-robot k8s-ci-robot merged commit 716b8b9 into kubernetes:master Sep 14, 2023
15 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.29 milestone Sep 14, 2023
@aojea
Copy link
Member Author

aojea commented Sep 14, 2023

hmm, I wonder if anyone else hits this..

everyone using loadbalancers #120536 , the clusterip and nodeport reconcilers saved us multiple times, I'm going to add a metrics to expose how many times so we can be more proactive

This was referenced Sep 14, 2023
Merged
Merged
Merged
Merged
@liggitt liggitt added the kind/regression Categorizes issue or PR as related to a regression from a prior release. label Sep 14, 2023
k8s-ci-robot added a commit that referenced this pull request Sep 21, 2023
@k8s-ci-robot
Merge pull request #120655 from aojea/automated-cherry-pick-of-#12062…
74be421 
…3-upstream-release-1.27

Automated cherry pick of #120623: sync Service API status rest storage
k8s-ci-robot added a commit that referenced this pull request Sep 21, 2023
@k8s-ci-robot
Merge pull request #120656 from aojea/automated-cherry-pick-of-#12062…
fc411f9 
…3-upstream-release-1.26

Automated cherry pick of #120623: sync Service API status rest storage
k8s-ci-robot added a commit that referenced this pull request Sep 21, 2023
@k8s-ci-robot
Merge pull request #120654 from aojea/automated-cherry-pick-of-#12062…
dabe116 
…3-upstream-release-1.28

Automated cherry pick of #120623: sync Service API status rest storage
k8s-ci-robot added a commit that referenced this pull request Sep 21, 2023
@k8s-ci-robot
Merge pull request #120657 from aojea/automated-cherry-pick-of-#12062…
b3d85ce 
…3-upstream-release-1.25

Automated cherry pick of #120623: sync Service API status rest storage
@aojea aojea mentioned this pull request Sep 23, 2023
Merged
@aojea aojea mentioned this pull request Oct 7, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liggitt liggitt Awaiting requested review from liggitt

@thockin thockin Awaiting requested review from thockin

@wojtek-t wojtek-t Awaiting requested review from wojtek-t

Assignees

@thockin thockin

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. kind/regression Categorizes issue or PR as related to a regression from a prior release. lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.29
Development

Successfully merging this pull request may close these issues.

service nodeport cannot be released promptly.
5 participants
@aojea @k8s-ci-robot @thockin @lojies @liggitt