Skip to content
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.

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

Rename API groups from *.antrea.tanzu.vmware.com to *.antrea.io #1799

Merged
merged 3 commits into from Apr 1, 2021

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Jan 29, 2021

Rename API groups from *.antrea.tanzu.vmware.com to *.antrea.io

Extension API groups as well as CRD API groups are renamed from *.antrea.tanzu.vmware.com to *.antrea.io.
Old version of extension API groups is not renamed. Legacy CRD API groups ops.antrea.tanzu.vmware.com, security.antrea.tanzu.vmware.com, core.antrea.tanzu.vmware.com are merged into crd.antrea.io.

New API group New API version Legacy API group Legacy API version
controlplane.antrea.tanzu.vmware.com v1beta1
networking.antrea.tanzu.vmware.com v1beta1
controlplane.antrea.io v1beta2 controlplane.antrea.tanzu.vmware.com v1beta2
stats.antrea.io v1alpha1 stats.antrea.tanzu.vmware.com v1alpha1
system.antrea.io v1beta1 system.antrea.tanzu.vmware.com v1beta1
crd.antrea.io v1alpha2 core.antrea.tanzu.vmware.com v1alpha2
crd.antrea.io v1alpha1 ops.antrea.tanzu.vmware.com v1alpha1
crd.antrea.io v1alpha1 security.antrea.tanzu.vmware.com v1alpha1
crd.antrea.io v1beta1 clusterinformation.antrea.tanzu.vmware.com v1beta1

Legacy extension and CRD API groups are reservered. Legacy extension API groups can be used directly.

For legacy API groups, option LegacyCRDMirroring should be enabled in feature gates.When the mirroring is enabled,if a legacy CRD is created with legacy API groups, mirroring-controllerwill create a new CRD with the Spec and Labels from the legacy CRD. Afterwards, the modification of Spec and Label in legacy CRD will be synchronized to new CRD automatically. In addition, the modification of Status in new CRD will also be synchronized to legacy CRD automatically. If legacy CRD is deleted, the corresponding new CRD will be deleted.

Note that, a new CRD created by mirroring-controller has an annotation managed-by, and the new CRD cannot be updated or delete with new API groups(If the new CRD is manually forcibly updated or deleted, it will be restored by mirroring-controller) unless the corresponding legacy CRD is annotated with crd.antrea.io/stop-mirror by user. Once the legacy CRD is annotated with crd.antrea.io/stop-mirror, the annotation of managed-by in corresponding new CRD will be removed by mirroring-controller, then the new CRD is decoupled from the legacy CRD.

If Antrea is upgraded from version <= v0.13 and legacy CRDs is used, option LegacyCRDMirroring should be enabled, otherwise the legacy CRDs created with the legacy API groups will not take any effect and work as expected.

@codecov-io
Copy link

codecov-io commented Jan 29, 2021

Codecov Report

Merging #1799 (8cbb8f2) into main (25c15ad) will decrease coverage by 3.91%.
The diff coverage is 35.90%.

❗ Current head 8cbb8f2 differs from pull request most recent head bdde2fc. Consider uploading reports for the commit bdde2fc to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1799      +/-   ##
==========================================
- Coverage   59.64%   55.72%   -3.92%     
==========================================
  Files         197      262      +65     
  Lines       17507    19487    +1980     
==========================================
+ Hits        10442    10860     +418     
- Misses       5955     7472    +1517     
- Partials     1110     1155      +45     
Flag Coverage Δ
kind-e2e-tests 41.93% <23.39%> (-3.38%) ⬇️
unit-tests 41.87% <61.53%> (+0.33%) ⬆️

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

