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

Update ClusterClaim to v1alpha2 #3755

Merged
merged 1 commit into from
Jul 6, 2022
Merged

Conversation

bangqipropel
Copy link
Contributor

@bangqipropel bangqipropel commented May 9, 2022

Update ClusterClaim to v1alpha2

  1. ClusterClaim upgraded to v1alpha2, with removeing the Name of it
  2. meta.name in ClusterClaim can only be created as id.k8s.io or
    clusterset.k8s.io
  3. Deprecate ClusterClaim version v1alpha1

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2022

Codecov Report

Merging #3755 (108bf2f) into main (a07959e) will increase coverage by 6.74%.
The diff coverage is 54.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3755      +/-   ##
==========================================
+ Coverage   49.03%   55.78%   +6.74%     
==========================================
  Files         261      417     +156     
  Lines       39245    60810   +21565     
==========================================
+ Hits        19245    33921   +14676     
- Misses      18128    24112    +5984     
- Partials     1872     2777     +905     
Flag Coverage Δ
integration-tests 35.20% <ø> (?)
kind-e2e-tests 50.11% <ø> (+1.08%) ⬆️
unit-tests 44.34% <54.54%> (?)

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

Impacted Files Coverage Δ
...ontrollers/multicluster/clusterclaim_controller.go 0.00% <0.00%> (ø)
...icluster/cmd/multicluster-controller/controller.go 7.44% <50.00%> (ø)
...uster/controllers/multicluster/controller_utils.go 27.55% <50.00%> (ø)
...s/multicluster/memberclusterannounce_controller.go 71.79% <100.00%> (ø)
...agent/flowexporter/connections/deny_connections.go 68.96% <0.00%> (-11.50%) ⬇️
pkg/agent/apiserver/apiserver.go 67.02% <0.00%> (-4.26%) ⬇️
pkg/agent/flowexporter/utils.go 72.34% <0.00%> (-4.26%) ⬇️
pkg/agent/controller/networkpolicy/reject.go 82.48% <0.00%> (-3.39%) ⬇️
pkg/flowaggregator/flowaggregator.go 58.17% <0.00%> (-2.43%) ⬇️
...lowaggregator/clickhouseclient/clickhouseclient.go 82.98% <0.00%> (-0.71%) ⬇️
... and 284 more

@bangqipropel bangqipropel force-pushed the CRD_v1alpha2 branch 4 times, most recently from edca648 to fbe85da Compare May 11, 2022 00:40
@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label May 11, 2022
@bangqipropel bangqipropel force-pushed the CRD_v1alpha2 branch 4 times, most recently from f9ef0ed to e6295af Compare May 22, 2022 09:03
@bangqipropel
Copy link
Contributor Author

@luolanzone this can be reviewed, thanks!

@bangqipropel bangqipropel changed the title [WIP]: ClusterClaim v1alpha2 ClusterClaim v1alpha2 May 22, 2022
@bangqipropel bangqipropel changed the title ClusterClaim v1alpha2 Update ClusterClaim to v1alpha2 May 22, 2022
@bangqipropel bangqipropel changed the title Update ClusterClaim to v1alpha2 [WIP]:Update ClusterClaim to v1alpha2 Jun 8, 2022
docs/multicluster/api.md Outdated Show resolved Hide resolved
multicluster/test/integration/suite_test.go Outdated Show resolved Hide resolved
multicluster/test/yamls/east-member-cluster.yml Outdated Show resolved Hide resolved
multicluster/config/rbac/role.yaml Outdated Show resolved Hide resolved
@luolanzone
Copy link
Contributor

luolanzone commented Jun 14, 2022

@bangqipropel Please don't forget to add your sign-off info, and there are new conflicts, please resolve them. thanks.

@bangqipropel bangqipropel force-pushed the CRD_v1alpha2 branch 2 times, most recently from 8916036 to 953f36c Compare June 14, 2022 08:20
docs/multicluster/api.md Outdated Show resolved Hide resolved
multicluster/test/integration/suite_test.go Outdated Show resolved Hide resolved
multicluster/test/integration/suite_test.go Outdated Show resolved Hide resolved
multicluster/test/yamls/clusterset.yml Outdated Show resolved Hide resolved
multicluster/test/yamls/clusterset.yml Outdated Show resolved Hide resolved
@luolanzone
Copy link
Contributor

/test-multicluster-dataplane-e2e

@bangqipropel bangqipropel force-pushed the CRD_v1alpha2 branch 3 times, most recently from c414b2b to d2ad946 Compare June 14, 2022 22:49
@bangqipropel bangqipropel changed the title [WIP]:Update ClusterClaim to v1alpha2 Update ClusterClaim to v1alpha2 Jun 15, 2022
@bangqipropel
Copy link
Contributor Author

@luolanzone this can be reviewed again, thanks!

