Add command 'antctl upgrade api-storage' in antctl #5198

hongliangl · 2023-07-04T09:40:11Z

Add command 'antctl upgrade api-storage' in antctl to upgrade
existing objects of Antrea CRDs to the latest API version.

For #4832

TODO:

unit tests
doc
misc

pkg/antctl/raw/upgrade/apistorage/command.go

pkg/antctl/raw/upgrade/apistorage/command_test.go

luolanzone · 2023-07-18T07:29:40Z

pkg/antctl/raw/upgrade/apistorage/command.go

+}
+
+// patchCRDStoredVersions is used to patch field status.storedVersion of a CRD.
+func patchCRDStoredVersions(k8sClient client.Client, crd *apiextv1.CustomResourceDefinition) error {


I suppose user have to set one and only one version's storage as true when the CRD has multiple versions, so why do we need to patch the status.storedVersion of a CRD?

We may a CRD which has more than one value in status.storedVersion in this case:

create v1alpha1 of the CRD, and field of storage in v1alpha is set to true.

create a CR of version v1alpha1,

add v1beta1 of the CRD, and field of storage in v1beta1 is set to true and storage in v1beta1 is set to false.

create a CR of version v1beta1

We will get a CRD whose status.storedVersion has two values.

User may have the objects of the CRD in multiple versions. After upgrading all objects, the status.storedVersion should only have the latest version value.

Will status.storedVersion be managed and updated by K8s automatically? Have you tried to update all CRs to latest version and check after upgrade later?
cc @tnqn may have more ideas.

I remember K8s doesn't remove versions from it automatically, this is also the doc says.

pkg/antctl/raw/upgrade/apistorage/command_test.go

docs/antctl.md

luolanzone · 2023-07-20T01:30:39Z

docs/antctl.md

+antctl upgrade api-storage
+```
+
+This command is used to upgrade persisted resources for Antrea CRD `antreaagentinfos.crd.antrea.io` to the latest API


docs/antctl.md

luolanzone · 2023-07-20T01:41:22Z

pkg/antctl/command_definition.go

+		Short: "Execute a user-provided upgrade",
+		Long:  "Execute a user-provided upgrade",


This is not clear to understand, maybe:

Suggested change

Short: "Execute a user-provided upgrade",

Long: "Execute a user-provided upgrade",

Short: "Upgrade version of Kubernetes resources",

Long: "Upgrade version of Kubernetes resources",

@antoninbas @tnqn any suggestion?

How about:
"Upgrade Antrea CRD resources"
"Upgrade Antrea CRD resources to the latest API version"

I thought of that upgrade will be a group of commands. We could upgrade not only CRD resources, maybe we could upgrade something else used by Antrea. Do we need to limit the command upgrade to K8s resources or CRD right now?

pkg/antctl/raw/upgrade/apistorage/command.go

docs/antctl.md

pkg/antctl/raw/upgrade/apistorage/command.go

tnqn · 2023-07-20T16:08:27Z

pkg/antctl/raw/upgrade/apistorage/command.go

+			Duration: time.Second,
+			Steps:    5,
+		}, func(err error) bool {
+			return validateErr(err) != nil


validateErr also checks if err is nil, which can never happen here.
It's wrong to retry on NotFound error.
It should just check apierrors.IsConflict(err)

We should not retry on NotFound, but we should retry on other errors, not only IsConfict. Is it right?

tnqn · 2023-07-20T16:10:36Z

pkg/antctl/raw/upgrade/apistorage/command.go

+		}, func(err error) bool {
+			return validateErr(err) != nil
+		}, func() error {
+			return k8sClient.Update(context.TODO(), &copyObj)


shouldn't it get the object once when retrying? otherwise this can never succeed since it has a stale resource version. Even if it succeeds by removing the resource version in the request, it would override the updates made after you list the objects.

Updated here, every time when to update the object, get the object first by the namespace+name from the list.

pkg/antctl/raw/upgrade/apistorage/command.go

tnqn · 2023-07-20T16:17:20Z

pkg/antctl/raw/upgrade/apistorage/command.go

+	expectedStorageVersion := getCRDStorageVersion(crd)
+	// This ensures that the storage version is not changed during the upgrade.
+	if getCRDStorageVersion(freshCRD) != expectedStorageVersion {
+		return newUnexpectedChangeError(crd)
+	}


This is perhaps unnecessary because:

Even if you check it, it's still possible that the CRD is updated after you get the "fresh" version.

We should leverage the resourceVersion to let apiserver fail the update request when the CRD is updated after we get it.

Even if you check it, it's still possible that the CRD is updated after you get the "fresh" version.

I got this and removed related code.

We should leverage the resourceVersion to let apiserver fail the update request when the CRD is updated after we get it.

Could you give more details about this?

The code seems not removed. What I mean is, we could assume the CRD is not updated during the upgrade. It could just update the StoredVersions to the storage version in the CRD's spec. If the CRD is updated during the upgrade, the update will fail because the resourceVersion doesn't match. ResourceVersion is used for this situation, preventing one client overriding another's write accidently.

If the update request fails with Conflict error, it means the CRD might have been updated in some way by users. We could just return error and abort the process.

jianjuns

The documentation part looks good to me.

docs/antctl.md

luolanzone

LGTM overall, one comment about logs.

pkg/antctl/raw/upgrade/apistorage/command.go

tnqn · 2023-07-25T15:25:22Z

pkg/antctl/raw/upgrade/apistorage/command.go

+			failed++
+		}
+	}
+	fmt.Fprintf(writer, "Successfully upgraded %d objects of CRD %q.\n", len(objList.Items)-failed, crd.Name)


My comment was not really about logging. The command should NOT succeed when there is anything wrong. Think about when the tool is integrated into a cluster management system: how can its caller, a program, know if it succeeds or fails. The exit code must indicate that.

I think it should just return error and abort the process when encountering the first error.

Got that, I have updated the PR as you suggested.

pkg/antctl/raw/upgrade/apistorage/command.go

tnqn · 2023-07-25T15:41:25Z

pkg/antctl/raw/upgrade/apistorage/command.go

+	expectedStorageVersion := getCRDStorageVersion(crd)
+	// This ensures that the storage version is not changed during the upgrade.
+	if getCRDStorageVersion(freshCRD) != expectedStorageVersion {
+		return newUnexpectedChangeError(crd)
+	}


The code seems not removed. What I mean is, we could assume the CRD is not updated during the upgrade. It could just update the StoredVersions to the storage version in the CRD's spec. If the CRD is updated during the upgrade, the update will fail because the resourceVersion doesn't match. ResourceVersion is used for this situation, preventing one client overriding another's write accidently.

If the update request fails with Conflict error, it means the CRD might have been updated in some way by users. We could just return error and abort the process.

pkg/antctl/raw/upgrade/apistorage/command.go

pkg/antctl/command_definition.go

docs/antctl.md

tnqn · 2023-07-27T07:38:10Z

docs/antctl.md

+### Upgrade existing objects of CRDs
+
+antctl supports upgrading existing objects of Antrea CRDs to the latest version.
+The related sub-commands should be run out-of-cluster.


We should mention the required permissions. It includes the get, list, and update of each CRD and its objects.
And make sure proper error is printed when some permissions are missing.

Like multicluster sub-commands, currently, antctl upgrade can be only used outside the cluster with kubeconfig file to access to K8s-apiserver. If so, do we still need to mention the required permissions?

Running outside the cluster doesn't mean the kubeconfig file has admin priviledge. Normally we should add the required permissions of this command to ClusterRole antctl as the existence of the ClusterRole implies it contains all permissions its commands require. However, the required permissions of upgrade api-storage could make antctl a super user, while its privilege is currently quite limited, the only create permission it get is creating supportbundles.

So a conservative approach is to document clearly what permissions are required for the upgrade command, then users know what they need to do when they encounter the issue.

I don't know how antctl mc guides users to get required permissions. But for other commands, we have added all permissions to ClusterRole antctl. For upgrage api-storage, we need at least document it.

cc @jianjuns @antoninbas

Got that, I'll add the required permissions in doc and give some error prints for lacking permissions.

I think there is no document or permissions added to ClusterRole when we add the antctl mc, I will create an issue to track this first.

docs/antctl.md

pkg/antctl/raw/upgrade/apistorage/command.go

luolanzone · 2023-07-28T01:20:21Z

pkg/antctl/raw/upgrade/apistorage/command.go

+				Kind:    crd.Spec.Names.Kind,
+			})
+			if getErr = k8sClient.Get(context.TODO(), client.ObjectKeyFromObject(&obj), objToUpdate); getErr != nil {
+				return getErr


I think we can ignore the not found error here if the object is deleted during retry?

If we got not found error, it will not retry and return the error directly.

existing objects of Antrea CRDs to the storage version. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

tnqn

LGTM

tnqn · 2023-07-28T06:40:11Z

/test-all

hongliangl requested review from tnqn, luolanzone and jianjuns July 4, 2023 09:51

hongliangl assigned antoninbas Jul 4, 2023

hongliangl requested a review from antoninbas July 4, 2023 09:56

luolanzone added this to the Antrea v1.13 release milestone Jul 4, 2023

luolanzone reviewed Jul 7, 2023

View reviewed changes

hongliangl marked this pull request as ready for review July 12, 2023 05:02

hongliangl changed the title ~~[WIP] Add command 'antctl upgrade api-storage' in antctl~~ Add command 'antctl upgrade api-storage' in antctl Jul 12, 2023

luolanzone reviewed Jul 18, 2023

View reviewed changes

luolanzone unassigned antoninbas Jul 18, 2023

hongliangl force-pushed the 20230628-api-upgrade-tool branch from e71e08b to a5644a5 Compare July 19, 2023 08:31

hongliangl requested a review from luolanzone July 19, 2023 08:31

hongliangl force-pushed the 20230628-api-upgrade-tool branch from a5644a5 to 9d29e39 Compare July 19, 2023 08:32

luolanzone reviewed Jul 20, 2023

View reviewed changes

luolanzone linked an issue Jul 20, 2023 that may be closed by this pull request

[Antrea 2.0]CLI for API Deprecation/Removal #5067

Closed

luolanzone added the action/release-note Indicates a PR that should be included in release notes. label Jul 20, 2023

jianjuns reviewed Jul 20, 2023

View reviewed changes

hongliangl force-pushed the 20230628-api-upgrade-tool branch 4 times, most recently from abd6ca8 to 16fbaf1 Compare July 20, 2023 08:22

hongliangl requested review from jianjuns and luolanzone July 20, 2023 08:23

hongliangl force-pushed the 20230628-api-upgrade-tool branch 2 times, most recently from a7cd65a to aa88234 Compare July 20, 2023 10:26

tnqn reviewed Jul 20, 2023

View reviewed changes

jianjuns reviewed Jul 20, 2023

View reviewed changes

docs/antctl.md Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230628-api-upgrade-tool branch from aa88234 to b7644d8 Compare July 22, 2023 02:58

luolanzone reviewed Jul 24, 2023

View reviewed changes

pkg/antctl/raw/upgrade/apistorage/command.go Show resolved Hide resolved

tnqn reviewed Jul 25, 2023

View reviewed changes

hongliangl force-pushed the 20230628-api-upgrade-tool branch from b7644d8 to 84bded8 Compare July 26, 2023 03:35

hongliangl requested review from tnqn, luolanzone and jianjuns July 26, 2023 03:37

hongliangl force-pushed the 20230628-api-upgrade-tool branch from 84bded8 to 4a66a72 Compare July 26, 2023 03:50

tnqn reviewed Jul 26, 2023

View reviewed changes

pkg/antctl/raw/upgrade/apistorage/command.go Outdated Show resolved Hide resolved

pkg/antctl/raw/upgrade/apistorage/command.go Outdated Show resolved Hide resolved

pkg/antctl/raw/upgrade/apistorage/command.go Outdated Show resolved Hide resolved

hongliangl force-pushed the 20230628-api-upgrade-tool branch 2 times, most recently from 19a029c to 4beeeba Compare July 27, 2023 01:51

hongliangl requested a review from tnqn July 27, 2023 01:54

tnqn reviewed Jul 27, 2023

View reviewed changes

pkg/antctl/raw/upgrade/apistorage/command.go Outdated Show resolved Hide resolved

pkg/antctl/command_definition.go Outdated Show resolved Hide resolved

tnqn reviewed Jul 27, 2023

View reviewed changes

hongliangl force-pushed the 20230628-api-upgrade-tool branch from 4beeeba to d6ea9d2 Compare July 27, 2023 09:55

luolanzone mentioned this pull request Jul 27, 2023

Update antctl ClusterRole for antctl mc or document required permissions #5327

Closed

hongliangl force-pushed the 20230628-api-upgrade-tool branch from d6ea9d2 to 83628ba Compare July 27, 2023 15:48

hongliangl requested a review from tnqn July 27, 2023 15:48

luolanzone reviewed Jul 28, 2023

View reviewed changes

hongliangl force-pushed the 20230628-api-upgrade-tool branch from 83628ba to 68b6570 Compare July 28, 2023 01:41

Add command 'antctl upgrade api-storage' in antctl to upgrade

6d69400

existing objects of Antrea CRDs to the storage version. Signed-off-by: Hongliang Liu <lhongliang@vmware.com>

hongliangl force-pushed the 20230628-api-upgrade-tool branch from 68b6570 to 6d69400 Compare July 28, 2023 01:43

hongliangl requested a review from luolanzone July 28, 2023 01:44

tnqn approved these changes Jul 28, 2023

View reviewed changes

tnqn merged commit ddf4d44 into antrea-io:main Jul 28, 2023
46 of 55 checks passed

hongliangl deleted the 20230628-api-upgrade-tool branch July 31, 2023 02:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add command 'antctl upgrade api-storage' in antctl #5198

Add command 'antctl upgrade api-storage' in antctl #5198

hongliangl commented Jul 4, 2023 •

edited

luolanzone Jul 18, 2023

hongliangl Jul 19, 2023

luolanzone Jul 19, 2023

tnqn Jul 20, 2023

luolanzone Jul 20, 2023

luolanzone Jul 20, 2023

jianjuns Jul 20, 2023

hongliangl Jul 20, 2023

tnqn Jul 20, 2023

hongliangl Jul 24, 2023

tnqn Jul 20, 2023

hongliangl Jul 24, 2023

tnqn Jul 20, 2023

hongliangl Jul 24, 2023

tnqn Jul 25, 2023

jianjuns left a comment

luolanzone left a comment

tnqn Jul 25, 2023

hongliangl Jul 26, 2023

tnqn Jul 25, 2023

tnqn Jul 27, 2023

hongliangl Jul 27, 2023

hongliangl Jul 27, 2023

tnqn Jul 27, 2023 •

edited

hongliangl Jul 27, 2023

luolanzone Jul 27, 2023

hongliangl Jul 27, 2023

luolanzone Jul 28, 2023

hongliangl Jul 28, 2023

tnqn left a comment

tnqn commented Jul 28, 2023

		Short: "Execute a user-provided upgrade",
		Long: "Execute a user-provided upgrade",

Add command 'antctl upgrade api-storage' in antctl #5198

Add command 'antctl upgrade api-storage' in antctl #5198

Conversation

hongliangl commented Jul 4, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns left a comment

Choose a reason for hiding this comment

luolanzone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn Jul 27, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jul 28, 2023

hongliangl commented Jul 4, 2023 •

edited

tnqn Jul 27, 2023 •

edited