Rename API group "networking" to "controlplane" #1147

jianjuns · 2020-08-26T00:23:09Z

antrea-bot · 2020-08-26T00:23:25Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

jianjuns · 2020-08-26T01:09:49Z

/test-all

antoninbas

Should this close #1122? The issue was about revisiting all API groups, not just networking?

Do you know if this affects upgrade? IIRC, renaming the API group used in an APIService may create issues, if we stop serving the original API in the Antrea Controller apiserver. As much as possible we should try to concentrate API changes that may affect upgrade within the same release.

jianjuns · 2020-08-26T06:34:51Z

Should this close #1122? The issue was about revisiting all API groups, not just networking?

So far we just concluded on this change only. But if you like to keep the issue open, I am fine too.

Do you know if this affects upgrade? IIRC, renaming the API group used in an APIService may create issues, if we stop serving the original API in the Antrea Controller apiserver. As much as possible we should try to concentrate API changes that may affect upgrade within the same release.

There can be period that controller and agent are not at the same version, and then agent cannot watch the computed networkpolicy objects from controller (so networkpolicy changes will not take effect). I feel this is acceptable, but like to hear your thoughts and suggestions.
@tnqn : any thoughts?

antoninbas · 2020-08-26T06:38:17Z

@jianjuns that's true, but I consider this acceptable given that we haven't released Antrea v1.0 yet. In this case I was more referring to an issue like this one: #494

jianjuns · 2020-08-26T06:57:47Z

Yes I forgot this one! Does it require API group versions not changed too? If not we can keep an empty networking group. But I guess version must be the same too.

tnqn

Found some unexpected changes, guess the PR is not done yet.

build/yamls/base/agent.yml

docs/ovs-pipeline.md

docs/prometheus-integration.md

tnqn · 2020-08-26T11:10:55Z

The problem is applying the new yaml won't delete the stale "v1beta1.networking.antrea.tanzu.vmware.com" APIService, but antrea-controller is no longer serving it so we would meet the same issue as #494 as the upgrade test failure shows:

##[error]    upgrade_test.go:96: Deleting namespace 'test-namespace-cyw88041'
##[error]    upgrade_test.go:98: Namespace deletion failed: timed out waiting for the condition
##[error]    basic_test.go:83: The Antrea Pod for Node 'kind-control-plane' is 'antrea-agent-z9rx7'

@antoninbas and I discussed how to deal with it before, we got two options:

Grant antrea-controller serviceaccount the permission of deleting specific APIServices and do it in antrea-controller.
Ask users to use kubectl apply --prune --prune-whitelist=apiregistration.k8s.io/v1/APIService when applying new yaml.

Along with the evolution of APIs, like v1alpha1 -> v1beta1 -> v1, we may need to handle this case anyway.

jianjuns · 2020-08-26T17:15:12Z

Grant antrea-controller serviceaccount the permission of deleting specific APIServices and do it in antrea-controller.

I guess let us go this approach. What you think a separate initContainer which can be activated when we need to handle API service in upgrade, and can be removed when not needed? For example, we execute an antctl command or run controller in special mode to do the cleanup.

antoninbas · 2020-08-26T19:40:41Z

I like the idea of doing it in the Antrea Controller even if it does require additional permissions.
I see that the Controller already has the following permissions:

- apiGroups:
  - apiregistration.k8s.io
  resourceNames:
  - v1beta1.system.antrea.tanzu.vmware.com
  - v1beta1.networking.antrea.tanzu.vmware.com
  resources:
  - apiservices
  verbs:
  - get
  - update

It seems that wildcards are not supported in resourceNames, so we would need to either give the Controller "delete" access to all APIServices, or make sure that we always include all deprecated APIs in resourceNames.

abhiraut

mid way i saw you plan to address goland mistakes..

build/yamls/base/agent.yml

docs/ovs-pipeline.md

pkg/agent/agent.go

pkg/agent/agent_windows.go

pkg/agent/config/node_config.go

pkg/agent/doc.go

pkg/agent/openflow/client.go

pkg/agent/route/route_windows.go

abhiraut · 2020-08-26T20:51:51Z

I like the idea of doing it in the Antrea Controller even if it does require additional permissions.
I see that the Controller already has the following permissions:
- apiGroups:
  - apiregistration.k8s.io
  resourceNames:
  - v1beta1.system.antrea.tanzu.vmware.com
  - v1beta1.networking.antrea.tanzu.vmware.com
  resources:
  - apiservices
  verbs:
  - get
  - update
It seems that wildcards are not supported in resourceNames, so we would need to either give the Controller "delete" access to all APIServices, or make sure that we always include all deprecated APIs in resourceNames.

+1 seems okay to grant this to Antrea controller with specific resource names

jianjuns · 2020-08-26T21:51:07Z

Thanks @abhiraut for the review. Let me do a thorough check of all changes.
For APIService cleanup, I am still thinking about an initContainer. Will do some tests first.

jianjuns · 2020-08-27T01:07:46Z

After looking at the code, I also agree let controller delete the APIService is much simpler, as controller now already updates APIServices. Made the change. Please check. @antoninbas @abhiraut @tnqn

antoninbas

Deletion logic LGTM, a few comments

antoninbas · 2020-08-27T01:18:09Z

pkg/apiserver/apiserver.go

+	// otherwise K8s will reject to delete an existing Namespace created
+	// before the upgrade. Also check the information at:


suggestion: s/will reject to delete/will fail to delete

also question: is it just for Namespaces that existed before the upgrade? I thought that was for all Namespaces (because every time a Namespace is deleted, K8s reaches out to all APIServices to see if they have any object that needs to be deleted). See kubernetes/kubernetes#60807 (comment). I may be wrong though.

Changed to "will fail to delete".

You are right. It should be all Namespaces before the APIService is deleted. Updated the comments.

pkg/apiserver/apiserver.go

antoninbas · 2020-08-27T01:20:16Z

cmd/antrea-controller/controller.go

