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

Identity GC skips IDs that are used in CESs #25001

Merged
merged 1 commit into from Jun 7, 2023
Merged

Identity GC skips IDs that are used in CESs #25001

merged 1 commit into from Jun 7, 2023

Conversation

dlapcevic
Copy link
Contributor

@dlapcevic dlapcevic commented Apr 20, 2023 •
edited

When Cilium Endpoint Slices are enabled, Cilium Identities shouldn’t be deleted while they are still used by CES.

This is a fix in the operator to check if CESs from the CES store are using Identities before deleting Identities.

Fix for Identities that can be deleted before CESs are reconciled

Signed-off-by: Dorde Lapcevic <dordel@google.com>

@dlapcevic dlapcevic requested a review from a team as a code owner April 20, 2023 13:57
@dlapcevic dlapcevic requested a review from joamaki April 20, 2023 13:57
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 20, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Apr 20, 2023
@dlapcevic
Copy link
Contributor Author

cc @alan-kut

joamaki
joamaki requested changes Apr 24, 2023
View reviewed changes
operator/pkg/ciliumendpointslice/endpointslice.go Outdated Show resolved Hide resolved
operator/identitygc/crd_gc.go Outdated Show resolved Hide resolved
@dlapcevic
Copy link
Contributor Author

Thank you @joamaki for the review.
I made the suggested changes.

@dlapcevic dlapcevic requested a review from joamaki April 24, 2023 09:07
@joamaki joamaki added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Apr 24, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 24, 2023
@joamaki
Copy link
Contributor

joamaki commented Apr 24, 2023

/test-runtime

@joamaki
Copy link
Contributor

joamaki commented Apr 25, 2023 •
edited by maintainer-s-little-helper bot

/test-1.27-net-next

Job 'Cilium-PR-K8s-1.27-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing with L7 policy Tests NodePort with L7 Policy from outside

Failure Output

FAIL: Can not connect to service "http://192.168.56.11:31206" from outside cluster (2/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.27-kernel-net-next/115/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.27-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@dlapcevic
Copy link
Contributor Author

Hi @joamaki, both failed tests look like flakes.
Could you please rerun the tests and help me merge this?

@joestringer
Copy link
Member

joestringer commented May 5, 2023 •
edited

Typically we'll try to triage the flakes and identify whether there are already issues in the repo, and link them - the breadcrumbs help identify frequently-failing tests. Example:

I'll re-kick CI to get the full view on the PR.

@joestringer
Copy link
Member

/test

@joestringer
Copy link
Member

e2e test run hit #24965
k8s-1.27-kernel-net-next hit #15455 again

Three of the test jobs didn't trigger. You might need to rebase to get those to run.

@joestringer joestringer added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label May 5, 2023
@dlapcevic
Copy link
Contributor Author

Thanks. Rebased.

@dlapcevic
Copy link
Contributor Author

/test

@dlapcevic
Copy link
Contributor Author

Rebased again. Another flaky test should be fixed.

@dlapcevic
Copy link
Contributor Author

dlapcevic commented May 10, 2023 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.25-kernel-4.19' failed:

Click to show.

Test Name

K8sAgentPolicyTest Multi-node policy test with L7 policy using connectivity-check to check datapath

Failure Output

FAIL: cannot install connectivity-check

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-4.19/2095/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-4.19 so I can create one.

Then please upload the Jenkins artifacts to that issue.

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Request from k8s1 to service http://[::ffff:192.168.56.11]:31506 failed

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/2199/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

@dlapcevic
Copy link
Contributor Author

Two flaky tests.
@joestringer could you please help me merge it?

@joestringer
Copy link
Member

joestringer commented May 12, 2023 via email

@dlapcevic
Identity GC skips IDs that are used in CESs
d45e51d 
When Cilium Endpoint Slices are enabled, Cilium Identities shouldn’t be deleted while they are still used by CES.

This is a fix in the operator to check if CESs from the CES store are using Identities before deleting them.

```release-note
Fix for Identities that can be deleted before CESs are reconciled
```

Signed-off-by: Dorde Lapcevic <dordel@google.com>
@dlapcevic
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 6, 2023
@dylandreimerink dylandreimerink merged commit 05bb6ce into cilium:main Jun 7, 2023
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@alan-kut alan-kut alan-kut approved these changes

Assignees
No one assigned
Labels
dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@dlapcevic @joamaki @joestringer @alan-kut @dylandreimerink