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

Cache minimal data #52

Merged
merged 2 commits into from
Aug 12, 2024
Merged

Conversation

maiqueb
Copy link
Collaborator

@maiqueb maiqueb commented Aug 6, 2024

What this PR does / why we need it:
This PR prunes the unnecessary pod information before caching it in the informer.

We only need a subset of the metadata present in the pods, and we for sure do not care about its spec or status.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
This PR supersedes #14

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:

Cache only the required metadata for pods, thus using a lot less memory.

@kubevirt-bot kubevirt-bot added the dco-signoff: yes Indicates the PR's author has DCO signed all their commits. label Aug 6, 2024
@maiqueb maiqueb mentioned this pull request Aug 6, 2024
@maiqueb maiqueb closed this Aug 6, 2024
@maiqueb maiqueb reopened this Aug 6, 2024
@maiqueb maiqueb changed the base branch from cache-minimal-data to main August 6, 2024 10:34
@maiqueb
Copy link
Collaborator Author

maiqueb commented Aug 6, 2024

/test e2e

@oshoval
Copy link
Collaborator

oshoval commented Aug 6, 2024

"and we for sure do not care about its spec or status"

just making sure, once we will support hot unplug, we will use the VMI informer to get VMI.status
(or just poll) in order to see what ipamclaim we can delete (according multus infoSource)
so we wont need anything that was dropped in this PR because we use VMI not pod right?

@maiqueb
Copy link
Collaborator Author

maiqueb commented Aug 6, 2024

"and we for sure do not care about its spec or status"

just making sure, once we will support hot unplug, we will use the VMI informer to get VMI.status (or just poll) in order to see what ipamclaim we can delete (according multus infoSource) so we wont need anything that was dropped in this PR because we use VMI not pod right?

I am unsure as to how / when we'll support that, but surely we won't care about the pod's status or spec when implementing it.

And if we end up requiring it (for whatever reason), let's adapt the code to make it work.

Comment on lines +194 to +196
if !ok {
return obj, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

i am not familiar with Transform functions by heart
should it return error here ? or it is fine as you did because it can catch other types and if the cast failed it should just ignore and continue ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is common for all types which we use in controller runtime (pods / vmis / vms).

I just want to act on pods.

Copy link
Collaborator

@oshoval oshoval left a comment

Choose a reason for hiding this comment

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

Thanks

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 6, 2024
@oshoval
Copy link
Collaborator

oshoval commented Aug 11, 2024

also clean main branch failing
so not sure failures are related to this PR
#55

@oshoval
Copy link
Collaborator

oshoval commented Aug 11, 2024

#56
this fix the main branch problems

@oshoval
Copy link
Collaborator

oshoval commented Aug 12, 2024

please rebase, tests might work now
they were broken not due to this PR

We only need the pod name / namespace / annotations / labels.

Thus we prune everything else.

Signed-off-by: Miguel Duarte Barroso <[email protected]>
Signed-off-by: Miguel Duarte Barroso <[email protected]>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2024
@oshoval
Copy link
Collaborator

oshoval commented Aug 12, 2024

Thanks
/lgtm

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 12, 2024
@maiqueb
Copy link
Collaborator Author

maiqueb commented Aug 12, 2024

/approve

@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: maiqueb

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

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 12, 2024
@kubevirt-bot kubevirt-bot merged commit ae2d7b3 into kubevirt:main Aug 12, 2024
4 checks passed
@maiqueb maiqueb deleted the cache-minimal-data branch August 12, 2024 09:03
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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants