DataStore.GetStats() refactoring to simplify adding new fields #1704

veshij · 2021-10-21T07:20:22Z

What type of PR is this?: cleanup
What does this PR do / Why do we need it: In order to implement #1700 we need to add additional field to GetStats response.
Refactoring code to return struct instead list of ints to simplify future updates and decrease size of diffs.
Also adding AvailableAddresses() function implementing existing logic - since there are many places in the codebase where this number is calculated it makes sense to have in implemented as a function. AvailableAddresses() will be updated in following PR to treat cooldown IPs as 'taken'.
Expected to be a no-op change.

Testing done on this change: Y
Automation added to e2e: N
Will this break upgrades or downgrades. Has updating a running cluster been tested?: N/N
Does this change require updates to the CNI daemonset config files to work?: N
Does this PR introduce any user-facing change?: N (slightly changes log messages)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

haouc · 2021-10-26T19:06:23Z

Thanks for the efforts to clean up the code. I have some comments.

pkg/ipamd/datastore/data_store_test.go

pkg/ipamd/ipamd.go

haouc

Thanks for updating the PR. Please check the comment.

veshij · 2021-11-04T19:28:16Z

Can I get a review for the change pls? There will be one more on top of this diff which actually fixes the #1700

haouc · 2021-11-04T21:41:00Z

Hey @veshij, thanks for the update. Since this is one of series of changes that will have impacts on critical paths of VPC CNI, we have brought two more pairs of eyes to review your changes.

veshij · 2021-11-05T00:44:12Z

Hey @veshij, thanks for the update. Since this is one of series of changes that will have impacts on critical paths of VPC CNI, we have brought two more pairs of eyes to review your changes.

Thanks for update. This change itself should not change the behavior, next one will.
I'll publish it once this one is merged (I'm not sure there is a way to publish "stacked" PRs from forked repo).

veshij · 2021-11-09T20:11:45Z

@haouc gentle ping on the change. Or maybe you have some date in mind when I should come back to check status?

haouc · 2021-11-11T20:44:12Z

@veshij
Sorry for the delay. The changes look good to me. Thanks for making efforts to clean up the code. Let's wait for a bit for comments from other reviewers.

veshij · 2021-11-17T22:49:42Z

@veshij Sorry for the delay. The changes look good to me. Thanks for making efforts to clean up the code. Let's wait for a bit for comments from other reviewers.

It's been almost a month since I published this PR. I'd appreciate if someone can take a look and either request some changes or merge it.
Thank you!

cgchinmay · 2021-11-18T07:55:59Z

@veshij Sorry for the delay. The changes look good to me. Thanks for making efforts to clean up the code. Let's wait for a bit for comments from other reviewers.

It's been almost a month since I published this PR. I'd appreciate if someone can take a look and either request some changes or merge it. Thank you!

Taking a look

cgchinmay

Have couple of minor comments, yet to review other files

pkg/ipamd/datastore/data_store.go

veshij · 2021-11-18T09:15:14Z

Thank you, will take a look first thing tomorrow Чт, 18 нояб. 2021 г. в 01:11, cgchinmay ***@***.***>:

***@***.**** commented on this pull request. Have couple of minor comments, yet to review other files ------------------------------ In pkg/ipamd/datastore/data_store.go <#1704 (comment)> : > + AssignedIPs int + // Number of addresses in cooldown + CooldownIPs int +} + +func (stats DataStoreStats) String() string { + return fmt.Sprintf("Total IPs/Prefixes = %d/%d, AssignedIPs/CooldownIPs: %d/%d", + stats.TotalIPs, stats.TotalPrefixes, stats.AssignedIPs, stats.CooldownIPs) +} + +func (stats *DataStoreStats) AvailableAddresses() int { + return stats.TotalIPs - stats.AssignedIPs +} + +// GetStats returns DataStoreStats for addressFamily +func (ds *DataStore) GetStats(addressFamily string) DataStoreStats { [Non-blocking] nit: Can we call this GetIPStats ? or change GetIPStatsFromCidr to GetStatsFromCidr so we have consistent meaning about Stats. ------------------------------ In pkg/ipamd/datastore/data_store.go <#1704 (comment)> : > @@ -832,32 +836,53 @@ func (ds *DataStore) unassignPodIPAddressUnsafe(addr *AddressInfo) { assignedIPs.Set(float64(ds.assigned)) } -// GetStats returns total number of IP addresses, number of assigned IP addresses, total prefixes and IPs in cooldown period -func (ds *DataStore) GetStats(addressFamily string) (int, int, int, int) { +type DataStoreStats struct { + // Total number of addresses allocated + TotalIPs int + // Total number of prefixes allocated + TotalPrefixes int + + // Number of assigned addresses + AssignedIPs int + // Number of addresses in cooldown + CooldownIPs int +} + +func (stats DataStoreStats) String() string { Any reason for not using stats *DataStoreStats ? since we have this pattern for other methods — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1704 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABOURULQ57VLBCUCGRVALTDUMS7L7ANCNFSM5GNMTC6A> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

-- Oleg Guba

pkg/ipamd/ipamd.go

cgchinmay · 2021-11-18T19:12:48Z

/lgtm except those 3 suggestions

jayanthvn · 2021-11-23T00:07:50Z

@cgchinmay will review the updated files. Thanks @veshij

pkg/ipamd/ipamd.go

pkg/ipamd/datastore/data_store.go

cgchinmay

nit: minor change about comments

pkg/ipamd/datastore/data_store_test.go

cgchinmay

/lgtm

jayanthvn · 2021-11-25T00:59:19Z

/lgtm

This VlanId will appear in the prevResult during cmdDel request CleanUp Pod Network using vlanId from prevResult in CNI itself No need to call ipamd Log formatting changes Added hostNetworking Setup test for pods using security groups Updated cleanUpPodENI method Skip processing Delete request if prevResult is nil Add Logging vlanId to ipamd Add support to test with containerd nodegroup in pod-eni test Add check for empty Netns() in cni Manifests and Readme updates (aws#1732) * Manifests and Readme updates * update manifest.jsonnet Readme updates (aws#1735) Updates to troubleshooting doc (aws#1737) * Updates to troubleshooting doc * updates to troubleshooting doc imdsv2 changes (aws#1743) fix flaky canary test (aws#1742) add CODEOWNERS (aws#1747) Snat tests: [agent is already updated] (aws#1513) * resolved conflicts with go.sum * Updated test agent image * Removed redundant files * Addressed PR comments Fixed go.sum in root folder Changed DescribeInstanceWithFilter to DescribeInstances Moved GetPrimaryInstanceId from ec2 interface Added GinkgoWriter Updated Readme for Snat test Rearranged snat_test logic Updated Readme for test/e2e * Minor change to logging Fix compilation errors (aws#1751) add support for running canary script in different regions (aws#1752) Regenerate pod eni values for new instance types (aws#1754) * Regenerate pod eni values for new instance types Co-authored-by: Senthil Kumaran <senthilx@amazon.com> Minor change to container runtime argument Check for Empty NetNs() first Fallback to older method if prevResult is nil Closed issue message (aws#1761) * closed issue message * update message fix typo in upload script (aws#1763) Update calico file path Use an unique s3 bucket name (aws#1760) Update region Workflow to build arm and x86 images (aws#1764) DataStore.GetStats() refactoring to simplify adding new fields (aws#1704) * DataStore.GetStats() refactoring to simplify adding new fields * cleanup * cleanup * cleanup * goimports * rename test to TestGetStatsV4 * address comments * fix typo * update * update "IP pool is too low" logging * GetStats() -> GetIpStats() * GetStats() -> GetIpStats() in tests and comments * update test * cleanup test * add logPoolStats comment Fix KOPS_STATE_STORE (aws#1770) Automation script for running IT (aws#1759) Update issue template Update issue template with email address Update issue template Update go.mod for integration folder (aws#1741) * Update go.mod for integration folder - Update go.mod for integration folder * Change integration test to use new K8s test framework * Modify server pod image * Switch to Nginx port 80 for server pod * Switch server port in client test * Remove custom command directive for Nginx pod * Added ping command for host checks README: mention arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy (aws#1768) Co-authored-by: Shreya027 <shrenaik@amazon.com> Add dl1.24xlarge to ENILimits override list (aws#1777) Chart and Manifest updates (aws#1771) * Chart and Manifest updates * Update probe timeout values Change workflow to use git install (aws#1785) - Change workflow to use git install as the go get command was altering go.mod file without updating go.sum file

Add VlanId in the cmdAdd Result struct This VlanId will appear in the prevResult during cmdDel request CleanUp Pod Network using vlanId from prevResult in CNI itself No need to call ipamd Log formatting changes Added hostNetworking Setup test for pods using security groups Updated cleanUpPodENI method Skip processing Delete request if prevResult is nil Add Logging vlanId to ipamd Add support to test with containerd nodegroup in pod-eni test Add check for empty Netns() in cni Manifests and Readme updates (aws#1732) * Manifests and Readme updates * update manifest.jsonnet Readme updates (aws#1735) Updates to troubleshooting doc (aws#1737) * Updates to troubleshooting doc * updates to troubleshooting doc imdsv2 changes (aws#1743) fix flaky canary test (aws#1742) add CODEOWNERS (aws#1747) Snat tests: [agent is already updated] (aws#1513) * resolved conflicts with go.sum * Updated test agent image * Removed redundant files * Addressed PR comments Fixed go.sum in root folder Changed DescribeInstanceWithFilter to DescribeInstances Moved GetPrimaryInstanceId from ec2 interface Added GinkgoWriter Updated Readme for Snat test Rearranged snat_test logic Updated Readme for test/e2e * Minor change to logging Fix compilation errors (aws#1751) add support for running canary script in different regions (aws#1752) Regenerate pod eni values for new instance types (aws#1754) * Regenerate pod eni values for new instance types Co-authored-by: Senthil Kumaran <senthilx@amazon.com> Minor change to container runtime argument Check for Empty NetNs() first Fallback to older method if prevResult is nil Closed issue message (aws#1761) * closed issue message * update message fix typo in upload script (aws#1763) Update calico file path Use an unique s3 bucket name (aws#1760) Update region Workflow to build arm and x86 images (aws#1764) DataStore.GetStats() refactoring to simplify adding new fields (aws#1704) * DataStore.GetStats() refactoring to simplify adding new fields * cleanup * cleanup * cleanup * goimports * rename test to TestGetStatsV4 * address comments * fix typo * update * update "IP pool is too low" logging * GetStats() -> GetIpStats() * GetStats() -> GetIpStats() in tests and comments * update test * cleanup test * add logPoolStats comment Fix KOPS_STATE_STORE (aws#1770) Automation script for running IT (aws#1759) Update issue template Update issue template with email address Update issue template Update go.mod for integration folder (aws#1741) * Update go.mod for integration folder - Update go.mod for integration folder * Change integration test to use new K8s test framework * Modify server pod image * Switch to Nginx port 80 for server pod * Switch server port in client test * Remove custom command directive for Nginx pod * Added ping command for host checks README: mention arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy (aws#1768) Co-authored-by: Shreya027 <shrenaik@amazon.com> Add dl1.24xlarge to ENILimits override list (aws#1777) Chart and Manifest updates (aws#1771) * Chart and Manifest updates * Update probe timeout values Change workflow to use git install (aws#1785) - Change workflow to use git install as the go get command was altering go.mod file without updating go.sum file

Fix compilation errors (aws#1751) add support for running canary script in different regions (aws#1752) Regenerate pod eni values for new instance types (aws#1754) * Regenerate pod eni values for new instance types Co-authored-by: Senthil Kumaran <senthilx@amazon.com> Closed issue message (aws#1761) * closed issue message * update message fix typo in upload script (aws#1763) Update calico file path Use an unique s3 bucket name (aws#1760) Update region Workflow to build arm and x86 images (aws#1764) DataStore.GetStats() refactoring to simplify adding new fields (aws#1704) * DataStore.GetStats() refactoring to simplify adding new fields * cleanup * cleanup * cleanup * goimports * rename test to TestGetStatsV4 * address comments * fix typo * update * update "IP pool is too low" logging * GetStats() -> GetIpStats() * GetStats() -> GetIpStats() in tests and comments * update test * cleanup test * add logPoolStats comment Fix KOPS_STATE_STORE (aws#1770) Automation script for running IT (aws#1759) Update issue template Update issue template with email address Update issue template Update go.mod for integration folder (aws#1741) * Update go.mod for integration folder - Update go.mod for integration folder * Change integration test to use new K8s test framework * Modify server pod image * Switch to Nginx port 80 for server pod * Switch server port in client test * Remove custom command directive for Nginx pod * Added ping command for host checks README: mention arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy (aws#1768) Co-authored-by: Shreya027 <shrenaik@amazon.com> Add dl1.24xlarge to ENILimits override list (aws#1777) Chart and Manifest updates (aws#1771) * Chart and Manifest updates * Update probe timeout values Change workflow to use git install (aws#1785) - Change workflow to use git install as the go get command was altering go.mod file without updating go.sum file

Fix compilation errors (aws#1751) add support for running canary script in different regions (aws#1752) Regenerate pod eni values for new instance types (aws#1754) * Regenerate pod eni values for new instance types Co-authored-by: Senthil Kumaran <senthilx@amazon.com> Closed issue message (aws#1761) * closed issue message * update message fix typo in upload script (aws#1763) Update calico file path Use an unique s3 bucket name (aws#1760) Update region Workflow to build arm and x86 images (aws#1764) DataStore.GetStats() refactoring to simplify adding new fields (aws#1704) * DataStore.GetStats() refactoring to simplify adding new fields * cleanup * cleanup * cleanup * goimports * rename test to TestGetStatsV4 * address comments * fix typo * update * update "IP pool is too low" logging * GetStats() -> GetIpStats() * GetStats() -> GetIpStats() in tests and comments * update test * cleanup test * add logPoolStats comment Fix KOPS_STATE_STORE (aws#1770) Automation script for running IT (aws#1759) Update issue template Update issue template with email address Update issue template Update go.mod for integration folder (aws#1741) * Update go.mod for integration folder - Update go.mod for integration folder * Change integration test to use new K8s test framework * Modify server pod image * Switch to Nginx port 80 for server pod * Switch server port in client test * Remove custom command directive for Nginx pod * Added ping command for host checks README: mention arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy (aws#1768) Co-authored-by: Shreya027 <shrenaik@amazon.com> Add dl1.24xlarge to ENILimits override list (aws#1777) Chart and Manifest updates (aws#1771) * Chart and Manifest updates * Update probe timeout values Change workflow to use git install (aws#1785) - Change workflow to use git install as the go get command was altering go.mod file without updating go.sum file Add HostNetworking Test for PPSG in test agent (aws#1720) * Add HostNetworking Test for PPSG in test agent * Updated PPSG test to validate vlan.eth link

…1715) * merge conflicts with cni-metrics-helper chart Fix compilation errors (#1751) add support for running canary script in different regions (#1752) Regenerate pod eni values for new instance types (#1754) * Regenerate pod eni values for new instance types Co-authored-by: Senthil Kumaran <senthilx@amazon.com> Closed issue message (#1761) * closed issue message * update message fix typo in upload script (#1763) Update calico file path Use an unique s3 bucket name (#1760) Update region Workflow to build arm and x86 images (#1764) DataStore.GetStats() refactoring to simplify adding new fields (#1704) * DataStore.GetStats() refactoring to simplify adding new fields * cleanup * cleanup * cleanup * goimports * rename test to TestGetStatsV4 * address comments * fix typo * update * update "IP pool is too low" logging * GetStats() -> GetIpStats() * GetStats() -> GetIpStats() in tests and comments * update test * cleanup test * add logPoolStats comment Fix KOPS_STATE_STORE (#1770) Automation script for running IT (#1759) Update issue template Update issue template with email address Update issue template Update go.mod for integration folder (#1741) * Update go.mod for integration folder - Update go.mod for integration folder * Change integration test to use new K8s test framework * Modify server pod image * Switch to Nginx port 80 for server pod * Switch server port in client test * Remove custom command directive for Nginx pod * Added ping command for host checks README: mention arn:aws:iam::aws:policy/AmazonEKS_CNI_Policy (#1768) Co-authored-by: Shreya027 <shrenaik@amazon.com> Add dl1.24xlarge to ENILimits override list (#1777) Chart and Manifest updates (#1771) * Chart and Manifest updates * Update probe timeout values Change workflow to use git install (#1785) - Change workflow to use git install as the go get command was altering go.mod file without updating go.sum file Add HostNetworking Test for PPSG in test agent (#1720) * Add HostNetworking Test for PPSG in test agent * Updated PPSG test to validate vlan.eth link * Minor change to logging CLUSTER_ID and Region values Fix for cni-metrics-helper failing integration test * Fixed merge conflicts * Readme update * Updated Readme with more description of AWS_CLUSTER_ID * minor change

) * DataStore.GetStats() refactoring to simplify adding new fields * cleanup * cleanup * cleanup * goimports * rename test to TestGetStatsV4 * address comments * fix typo * update * update "IP pool is too low" logging * GetStats() -> GetIpStats() * GetStats() -> GetIpStats() in tests and comments * update test * cleanup test * add logPoolStats comment

DataStore.GetStats() refactoring to simplify adding new fields

e4349a8

veshij mentioned this pull request Oct 21, 2021

Count addresses in cooldown against used ones in datastoreTargetState() #1700

Closed

veshij and others added 2 commits October 21, 2021 17:19

cleanup

1812817

Merge branch 'aws:master' into stats_update

48aed21

veshij marked this pull request as draft October 22, 2021 00:30

veshij marked this pull request as ready for review October 22, 2021 00:32

veshij mentioned this pull request Oct 22, 2021

Delayed unused IP address cleanup. veshij/amazon-vpc-cni-k8s#1

Closed

veshij added 3 commits October 21, 2021 23:12

cleanup

4d47f9b

cleanup

f625ef6

goimports

3787c1a

jayanthvn requested review from cgchinmay and haouc October 25, 2021 18:40

haouc reviewed Oct 26, 2021

View reviewed changes

pkg/ipamd/datastore/data_store_test.go Outdated Show resolved Hide resolved

pkg/ipamd/ipamd.go Outdated Show resolved Hide resolved

pkg/ipamd/ipamd.go Outdated Show resolved Hide resolved

veshij added 2 commits October 26, 2021 13:14

rename test to TestGetStatsV4

7dfff7b

address comments

e2d793a

haouc reviewed Oct 27, 2021

View reviewed changes

pkg/ipamd/ipamd.go Show resolved Hide resolved

haouc reviewed Oct 27, 2021

View reviewed changes

veshij added 2 commits October 28, 2021 18:01

fix typo

a8d5ffd

update

7561073

jayanthvn requested review from achevuru and M00nF1sh November 2, 2021 18:13

veshij requested a review from haouc November 3, 2021 20:46

cgchinmay reviewed Nov 18, 2021

View reviewed changes

pkg/ipamd/datastore/data_store.go Outdated Show resolved Hide resolved

pkg/ipamd/datastore/data_store.go Outdated Show resolved Hide resolved

cgchinmay reviewed Nov 18, 2021

View reviewed changes

pkg/ipamd/ipamd.go Outdated Show resolved Hide resolved

veshij added 2 commits November 22, 2021 15:52

update "IP pool is too low" logging

1144387

GetStats() -> GetIpStats()

270c7e7

veshij requested a review from a team as a code owner November 23, 2021 00:04

cgchinmay reviewed Nov 23, 2021

View reviewed changes

pkg/ipamd/ipamd.go Outdated Show resolved Hide resolved

cgchinmay reviewed Nov 23, 2021

View reviewed changes

pkg/ipamd/datastore/data_store.go Outdated Show resolved Hide resolved

cgchinmay reviewed Nov 23, 2021

View reviewed changes

veshij added 4 commits November 22, 2021 18:03

GetStats() -> GetIpStats() in tests and comments

ced4b5e

update test

e33f8e3

cleanup test

a6b9751

add logPoolStats comment

029bf5f

cgchinmay reviewed Nov 23, 2021

View reviewed changes

pkg/ipamd/datastore/data_store_test.go Show resolved Hide resolved

cgchinmay approved these changes Nov 23, 2021

View reviewed changes

jayanthvn approved these changes Nov 25, 2021

View reviewed changes

jayanthvn merged commit aad0415 into aws:master Nov 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DataStore.GetStats() refactoring to simplify adding new fields #1704

DataStore.GetStats() refactoring to simplify adding new fields #1704

veshij commented Oct 21, 2021 •

edited

haouc commented Oct 26, 2021

haouc left a comment

veshij commented Nov 4, 2021

haouc commented Nov 4, 2021

veshij commented Nov 5, 2021

veshij commented Nov 9, 2021

haouc commented Nov 11, 2021

veshij commented Nov 17, 2021

cgchinmay commented Nov 18, 2021

cgchinmay left a comment

veshij commented Nov 18, 2021 via email

cgchinmay commented Nov 18, 2021

jayanthvn commented Nov 23, 2021

cgchinmay left a comment •

edited

cgchinmay left a comment •

edited

jayanthvn commented Nov 25, 2021

DataStore.GetStats() refactoring to simplify adding new fields #1704

DataStore.GetStats() refactoring to simplify adding new fields #1704

Conversation

veshij commented Oct 21, 2021 • edited

haouc commented Oct 26, 2021

haouc left a comment

Choose a reason for hiding this comment

veshij commented Nov 4, 2021

haouc commented Nov 4, 2021

veshij commented Nov 5, 2021

veshij commented Nov 9, 2021

haouc commented Nov 11, 2021

veshij commented Nov 17, 2021

cgchinmay commented Nov 18, 2021

cgchinmay left a comment

Choose a reason for hiding this comment

veshij commented Nov 18, 2021 via email

cgchinmay commented Nov 18, 2021

jayanthvn commented Nov 23, 2021

cgchinmay left a comment • edited

Choose a reason for hiding this comment

cgchinmay left a comment • edited

Choose a reason for hiding this comment

jayanthvn commented Nov 25, 2021

veshij commented Oct 21, 2021 •

edited

cgchinmay left a comment •

edited

cgchinmay left a comment •

edited