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.

Sign up for GitHub

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

Jump to bottom

Enhance ClusterSet controller to support ClusterSet version upgrade #5250

Merged
merged 1 commit into from
Jul 19, 2023
Merged

Enhance ClusterSet controller to support ClusterSet version upgrade #5250

merged 1 commit into from
Jul 19, 2023

Conversation

luolanzone
Copy link
Contributor

Refine ClusterSet controller to watch ClusterClaims to handle the ClusterSet upgrade from v1alpha1 to v1alpha2. When the ClusterID is not in ClusterSet spec, controller will try to get ClusterID from ClusterClaims.

@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@luolanzone luolanzone added this to the Antrea v1.13 release milestone Jul 14, 2023
jianjuns
jianjuns reviewed Jul 14, 2023
View reviewed changes
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.

We should still at least document how to manually convert the CRs to new version.

multicluster/cmd/multicluster-controller/controller.go Outdated Show resolved Hide resolved
multicluster/controllers/multicluster/common/controller_utils.go Outdated
@@ -78,3 +81,49 @@ func parseServiceCIDRFromError(msg string) (string, error) {
func NewClusterInfoResourceExportName(clusterID string) string {
return clusterID + "-clusterinfo"
}

func ValidateLocalClusterClaim(c client.Client, clusterSet *mcv1alpha2.ClusterSet) (clusterID ClusterID, err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it need to be public?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed it to getClusterIDFromClusterClaim.

multicluster/controllers/multicluster/common/controller_utils.go Outdated Show resolved Hide resolved
multicluster/controllers/multicluster/common/controller_utils.go Outdated
}
return clusterID, nil
}
return "", fmt.Errorf("the spec.clusterID field of ClusterSet %s is required", req.NamespacedName)
Copy link
Contributor

Choose a reason for hiding this comment

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

"clusterID" is not set in the ClusterSet %s spec"?

