Further IPAM simplifcations by tgraf · Pull Request #10569 · cilium/cilium

tgraf · 2020-03-13T09:49:56Z

This PR includes further simplifications of the IPAM code to reduce the IPAM module-specific code and to simplify the introduction of additional modes.

This change is

maintainer-s-little-helper · 2020-03-13T09:49:58Z

Release note label not set, please set the appropriate release note.

maintainer-s-little-helper · 2020-03-13T09:49:59Z

Release note label not set, please set the appropriate release note.

maintainer-s-little-helper · 2020-03-13T09:49:59Z

Release note label not set, please set the appropriate release note.

maintainer-s-little-helper · 2020-03-13T09:49:59Z

Release note label not set, please set the appropriate release note.

maintainer-s-little-helper · 2020-03-13T09:49:59Z

Release note label not set, please set the appropriate release note.

coveralls · 2020-03-13T10:18:15Z

Coverage decreased (-0.02%) to 45.581% when pulling 274531b on pr/tgraf/ipam-simplifcations into 8462fe6 on master.

tgraf · 2020-03-13T10:44:32Z

test-me-please

tgraf · 2020-03-18T13:08:17Z

test-me-please

Single test failed:

 Suite-k8s-1.11.K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

tgraf · 2020-03-18T23:15:45Z

test-me-please

The field has been obsoleted in favour of Spec.IPAM.PreAllocate. It no longer makes sense to initialize this to the default if unset. This change is backwards compatible because even the operator is upgraded after a respective Cilium node, it would still fall back to this default value. The value in the Spec field is primiarly for documentation purposes when set to the default value. Signed-off-by: Thomas Graf <thomas@cilium.io>

The sole use of PopulateSpecFields() is to set Spec.ENI.PreAllocate. Given this field has been moved to Spec.IPAM.PreAllocate, it can be set in the generic IPAM layer and thus PopulateSpecFields() can be removed entirely. Signed-off-by: Thomas Graf <thomas@cilium.io>

The allocation limits have been moved into a generic section of the CilumNode resource so it is no longer required for the specific IPAM implementation to implement the retrieval of these values. This means that some ENI specific code will live in pkg/ipam for backwards compatibility, it can be removed in Cilium 1.9. This also resolves an indirect data race: It is now guarnteed and documented that n.resource is locked while accessing whereas previously, the pointer to the CiliumNode was unprotected in the IPAM specific node structure. It was indirectly protected via the n.mutex though. Signed-off-by: Thomas Graf <thomas@cilium.io>

All IPAM modes relying on external APIs require an instance ID of some sort. Instead of requiring each IPAM module to carry this in a separate sub-level spec, add it to the top-level. This allows to remove AzureSpec entirely and it also allows to remove LogFields() from the interface. Spec.ENI.InstanceID continues to be supported. Spec.Azure.InstanceID can be safely removed as it has not been released yet. Signed-off-by: Thomas Graf <thomas@cilium.io>

Some ENI references where left over due to basing this code off the ENI implementation. Remove them. Signed-off-by: Thomas Graf <thomas@cilium.io>

Ensure that deep copies are made for data being passed in to avoid cross contamination between tests. Signed-off-by: Thomas Graf <thomas@cilium.io>

Fixes: ``` WARNING: DATA RACE Read at 0x00c000340628 by goroutine 37: github.com/cilium/cilium/pkg/ipam.(*nodeOperationsMock).PrepareIPAllocation() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager_test.go:95 +0x123 github.com/cilium/cilium/pkg/ipam.(*Node).determineMaintenanceAction() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:475 +0x8ce github.com/cilium/cilium/pkg/ipam.(*Node).maintainIPPool() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:506 +0x53 github.com/cilium/cilium/pkg/ipam.(*Node).MaintainIPPool() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:572 +0x90 github.com/cilium/cilium/pkg/ipam.(*NodeManager).Update.func1() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager.go:210 +0x8b github.com/cilium/cilium/pkg/trigger.(*Trigger).waiter() /home/vagrant/go/src/github.com/cilium/cilium/pkg/trigger/trigger.go:206 +0x4b9 Previous write at 0x00c000340628 by goroutine 35: github.com/cilium/cilium/pkg/ipam.(*nodeOperationsMock).AllocateIPs() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager_test.go:101 +0xe0 github.com/cilium/cilium/pkg/ipam.(*Node).maintainIPPool() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:542 +0xa0d github.com/cilium/cilium/pkg/ipam.(*Node).MaintainIPPool() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node.go:572 +0x90 github.com/cilium/cilium/pkg/ipam.(*NodeManager).Update.func1() /home/vagrant/go/src/github.com/cilium/cilium/pkg/ipam/node_manager.go:210 +0x8b github.com/cilium/cilium/pkg/trigger.(*Trigger).waiter() /home/vagrant/go/src/github.com/cilium/cilium/pkg/trigger/trigger.go:206 +0x4b9 ``` Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf · 2020-03-20T22:47:03Z

test-me-please

tgraf · 2020-03-21T11:30:31Z

test-me-please

Flake:

Suite-k8s-1.11.K8sServicesTest Checks service across nodes Tests NodePort BPF Tests with direct routing Tests with MetalLB Connectivity to endpoint via LB