@@ -29,13 +29,12 @@ const (

// +genclient
//+kubebuilder:object:root=true
// +kubebuilder:storageversion
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this if v1alpha1 is removed?

func (r *ClusterClaimReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
_ = log.FromContext(ctx)

// your logic here
// TODO(user): your logic here
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove this file as well? it has no reconcile logic and is not used.

@luolanzone
Copy link
Contributor

luolanzone commented Jun 16, 2022

Please update your PR summary and commit messages.

Update ClusterClaim to v1alpha2

1. ClusterClaim upgraded to v1alpha2, with removing the Name of it

2. metadata.name in ClusterClaim can only be `id.k8s.io` or
   `clusterset.k8s.io`

3. Deprecate ClusterClaim version v1alpha1

@bangqipropel
Copy link
Contributor Author

/test-multicluster-dataplane-e2e

@bangqipropel
Copy link
Contributor Author

@jianjuns can this be reviewed? thanks!

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.

LGTM

@bangqipropel
Copy link
Contributor Author

@tnqn can this be reviewed? thanks!

@luolanzone
Copy link
Contributor

@bangqipropel Could you please resolve conflicts? and update the PR summary to link it with issue #3328. Thanks.

@bangqipropel
Copy link
Contributor Author

/test-multicluster-dataplane-e2e

@bangqipropel bangqipropel linked an issue Jun 26, 2022 that may be closed by this pull request
@bangqipropel
Copy link
Contributor Author

@luolanzone it is good now, thanks!

@bangqipropel
Copy link
Contributor Author

@tnqn can this be reviewed? thanks!

@tnqn
Copy link
Member

tnqn commented Jun 28, 2022

@tnqn can this be reviewed? thanks!

will take a look tomorrow.

@bangqipropel bangqipropel force-pushed the CRD_v1alpha2 branch 2 times, most recently from 117a518 to 108bf2f Compare June 29, 2022 01:19
@bangqipropel
Copy link
Contributor Author

/test-multicluster-dataplane-e2e

@luolanzone
Copy link
Contributor

/test-multicluster-e2e

@luolanzone
Copy link
Contributor

Hi @bangqipropel , forget to tell you that the comment trigger of MC e2e is changed to test-multicluster-e2e, so please use it next time.

@luolanzone
Copy link
Contributor

@bangqipropel , e2e failed, could you check here http://10.176.27.169:8080/job/antrea-multicluster-e2e-for-pull-request/30/console and fix the error?

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 overall


var _ webhook.Validator = &ClusterClaim{}

// ValidateCreate implements webhook.Validator so a webhook will be registered for the type
func (r *ClusterClaim) ValidateCreate() error {
clusterclaimlog.Info("validate create", "name", r.Name)
if r.Name != WellKnownClusterClaimClusterSet && r.Name != WellKnownClusterClaimID {
Copy link
Member

Choose a reason for hiding this comment

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

But webhooks is not set in multicluster/PROJECT, will this be called?

Copy link
Contributor Author

@bangqipropel bangqipropel Jul 5, 2022

Choose a reason for hiding this comment

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

➜  multicluster git:(CRD_v1alpha2) ✗ kubectl apply -f config/samples/clusterset_init/multicluster_membercluster_template.yaml
clusterclaim.multicluster.crd.antrea.io/clusterset.k8s.io unchanged
clusterset.multicluster.crd.antrea.io/test-clusterset unchanged
Error from server (The name id.k8s.ioerror is not valid, only 'id.k8s.io' and 'clusterset.k8s.io' are valid name for ClusterClaim): error when creating "config/samples/clusterset_init/multicluster_member

@tnqn I think this is called?

Copy link
Member

Choose a reason for hiding this comment

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

I think the change in multicluster/PROJECT is confusing: It removes webhooks when bumping up to v1alpha2 which looks like it doesn't need webhooks any more. But I guess the deleted options were just using default values so it doesn't matter whether they exist. However, there are still some apis in that file setting webhooks to true explicitly, which is more confusing. What does it imply when it's set explictly and when it's not set?

multicluster/test/yamls/east-member-cluster.yml Outdated Show resolved Hide resolved
@luolanzone
Copy link
Contributor

@bangqipropel Could you address quan's comment and resolve conflicts? I would suggest to merge this as early as possible. Thanks.

@bangqipropel
Copy link
Contributor Author

/test-multicluster-e2e

1. ClusterClaim upgraded to v1alpha2, with removing the Name of it

2. metadata.name in ClusterClaim can only be `id.k8s.io` or
`clusterset.k8s.io`

3. Deprecate ClusterClaim version v1alpha1

Signed-off-by: zbangqi <[email protected]>
@bangqipropel
Copy link
Contributor Author

/test-multicluster-e2e

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
Copy link
Contributor

jianjuns commented Jul 6, 2022

/skip-all
The change impacts only MC controller.

@jianjuns jianjuns merged commit b766cdc into antrea-io:main Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ClusterID uniqueness validation in a ClusterSet
5 participants