From cce4d4815a3050f9ecbc8f9732f38e466098361c Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Thu, 5 Dec 2024 18:39:33 +0800 Subject: [PATCH 1/2] issue 8433: add ask label to data mover pods Signed-off-by: Lyndon-Li --- changelogs/unreleased/8487-Lyndon-Li | 1 + pkg/exposer/csi_snapshot.go | 4 ++++ pkg/exposer/generic_restore.go | 8 ++++++++ pkg/exposer/types.go | 4 ++++ 4 files changed, 17 insertions(+) create mode 100644 changelogs/unreleased/8487-Lyndon-Li diff --git a/changelogs/unreleased/8487-Lyndon-Li b/changelogs/unreleased/8487-Lyndon-Li new file mode 100644 index 000000000..8ea6009cf --- /dev/null +++ b/changelogs/unreleased/8487-Lyndon-Li @@ -0,0 +1 @@ +Fix issue #8433, add aks label to data mover pods \ No newline at end of file diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 59aabf59e..30180fa8d 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -488,6 +488,10 @@ func (e *csiSnapshotExposer) createBackupPod( } label[podGroupLabel] = podGroupSnapshot + for k, v := range thirdPartyLabels { + label[k] = v + } + volumeMode := corev1.PersistentVolumeFilesystem if backupPVC.Spec.VolumeMode != nil { volumeMode = *backupPVC.Spec.VolumeMode diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index d498470a7..1900ad698 100644 --- a/pkg/exposer/generic_restore.go +++ b/pkg/exposer/generic_restore.go @@ -323,6 +323,14 @@ func (e *genericRestoreExposer) createRestorePod(ctx context.Context, ownerObjec }} volumes = append(volumes, podInfo.volumes...) + if label == nil { + label = make(map[string]string) + } + + for k, v := range thirdPartyLabels { + label[k] = v + } + volumeMode := corev1.PersistentVolumeFilesystem if targetPVC.Spec.VolumeMode != nil { volumeMode = *targetPVC.Spec.VolumeMode diff --git a/pkg/exposer/types.go b/pkg/exposer/types.go index d4d8c8730..a75f5d379 100644 --- a/pkg/exposer/types.go +++ b/pkg/exposer/types.go @@ -39,3 +39,7 @@ type ExposeByPod struct { HostingContainer string VolumeName string } + +var thirdPartyLabels map[string]string = map[string]string{ + "azure.workload.identity/use": "true", +} From 3f317300031368371cc31d72223c751ea31e090d Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Fri, 6 Dec 2024 17:32:04 +0800 Subject: [PATCH 2/2] check existence of the same label from node-agent Signed-off-by: Lyndon-Li --- changelogs/unreleased/8487-Lyndon-Li | 1 - changelogs/unreleased/8501-Lyndon-Li | 1 + pkg/controller/data_download_controller.go | 11 ++ pkg/controller/data_upload_controller.go | 14 ++- pkg/exposer/csi_snapshot.go | 4 - pkg/exposer/generic_restore.go | 8 -- pkg/exposer/types.go | 4 - pkg/nodeagent/node_agent.go | 21 +++- pkg/nodeagent/node_agent_test.go | 129 +++++++++++++++++++++ pkg/util/third_party.go | 21 ++++ 10 files changed, 195 insertions(+), 19 deletions(-) delete mode 100644 changelogs/unreleased/8487-Lyndon-Li create mode 100644 changelogs/unreleased/8501-Lyndon-Li create mode 100644 pkg/util/third_party.go diff --git a/changelogs/unreleased/8487-Lyndon-Li b/changelogs/unreleased/8487-Lyndon-Li deleted file mode 100644 index 8ea6009cf..000000000 --- a/changelogs/unreleased/8487-Lyndon-Li +++ /dev/null @@ -1 +0,0 @@ -Fix issue #8433, add aks label to data mover pods \ No newline at end of file diff --git a/changelogs/unreleased/8501-Lyndon-Li b/changelogs/unreleased/8501-Lyndon-Li new file mode 100644 index 000000000..cd67ee404 --- /dev/null +++ b/changelogs/unreleased/8501-Lyndon-Li @@ -0,0 +1 @@ +Fix issue #8433, add third party labels to data mover pods when the same labels exist in node-agent pods \ No newline at end of file diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index 67e198896..d607b13b0 100644 --- a/pkg/controller/data_download_controller.go +++ b/pkg/controller/data_download_controller.go @@ -47,7 +47,9 @@ import ( "github.com/vmware-tanzu/velero/pkg/datapath" "github.com/vmware-tanzu/velero/pkg/exposer" "github.com/vmware-tanzu/velero/pkg/metrics" + "github.com/vmware-tanzu/velero/pkg/nodeagent" "github.com/vmware-tanzu/velero/pkg/uploader" + "github.com/vmware-tanzu/velero/pkg/util" "github.com/vmware-tanzu/velero/pkg/util/kube" ) @@ -178,6 +180,15 @@ func (r *DataDownloadReconciler) Reconcile(ctx context.Context, req ctrl.Request } hostingPodLabels := map[string]string{velerov1api.DataDownloadLabel: dd.Name} + for _, k := range util.ThirdPartyLabels { + if v, err := nodeagent.GetLabelValue(ctx, r.kubeClient, dd.Namespace, k); err != nil { + if err != nodeagent.ErrNodeAgentLabelNotFound { + log.WithError(err).Warnf("Failed to check node-agent label, skip adding host pod label %s", k) + } + } else { + hostingPodLabels[k] = v + } + } // Expose() will trigger to create one pod whose volume is restored by a given volume snapshot, // but the pod maybe is not in the same node of the current controller, so we need to return it here. diff --git a/pkg/controller/data_upload_controller.go b/pkg/controller/data_upload_controller.go index eac2a876b..5723e6323 100644 --- a/pkg/controller/data_upload_controller.go +++ b/pkg/controller/data_upload_controller.go @@ -50,6 +50,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/metrics" "github.com/vmware-tanzu/velero/pkg/nodeagent" "github.com/vmware-tanzu/velero/pkg/uploader" + "github.com/vmware-tanzu/velero/pkg/util" "github.com/vmware-tanzu/velero/pkg/util/kube" ) @@ -808,11 +809,22 @@ func (r *DataUploadReconciler) setupExposeParam(du *velerov2alpha1api.DataUpload accessMode = exposer.AccessModeBlock } + hostingPodLabels := map[string]string{velerov1api.DataUploadLabel: du.Name} + for _, k := range util.ThirdPartyLabels { + if v, err := nodeagent.GetLabelValue(context.Background(), r.kubeClient, du.Namespace, k); err != nil { + if err != nodeagent.ErrNodeAgentLabelNotFound { + r.logger.WithError(err).Warnf("Failed to check node-agent label, skip adding host pod label %s", k) + } + } else { + hostingPodLabels[k] = v + } + } + return &exposer.CSISnapshotExposeParam{ SnapshotName: du.Spec.CSISnapshot.VolumeSnapshot, SourceNamespace: du.Spec.SourceNamespace, StorageClass: du.Spec.CSISnapshot.StorageClass, - HostingPodLabels: map[string]string{velerov1api.DataUploadLabel: du.Name}, + HostingPodLabels: hostingPodLabels, AccessMode: accessMode, OperationTimeout: du.Spec.OperationTimeout.Duration, ExposeTimeout: r.preparingTimeout, diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 30180fa8d..59aabf59e 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -488,10 +488,6 @@ func (e *csiSnapshotExposer) createBackupPod( } label[podGroupLabel] = podGroupSnapshot - for k, v := range thirdPartyLabels { - label[k] = v - } - volumeMode := corev1.PersistentVolumeFilesystem if backupPVC.Spec.VolumeMode != nil { volumeMode = *backupPVC.Spec.VolumeMode diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index 1900ad698..d498470a7 100644 --- a/pkg/exposer/generic_restore.go +++ b/pkg/exposer/generic_restore.go @@ -323,14 +323,6 @@ func (e *genericRestoreExposer) createRestorePod(ctx context.Context, ownerObjec }} volumes = append(volumes, podInfo.volumes...) - if label == nil { - label = make(map[string]string) - } - - for k, v := range thirdPartyLabels { - label[k] = v - } - volumeMode := corev1.PersistentVolumeFilesystem if targetPVC.Spec.VolumeMode != nil { volumeMode = *targetPVC.Spec.VolumeMode diff --git a/pkg/exposer/types.go b/pkg/exposer/types.go index a75f5d379..d4d8c8730 100644 --- a/pkg/exposer/types.go +++ b/pkg/exposer/types.go @@ -39,7 +39,3 @@ type ExposeByPod struct { HostingContainer string VolumeName string } - -var thirdPartyLabels map[string]string = map[string]string{ - "azure.workload.identity/use": "true", -} diff --git a/pkg/nodeagent/node_agent.go b/pkg/nodeagent/node_agent.go index b83efc6f4..ff5d011ec 100644 --- a/pkg/nodeagent/node_agent.go +++ b/pkg/nodeagent/node_agent.go @@ -38,7 +38,8 @@ const ( ) var ( - ErrDaemonSetNotFound = errors.New("daemonset not found") + ErrDaemonSetNotFound = errors.New("daemonset not found") + ErrNodeAgentLabelNotFound = errors.New("node-agent label not found") ) type LoadConcurrency struct { @@ -161,3 +162,21 @@ func GetConfigs(ctx context.Context, namespace string, kubeClient kubernetes.Int return configs, nil } + +func GetLabelValue(ctx context.Context, kubeClient kubernetes.Interface, namespace string, key string) (string, error) { + ds, err := kubeClient.AppsV1().DaemonSets(namespace).Get(ctx, daemonSet, metav1.GetOptions{}) + if err != nil { + return "", errors.Wrap(err, "error getting node-agent daemonset") + } + + if ds.Spec.Template.Labels == nil { + return "", ErrNodeAgentLabelNotFound + } + + val, found := ds.Spec.Template.Labels[key] + if !found { + return "", ErrNodeAgentLabelNotFound + } + + return val, nil +} diff --git a/pkg/nodeagent/node_agent_test.go b/pkg/nodeagent/node_agent_test.go index 9bbf0f46d..1c24427b1 100644 --- a/pkg/nodeagent/node_agent_test.go +++ b/pkg/nodeagent/node_agent_test.go @@ -331,3 +331,132 @@ func TestGetConfigs(t *testing.T) { }) } } + +func TestGetLabelValue(t *testing.T) { + daemonSet := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "fake-ns", + Name: "node-agent", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "DaemonSet", + }, + } + + daemonSetWithOtherLabel := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "fake-ns", + Name: "node-agent", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "DaemonSet", + }, + Spec: appsv1.DaemonSetSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "fake-other-label": "fake-value-1", + }, + }, + }, + }, + } + + daemonSetWithLabel := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "fake-ns", + Name: "node-agent", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "DaemonSet", + }, + Spec: appsv1.DaemonSetSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "fake-label": "fake-value-2", + }, + }, + }, + }, + } + + daemonSetWithEmptyLabel := &appsv1.DaemonSet{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "fake-ns", + Name: "node-agent", + }, + TypeMeta: metav1.TypeMeta{ + Kind: "DaemonSet", + }, + Spec: appsv1.DaemonSetSpec{ + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{ + Labels: map[string]string{ + "fake-label": "", + }, + }, + }, + }, + } + + tests := []struct { + name string + kubeClientObj []runtime.Object + namespace string + expectedValue string + expectErr string + }{ + { + name: "ds get error", + namespace: "fake-ns", + expectErr: "error getting node-agent daemonset: daemonsets.apps \"node-agent\" not found", + }, + { + name: "no label", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{ + daemonSet, + }, + expectErr: ErrNodeAgentLabelNotFound.Error(), + }, + { + name: "no expecting label", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{ + daemonSetWithOtherLabel, + }, + expectErr: ErrNodeAgentLabelNotFound.Error(), + }, + { + name: "expecting label", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{ + daemonSetWithLabel, + }, + expectedValue: "fake-value-2", + }, + { + name: "expecting empty label", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{ + daemonSetWithEmptyLabel, + }, + expectedValue: "", + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + fakeKubeClient := fake.NewSimpleClientset(test.kubeClientObj...) + + value, err := GetLabelValue(context.TODO(), fakeKubeClient, test.namespace, "fake-label") + if test.expectErr == "" { + assert.NoError(t, err) + assert.Equal(t, test.expectedValue, value) + } else { + assert.EqualError(t, err, test.expectErr) + } + }) + } +} diff --git a/pkg/util/third_party.go b/pkg/util/third_party.go new file mode 100644 index 000000000..2be158681 --- /dev/null +++ b/pkg/util/third_party.go @@ -0,0 +1,21 @@ +/* +Copyright the Velero contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package util + +var ThirdPartyLabels []string = []string{ + "azure.workload.identity/use", +}