-
Notifications
You must be signed in to change notification settings - Fork 366
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
Conversation
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this 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)
Got it, thanks for reminding. @antoninbas |
043dc82
to
527038b
Compare
There was a problem hiding this 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?
@jianjuns I suggest we merge them together in the next release cycle |
Works for me. Just want to confirm. |
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. |
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. |
3e44ba5
to
b2b0605
Compare
0862a9a
to
d10347f
Compare
ed4c542
to
2efc5df
Compare
There was a problem hiding this 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 ?
There was a problem hiding this 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?
There was a problem hiding this 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)
@@ -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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| `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 | |
There was a problem hiding this comment.
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.
ci/kind/test-e2e-kind.sh
Outdated
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok.
There was a problem hiding this comment.
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.
- crdmutator.antrea.tanzu.vmware.com | ||
- crdvalidator.antrea.tanzu.vmware.com | ||
- labelsmutator.antrea.io | ||
- crdmutator.antrea.io | ||
- crdvalidator.antrea.io |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@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 |
func (c *ClusterGroupHandler) AddNewObject(obj metav1.Object) error { | ||
l := obj.(*legacycore.ClusterGroup) | ||
n := c.buildNewObject(l) | ||
client := c.client.CrdV1alpha2().ClusterGroups() |
There was a problem hiding this comment.
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)?
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. |
bdde2fc
to
780959c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/test-all |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
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 intocrd.antrea.io
.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 withcrd.antrea.io/stop-mirror
by user. Once the legacy CRD is annotated withcrd.antrea.io/stop-mirror
, the annotation ofmanaged-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.