@@ -130,6 +130,12 @@ func run(o *Options) error {
 	if err != nil {
 		return fmt.Errorf("error creating API server: %v", err)
 	}
+
+	err = apiserver.CleanupDeprecatedAPIServices(aggregatorClient)


I didn't think of it too much but does the order matter here? should we remove deprecated APIServices before installing the new ones?

Installation happens locally on controller, and new APIServices are already registered when applying the YAML. Before apiserver starts, K8s knows nothing about the new API groups. So, I think it does not matter.

antoninbas · 2020-08-27T01:20:43Z

build/yamls/base/controller-rbac.yml

+    resources:
+      - apiservices
+    resourceNames:
+      # Add the APIServices for the deprecated APIGroups here. antrea-controller


Thanks for adding the comment!

do we have a timeline in terms of releases for how long we plan to attempt deleting old apiservices? ie after X releases we will no longer clean up..

jianjuns · 2020-08-27T05:44:41Z

/test-all

pkg/apiserver/apiserver.go

tnqn

LGTM

jianjuns · 2020-08-28T05:47:39Z

/test-all

tnqn

LGTM

antoninbas

LGTM

abhiraut

lgtm.. one minor nit.. but lets merge so we can resolve conflicts on other PRs

abhiraut · 2020-08-28T18:15:25Z

build/yamls/base/controller-rbac.yml

+    resources:
+      - apiservices
+    resourceNames:
+      # Add the APIServices for the deprecated APIGroups here. antrea-controller


do we have a timeline in terms of releases for how long we plan to attempt deleting old apiservices? ie after X releases we will no longer clean up..

pkg/apiserver/apiserver.go

antoninbas · 2020-08-28T18:54:59Z

@abhiraut I think we should keep including all deprecated APIServices until we make a v1.0 release

abhiraut · 2020-08-28T19:03:34Z

@abhiraut I think we should keep including all deprecated APIServices until we make a v1.0 release

thanks for the answer!

codecov-commenter · 2020-08-28T20:50:02Z

Codecov Report

Merging #1147 into master will decrease coverage by 0.30%.
The diff coverage is 17.11%.

@@            Coverage Diff             @@
##           master    #1147      +/-   ##
==========================================
- Coverage   43.79%   43.48%   -0.31%     
==========================================
  Files         140      140              
  Lines       15146    15246     +100     
==========================================
- Hits         6633     6630       -3     
- Misses       7928     8027      +99     
- Partials      585      589       +4

Flag	Coverage Δ
#integration-tests	`31.23% <0.55%> (-0.29%)`	⬇️
#unit-tests	`39.84% <90.24%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/cniserver/ipam/testing/mock_ipam.go	`100.00% <ø> (ø)`
pkg/agent/cniserver/server.go	`51.69% <ø> (ø)`
pkg/agent/controller/networkpolicy/cache.go	`79.57% <ø> (ø)`
pkg/agent/controller/networkpolicy/reconciler.go	`67.92% <ø> (ø)`
pkg/agent/openflow/network_policy.go	`74.45% <ø> (ø)`
pkg/agent/route/testing/mock_route.go	`40.74% <ø> (ø)`
pkg/agent/types/networkpolicy.go	`25.00% <ø> (ø)`
pkg/antctl/antctl.go	`100.00% <ø> (ø)`
pkg/antctl/command_definition.go	`45.43% <ø> (ø)`
...lusterinformation/v1beta1/zz_generated.deepcopy.go	`0.00% <ø> (ø)`
... and 35 more

jianjuns · 2020-08-28T21:08:54Z

/test-all

* Rename API group "networking" to "controlplane" * Delete the registered APIService for "networking" API group

vmwclabot added the cla-not-required label Aug 26, 2020

jianjuns force-pushed the apigroup branch from 6def8ab to 04b6320 Compare August 26, 2020 00:47

jianjuns force-pushed the apigroup branch from 04b6320 to c676682 Compare August 26, 2020 06:22

jianjuns changed the title ~~WIP: Rename API group "networking" to "controlplane"~~ Rename API group "networking" to "controlplane" Aug 26, 2020

jianjuns requested review from tnqn, abhiraut and antoninbas August 26, 2020 06:24

antoninbas reviewed Aug 26, 2020

View reviewed changes

antoninbas added api-review Categorizes an issue or PR as actively needing an API review. area/api Issues or PRs related to an API. labels Aug 26, 2020

tnqn reviewed Aug 26, 2020

View reviewed changes

build/yamls/base/agent.yml Outdated Show resolved Hide resolved

docs/ovs-pipeline.md Outdated Show resolved Hide resolved

docs/prometheus-integration.md Outdated Show resolved Hide resolved

abhiraut reviewed Aug 26, 2020

View reviewed changes

jianjuns changed the title ~~Rename API group "networking" to "controlplane"~~ WIP: Rename API group "networking" to "controlplane" Aug 26, 2020

jianjuns force-pushed the apigroup branch 2 times, most recently from 732e426 to 81c4e8c Compare August 27, 2020 00:28

jianjuns changed the title ~~WIP: Rename API group "networking" to "controlplane"~~ Rename API group "networking" to "controlplane" Aug 27, 2020

antoninbas reviewed Aug 27, 2020

View reviewed changes

jianjuns force-pushed the apigroup branch 2 times, most recently from 91c35fc to 4a819c0 Compare August 27, 2020 05:44

tnqn reviewed Aug 27, 2020

View reviewed changes

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

jianjuns force-pushed the apigroup branch from 4a819c0 to a6fc4e7 Compare August 27, 2020 19:11

tnqn previously approved these changes Aug 28, 2020

View reviewed changes

jianjuns dismissed tnqn’s stale review via d8a6d2b August 28, 2020 03:51

jianjuns force-pushed the apigroup branch from a6fc4e7 to d8a6d2b Compare August 28, 2020 03:51

tnqn previously approved these changes Aug 28, 2020

View reviewed changes

antoninbas previously approved these changes Aug 28, 2020

View reviewed changes

abhiraut previously approved these changes Aug 28, 2020

View reviewed changes

jianjuns added 2 commits August 28, 2020 13:41

Rename API group "networking" to "controlplane"

936e3ee

Delete the registered APIService for "networking" API group

5367fd0

jianjuns dismissed stale reviews from abhiraut, antoninbas, and tnqn via 5367fd0 August 28, 2020 20:46

jianjuns force-pushed the apigroup branch from d8a6d2b to 5367fd0 Compare August 28, 2020 20:46

abhiraut approved these changes Aug 28, 2020

View reviewed changes

jianjuns merged commit b2af9f4 into antrea-io:master Aug 28, 2020

antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Sep 3, 2020

Rename API group "networking" to "controlplane" (antrea-io#1147)

13d82d8

* Rename API group "networking" to "controlplane" * Delete the registered APIService for "networking" API group

antoninbas pushed a commit to antoninbas/antrea that referenced this pull request Sep 3, 2020

Rename API group "networking" to "controlplane" (antrea-io#1147)

912b439

* Rename API group "networking" to "controlplane" * Delete the registered APIService for "networking" API group

antoninbas pushed a commit that referenced this pull request Sep 3, 2020

Rename API group "networking" to "controlplane" (#1147)

a3417be

* Rename API group "networking" to "controlplane" * Delete the registered APIService for "networking" API group

tnqn mentioned this pull request Sep 15, 2020

antrea-controller should be backward compatible with antrea-agent to allow graceful upgrade #1250

Closed

GraysonWu pushed a commit to GraysonWu/antrea that referenced this pull request Sep 22, 2020

Rename API group "networking" to "controlplane" (antrea-io#1147)

6b23f95

* Rename API group "networking" to "controlplane" * Delete the registered APIService for "networking" API group

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename API group "networking" to "controlplane" #1147

Rename API group "networking" to "controlplane" #1147

jianjuns commented Aug 26, 2020

antrea-bot commented Aug 26, 2020

jianjuns commented Aug 26, 2020

antoninbas left a comment

jianjuns commented Aug 26, 2020

antoninbas commented Aug 26, 2020

jianjuns commented Aug 26, 2020

tnqn left a comment

tnqn commented Aug 26, 2020

jianjuns commented Aug 26, 2020 •

edited

Loading

antoninbas commented Aug 26, 2020

abhiraut left a comment

abhiraut commented Aug 26, 2020

jianjuns commented Aug 26, 2020

jianjuns commented Aug 27, 2020

antoninbas left a comment

antoninbas Aug 27, 2020

jianjuns Aug 27, 2020

antoninbas Aug 27, 2020

jianjuns Aug 27, 2020

antoninbas Aug 27, 2020

abhiraut Aug 28, 2020

jianjuns commented Aug 27, 2020

tnqn left a comment

jianjuns commented Aug 28, 2020

tnqn left a comment

antoninbas left a comment

abhiraut left a comment

abhiraut Aug 28, 2020

antoninbas commented Aug 28, 2020

abhiraut commented Aug 28, 2020

codecov-commenter commented Aug 28, 2020 •

edited

Loading

jianjuns commented Aug 28, 2020

		// otherwise K8s will reject to delete an existing Namespace created
		// before the upgrade. Also check the information at:

Rename API group "networking" to "controlplane" #1147

Rename API group "networking" to "controlplane" #1147

Conversation

jianjuns commented Aug 26, 2020

antrea-bot commented Aug 26, 2020

jianjuns commented Aug 26, 2020

antoninbas left a comment

Choose a reason for hiding this comment

jianjuns commented Aug 26, 2020

antoninbas commented Aug 26, 2020

jianjuns commented Aug 26, 2020

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Aug 26, 2020

jianjuns commented Aug 26, 2020 • edited Loading

antoninbas commented Aug 26, 2020

abhiraut left a comment

Choose a reason for hiding this comment

abhiraut commented Aug 26, 2020

jianjuns commented Aug 26, 2020

jianjuns commented Aug 27, 2020

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas Aug 27, 2020

Choose a reason for hiding this comment

jianjuns Aug 27, 2020

Choose a reason for hiding this comment

antoninbas Aug 27, 2020

Choose a reason for hiding this comment

jianjuns Aug 27, 2020

Choose a reason for hiding this comment

antoninbas Aug 27, 2020

Choose a reason for hiding this comment

abhiraut Aug 28, 2020

Choose a reason for hiding this comment

jianjuns commented Aug 27, 2020

tnqn left a comment

Choose a reason for hiding this comment

jianjuns commented Aug 28, 2020

tnqn left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

abhiraut left a comment

Choose a reason for hiding this comment

abhiraut Aug 28, 2020

Choose a reason for hiding this comment

antoninbas commented Aug 28, 2020

abhiraut commented Aug 28, 2020

codecov-commenter commented Aug 28, 2020 • edited Loading

Codecov Report

jianjuns commented Aug 28, 2020

jianjuns commented Aug 26, 2020 •

edited

Loading

codecov-commenter commented Aug 28, 2020 •

edited

Loading