multicluster/controllers/multicluster/common/controller_utils.go Outdated
// Here we try to get the ClusterID from ClusterClaim before return any error.
if clusterCalimCRDAvailable {
clusterID, err := ValidateLocalClusterClaim(client, clusterSet)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider:

if err == nil {
        return clusterID, nil
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

multicluster/controllers/multicluster/common/controller_utils.go Outdated Show resolved Hide resolved
multicluster/controllers/multicluster/common/controller_utils.go Outdated Show resolved Hide resolved
multicluster/controllers/multicluster/leader/clusterset_controller.go Outdated Show resolved Hide resolved
multicluster/controllers/multicluster/member/clusterset_controller.go Outdated Show resolved Hide resolved
@luolanzone luolanzone added the area/multi-cluster Issues or PRs related to multi cluster. label Jul 17, 2023
@luolanzone luolanzone force-pushed the refine-clusterset-controller branch 4 times, most recently from f25ef66 to 5238f42 Compare July 17, 2023 02:47
jianjuns
jianjuns reviewed Jul 17, 2023
View reviewed changes
docs/multicluster/upgrade.md Outdated
the `ClusterSet` CRD solely defines a ClusterSet. The name of a ClusterSet CR must match the
ClusterSet ID, and a new `clusterID` field specifies the Cluster ID.

To upgrade from a version older than v1.13, once you apply the latest manifests for an
Copy link
Contributor

Choose a reason for hiding this comment

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

We should talk about the new version MC Controller will still recognize old version ClusterClaim and ClusterSet CRs, but if the ClusterSet name does not match the ClusterSet ID, you should delete the ClusterSet CR and create a new one with using ClusterSet ID as the name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found that the old codes already validate the ClusterSet ID and the ClusterSet CR's name to make sure they are the same, so I suppose there is no need to handle the recreate case. Both leader and member ClusterSet controller will call ValidateLocalClusterClaim:

antrea/multicluster/controllers/multicluster/member/clusterset_controller.go

Line 116 in 1e0fbd6

clusterID, clusterSetID, err := common.ValidateLocalClusterClaim(r.Client, clusterSet)

The method will check if the ClusterSet ID and ClusterSet's name matched.

antrea/multicluster/controllers/multicluster/common/controller_utils.go

Lines 74 to 78 in 1e0fbd6

if clusterSet.Name != string(clusterSetID) {
err = fmt.Errorf("ClusterSet Name=%s is not same as ClusterClaim Value=%s for Name=%s",
clusterSet.Name, clusterSetID, multiclusterv1alpha2.WellKnownClusterClaimClusterSet)
return
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. This is good to know.

docs/multicluster/upgrade.md Outdated Show resolved Hide resolved
docs/multicluster/upgrade.md Outdated Show resolved Hide resolved
@jianjuns jianjuns changed the title Refine ClusterSet controller to support ClusterSet version upgrade Enhance ClusterSet controller to support ClusterSet version change Jul 17, 2023
@jianjuns jianjuns changed the title Enhance ClusterSet controller to support ClusterSet version change Enhance ClusterSet controller to support ClusterSet version upgrade Jul 17, 2023
@luolanzone luolanzone force-pushed the refine-clusterset-controller branch from 5238f42 to 95335a0 Compare July 18, 2023 01:34
jianjuns
jianjuns reviewed Jul 18, 2023
View reviewed changes
docs/multicluster/upgrade.md Outdated
the `ClusterSet` CRD solely defines a ClusterSet. The name of a ClusterSet CR must match the
ClusterSet ID, and a new `clusterID` field specifies the Cluster ID.

To upgrade from a version older than v1.13, once you apply the latest manifests for an
Copy link
Contributor

Choose a reason for hiding this comment

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

We should still revise the paragraph to explain the process clearer. Consider the following:

"After upgrading Antrea Multi-cluster Controller from a version older than v1.13, the new version Multi-cluster Controller can still recognize and work with the old version ClusterClaim and ClusterSet CRs. However, we still suggest updating the ClusterSet CR to the new version after upgrading Multi-cluster Controller. You just need to update the existing ClusterSet CR and add the right clusterID to the spec."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated, thanks.

docs/multicluster/upgrade.md Outdated Show resolved Hide resolved
docs/multicluster/upgrade.md Outdated
version of Antrea Multi-cluster controller will update the existing `ClusterSet` CR with
the valid Cluster ID. You can verify this via `kubectl get clusterclaim id.k8s.io -o json -n kube-system | jq -r '.value'`.

Once you finish the cluster upgrade, you should also delete the `ClusterClaim` CRD
Copy link
Contributor

Choose a reason for hiding this comment

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

"You may also delete ClusterClaim CRD after the upgrade, ..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@luolanzone luolanzone force-pushed the refine-clusterset-controller branch from 95335a0 to c84a234 Compare July 18, 2023 08:13
@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

jianjuns
jianjuns reviewed Jul 18, 2023
View reviewed changes
docs/multicluster/upgrade.md Outdated Show resolved Hide resolved
docs/multicluster/upgrade.md Outdated Show resolved Hide resolved
docs/multicluster/upgrade.md Outdated Show resolved Hide resolved
docs/multicluster/upgrade.md Outdated Show resolved Hide resolved
docs/multicluster/upgrade.md Outdated Show resolved Hide resolved
@luolanzone luolanzone force-pushed the refine-clusterset-controller branch from c84a234 to 4eaf553 Compare July 19, 2023 01:29
@luolanzone
Refine ClusterSet controller to support ClusterSet version upgrade
5abd2c6 
Refine ClusterSet controller to watch ClusterClaims to handle the
ClusterSet upgrade from v1alpha1 to v1alpha2. When the ClusterID
is not in ClusterSet spec, controller will try to get ClusterID from
ClusterClaims.

Signed-off-by: Lan Luo <[email protected]>
@luolanzone luolanzone force-pushed the refine-clusterset-controller branch from 4eaf553 to 5abd2c6 Compare July 19, 2023 01:30
jianjuns
jianjuns approved these changes Jul 19, 2023
View reviewed changes
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

@luolanzone
Copy link
Contributor Author

/test-multicluster-e2e

@jianjuns
Copy link
Contributor

/skip-all

@jianjuns jianjuns merged commit c0b9027 into antrea-io:main Jul 19, 2023
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jianjuns jianjuns jianjuns approved these changes

Assignees
No one assigned
Labels
area/multi-cluster Issues or PRs related to multi cluster.
Projects
None yet
Milestone
Antrea v1.13 release
Development

Successfully merging this pull request may close these issues.
2 participants
@luolanzone @jianjuns