feat(k8sd): Add datastore and nodeTaints to the GetClusterConfig response #1065

HomayoonAlimohammadi · 2025-02-12T18:17:10Z

🚫 Depends on canonical/k8s-snap-api#24 🚫

Overview

This PR adds datastore and nodeTaints to the GetClusterConfig response.

Rationale

k8s-operator needs to know the cluster configuration. That's because we need to prevent the user from changing the bootstrap- charm configs that are not supposed to be changed after the the cluster is bootstrapped.

tests/integration/tests/test_smoke.py

src/k8s/pkg/k8sd/database/cluster_config.go

eaudetcobello

Nice work, please review suggestions before merge

HomayoonAlimohammadi · 2025-02-13T10:34:44Z

Apparently it needs further adjustments. We need to store more data.

HomayoonAlimohammadi · 2025-02-13T10:45:11Z

~~Nevermind, looks like we have everything.~~
NVM we really do need to change somethings 🤕

HomayoonAlimohammadi · 2025-02-14T16:50:38Z

src/k8s/go.mod

@@ -7,7 +7,7 @@ toolchain go1.23.4
 require (
 	dario.cat/mergo v1.0.0
 	github.com/canonical/go-dqlite/v2 v2.0.0
-	github.com/canonical/k8s-snap-api v1.0.17
+	github.com/canonical/k8s-snap-api v1.0.18-0.20250214142145-772178e23c39


Needs to change whenever the api is bumped.

HomayoonAlimohammadi · 2025-02-14T16:51:12Z

src/k8s/pkg/k8sd/setup/kubelet.go

@@ -41,8 +41,8 @@ func KubeletControlPlane(snap snap.Snap, hostname string, nodeIP net.IP, cluster
 }

 // KubeletWorker configures kubelet on a worker node.
-func KubeletWorker(snap snap.Snap, hostname string, nodeIP net.IP, clusterDNS string, clusterDomain string, cloudProvider string, extraArgs map[string]*string) error {
-	return kubelet(snap, hostname, nodeIP, clusterDNS, clusterDomain, cloudProvider, nil, kubeletWorkerLabels, extraArgs)
+func KubeletWorker(snap snap.Snap, hostname string, nodeIP net.IP, clusterDNS string, clusterDomain string, cloudProvider string, extraArgs map[string]*string, taints []string) error {


Turns out that we were not passing the taints to Kubelet for worker nodes at all.

I guess we didn't have a test for this?

Well we did test the kubelet package, but the "taints" were not being tested specifically.

berkayoz

How do we want to handle the per-node bootstrap config? For worker nodes this is simpler since the db is per node already, what about the joining control plane nodes? Seems like we store a single key value for all?

HomayoonAlimohammadi · 2025-02-17T14:03:39Z

Interesting comment @berkayoz. Thanks for the attention to details. Sparked a conversation in MM.

HomayoonAlimohammadi · 2025-02-18T09:32:28Z

Needs further investigation. Converting to draft for now.

HomayoonAlimohammadi · 2025-02-20T08:59:25Z

tests/integration/tests/test_cluster_config.py

+    assert cp_resp["datastore"] == exp_cp_datastore, f"Mismatch in {cp_resp['datastore']} and {exp_cp_datastore=}"
+    assert cp_resp["nodeTaints"] == exp_cp_taints, f"Mismatch in {cp_resp['nodeTaints']} and {exp_cp_taints=}"
+    assert worker_resp["nodeTaints"] == exp_worker_taints, f"Mismatch in {worker_resp['nodeTaints']} and {exp_worker_taints=}"
+    assert cp_resp["status"]["network"] == exp_cp_config["network"], f"Mismatch in {cp_resp['status']['network']} and {exp_cp_config['network']=}"


The reason we can't simply check the cp_resp["status"] == exp_cp_config is that cp_resp["status"]["dns"]["service-ip"] changes each time.

HomayoonAlimohammadi · 2025-02-20T09:29:21Z

src/k8s/pkg/docgen/godoc.go

@@ -49,7 +50,11 @@ func getStructTypeFromDoc(packageDoc *doc.Package, structName string) (*ast.Stru

 func parsePackageDir(packageDir string) (*ast.Package, error) {
 	fset := token.NewFileSet()
-	packages, err := parser.ParseDir(fset, packageDir, nil, parser.ParseComments)
+	// NOTE(Hue): We only want to parse non-test files.


On my local machine, the make go.doc basically deleted all the generated docs due to multiple packages error. I don't know how this seems to be working on the CI. Had to make this change to be able to run locally. I think this issue was due to a very recent change in k8s-snap-api where we added a test file: https://github.com/canonical/k8s-snap-api/pull/26/files

berkayoz

LGTM overall, some small nits and let's rename the PR title (also conventional commit structure would be nice)

berkayoz · 2025-02-21T09:11:18Z

src/k8s/pkg/k8sd/api/endpoints.go

@@ -112,7 +112,7 @@ func (e *Endpoints) Endpoints() []rest.Endpoint {
 			Name: "ClusterConfig",
 			Path: apiv1.GetClusterConfigRPC, // == apiv1.SetClusterConfigRPC
 			Put:  rest.EndpointAction{Handler: e.putClusterConfig, AccessHandler: e.restrictWorkers},
-			Get:  rest.EndpointAction{Handler: e.getClusterConfig, AccessHandler: e.restrictWorkers},
+			Get:  rest.EndpointAction{Handler: e.getClusterConfig},


How does this affect k8s get? Do we still see this command is not available on workers message?

That's a really good catch. I'll add an isWorker check.

Added this check in k8s_get.go and k8s_x_snapd_config.go.

berkayoz · 2025-02-21T09:12:53Z

src/k8s/pkg/k8sd/app/hooks_bootstrap.go

+	if taintsStr, ok := joinConfig.ExtraNodeKubeletArgs["--register-with-taints"]; ok {
+		if taintsStr != nil {


Suggested change

if taintsStr, ok := joinConfig.ExtraNodeKubeletArgs["--register-with-taints"]; ok {

if taintsStr != nil {

if taintsStr, ok := joinConfig.ExtraNodeKubeletArgs["--register-with-taints"]; ok && taintsStr != nil {

berkayoz · 2025-02-21T09:15:55Z

src/k8s/pkg/k8sd/types/cluster_config_merge.go

@@ -78,6 +78,7 @@ func MergeClusterConfig(existing ClusterConfig, new ClusterConfig) (ClusterConfi
 		{name: "load balancer CIDRs", val: &config.LoadBalancer.CIDRs, old: existing.LoadBalancer.CIDRs, new: new.LoadBalancer.CIDRs, allowChange: true},
 		{name: "load balancer L2 interfaces", val: &config.LoadBalancer.L2Interfaces, old: existing.LoadBalancer.L2Interfaces, new: new.LoadBalancer.L2Interfaces, allowChange: true},
 		{name: "control-plane register with taints", val: &config.Kubelet.ControlPlaneTaints, old: existing.Kubelet.ControlPlaneTaints, new: new.Kubelet.ControlPlaneTaints, allowChange: false},
+		{name: "worker kubelet register with taints", val: &config.Kubelet.WorkerTaints, old: existing.Kubelet.WorkerTaints, new: new.Kubelet.WorkerTaints, allowChange: false},


Suggested change

{name: "worker kubelet register with taints", val: &config.Kubelet.WorkerTaints, old: existing.Kubelet.WorkerTaints, new: new.Kubelet.WorkerTaints, allowChange: false},

{name: "worker register with taints", val: &config.Kubelet.WorkerTaints, old: existing.Kubelet.WorkerTaints, new: new.Kubelet.WorkerTaints, allowChange: false},

HomayoonAlimohammadi requested a review from a team as a code owner February 12, 2025 18:17

eaudetcobello reviewed Feb 12, 2025

View reviewed changes

tests/integration/tests/test_smoke.py Outdated Show resolved Hide resolved

eaudetcobello reviewed Feb 12, 2025

View reviewed changes

src/k8s/pkg/k8sd/database/cluster_config.go Outdated Show resolved Hide resolved

eaudetcobello approved these changes Feb 12, 2025

View reviewed changes

HomayoonAlimohammadi marked this pull request as draft February 13, 2025 10:34

HomayoonAlimohammadi marked this pull request as ready for review February 13, 2025 10:45

HomayoonAlimohammadi marked this pull request as draft February 13, 2025 10:48

HomayoonAlimohammadi force-pushed the KU-2664/k8sd-get-bootstrap-config-endpoint branch 2 times, most recently from ed1d7f7 to ba06cac Compare February 14, 2025 16:20

HomayoonAlimohammadi marked this pull request as ready for review February 14, 2025 16:21

HomayoonAlimohammadi commented Feb 14, 2025

View reviewed changes

berkayoz reviewed Feb 17, 2025

View reviewed changes

HomayoonAlimohammadi marked this pull request as draft February 18, 2025 09:32

HomayoonAlimohammadi force-pushed the KU-2664/k8sd-get-bootstrap-config-endpoint branch from 44d55b9 to b375077 Compare February 20, 2025 08:55

HomayoonAlimohammadi commented Feb 20, 2025

View reviewed changes

HomayoonAlimohammadi added 2 commits February 20, 2025 13:26

k8sd: Improve the GetClusterConfig rpc

43d3537

docs: Exclude test files from dockgen, make go.doc

69b9ee6

HomayoonAlimohammadi force-pushed the KU-2664/k8sd-get-bootstrap-config-endpoint branch from f31c8de to a14fe1e Compare February 20, 2025 09:27

HomayoonAlimohammadi commented Feb 20, 2025

View reviewed changes

HomayoonAlimohammadi force-pushed the KU-2664/k8sd-get-bootstrap-config-endpoint branch from a14fe1e to 47586fb Compare February 20, 2025 09:34

fix lint and test

55bd546

HomayoonAlimohammadi force-pushed the KU-2664/k8sd-get-bootstrap-config-endpoint branch from 47586fb to 55bd546 Compare February 20, 2025 09:44

HomayoonAlimohammadi marked this pull request as ready for review February 20, 2025 09:52

berkayoz reviewed Feb 21, 2025

View reviewed changes

HomayoonAlimohammadi changed the title ~~k8sd: Add logic to set and get cluster bootstrap config~~ feat(k8sd): Add datastore and nodeTaints to the GetClusterConfig response Feb 21, 2025

HomayoonAlimohammadi added 2 commits February 21, 2025 18:11

fix: Address comments

0132f84

fix: Update nodeTaints string to node-taints per PR feedback

2203a96

HomayoonAlimohammadi marked this pull request as draft February 25, 2025 13:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(k8sd): Add datastore and nodeTaints to the GetClusterConfig response #1065

feat(k8sd): Add datastore and nodeTaints to the GetClusterConfig response #1065

HomayoonAlimohammadi commented Feb 12, 2025 •

edited

Loading

eaudetcobello left a comment

HomayoonAlimohammadi commented Feb 13, 2025

HomayoonAlimohammadi commented Feb 13, 2025 •

edited

Loading

HomayoonAlimohammadi Feb 14, 2025

HomayoonAlimohammadi Feb 14, 2025

eaudetcobello Feb 14, 2025

HomayoonAlimohammadi Feb 20, 2025

berkayoz left a comment

HomayoonAlimohammadi commented Feb 17, 2025

HomayoonAlimohammadi commented Feb 18, 2025

HomayoonAlimohammadi Feb 20, 2025

HomayoonAlimohammadi Feb 20, 2025

berkayoz left a comment

berkayoz Feb 21, 2025

HomayoonAlimohammadi Feb 21, 2025 •

edited

Loading

HomayoonAlimohammadi Feb 21, 2025

berkayoz Feb 21, 2025

berkayoz Feb 21, 2025

		if taintsStr, ok := joinConfig.ExtraNodeKubeletArgs["--register-with-taints"]; ok {
		if taintsStr != nil {

	{name: "worker kubelet register with taints", val: &config.Kubelet.WorkerTaints, old: existing.Kubelet.WorkerTaints, new: new.Kubelet.WorkerTaints, allowChange: false},
	{name: "worker register with taints", val: &config.Kubelet.WorkerTaints, old: existing.Kubelet.WorkerTaints, new: new.Kubelet.WorkerTaints, allowChange: false},

feat(k8sd): Add datastore and nodeTaints to the GetClusterConfig response #1065

Are you sure you want to change the base?

feat(k8sd): Add datastore and nodeTaints to the GetClusterConfig response #1065

Conversation

HomayoonAlimohammadi commented Feb 12, 2025 • edited Loading

🚫 Depends on canonical/k8s-snap-api#24 🚫

Overview

Rationale

eaudetcobello left a comment

Choose a reason for hiding this comment

HomayoonAlimohammadi commented Feb 13, 2025

HomayoonAlimohammadi commented Feb 13, 2025 • edited Loading

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

berkayoz left a comment

Choose a reason for hiding this comment

HomayoonAlimohammadi commented Feb 17, 2025

HomayoonAlimohammadi commented Feb 18, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berkayoz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HomayoonAlimohammadi Feb 21, 2025 • edited Loading

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

HomayoonAlimohammadi commented Feb 12, 2025 •

edited

Loading

HomayoonAlimohammadi commented Feb 13, 2025 •

edited

Loading

HomayoonAlimohammadi Feb 21, 2025 •

edited

Loading