Impacted Files Coverage Δ
pkg/agent/apiserver/handlers/agentinfo/handler.go 38.23% <ø> (ø)
pkg/agent/controller/networkpolicy/cache.go 84.64% <ø> (ø)
pkg/agent/openflow/network_policy.go 69.10% <0.00%> (-1.95%) ⬇️
pkg/agent/querier/querier.go 87.50% <ø> (ø)
pkg/agent/types/networkpolicy.go 37.50% <ø> (ø)
pkg/antctl/raw/traceflow/command.go 27.05% <0.00%> (ø)
pkg/apis/controlplane/register.go 88.23% <ø> (ø)
pkg/apis/controlplane/v1beta1/conversion.go 81.00% <ø> (ø)
pkg/apis/controlplane/v1beta1/register.go 92.85% <ø> (ø)
pkg/apis/controlplane/v1beta2/register.go 94.11% <ø> (ø)
... and 142 more

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

you only need to port the latest version of each supported API, see https://github.com/vmware-tanzu/antrea/blob/main/docs/api.md

so looking at this list:

  - v1alpha1.stats.antrea.io
  - v1beta1.system.antrea.io
  - v1beta2.controlplane.antrea.io
  - v1beta1.controlplane.antrea.io
  - v1beta1.networking.antrea.io

you only need

  - v1alpha1.stats.antrea.io
  - v1beta1.system.antrea.io
  - v1beta2.controlplane.antrea.io

(note that the networking API group was deprecated and replaced with controlplane)

@hongliangl
Copy link
Contributor Author

Got it, thanks for reminding. @antoninbas

@hongliangl hongliangl force-pushed the api-rename branch 2 times, most recently from 043dc82 to 527038b Compare January 29, 2021 07:47
@jianjuns jianjuns requested a review from tnqn February 1, 2021 23:55
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

Do we plan to merge this one in 0.13, or merge it together with the CRD group change in the next release?

@antoninbas
Copy link
Contributor

@jianjuns I suggest we merge them together in the next release cycle

@jianjuns
Copy link
Contributor

jianjuns commented Feb 4, 2021

@jianjuns I suggest we merge them together in the next release cycle

Works for me. Just want to confirm.

@hongliangl
Copy link
Contributor Author

Do we plan to merge this one in 0.13, or merge it together with the CRD group change in the next release?

Sorry for replying late, I don't know why I didn't received a notice email when there are new comments in this PR.

The part of renaming CRD is in progressing.

@vicky-liu
Copy link

It's the plan to merge the CRD converter and API name change together in 0.14 if we cannt do that in 0.13.

@hongliangl hongliangl changed the title Rename extension API from *.antrea.tanzu.vmware.com to *.antrea.io Rename extension and CRD API from *.antrea.tanzu.vmware.com to *.antrea.io Feb 6, 2021
@hongliangl hongliangl marked this pull request as draft February 7, 2021 01:35
@hongliangl hongliangl force-pushed the api-rename branch 5 times, most recently from 0862a9a to d10347f Compare February 7, 2021 08:57
@hongliangl hongliangl marked this pull request as ready for review February 8, 2021 04:20
@hongliangl hongliangl force-pushed the api-rename branch 5 times, most recently from ed4c542 to 2efc5df Compare February 28, 2021 09:02
build/yamls/base/controller.yml Outdated Show resolved Hide resolved
build/yamls/base/controller.yml Outdated Show resolved Hide resolved
build/yamls/base/crds.yml Outdated Show resolved Hide resolved
build/yamls/base/crds.yml Outdated Show resolved Hide resolved
cmd/antrea-controller/controller.go Outdated Show resolved Hide resolved
pkg/controller/crdmirroring/crdmirroring_controller.go Outdated Show resolved Hide resolved
pkg/controller/crdmirroring/crdmirroring_handlers.go Outdated Show resolved Hide resolved
pkg/controller/crdmirroring/crdmirroring_tracker.go Outdated Show resolved Hide resolved
pkg/controller/crdmirroring/crdmirroring_tracker.go Outdated Show resolved Hide resolved
pkg/k8s/client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

