Implement resource pruning in k8s plugin #5455

Warashi · 2024-12-26T07:22:35Z

What this PR does:

as title

Why we need it:

to prune the resources which removed in the git repository

Which issue(s) this PR fixes:

Part of #4980

Does this PR introduce a user-facing change?: No

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

codecov · 2024-12-26T07:27:09Z

Codecov Report

Attention: Patch coverage is 41.07143% with 132 lines in your changes missing coverage. Please review.

Project coverage is 26.19%. Comparing base (792015b) to head (40af37c).
Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
.../app/pipedv1/plugin/kubernetes/provider/kubectl.go	0.00%	77 Missing ⚠️
...app/pipedv1/plugin/kubernetes/deployment/server.go	65.51%	16 Missing and 4 partials ⚠️
...app/pipedv1/plugin/kubernetes/provider/manifest.go	19.04%	17 Missing ⚠️
...g/app/pipedv1/plugin/kubernetes/provider/loader.go	0.00%	13 Missing ⚠️
...app/pipedv1/plugin/kubernetes/provider/resource.go	89.36%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5455      +/-   ##
==========================================
+ Coverage   26.15%   26.19%   +0.04%     
==========================================
  Files         456      457       +1     
  Lines       48992    49303     +311     
==========================================
+ Hits        12814    12916     +102     
- Misses      35156    35362     +206     
- Partials     1022     1025       +3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi · 2024-12-26T07:25:17Z

pkg/app/pipedv1/plugin/kubernetes/provider/loader.go

+			// Set the namespace for all manifests.
+			manifests[i].body.SetNamespace(input.Namespace)


We need this to ensure the loaded manifests' namespaces are matched with applied resource.

Oops, we have to check the input.Namespace is empty or not.

Warashi · 2024-12-26T07:25:56Z

pkg/app/pipedv1/plugin/kubernetes/provider/loader.go

+			// Add builtin labels and annotations for tracking application live state.
+			manifests[i].AddLabels(map[string]string{
+				LabelManagedBy:   ManagedByPiped,
+				LabelPiped:       input.PipedID,
+				LabelApplication: input.AppID,
+				LabelCommitHash:  input.CommitHash,
+			})


I want these labels to retrieve live resources when pruning the resources.

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

ffjlabo

I haven't seen everything yet, but I left some comments 🙏

ffjlabo · 2024-12-27T05:15:54Z

pkg/app/pipedv1/plugin/kubernetes/provider/kubectl.go

+// getAPIResources retrieves the list of available API resources from the Kubernetes cluster.
+// It runs the `kubectl api-resources` command with the specified kubeconfig and returns the
+// names of the resources that support the "list", "get", and "delete" verbs.
+func (c *Kubectl) getAPIResources(ctx context.Context, kubeconfig string) ([]string, error) {
+	args := []string{"api-resources", "--namespaced=false", "--verbs=list,get,delete", "--output=name"}


[nit] How about fixing the name as getClusterScopedAPIResources?
It seems that the method calls kubectl api-resources --namespaced=false.

I fixed this on this commit.
c2a747f

ffjlabo · 2024-12-27T05:35:19Z

pkg/app/pipedv1/plugin/kubernetes/provider/resource.go

+	groupKind schema.GroupKind
+	namespace string
+	name      string


It would be nice to comment on why piped identifies these three values. 🙏

I added comments on this commit.
6fb0aab

ffjlabo · 2024-12-27T05:42:31Z

pkg/app/pipedv1/plugin/kubernetes/provider/kubectl.go

+	if namespace == "" {
+		args = append(args, "--all-namespaces")


[ask] In this case, does the result include the cluster scoped resources?

The result does not include the cluster scoped resources.
I checked with the kubectl v1.32.0.

$ kubectl version Client Version: v1.32.0 Kustomize Version: v5.5.0 Server Version: v1.31.0 $ kubectl get all --all-namespaces NAMESPACE NAME READY STATUS RESTARTS AGE kube-system pod/coredns-6f6b679f8f-czgq7 1/1 Running 0 2d5h kube-system pod/coredns-6f6b679f8f-j7vcr 1/1 Running 0 2d5h kube-system pod/etcd-kind-control-plane 1/1 Running 0 2d5h kube-system pod/kindnet-rxcpn 1/1 Running 0 2d5h kube-system pod/kube-apiserver-kind-control-plane 1/1 Running 0 2d5h kube-system pod/kube-controller-manager-kind-control-plane 1/1 Running 0 2d5h kube-system pod/kube-proxy-58jbg 1/1 Running 0 2d5h kube-system pod/kube-scheduler-kind-control-plane 1/1 Running 0 2d5h local-path-storage pod/local-path-provisioner-57c5987fd4-52wtm 1/1 Running 0 2d5h NAMESPACE NAME TYPE CLUSTER-IP EXTERNAL-IP PORT(S) AGE default service/kubernetes ClusterIP 10.96.0.1 <none> 443/TCP 2d5h kube-system service/kube-dns ClusterIP 10.96.0.10 <none> 53/UDP,53/TCP,9153/TCP 2d5h NAMESPACE NAME DESIRED CURRENT READY UP-TO-DATE AVAILABLE NODE SELECTOR AGE kube-system daemonset.apps/kindnet 1 1 1 1 1 kubernetes.io/os=linux 2d5h kube-system daemonset.apps/kube-proxy 1 1 1 1 1 kubernetes.io/os=linux 2d5h NAMESPACE NAME READY UP-TO-DATE AVAILABLE AGE kube-system deployment.apps/coredns 2/2 2 2 2d5h local-path-storage deployment.apps/local-path-provisioner 1/1 1 1 2d5h NAMESPACE NAME DESIRED CURRENT READY AGE kube-system replicaset.apps/coredns-6f6b679f8f 2 2 2 2d5h local-path-storage replicaset.apps/local-path-provisioner-57c5987fd4 1 1 1 2d5h

ffjlabo · 2024-12-27T05:55:37Z

pkg/app/pipedv1/plugin/kubernetes/provider/manifest.go

@@ -171,8 +200,7 @@ func FindSameManifests(olds, news []Manifest) []WorkloadPair {
 	pairs := make([]WorkloadPair, 0)
 	oldMap := make(map[ResourceKey]Manifest, len(olds))
 	nomalizeKey := func(k ResourceKey) ResourceKey {
-		// Ignoring APIVersion because user can upgrade to the new APIVersion for the same workload.
-		k.apiVersion = ""
+		// Normalize the key by removing the default namespace.


Can we use k.normalize() or k.normalizeNamespace() ?

I fixed to use k.normalize() on this commit.
48c0fb3

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

ffjlabo

Sorry for the more comments 🙏

ffjlabo · 2024-12-27T06:15:42Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server.go

+		return model.StageStatus_STAGE_FAILURE
+	}
+
+	clusterLiveResources, err := kubectl.GetAllClusterScoped(ctx, deployTargetConfig.KubeConfigPath,


[nit] clusterLiveResources -> clusterScopedLiveResources

applied on this commit
6392bca

pkg/app/pipedv1/plugin/kubernetes/provider/resource.go

ffjlabo · 2024-12-27T06:24:08Z

pkg/app/pipedv1/plugin/kubernetes/provider/resource.go

+	)
+
+	{
+		keys := make(map[ResourceKey]struct{}, len(manifests))


[IMO] How about changing keys -> normalizedKeys to distinguish the original key?

applied on this commit
e1dec97

ffjlabo · 2024-12-27T06:26:02Z

pkg/app/pipedv1/plugin/kubernetes/provider/resource.go

+	}
+
+	{
+		keys := make(map[ResourceKey]struct{}, len(manifests))


[IMO] How about changing keys -> normalizedKeys to distinguish the original key?

Same as above

applied on this commit
e1dec97

ffjlabo · 2024-12-27T06:27:28Z

pkg/app/pipedv1/plugin/kubernetes/provider/resource.go

+	{
+		keys := make(map[ResourceKey]struct{}, len(manifests))
+		for _, m := range manifests {
+			keys[m.Key().normalize().withoutNamespace()] = struct{}{}


[nit] It would be nice to add comment to show why using withoutNamespace()

I added comments on this commit.
e1dec97

ffjlabo · 2024-12-27T06:36:10Z

pkg/app/pipedv1/plugin/kubernetes/provider/resource.go

+func normalizeGroupKind(gk schema.GroupKind) schema.GroupKind {
+	gk.Group = strings.ToLower(gk.Group)
+	gk.Kind = strings.ToLower(gk.Kind)
+	return gk
+}


[imo] How about unifying to normalize to avoid using only this method in other code?

applied on this commit.
bcfd121

Co-authored-by: Yoshiki Fujikane <[email protected]> Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

ffjlabo · 2024-12-27T06:57:33Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go

+
+	running := filepath.Join("./", "testdata", "prune", "running")
+
+	// read the running application config from the example file


Suggested change

// read the running application config from the example file

// read the running application config from the testdata file

Thank you. applied on this commit.
f47da4b

ffjlabo · 2024-12-27T06:59:03Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go

+
+	target := filepath.Join("./", "testdata", "prune", "target")
+
+	// read the running application config from the example file


Suggested change

// read the running application config from the example file

// read the running application config from the testdata file

Thank you. applied on this commit.
f47da4b

ffjlabo · 2024-12-27T09:54:41Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go

@@ -136,6 +136,8 @@ func setupTestPluginConfigAndDynamicClient(t *testing.T) (*config.PipedPlugin, d
 }

 func TestDeploymentService_executeK8sSyncStage(t *testing.T) {
+	t.Parallel()


[imo] It might not be set to avoid operation collision for each tests. WDYT>

I think the operation collision does not occur because the envtest's dummy cluster is launched in each test.

I see. It is in setupTestPluginConfigAndDynamicClient. Thanks, I got your point.

ffjlabo · 2024-12-27T10:02:23Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go

+
+	// The service should be created in the new namespace
+	service, err = dynamicClient.Resource(schema.GroupVersionResource{Group: "", Version: "v1", Resource: "services"}).Namespace("test-2").Get(context.Background(), "simple", metav1.GetOptions{})
+	require.NoError(t, err)


[imo] If possible, it would be nice to check the error is not found. (I'm not sure about the spec for the dynamic client 🙏

Thank you. I added the assertion on this commit.
b12e317

ffjlabo · 2024-12-27T10:04:56Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server_test.go

+	require.Equal(t, "piped", service.GetLabels()["pipecd.dev/managed-by"])
+	require.Equal(t, "piped-id", service.GetLabels()["pipecd.dev/piped"])
+	require.Equal(t, "app-id", service.GetLabels()["pipecd.dev/application"])
+	require.Equal(t, "0012345678", service.GetLabels()["pipecd.dev/commit-hash"])


[imo] In addition to them, we need to check the resource key to check whether the namespace has changed correcty.

Thank you. I added resource key assertion on this commit.
d75fd94

ffjlabo · 2024-12-27T10:16:24Z

Also imo about the test 🙏

How about using t.Run for each preparating or checking.
It would be nice to add the testcase for the custom resouce's ones
- When the Custom Resource's preferred version is v1 and the actual resource's version is v1beta1, Is it possible to remove the resource?

That's all for now 🙏

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi · 2025-01-06T00:50:49Z

@ffjlabo Thanks.

How about using t.Run for each preparating or checking.

I fixed the tests on this commit.
34c7807

It would be nice to add the testcase for the custom resouce's ones

When the Custom Resource's preferred version is v1 and the actual resource's version is v1beta1, Is it possible to remove the resource?

I added v1beta1 pruning assertions in the cluster-scoped test because it already uses the custom resources.
40af37c

ffjlabo · 2025-01-07T02:46:27Z

@Warashi Thank you for the great contribution! Those fixes look good!

BTW, I noticed that it might also be nice to add a test case when updating the resource's API version and deleting it (e.g., HPA autoscaling/v1 -> autoscaling/v2, and later remove the resource).
But it might take some more time to do. Is it worth doing now? WDYT?

Warashi · 2025-01-07T02:49:07Z

@ffjlabo Thank you!

BTW, I noticed that it might also be nice to add a test case when updating the resource's API version and deleting it (e.g., HPA autoscaling/v1 -> autoscaling/v2, and later remove the resource). But it might take some more time to do. Is it worth doing now? WDYT?

This PR is already large, so I want to do it on another PR.

ffjlabo · 2025-01-07T02:55:51Z

@Warashi OK! Thanks!

ffjlabo

LGreatTM 👍

Warashi added 11 commits December 25, 2024 17:34

Add labels for application tracking in Kubernetes manifests

3f1509a

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add IsManagedByPiped method

e6f5bc9

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Remove Namespace method from ResourceKey

b150959

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Remove APIVersion method from ResourceKey

9c5d959

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Refactor manifest handling to use Name and Kind methods instead of Key

9aea549

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Change apiVersion and kind to groupKind

5134edb

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Implement pruning resources

f8f0c7e

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Implement resource pruning in k8s_sync

b9879ce

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix the pruning in the case namespace changes

bb444a7

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix pruning the cluster-scoped resources

54309c9

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Enable parallel execution for deployment service tests

b849517

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

github-actions bot added area/go area/pipedv1 labels Dec 26, 2024

Warashi added 2 commits December 26, 2024 16:30

Add documentation for Kubernetes resource retrieval functions

3ed4320

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add license header to resource_test.go

d1e0236

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi commented Dec 26, 2024

View reviewed changes

Fix the LoadManifests

85ba187

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi marked this pull request as ready for review December 26, 2024 07:47

Warashi requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo and t-kikuc as code owners December 26, 2024 07:47

ffjlabo reviewed Dec 27, 2024

View reviewed changes

Warashi added 3 commits December 27, 2024 15:16

Rename getAPIResources to getClusterScopedAPIResources for clarity

c2a747f

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Refactor manifest key normalization

48c0fb3

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add comments to ResourceKey

6fb0aab

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi requested a review from ffjlabo December 27, 2024 06:28

ffjlabo reviewed Dec 27, 2024

View reviewed changes

Warashi and others added 5 commits December 27, 2024 15:45

Refactor to use normalized key only to compare

69ac113

Co-authored-by: Yoshiki Fujikane <[email protected]> Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Rename variable clusterLiveResources to clusterScopedLiveResources

6392bca

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Rename keys to normalizedKeys and add comments

e1dec97

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Refactor to remove normalizeGroupKinds

bcfd121

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Fix to pass tests

2e7b2f5

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi requested a review from ffjlabo December 27, 2024 06:59

ffjlabo reviewed Dec 27, 2024

View reviewed changes

Warashi added 5 commits January 6, 2025 09:16

Update comments to clarify source of application config in tests

f47da4b

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add assertions to check for NotFound errors in deployment service tests

b12e317

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add resource-key assertions to deployment service tests

d75fd94

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Use subtest to prepare e2e tests

34c7807

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add v1beta1 version for CronTab and update tests for pruning behavior

40af37c

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi requested a review from ffjlabo January 6, 2025 00:57

ffjlabo approved these changes Jan 7, 2025

View reviewed changes

t-kikuc approved these changes Jan 14, 2025

View reviewed changes

t-kikuc merged commit 58fda21 into master Jan 14, 2025
18 checks passed

t-kikuc deleted the k8s-plugin-retrieve-live-state-from-cluster branch January 14, 2025 08:43

		// Set the namespace for all manifests.
		manifests[i].body.SetNamespace(input.Namespace)


		running := filepath.Join("./", "testdata", "prune", "running")

		// read the running application config from the example file


		target := filepath.Join("./", "testdata", "prune", "target")

		// read the running application config from the example file

Implement resource pruning in k8s plugin #5455

Implement resource pruning in k8s plugin #5455

Conversation

Warashi commented Dec 26, 2024

codecov bot commented Dec 26, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo 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

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

ffjlabo commented Dec 27, 2024

Warashi commented Jan 6, 2025

ffjlabo commented Jan 7, 2025

Warashi commented Jan 7, 2025

ffjlabo commented Jan 7, 2025

ffjlabo left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 26, 2024 •

edited

Loading