Skip to content

[NPM] Caching Pod information to compare against desired state #780

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
Merged
merged 18 commits into from
Mar 2, 2021
Merged

[NPM] Caching Pod information to compare against desired state #780

merged 18 commits into from
Mar 2, 2021

Conversation

@vakalapa
Copy link
Contributor

@vakalapa vakalapa commented Feb 8, 2021 •
edited
Loading

This PR includes below changes:

  1. Adding Caching ability for updated Pods to help relieve pressure on update events
  2. optimizing named ports code path for uniformity
  3. Removing generic PodMap and maintaining a NameSpace specific podmap
  4. ResourceVersions checks for Namespaces, Pods and Network Policies

@codecov
Copy link

codecov bot commented Feb 8, 2021 •
edited
Loading

Codecov Report

Merging #780 (833552f) into master (6586840) will increase coverage by 4.36%.
The diff coverage is 65.31%.

@@            Coverage Diff             @@
##           master     #780      +/-   ##
==========================================
+ Coverage   37.88%   42.24%   +4.36%     
==========================================
  Files         142      143       +1     
  Lines       15741    14024    -1717     
==========================================
- Hits         5963     5925      -38     
+ Misses       9043     7385    -1658     
+ Partials      735      714      -21

vakalapa
vakalapa commented Feb 16, 2021
View reviewed changes
@vakalapa vakalapa added enhancement npm Related to NPM. labels Feb 22, 2021
@vakalapa vakalapa marked this pull request as ready for review February 22, 2021 20:37
@vakalapa
Copy link
Contributor Author

vakalapa commented Feb 22, 2021

/azp run

@azure-pipelines
Copy link

azure-pipelines bot commented Feb 22, 2021

Azure Pipelines successfully started running 1 pipeline(s).

matmerr
matmerr reviewed Feb 24, 2021
View reviewed changes
npm/util/const.go

IpsetUDPFlag string = "udp:"
IpsetSCTPFlag string = "sctp:"
IpsetTCPFlag string = "tcp:"
Copy link
Member

@matmerr matmerr Feb 24, 2021

Choose a reason for hiding this comment

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

yeah how was this even missing :D

neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
@neaggarwMS
Copy link
Member

neaggarwMS commented Feb 24, 2021

if oldNsNs != newNsNs {

Shouldnt we check resourceVersion before this?

Refers to: npm/namespace.go:182 in 62af54d. [](commit_id = 62af54d, deletion_comment = False)

neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
npm/namespace.go
var err error
oldNsNs, oldNsLabel := "ns-"+oldNsObj.ObjectMeta.Name, oldNsObj.ObjectMeta.Labels
newNsNs, newNsLabel := "ns-"+newNsObj.ObjectMeta.Name, newNsObj.ObjectMeta.Labels
oldNsNs, oldNsLabel := util.GetNSNameWithPrefix(oldNsObj.ObjectMeta.Name), oldNsObj.ObjectMeta.Labels
Copy link
Member

@neaggarwMS neaggarwMS Feb 24, 2021

Choose a reason for hiding this comment

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

We should pick the oldNSObj from cache

Copy link
Contributor Author

@vakalapa vakalapa Feb 24, 2021

Choose a reason for hiding this comment

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

As discussed, we will creating a new Work item for these changes

@neaggarwMS
Copy link
Member

neaggarwMS commented Feb 24, 2021 

  if err = npMgr.DeleteNamespace(oldNsObj); err != nil {

Similarly delete all what we have in cache

Refers to: npm/namespace.go:183 in 62af54d. [](commit_id = 62af54d, deletion_comment = False)

@neaggarwMS
Copy link
Member

neaggarwMS commented Feb 24, 2021

if oldNsNs != newNsNs {

Is this check possible? I thought Resource Names are immutable

Refers to: npm/namespace.go:182 in 62af54d. [](commit_id = 62af54d, deletion_comment = False)

neaggarwMS
neaggarwMS previously requested changes Feb 24, 2021
View reviewed changes
Copy link
Member

@neaggarwMS neaggarwMS left a comment

Choose a reason for hiding this comment

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

🕐

@neaggarwMS
Copy link
Member

neaggarwMS commented Feb 24, 2021

}

Shouldnt this check be done after IsHostNetwork?

Refers to: npm/pod.go:192 in 62af54d. [](commit_id = 62af54d, deletion_comment = False)

neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
neaggarwMS
neaggarwMS reviewed Feb 24, 2021
View reviewed changes
@neaggarwMS neaggarwMS dismissed their stale review February 24, 2021 17:59

revoking review

neaggarwMS
neaggarwMS requested changes Feb 24, 2021
View reviewed changes
Copy link
Member

@neaggarwMS neaggarwMS left a comment

Choose a reason for hiding this comment

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

🕐

matmerr
matmerr reviewed Mar 1, 2021
View reviewed changes
npm/util/util.go
if rv == "" {
return 0
}
rvInt, err := strconv.ParseUint(rv, 10, 64)
Copy link
Member

@matmerr matmerr Mar 1, 2021

Choose a reason for hiding this comment

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

should potentially store resource version as uint if we're comparing every time, can take that in a later change

neaggarwMS
neaggarwMS approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member

@neaggarwMS neaggarwMS left a comment

Choose a reason for hiding this comment

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

:shipit:

@vakalapa vakalapa merged commit b93e4cc into master Mar 2, 2021
@vakalapa vakalapa deleted the vakr/podcache branch March 2, 2021 03:43
vakalapa pushed a commit that referenced this pull request Mar 2, 2021
Revert "[NPM] Caching Pod information to compare against desired state (
daf3a2f 
#780)"

This reverts commit b93e4cc.
@vakalapa vakalapa mentioned this pull request Mar 2, 2021
Merged
neaggarwMS pushed a commit that referenced this pull request Mar 2, 2021
@vakalapa
Revert "[NPM] Caching Pod information to compare against desired state (
1e26d07 
#780)" (#810)

This reverts commit b93e4cc.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matmerr matmerr matmerr approved these changes

@neaggarwMS neaggarwMS neaggarwMS approved these changes

@csfmomo csfmomo Awaiting requested review from csfmomo

Assignees

No one assigned

Labels

enhancement npm Related to NPM.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vakalapa @neaggarwMS @matmerr