I think we concluded to put all CRDs under: "crd.antrea.io", and not extra sub-groups like: ops, security, core, etc.
So, for example, traceflow should be: traceflows.crd.antrea.io.

Is that the case: @antoninbas @tnqn ?

tnqn
tnqn previously approved these changes Mar 31, 2021
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM. @jianjuns @antoninbas would you take a look at this PR?

@hongliangl hongliangl requested a review from tnqn March 31, 2021 16:43
Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

First thanks for the tremendous amount of work @hongliangl and thanks @tnqn for all your detailed rounds of review. The final "scheme" is pretty much what I had in mind, so this works well for me at a high level.

I added a few small comments and questions. The main one being that I think we should not run the legacy tests on Kind, because it takes too much time.

After this is merged we should:

  • update all the examples in the documentation to use the new APIs
  • write a short guide on how to upgrade an existing cluster / application from old APIs to the new ones (I can take care of this)

build/yamls/base/conf/antrea-controller.conf Outdated Show resolved Hide resolved
build/yamls/base/conf/antrea-controller.conf Outdated Show resolved Hide resolved
@@ -29,6 +29,7 @@ Generate a YAML manifest for Antrea using Kustomize and print it to stdout.
--ipsec Generate a manifest with IPSec encryption of tunnel traffic enabled
--all-features Generate a manifest with all alpha features enabled
--no-proxy Generate a manifest with Antrea proxy disabled
--no-legacy-crd Generate a manifest without legacy CRD mirroring support enabled
Copy link
Contributor

Choose a reason for hiding this comment

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

is this used anywhere so far?

Copy link
Contributor Author

@hongliangl hongliangl Apr 1, 2021

Choose a reason for hiding this comment

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

I may not got totally what you mean. But I added this for the reason: for new users of Antrea, they have never used the legacy CRDs, nor they need to use the legacy API groups, and they can use this option to disable the mirroring controllers. I added six mirroring controllers, every controller has 4 workers. For new users, it is redundant to run this controllers.

pkg/agent/controller/traceflow/packetin.go Outdated Show resolved Hide resolved
Comment on lines +25 to +27
| `crd.antrea.io` | `v1alpha1` | No | v1.0.0 | N/A | N/A |
| `crd.antrea.io` | `v1alpha2` | No | v1.0.0 | N/A | N/A |
| `crd.antrea.io` | `v1beta1` | No | v1.0.0 | N/A | N/A |
Copy link
Contributor

Choose a reason for hiding this comment

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

I will need to think about this once this PR is merged. Versioning will need to be done independently for each CRD, and not for a given API group.

test/e2e/legacyantreapolicy_test.go Outdated Show resolved Hide resolved
Comment on lines 135 to 137
go test -v -timeout=50m github.com/vmware-tanzu/antrea/test/e2e -provider=kind --logs-export-dir=$ANTREA_LOG_DIR --coverage --coverage-dir $ANTREA_COV_DIR
go test -v -timeout=70m github.com/vmware-tanzu/antrea/test/e2e -provider=kind --logs-export-dir=$ANTREA_LOG_DIR --coverage --coverage-dir $ANTREA_COV_DIR
else
go test -v -timeout=45m github.com/vmware-tanzu/antrea/test/e2e -provider=kind --logs-export-dir=$ANTREA_LOG_DIR
go test -v -timeout=65m github.com/vmware-tanzu/antrea/test/e2e -provider=kind --logs-export-dir=$ANTREA_LOG_DIR
Copy link
Contributor

Choose a reason for hiding this comment

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

can we run the legacy tests only on jenkins and disable them in Kind?

Copy link
Member

Choose a reason for hiding this comment

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

agree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A new issue, even the legacy tests are skipped in Kind, 50m is not enough for other tests.