This commit adds shortName and all CRD names via markers. It also updates the Validation schema via markers which the old method of hardcoding had a few fields missing / incorrect. Here's a summary of the main differences in the validation schema: 1) Old schema had eniTypes.ENISpec.MaxAllocate, which doesn't exist. It exists under ipamTypes.IPAMSpec.MaxAllocate. 2) Old schema was missing eniTypes.ENISpec.{InstanceID,InstanceType}, which is now included in this new generated schema. 3) Old schema was missing EncryptionSpec, which is now included in this new generated schema. 4) Old schema has azureTypes.AzureSpec.InstanceID, but that was removed and deprecated in favor of ipamTypes.IPAMSpec.InstanceID. See PR which forgot to remove this from the old validation schema: #10569 This CRD is generated using https://book.kubebuilder.io/reference/generating-crd.html. Signed-off-by: Chris Tarazi <chris@isovalent.com>

This commit adds resource name generation via markers. It also updates the validation schema via markers which the old method of hardcoding had a few fields missing / incorrect. Here's a summary of the main differences in the validation schema: 1) Old schema had eniTypes.ENISpec.MaxAllocate, which doesn't exist. It exists under ipamTypes.IPAMSpec.MaxAllocate. 2) Old schema was missing eniTypes.ENISpec.{InstanceID,InstanceType}, which is now included in this new generated schema. 3) Old schema was missing EncryptionSpec, which is now included in this new generated schema. 4) Old schema has azureTypes.AzureSpec.InstanceID, but that was removed and deprecated in favor of ipamTypes.IPAMSpec.InstanceID. See PR which forgot to remove this from the old validation schema. [1] This CRD is generated using https://book.kubebuilder.io/reference/generating-crd.html. [1]: #10569 Signed-off-by: Chris Tarazi <chris@isovalent.com>

Remove deprecated fields that have been unused for 5+ years: * Azure `AzureInterface.GatewayIP` (obsolete since v1.10, see #15182) * AWS `ENISpec.InstanceID` (obsolete since v1.8, see #10569) * AWS `ENISpec.{MinAllocate,PreAllocate,MaxAboveWatermark}` (obsolete since v1.8, see #10089) Signed-off-by: Hadrien Patte <hadrien.patte@datadoghq.com>

tgraf added wip area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. area/eni Impacts ENI based IPAM. labels Mar 13, 2020

tgraf requested a review from a team March 13, 2020 09:49

tgraf requested review from a team as code owners March 13, 2020 09:49

maintainer-s-little-helper bot added dont-merge/needs-release-note labels Mar 13, 2020

tgraf added release-note/misc This PR makes changes that have no direct user impact. and removed area/misc Impacts miscellaneous areas of the code not otherwise owned by another area. labels Mar 13, 2020

maintainer-s-little-helper bot removed the dont-merge/needs-release-note label Mar 13, 2020

tgraf added the integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. label Mar 13, 2020

tgraf added pending-review and removed wip labels Mar 13, 2020

tgraf force-pushed the pr/tgraf/ipam-simplifcations branch from b011ecd to 053e5ab Compare March 18, 2020 11:14

tgraf requested a review from a team as a code owner March 18, 2020 11:14

tgraf added pending-review and removed wip labels Mar 19, 2020

tgraf added 7 commits March 20, 2020 23:37

azure: Remove remaining ENI references in godoc

944cb23

Some ENI references where left over due to basing this code off the ENI implementation. Remove them. Signed-off-by: Thomas Graf <thomas@cilium.io>

azure/api/mock: Fix data races

98a9415

Ensure that deep copies are made for data being passed in to avoid cross contamination between tests. Signed-off-by: Thomas Graf <thomas@cilium.io>

tgraf force-pushed the pr/tgraf/ipam-simplifcations branch from 053e5ab to 274531b Compare March 20, 2020 22:46

tgraf merged commit da14950 into master Mar 21, 2020

tgraf deleted the pr/tgraf/ipam-simplifcations branch March 21, 2020 20:42

HadrienPatte mentioned this pull request Apr 2, 2026

ipam: Remove obsolete fields from cloud provider IPAM types #45138

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Further IPAM simplifcations#10569

Further IPAM simplifcations#10569
tgraf merged 8 commits intomasterfrom
pr/tgraf/ipam-simplifcations

tgraf commented Mar 13, 2020 •

edited

Loading

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

coveralls commented Mar 13, 2020 •

edited

Loading

Uh oh!

tgraf commented Mar 13, 2020

Uh oh!

tgraf commented Mar 18, 2020 •

edited

Loading

Uh oh!

tgraf commented Mar 18, 2020

Uh oh!

tgraf commented Mar 20, 2020

Uh oh!

tgraf commented Mar 21, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

tgraf commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

maintainer-s-little-helper bot commented Mar 13, 2020

Uh oh!

coveralls commented Mar 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgraf commented Mar 13, 2020

Uh oh!

tgraf commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgraf commented Mar 18, 2020

Uh oh!

tgraf commented Mar 20, 2020

Uh oh!

tgraf commented Mar 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tgraf commented Mar 13, 2020 •

edited

Loading

coveralls commented Mar 13, 2020 •

edited

Loading

tgraf commented Mar 18, 2020 •

edited

Loading

tgraf commented Mar 21, 2020 •

edited

Loading