Neg crd feature gate #1166

swetharepakula · 2020-06-23T21:48:31Z

Add feature gate for NEG CRDs and propagate flag to neg controller and manager.

Manager creates NEG CRs when NEG CRD is enabled
Controller deletes neg crds when deleting negs when NEG CRD is enabled

Depends on #1154 being merged

k8s-ci-robot · 2020-06-23T21:48:36Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

k8s-ci-robot · 2020-06-23T21:48:39Z

Hi @swetharepakula. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

freehan · 2020-06-24T18:07:47Z

/ok-to-test

pkg/neg/types/types.go

freehan · 2020-06-24T23:43:35Z

pkg/context/context.go

@@ -127,6 +130,7 @@ func NewControllerContext(
 		NodeInformer:            informerv1.NewNodeInformer(kubeClient, config.ResyncPeriod, utils.NewNamespaceIndexer()),
 		recorders:               map[string]record.EventRecorder{},
 		healthChecks:            make(map[string]func() error),
+		NegClient:               networkendpointgroupClient,


NegClient -> NegCrdClient?

Since the other clients other than kube client are backendconfigclient and frontendconfigclient I am leaning towards NegClient or SvcNegClient.

freehan · 2020-06-24T23:45:06Z

pkg/neg/manager.go

@@ -260,6 +273,30 @@ func (manager *syncerManager) ReadinessGateEnabled(syncerKey negtypes.NegSyncerK
 	return false
 }

+func (manager *syncerManager) DeleteNegServiceCRs(namespace, name string) error {


I think this can stay as private function. This can be trigger within other public facing interfaces

This should also be ensureDeleteNegServiceCRs.
It should first check if the deleteionTimestamp is set, then proceed to delete if necessary.

Made this a private function and called it from StopSyncer, and also checks for deletion timestamp before deleting.

pkg/neg/controller.go

freehan · 2020-06-24T23:53:08Z

pkg/neg/manager.go

+		},
+	}
+
+	_, err = manager.negClient.NetworkingV1beta1().ServiceNetworkEndpointGroups(svcKey.namespace).Create(context.Background(), &negCR, metav1.CreateOptions{})


You might want to do a get and validate if the object is in the desired state. Then create or update

In what situations can we update the existing CR? If any of the ObjectMeta differs, I am thinking that an error should be raised. In other cases, Initialized and Synced conditions are harder to update since we don't really know the state. I am thinking to leave the Initialized condition unchanged, and change the Synced Condition to ConditionUnknown if we find an existing CR that matches the Neg we wish to create. What do you think?

freehan · 2020-06-24T23:53:57Z

pkg/neg/manager.go

@@ -121,6 +129,11 @@ func (manager *syncerManager) EnsureSyncers(namespace, name string, newPorts neg
 			// determine the implementation that calculates NEG endpoints on each sync.
 			epc := negsyncer.GetEndpointsCalculator(manager.nodeLister, manager.podLister, manager.zoneGetter,
 				syncerKey, portInfo.RandomizeEndpoints)
+
+			if err := manager.createNegCR(key, portInfo); err != nil {


I think you need to mirror this behavior for removes. So that you can clean up the CRs that needs to be deleted.

freehan · 2020-06-24T23:58:14Z

pkg/neg/types/interfaces.go

@@ -79,6 +79,8 @@ type NegSyncerManager interface {
 	GC() error
 	// ShutDown shuts down the manager
 	ShutDown()
+	// DeleteNegServiceCRs marks all NEG CRs corresponding to a service for deletion
+	DeleteNegServiceCRs(namespace, name string) error


you should not add this.

You might just need to modify the StopSyncer interface.

freehan · 2020-06-25T00:00:43Z

pkg/neg/manager.go

+		ObjectMeta: metav1.ObjectMeta{
+			Name:            portInfo.NegName,
+			Namespace:       svcKey.namespace,
+			Finalizers:      []string{common.NegFinalizerKey},


I suggest you do not add the finalizer in this PR because it would break HEAD if NEG CRD is enabled.

Add a TODO here and remember to add the finalizer in the following PR that handles GC.
Maybe just comment out this line and add TODO

pkg/neg/utils.go

- when enabled, NEG CRD is ensured in k8s cluster

- names specified in service annotations are passed along into the port info map

swetharepakula · 2020-07-06T17:32:51Z

The initial pull request was quite big, so splitting it into two. This now will only cover the feature gate and the utils changes necessary to start using custom names.

freehan

There is one corner case I want to confirm:

Let us say the NEG annotation is {"ingress": "true", "exposed_ports": {80:{"name": "foo"}}}. Ingress and standalone NEG shared the same service port 80.

Should we allow this config? I think disallowing this config might be good enough. But want to see if it makes things easier for allowing it.

swetharepakula · 2020-07-07T18:22:20Z

A few small improvements to the ingress controller and the helper utils and structs are necessary to support the change. One benefit we would gain from supporting it, would not having to do any error handling for that situation.

swetharepakula · 2020-07-08T00:09:18Z

After discussing with @freehan, I think for now we should not support the config where Ingress is true with a custom named neg, as it would require the Ingress controller to also parse and store the information in service annotations. The custom named standalone neg implementation will not limit opening this restriction up in the future if desired.

freehan

/lgtm
/approve

k8s-ci-robot · 2020-07-08T17:48:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, swetharepakula

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:

~~OWNERS~~ [freehan]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 23, 2020

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jun 23, 2020

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 23, 2020

k8s-ci-robot requested review from bowei and freehan June 23, 2020 21:48

swetharepakula force-pushed the neg-crd-feature-gate branch from af1f4c6 to b41dea6 Compare June 24, 2020 17:38

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jun 24, 2020

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 24, 2020

freehan reviewed Jun 25, 2020

View reviewed changes

Add feature flag for custom neg names

e964640

- when enabled, NEG CRD is ensured in k8s cluster

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 27, 2020

swetharepakula force-pushed the neg-crd-feature-gate branch from b41dea6 to e9e8e5d Compare June 29, 2020 17:53

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 29, 2020

swetharepakula force-pushed the neg-crd-feature-gate branch from e9e8e5d to 7d3feb2 Compare July 1, 2020 00:12

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 1, 2020

swetharepakula force-pushed the neg-crd-feature-gate branch from 7d3feb2 to 9836768 Compare July 1, 2020 00:27

newServicePorts return desired custom neg names

991b046

- names specified in service annotations are passed along into the port info map

swetharepakula force-pushed the neg-crd-feature-gate branch 2 times, most recently from caa984c to 991b046 Compare July 6, 2020 17:31

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 6, 2020

swetharepakula marked this pull request as ready for review July 6, 2020 17:32

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 6, 2020

swetharepakula mentioned this pull request Jul 6, 2020

Create corresponding Neg CRs when NEG CRD is enabled #1172

Merged

freehan reviewed Jul 7, 2020

View reviewed changes

freehan approved these changes Jul 8, 2020

View reviewed changes

k8s-ci-robot assigned freehan Jul 8, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 8, 2020

k8s-ci-robot merged commit e288604 into kubernetes:master Jul 8, 2020

swetharepakula deleted the neg-crd-feature-gate branch July 29, 2020 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Neg crd feature gate #1166

Neg crd feature gate #1166

swetharepakula commented Jun 23, 2020 •

edited

Loading

k8s-ci-robot commented Jun 23, 2020

k8s-ci-robot commented Jun 23, 2020

freehan commented Jun 24, 2020

freehan Jun 24, 2020

swetharepakula Jul 6, 2020 •

edited

Loading

freehan Jun 24, 2020

freehan Jun 24, 2020

swetharepakula Jul 1, 2020

freehan Jun 24, 2020

swetharepakula Jun 25, 2020

freehan Jun 24, 2020

freehan Jun 24, 2020

freehan Jun 25, 2020

swetharepakula commented Jul 6, 2020

freehan left a comment

swetharepakula commented Jul 7, 2020

swetharepakula commented Jul 8, 2020

freehan left a comment

k8s-ci-robot commented Jul 8, 2020

Neg crd feature gate #1166

Neg crd feature gate #1166

Conversation

swetharepakula commented Jun 23, 2020 • edited Loading

k8s-ci-robot commented Jun 23, 2020

k8s-ci-robot commented Jun 23, 2020

freehan commented Jun 24, 2020

Choose a reason for hiding this comment

swetharepakula Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swetharepakula commented Jul 6, 2020

freehan left a comment

Choose a reason for hiding this comment

swetharepakula commented Jul 7, 2020

swetharepakula commented Jul 8, 2020

freehan left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jul 8, 2020

swetharepakula commented Jun 23, 2020 •

edited

Loading

swetharepakula Jul 6, 2020 •

edited

Loading