pkg/controller/crdmirroring/crdmirroring_controller.go Outdated Show resolved Hide resolved
pkg/controller/crdmirroring/crdmirroring_controller.go Outdated Show resolved Hide resolved
Comment on lines 98 to +102
- crdmutator.antrea.tanzu.vmware.com
- crdvalidator.antrea.tanzu.vmware.com
- labelsmutator.antrea.io
- crdmutator.antrea.io
- crdvalidator.antrea.io
Copy link
Contributor

Choose a reason for hiding this comment

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

@hongliangl @tnqn Just to confirm: we have to keep the old mutator / validator in addition to the new one? So if a legacy CR is created, and is mirrored, it will go through the webhooks twice?

BTW, the validation code / mutation code only use the new API types. I assume it works on the old legacy CRs because the type definitions are the same?

Copy link
Contributor Author

@hongliangl hongliangl Apr 1, 2021

Choose a reason for hiding this comment

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

Just to confirm: we have to keep the old mutator / validator in addition to the new one? So if a legacy CR is created, and is mirrored, it will go through the webhooks twice?

If a legacy CR is created, the legacy one goes through the legacy webhook, and the mirrored new goes through the new webhook.

BTW, the validation code / mutation code only use the new API types. I assume it works on the old legacy CRs because the type definitions are the same?

I didn't add anything new in the validation code / mutation code. What I added is MutatingWebhookConfiguration and ValidatingWebhookConfiguration in build/yamls/base/controller.yml. I have tried using crdmutator.antrea.io and crdvalidator.antrea.io to mutate/validate the legacy CRs, but it didn't work.

@Dyanngg
Copy link
Contributor

Dyanngg commented Mar 31, 2021

@tnqn @antoninbas I assume this PR has inter-dependency with #1993?

@antoninbas
Copy link
Contributor

@tnqn @antoninbas I assume this PR has inter-dependency with #1993?

yes, but only in the sense that there may be merge conflicts, which should be easy to resolve

build/yamls/base/conf/antrea-controller.conf Outdated Show resolved Hide resolved
pkg/agent/controller/traceflow/packetin.go Outdated Show resolved Hide resolved
func (c *ClusterGroupHandler) AddNewObject(obj metav1.Object) error {
l := obj.(*legacycore.ClusterGroup)
n := c.buildNewObject(l)
client := c.client.CrdV1alpha2().ClusterGroups()
Copy link
Contributor

Choose a reason for hiding this comment

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

Then should we handle Cluster scope CRD separately (esp. most our CRDs are cluster scoped)?

@hongliangl
Copy link
Contributor Author

hongliangl commented Apr 1, 2021

Then should we handle Cluster scope CRD separately (esp. most our CRDs are cluster scoped)?

Sorry, I didn't get what you mean. I don't understand what is the difference between Cluster scope and Non-Cluster scope in mirroring. @jianjuns

@tnqn
Copy link
Member

tnqn commented Apr 1, 2021

Then should we handle Cluster scope CRD separately (esp. most our CRDs are cluster scoped)?

Sorry, I didn't get what you mean. I don't understand what is the difference between Cluster scope and Non-Cluster scope in mirroring. @jianjuns

Please check the thread in "Files changed" panel to get the context and reply there so the conversation is easy to track.
For this question, cluster scope resource is the the CRD you don't need to set Namespace when updating their CRs.

Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

@hongliangl
Copy link
Contributor Author

/test-all

Copy link
Contributor

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

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

I just have a nit. Since this PR has been pending for long and blocks a few others, I would not block the merge any longer. So, just go ahead to merge, if no other comments/changes, and we can address nits with a followup PR.

@@ -142,15 +158,79 @@ func run(o *Options) error {
networkPolicyStatusController = networkpolicy.NewStatusController(crdClient, networkPolicyStore, cnpInformer, anpInformer)
}

var anpMirroringController *crdmirroring.Controller
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to move these lines and line 129 to 140 to a separate func.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for advices. This could be better.

Copy link
Contributor

@antoninbas antoninbas left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants