diff --git a/docs/design.md b/docs/design.md index da72834b..1de9970a 100644 --- a/docs/design.md +++ b/docs/design.md @@ -152,7 +152,7 @@ See [AKS RP And Flex Node Agent Interaction](design/agent-and-aks.md) for the de ## State And Idempotency -The agent persists local daemon state so it can recover after restart, reboot, or partial failure. Persisted state includes the applied Kubernetes/settings version and active nspawn machine side. +The agent persists local daemon state so it can recover after restart, reboot, or partial failure. Persisted state includes the current and previous applied Machine goals and the active nspawn machine side. The current state model separates desired state, applied state, and runtime discovery: diff --git a/docs/design/agent-and-aks.md b/docs/design/agent-and-aks.md index f1119ff3..1d44c1de 100644 --- a/docs/design/agent-and-aks.md +++ b/docs/design/agent-and-aks.md @@ -90,7 +90,7 @@ ARM machine creation must be idempotent. If AKS RP retries creation and the ARM The Flex Node agent reconciles two external signals. -The ARM machine resource provides desired settings and a version for those settings. The current minimal settings are desired Kubernetes version and settings version. The agent compares the settings version from ARM with its locally applied settings version to detect drift. Future schema extensions can add more settings, but the agent should treat the ARM machine resource as the source of truth for host/nspawn reconciliation. +The ARM machine resource provides desired Kubernetes version, max pods, custom labels and taints, kubelet image-GC settings, and a version for those settings. The agent compares the settings version from ARM with its locally applied settings version to detect drift. Future schema extensions can add more settings, but the agent should treat the ARM machine resource as the source of truth for host/nspawn reconciliation. The ARM machine resource does not own the nspawn side. Selecting `kube1` or `kube2` is an internal host implementation detail used by the agent to apply settings atomically. @@ -130,14 +130,14 @@ flowchart TD ## Current Repave Implementation -AKS Flex Node no longer runs a standalone local drift detector. Desired node settings come from an AKS machine resource. The agent compares the desired machine goal with locally persisted daemon state and repaves the nspawn-backed worker when Kubernetes `Node` deletion indicates AKS has approved replacement. +AKS Flex Node no longer runs a standalone local drift detector. Desired node settings come from an AKS machine resource. The agent compares the desired machine goal with locally persisted daemon state. It acknowledges label- and taint-only updates that AKS RP already reconciled onto the Kubernetes `Node`; other changes repave the nspawn-backed worker after `Node` deletion indicates AKS has approved replacement. The current machine goal comes from the ARM machine model: - `properties.kubernetes` contains the desired Kubernetes version and node settings. - `properties.eTag` is exposed internally as the settings version. -The ETag is the drift key. If it differs from the locally applied ETag, the agent waits for the Kubernetes `Node` object to disappear before mutating host state. Status-only updates must not change the ETag. +The ETag is the drift key. If it differs from the locally applied ETag, the agent first checks whether only labels or taints changed and whether the existing Kubernetes `Node` reflects that complete delta. A matching Node lets the agent persist the new goal without host mutation. Otherwise, it waits for the `Node` object to disappear before mutating host state. Status-only updates must not change the ETag. The daemon uses two inputs: @@ -170,7 +170,7 @@ This keeps scheduling and disruption decisions outside the agent. AKS RP, an ope 9. Start node-problem-detector inside the new side. 10. Clean up the old side's nspawn artifacts. -After successful repave, the daemon patches machine status and persists the applied goal locally. +After successful repave or in-place acknowledgement, the daemon persists the applied goal locally and reports status through the selected machine client. Direct ARM Machine status is currently read-only, so that client skips the status mutation while retaining local convergence. AKS Flex Node uses two local nspawn machine names: diff --git a/docs/design/in-cluster-machine.md b/docs/design/in-cluster-machine.md index 3d46a2ac..ca89456c 100644 --- a/docs/design/in-cluster-machine.md +++ b/docs/design/in-cluster-machine.md @@ -15,7 +15,7 @@ The controller serves the `armcontainerservice.Machine` JSON shape from the `kub "orchestratorVersion": "1.34.0", "maxPods": 110, "nodeLabels": { - "kubernetes.azure.com/managed": "false" + "workload": "edge" } }, "provisioningState": "Succeeded" @@ -27,18 +27,18 @@ Status updates use a separate patch model because the agent operation status is ## Bootstrap flow -The local bootstrap configuration is authoritative while `aks-flex-node start` is running: +The local bootstrap configuration seeds a Machine when one does not already exist. Once the endpoint returns a Machine, its complete goal is authoritative for bootstrap: 1. `NewMachineClient` selects the in-cluster backend without a supplied Kubernetes REST config. 2. The client builds a REST config from the bootstrap token or configured exec credential. 3. `EnsureMachine` reads the machine through the Kubernetes service proxy. 4. If the machine is absent, the client sends a PUT using the local bootstrap goal. -5. If its Kubernetes version differs, the client sends a PUT that overwrites the remote goal with the local version. -6. If its Kubernetes version already matches, local bootstrap settings remain authoritative; remote settings other than the ETag do not replace them. -7. The returned ETag becomes the reconciliation baseline for the locally applied goal. -8. The daemon state is seeded from that ETag before host or nspawn state is mutated. A later ETag change is treated as a new remote goal. +5. Whether read or created, the returned Machine is validated and its goal replaces the local bootstrap goal. This includes Kubernetes version, max pods, custom labels, taints, kubelet image-GC thresholds, and the ETag-backed settings version. Scalar defaults omitted by the API retain their validated local bootstrap values. +6. The daemon resolves nspawn settings and seeds its state from that same effective goal before mutating the host. A later ETag change is treated as a new remote goal. -The ConfigMap-backed controller is read-only: it accepts mutation requests but returns the pre-created machine. Its fixture must therefore already match the local bootstrap version. When machine registration is required, a mismatch fails bootstrap before host mutation. +When `orchestratorVersion` is a `major.minor` alias, the returned `currentOrchestratorVersion` supplies the exact patch used for artifact resolution. + +The ConfigMap-backed controller is read-only: it accepts mutation requests but returns the pre-created Machine. The agent adopts that returned goal even when it differs from local bootstrap configuration. When registration is required, a read, create, or validation failure stops bootstrap before host mutation. When registration is optional, bootstrap continues with the local goal. ## Daemon flow @@ -48,8 +48,11 @@ After bootstrap, the remote machine is authoritative: 2. `NewMachineClient` receives that REST config and selects the in-cluster backend. 3. The client periodically reads the ARM-compatible machine through the service-proxy endpoint. 4. The daemon compares `properties.eTag` with its locally applied settings version. -5. A changed ETag represents a new goal. The daemon waits for the Kubernetes `Node` deletion signal before applying it. -6. Reconciliation status is sent to the endpoint's `/status` subresource without changing the ETag. +5. A changed ETag represents a new goal. If only labels or taints changed and AKS RP already reconciled them onto the existing Kubernetes `Node`, the daemon acknowledges the observed goal without mutating or repaving the node. +6. Other goal changes wait for the Kubernetes `Node` deletion signal before the daemon applies them through blue-green repave. +7. Reconciliation status is sent to the endpoint's `/status` subresource without changing the ETag. + +Direct ARM Machine status is currently read-only. In that mode, acknowledgement still advances the local applied goal and ETag, while the status mutation is skipped by the ARM client. ## Request path diff --git a/hack/demo/aks-flex-node-upgrade.sh b/hack/demo/aks-flex-node-upgrade.sh index b85073c2..66b9a163 100755 --- a/hack/demo/aks-flex-node-upgrade.sh +++ b/hack/demo/aks-flex-node-upgrade.sh @@ -131,7 +131,7 @@ update_machine_goal() { .properties.eTag = $settings | .properties.kubernetes = (.properties.kubernetes // {}) | .properties.kubernetes.orchestratorVersion = $version | - .properties.kubernetes.nodeLabels = (.properties.kubernetes.nodeLabels // {"kubernetes.azure.com/managed":"false"}) + .properties.kubernetes.nodeLabels = (.properties.kubernetes.nodeLabels // {}) ' <<<"${current_json}" > "${tmp}" if [[ -z "${cm_json}" ]]; then diff --git a/hack/e2e/lib/controller.sh b/hack/e2e/lib/controller.sh index 67175fb1..39af3ce8 100644 --- a/hack/e2e/lib/controller.sh +++ b/hack/e2e/lib/controller.sh @@ -332,7 +332,7 @@ ensure_flex_controller() { } _render_machine_json() { - local node_name="$1" kubernetes_version="$2" settings_version="$3" + local node_name="$1" kubernetes_version="$2" settings_version="$3" max_pods="$4" local cluster_id machine_id cluster_id="$(state_get cluster_id)" machine_id="${cluster_id}/agentPools/${E2E_TARGET_AGENT_POOL_NAME}/machines/${node_name}" @@ -342,6 +342,7 @@ _render_machine_json() { --arg name "${node_name}" \ --arg kubernetesVersion "${kubernetes_version}" \ --arg eTag "${settings_version}" \ + --argjson maxPods "${max_pods}" \ '{ id: $id, name: $name, @@ -351,7 +352,7 @@ _render_machine_json() { provisioningState: "Succeeded", kubernetes: { orchestratorVersion: $kubernetesVersion, - maxPods: 110, + maxPods: $maxPods, nodeLabels: {}, nodeTaints: [], kubeletConfig: { @@ -364,11 +365,11 @@ _render_machine_json() { } _machine_configmap_upsert_unlocked() { - local node_name="$1" kubernetes_version="$2" settings_version="$3" + local node_name="$1" kubernetes_version="$2" settings_version="$3" max_pods="$4" local machine_file patch machine_file="${E2E_WORK_DIR}/machine-${node_name}.json" - _render_machine_json "${node_name}" "${kubernetes_version}" "${settings_version}" > "${machine_file}" + _render_machine_json "${node_name}" "${kubernetes_version}" "${settings_version}" "${max_pods}" > "${machine_file}" if ! kubectl -n "${E2E_CONTROLLER_NAMESPACE}" get configmap "${E2E_MACHINE_CONFIGMAP}" >/dev/null 2>&1; then kubectl -n "${E2E_CONTROLLER_NAMESPACE}" create configmap "${E2E_MACHINE_CONFIGMAP}" >/dev/null fi @@ -379,8 +380,8 @@ _machine_configmap_upsert_unlocked() { } machine_configmap_upsert() { - local node_name="$1" kubernetes_version="${2:-${E2E_KUBERNETES_VERSION}}" settings_version="${3:-${kubernetes_version}}" - with_cluster_lock _machine_configmap_upsert_unlocked "${node_name}" "${kubernetes_version}" "${settings_version}" + local node_name="$1" kubernetes_version="${2:-${E2E_KUBERNETES_VERSION}}" settings_version="${3:-${kubernetes_version}}" max_pods="${4:-110}" + with_cluster_lock _machine_configmap_upsert_unlocked "${node_name}" "${kubernetes_version}" "${settings_version}" "${max_pods}" } _machine_configmap_delete_unlocked() { diff --git a/hack/e2e/lib/node-join-token.sh b/hack/e2e/lib/node-join-token.sh index 5414d2e1..a35ddd15 100644 --- a/hack/e2e/lib/node-join-token.sh +++ b/hack/e2e/lib/node-join-token.sh @@ -90,7 +90,7 @@ node_join_token() { mv "${config_file}.tmp" "${config_file}" # Step 3: Publish the AKS Machine goal and deploy the agent. - machine_configmap_upsert "$(state_get token_vm_name)" "${E2E_KUBERNETES_VERSION}" "${E2E_KUBERNETES_VERSION}" + machine_configmap_upsert "$(state_get token_vm_name)" "${E2E_KUBERNETES_VERSION}" "${E2E_KUBERNETES_VERSION}" "${E2E_KUBELET_MAX_PODS}" _deploy_and_start_agent "${vm_ip}" "${config_file}" "aks-flex-node-token" log_success "Token node joined in $(timer_elapsed "${start}")s" diff --git a/hack/e2e/lib/upgrade-drift.sh b/hack/e2e/lib/upgrade-drift.sh index dd7686ab..c7727eea 100644 --- a/hack/e2e/lib/upgrade-drift.sh +++ b/hack/e2e/lib/upgrade-drift.sh @@ -102,12 +102,16 @@ _ensure_mode_joined() { _trigger_mode_repave() { local mode="$1" desired_version="$2" settings_version="$3" - local vm_ip vm_name + local vm_ip vm_name max_pods vm_ip="$(_mode_vm_ip "${mode}")" vm_name="$(_mode_vm_name "${mode}")" + max_pods="110" + if [[ "${mode}" == "token" ]]; then + max_pods="${E2E_KUBELET_MAX_PODS}" + fi log_info "Updating controller machine goal for ${mode} node to Kubernetes ${desired_version} (${settings_version})" - machine_configmap_upsert "${vm_name}" "${desired_version}" "${settings_version}" + machine_configmap_upsert "${vm_name}" "${desired_version}" "${settings_version}" "${max_pods}" remote_exec "${vm_ip}" 'sudo systemctl status aks-flex-node-agent.service --no-pager -l || true' log_info "Deleting Kubernetes Node ${vm_name} to trigger ${mode} repave" diff --git a/pkg/aksmachine/client_armapi.go b/pkg/aksmachine/client_armapi.go index f6c2327b..67a5d8c9 100644 --- a/pkg/aksmachine/client_armapi.go +++ b/pkg/aksmachine/client_armapi.go @@ -47,7 +47,7 @@ func newARMClient(cfg *config.Config, logger *slog.Logger) (MachineClient, error } func (c *armMachineClient) Create(ctx context.Context, desired GoalState) (*Machine, error) { - if err := desired.validate(); err != nil { + if err := desired.Validate(); err != nil { return nil, fmt.Errorf("validate goal state: %w", err) } params := armcontainerservice.Machine{ @@ -214,10 +214,14 @@ func clientCertificateCredentialOptions(clientOpts azcore.ClientOptions) *aziden func buildK8sProfile(goal GoalState) *armcontainerservice.MachineKubernetesProfile { // FlexNode RP accepts the registration surface below; local kubelet defaults // are consumed during node bootstrap and must not be sent as Machine fields. - maxPods := int32(goal.MaxPods) //nolint:gosec // validated non-negative and small + var maxPods *int32 + if goal.MaxPods != nil { + value := int32(*goal.MaxPods) //nolint:gosec // validated non-negative and small + maxPods = &value + } p := &armcontainerservice.MachineKubernetesProfile{ OrchestratorVersion: &goal.KubernetesVersion, - MaxPods: &maxPods, + MaxPods: maxPods, NodeLabels: stringPointerMap(goal.NodeLabels), NodeTaints: stringPointerSlice(goal.NodeTaints), } @@ -275,9 +279,16 @@ func machineFromARM(machine armcontainerservice.Machine, defaultID, defaultName kubernetes := properties.Kubernetes if kubernetes.OrchestratorVersion != nil { result.Goal.KubernetesVersion = *kubernetes.OrchestratorVersion + if kubernetes.CurrentOrchestratorVersion != nil { + result.Goal.KubernetesVersion = resolveKubernetesVersionAlias( + result.Goal.KubernetesVersion, + *kubernetes.CurrentOrchestratorVersion, + ) + } } if kubernetes.MaxPods != nil { - result.Goal.MaxPods = int(*kubernetes.MaxPods) + value := int(*kubernetes.MaxPods) + result.Goal.MaxPods = &value } if kubernetes.NodeLabels != nil { result.Goal.NodeLabels = stringMapFromPointers(kubernetes.NodeLabels) @@ -287,10 +298,12 @@ func machineFromARM(machine armcontainerservice.Machine, defaultID, defaultName } if kubernetes.KubeletConfig != nil { if kubernetes.KubeletConfig.ImageGcHighThreshold != nil { - result.Goal.KubeletConfig.ImageGCHighThreshold = int(*kubernetes.KubeletConfig.ImageGcHighThreshold) + value := int(*kubernetes.KubeletConfig.ImageGcHighThreshold) + result.Goal.KubeletConfig.ImageGCHighThreshold = &value } if kubernetes.KubeletConfig.ImageGcLowThreshold != nil { - result.Goal.KubeletConfig.ImageGCLowThreshold = int(*kubernetes.KubeletConfig.ImageGcLowThreshold) + value := int(*kubernetes.KubeletConfig.ImageGcLowThreshold) + result.Goal.KubeletConfig.ImageGCLowThreshold = &value } } } @@ -303,6 +316,15 @@ func machineFromARM(machine armcontainerservice.Machine, defaultID, defaultName return result } +func resolveKubernetesVersionAlias(desired, current string) string { + desiredVersion := strings.TrimPrefix(strings.TrimSpace(desired), "v") + currentVersion := strings.TrimPrefix(strings.TrimSpace(current), "v") + if len(strings.Split(desiredVersion, ".")) == 2 && strings.HasPrefix(currentVersion, desiredVersion+".") { + return currentVersion + } + return desiredVersion +} + func stringMapFromPointers(values map[string]*string) map[string]string { result := make(map[string]string, len(values)) for key, value := range values { diff --git a/pkg/aksmachine/client_armapi_test.go b/pkg/aksmachine/client_armapi_test.go index 0644171d..5e81f7cb 100644 --- a/pkg/aksmachine/client_armapi_test.go +++ b/pkg/aksmachine/client_armapi_test.go @@ -205,12 +205,12 @@ func TestBuildK8sProfile(t *testing.T) { profile := buildK8sProfile(GoalState{ KubernetesVersion: "1.35.1", - MaxPods: 42, + MaxPods: ptr(42), NodeLabels: map[string]string{"workload": "flex"}, NodeTaints: []string{"dedicated=flex:NoSchedule"}, KubeletConfig: KubeletConfig{ - ImageGCHighThreshold: 85, - ImageGCLowThreshold: 80, + ImageGCHighThreshold: ptr(85), + ImageGCLowThreshold: ptr(80), }, }) if profile.OrchestratorVersion == nil || *profile.OrchestratorVersion != "1.35.1" { @@ -240,7 +240,7 @@ func TestGoalStateValidate(t *testing.T) { }{ { name: "valid", - goal: GoalState{KubernetesVersion: "1.35.1"}, + goal: testGoal("1.35.1", ""), }, { name: "missing Kubernetes version", @@ -248,21 +248,42 @@ func TestGoalStateValidate(t *testing.T) { wantErr: "kubernetes version is empty", }, { - name: "negative max pods", - goal: GoalState{KubernetesVersion: "1.35.1", MaxPods: -1}, + name: "missing max pods", + goal: GoalState{KubernetesVersion: "1.35.1"}, + wantErr: "max pods is empty", + }, + { + name: "negative max pods", + goal: GoalState{ + KubernetesVersion: "1.35.1", + MaxPods: ptr(-1), + KubeletConfig: KubeletConfig{ + ImageGCHighThreshold: ptr(85), + ImageGCLowThreshold: ptr(80), + }, + }, wantErr: "max pods must be non-negative", }, { - name: "max pods exceeds int32", - goal: GoalState{KubernetesVersion: "1.35.1", MaxPods: math.MaxInt32 + 1}, + name: "max pods exceeds int32", + goal: GoalState{ + KubernetesVersion: "1.35.1", + MaxPods: ptr(math.MaxInt32 + 1), + KubeletConfig: KubeletConfig{ + ImageGCHighThreshold: ptr(85), + ImageGCLowThreshold: ptr(80), + }, + }, wantErr: "max pods must be less than or equal to", }, { name: "negative image GC high threshold", goal: GoalState{ KubernetesVersion: "1.35.1", + MaxPods: ptr(110), KubeletConfig: KubeletConfig{ - ImageGCHighThreshold: -1, + ImageGCHighThreshold: ptr(-1), + ImageGCLowThreshold: ptr(80), }, }, wantErr: "image GC high threshold must be non-negative", @@ -271,8 +292,10 @@ func TestGoalStateValidate(t *testing.T) { name: "negative image GC low threshold", goal: GoalState{ KubernetesVersion: "1.35.1", + MaxPods: ptr(110), KubeletConfig: KubeletConfig{ - ImageGCLowThreshold: -1, + ImageGCHighThreshold: ptr(85), + ImageGCLowThreshold: ptr(-1), }, }, wantErr: "image GC low threshold must be non-negative", @@ -283,7 +306,7 @@ func TestGoalStateValidate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - err := tt.goal.validate() + err := tt.goal.Validate() if tt.wantErr != "" { if err == nil || !strings.Contains(err.Error(), tt.wantErr) { t.Fatalf("validate() error = %v, want containing %q", err, tt.wantErr) @@ -325,13 +348,14 @@ func TestMachineFromARM(t *testing.T) { if machine.Goal.KubernetesVersion != "1.35.1" || machine.Goal.SettingsVersion != "settings-42" { t.Fatalf("goal versions = %#v", machine.Goal) } - if machine.Goal.MaxPods != 42 || machine.Goal.NodeLabels["workload"] != "flex" { + if machine.Goal.MaxPods == nil || *machine.Goal.MaxPods != 42 || machine.Goal.NodeLabels["workload"] != "flex" { t.Fatalf("goal settings = %#v", machine.Goal) } if len(machine.Goal.NodeTaints) != 1 || machine.Goal.NodeTaints[0] != "dedicated=flex:NoSchedule" { t.Fatalf("goal taints = %#v", machine.Goal.NodeTaints) } - if machine.Goal.KubeletConfig.ImageGCHighThreshold != 85 || machine.Goal.KubeletConfig.ImageGCLowThreshold != 80 { + if machine.Goal.KubeletConfig.ImageGCHighThreshold == nil || *machine.Goal.KubeletConfig.ImageGCHighThreshold != 85 || + machine.Goal.KubeletConfig.ImageGCLowThreshold == nil || *machine.Goal.KubeletConfig.ImageGCLowThreshold != 80 { t.Fatalf("kubelet config = %#v", machine.Goal.KubeletConfig) } if machine.Status.ProvisioningState != ProvisioningStateSucceeded { @@ -415,6 +439,24 @@ func TestMachineFromARMDoesNotUseCurrentOrchestratorVersionAsGoal(t *testing.T) } } +func TestMachineFromARMResolvesMinorVersionAlias(t *testing.T) { + t.Parallel() + + machine := machineFromARM(armcontainerservice.Machine{ + Properties: &armcontainerservice.MachineProperties{ + ETag: ptr("42"), + Kubernetes: &armcontainerservice.MachineKubernetesProfile{ + OrchestratorVersion: ptr(" v1.35 "), + CurrentOrchestratorVersion: ptr(" v1.35.2 "), + }, + }, + }, "", "") + + if machine.Goal.KubernetesVersion != "1.35.2" { + t.Fatalf("KubernetesVersion = %q, want resolved patch 1.35.2", machine.Goal.KubernetesVersion) + } +} + func TestMachineFromARMDoesNotSynthesizeSettingsVersion(t *testing.T) { t.Parallel() diff --git a/pkg/aksmachine/client_incluster.go b/pkg/aksmachine/client_incluster.go index 3b518a7a..42fddbc0 100644 --- a/pkg/aksmachine/client_incluster.go +++ b/pkg/aksmachine/client_incluster.go @@ -105,6 +105,9 @@ func clusterEndpointBaseURL(restCfg *rest.Config, endpointURL string) (*url.URL, } func (c *clusterEndpointClient) Create(ctx context.Context, desired GoalState) (*Machine, error) { + if err := desired.Validate(); err != nil { + return nil, fmt.Errorf("validate goal state: %w", err) + } requestURL := c.machineURL(c.nodeName) payload := armcontainerservice.Machine{ Properties: &armcontainerservice.MachineProperties{ @@ -130,7 +133,7 @@ func (c *clusterEndpointClient) Create(ctx context.Context, desired GoalState) ( if resp.StatusCode == http.StatusForbidden || resp.StatusCode == http.StatusMethodNotAllowed || resp.StatusCode == http.StatusNoContent { c.logger.Debug("cluster endpoint did not apply machine create request; verifying pre-created machine", "status", resp.Status) - return c.adoptExistingMachine(ctx, desired) + return c.adoptExistingMachine(ctx) } if resp.StatusCode < 200 || resp.StatusCode > 299 { return nil, clusterEndpointHTTPError("create machine through cluster endpoint", requestURL, resp) @@ -140,7 +143,7 @@ func (c *clusterEndpointClient) Create(ctx context.Context, desired GoalState) ( return nil, fmt.Errorf("read cluster endpoint machine create response: %w", err) } if strings.TrimSpace(string(data)) == "" { - return c.adoptExistingMachine(ctx, desired) + return c.adoptExistingMachine(ctx) } machine, err := machineFromEndpointJSON(data) if err != nil { @@ -152,33 +155,17 @@ func (c *clusterEndpointClient) Create(ctx context.Context, desired GoalState) ( if machine.Name == "" { machine.Name = c.nodeName } - if err := validateAdoptedMachine(machine, desired); err != nil { - return nil, err - } return machine, nil } -func (c *clusterEndpointClient) adoptExistingMachine(ctx context.Context, desired GoalState) (*Machine, error) { +func (c *clusterEndpointClient) adoptExistingMachine(ctx context.Context) (*Machine, error) { machine, err := c.Get(ctx) if err != nil { return nil, fmt.Errorf("verify pre-created machine from cluster endpoint: %w", err) } - if err := validateAdoptedMachine(machine, desired); err != nil { - return nil, err - } return machine, nil } -func validateAdoptedMachine(machine *Machine, desired GoalState) error { - if err := machine.Validate(); err != nil { - return fmt.Errorf("cluster endpoint returned invalid machine: %w", err) - } - if desired.KubernetesVersion != "" && machine.Goal.KubernetesVersion != "" && machine.Goal.KubernetesVersion != desired.KubernetesVersion { - return fmt.Errorf("pre-created machine Kubernetes version %q does not match desired %q", machine.Goal.KubernetesVersion, desired.KubernetesVersion) - } - return nil -} - func (c *clusterEndpointClient) Get(ctx context.Context) (*Machine, error) { requestURL := c.machineURL(c.nodeName) req, err := http.NewRequestWithContext(ctx, http.MethodGet, requestURL.String(), nil) diff --git a/pkg/aksmachine/client_incluster_test.go b/pkg/aksmachine/client_incluster_test.go index 77df721d..f1244f53 100644 --- a/pkg/aksmachine/client_incluster_test.go +++ b/pkg/aksmachine/client_incluster_test.go @@ -54,7 +54,7 @@ func TestClusterEndpointClientGet(t *testing.T) { if machine.Goal.KubernetesVersion != "1.34.0" || machine.Goal.SettingsVersion != "42" { t.Fatalf("goal = %#v", machine.Goal) } - if machine.Goal.MaxPods != 42 || machine.Goal.NodeLabels["workload"] != "flex" || len(machine.Goal.NodeTaints) != 1 { + if machine.Goal.MaxPods == nil || *machine.Goal.MaxPods != 42 || machine.Goal.NodeLabels["workload"] != "flex" || len(machine.Goal.NodeTaints) != 1 { t.Fatalf("extended goal = %#v", machine.Goal) } if machine.Status.ProvisioningState != ProvisioningStateSucceeded { @@ -70,7 +70,11 @@ func TestMachineFromEndpointJSONUsesARMModel(t *testing.T) { "name": "node1", "properties": { "eTag": "42", - "kubernetes": {"orchestratorVersion": "1.34.0"}, + "kubernetes": { + "orchestratorVersion": "1.34.0", + "maxPods": 110, + "kubeletConfig": {"imageGcHighThreshold": 85, "imageGcLowThreshold": 80} + }, "provisioningState": "Succeeded" } }`)) @@ -93,7 +97,11 @@ func TestMachineFromEndpointJSONRejectsMissingETag(t *testing.T) { _, err := machineFromEndpointJSON([]byte(`{ "properties": { - "kubernetes": {"orchestratorVersion": "1.34.0"} + "kubernetes": { + "orchestratorVersion": "1.34.0", + "maxPods": 110, + "kubeletConfig": {"imageGcHighThreshold": 85, "imageGcLowThreshold": 80} + } } }`)) if err == nil || !strings.Contains(err.Error(), "goal settings version is empty") { @@ -137,29 +145,32 @@ func TestClusterEndpointCreateSendsMutation(t *testing.T) { return } w.Header().Set("Content-Type", "application/json") - _, _ = fmt.Fprint(w, `{"properties":{"eTag":"42","kubernetes":{"orchestratorVersion":"1.34.0"}}}`) + _, _ = fmt.Fprint(w, `{"properties":{"eTag":"42","kubernetes":{"orchestratorVersion":"1.34.0","maxPods":110,"kubeletConfig":{"imageGcHighThreshold":85,"imageGcLowThreshold":80}}}}`) })) defer server.Close() client := newTestClusterEndpointClient(t, server.URL, "node1") - if _, err := client.Create(context.Background(), GoalState{KubernetesVersion: "1.34.0", SettingsVersion: "42"}); err != nil { + if _, err := client.Create(context.Background(), testGoal("1.34.0", "42")); err != nil { t.Fatalf("Create() error = %v", err) } } -func TestClusterEndpointCreateVerifiesPrecreatedMachine(t *testing.T) { +func TestClusterEndpointCreateAdoptsPrecreatedMachine(t *testing.T) { t.Parallel() server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { w.Header().Set("Content-Type", "application/json") - _, _ = fmt.Fprint(w, `{"properties":{"eTag":"42","kubernetes":{"orchestratorVersion":"1.34.0"}}}`) + _, _ = fmt.Fprint(w, `{"properties":{"eTag":"42","kubernetes":{"orchestratorVersion":"1.34.0","maxPods":110,"kubeletConfig":{"imageGcHighThreshold":85,"imageGcLowThreshold":80}}}}`) })) defer server.Close() client := newTestClusterEndpointClient(t, server.URL, "node1") - _, err := client.Create(context.Background(), GoalState{KubernetesVersion: "1.35.0", SettingsVersion: "42"}) - if err == nil || !strings.Contains(err.Error(), "Kubernetes version") { - t.Fatalf("Create() error = %v, want version mismatch", err) + machine, err := client.Create(context.Background(), testGoal("1.35.0", "local")) + if err != nil { + t.Fatalf("Create() error = %v", err) + } + if machine.Goal.KubernetesVersion != "1.34.0" || machine.Goal.SettingsVersion != "42" { + t.Fatalf("Create() goal = %#v, want pre-created Machine goal", machine.Goal) } } diff --git a/pkg/aksmachine/ensure.go b/pkg/aksmachine/ensure.go index f08178c6..decc3849 100644 --- a/pkg/aksmachine/ensure.go +++ b/pkg/aksmachine/ensure.go @@ -17,10 +17,8 @@ type ensureMachineTask struct { } // EnsureMachine returns a task that ensures this machine is registered in AKS. -// Local configuration remains authoritative during bootstrap. When the remote -// Kubernetes version already matches, the task adopts only the remote ETag as -// the reconciliation baseline; other remote settings do not replace the local -// goal. Subsequent ETag changes are handled by the daemon as new remote goals. +// Local configuration seeds a Machine when one does not exist. A Machine +// returned by AKS is authoritative for bootstrap and later reconciliation. func EnsureMachine(machines MachineClient, goal *GoalState, require bool, logger *slog.Logger) phases.Task { return &ensureMachineTask{machines: machines, goal: goal, require: require, logger: logger} } @@ -33,22 +31,16 @@ func (t *ensureMachineTask) Do(ctx context.Context) error { return t.handleError("get machine", err) } - switch { - case remoteMachine == nil: + switch remoteMachine { + case nil: remoteMachine, err = t.createRemoteMachineFromGoal(ctx) if err != nil { return t.handleError("create machine", err) } - case machineGoalHasDrift(remoteMachine.Goal, *t.goal): - remoteMachine, err = t.updateRemoteMachineFromGoal(ctx, remoteMachine) - if err != nil { - return t.handleError("update machine", err) - } default: - t.logger.Info("machine already registered, skipping") + t.logger.Info("machine already registered, adopting remote goal") } - t.applyGoalStateWithRemoteMachineSettingsVersion(remoteMachine) - return nil + return t.applyRemoteMachineGoal(remoteMachine) } func (t *ensureMachineTask) fetchRemoteMachine(ctx context.Context) (*Machine, error) { @@ -71,47 +63,18 @@ func (t *ensureMachineTask) createRemoteMachineFromGoal(ctx context.Context) (*M if err != nil { return nil, err } - if err := validateMachineForGoal(machine, *t.goal); err != nil { - return nil, err + if err := machine.Validate(); err != nil { + return nil, fmt.Errorf("AKS returned an invalid machine: %w", err) } return machine, nil } -func (t *ensureMachineTask) updateRemoteMachineFromGoal(ctx context.Context, current *Machine) (*Machine, error) { - t.logger.Info( - "updating registered machine from local bootstrap config", - "remoteKubernetesVersion", current.Goal.KubernetesVersion, - "localKubernetesVersion", t.goal.KubernetesVersion, - ) - machine, err := t.machines.Create(ctx, *t.goal) +func (t *ensureMachineTask) applyRemoteMachineGoal(machine *Machine) error { + effectiveGoal, err := EffectiveGoal(machine.Goal, *t.goal) if err != nil { - return nil, err - } - if err := validateMachineForGoal(machine, *t.goal); err != nil { - return nil, err - } - return machine, nil -} - -func (t *ensureMachineTask) applyGoalStateWithRemoteMachineSettingsVersion(machine *Machine) { - t.goal.SettingsVersion = machine.Goal.SettingsVersion -} - -func machineGoalHasDrift(remote, desired GoalState) bool { - return remote.KubernetesVersion != desired.KubernetesVersion -} - -func validateMachineForGoal(machine *Machine, goal GoalState) error { - if err := machine.Validate(); err != nil { - return fmt.Errorf("AKS returned an invalid machine: %w", err) - } - if machine.Goal.KubernetesVersion != goal.KubernetesVersion { - return fmt.Errorf( - "AKS machine Kubernetes version %q does not match local bootstrap version %q", - machine.Goal.KubernetesVersion, - goal.KubernetesVersion, - ) + return t.handleError("apply machine goal", err) } + *t.goal = effectiveGoal return nil } diff --git a/pkg/aksmachine/ensure_test.go b/pkg/aksmachine/ensure_test.go index 3e8b0750..a8774ee0 100644 --- a/pkg/aksmachine/ensure_test.go +++ b/pkg/aksmachine/ensure_test.go @@ -28,7 +28,7 @@ func TestEnsureMachineCreateFailure(t *testing.T) { t.Parallel() client := &ensureMachineClient{createErr: errors.New("boom")} - goal := GoalState{KubernetesVersion: "1.35.1"} + goal := testGoal("1.35.1", "") task := EnsureMachine(client, &goal, tt.require, slog.New(slog.NewTextHandler(io.Discard, nil))) err := task.Do(context.Background()) @@ -64,7 +64,7 @@ func TestEnsureMachineGetFailure(t *testing.T) { t.Parallel() client := &ensureMachineClient{getErr: errors.New("boom")} - goal := GoalState{KubernetesVersion: "1.35.1"} + goal := testGoal("1.35.1", "") task := EnsureMachine(client, &goal, tt.require, slog.New(slog.NewTextHandler(io.Discard, nil))) err := task.Do(context.Background()) @@ -84,11 +84,10 @@ func TestEnsureMachineGetFailure(t *testing.T) { func TestEnsureMachineCreatesAndAdoptsSettingsVersion(t *testing.T) { t.Parallel() - goal := GoalState{KubernetesVersion: "1.35.1"} - client := &ensureMachineClient{createResult: &Machine{Goal: GoalState{ - KubernetesVersion: "1.35.1", - SettingsVersion: "etag-created", - }}} + goal := testGoal("1.35.1", "") + createdGoal := testGoal("1.35.1", "etag-created") + createdGoal.MaxPods = ptr(42) + client := &ensureMachineClient{createResult: &Machine{Goal: createdGoal}} task := EnsureMachine(client, &goal, true, slog.New(slog.NewTextHandler(io.Discard, nil))) if err := task.Do(context.Background()); err != nil { @@ -100,30 +99,33 @@ func TestEnsureMachineCreatesAndAdoptsSettingsVersion(t *testing.T) { if goal.SettingsVersion != "etag-created" { t.Fatalf("SettingsVersion = %q, want etag-created", goal.SettingsVersion) } + if goal.MaxPods == nil || *goal.MaxPods != 42 { + t.Fatalf("MaxPods = %v, want server-normalized value 42", goal.MaxPods) + } } -func TestEnsureMachineAdoptsExistingSettingsVersionWithoutReplacingLocalGoal(t *testing.T) { +func TestEnsureMachineAdoptsExistingGoal(t *testing.T) { t.Parallel() goal := GoalState{ KubernetesVersion: "1.35.1", - MaxPods: 30, + MaxPods: ptr(30), NodeLabels: map[string]string{"source": "local"}, NodeTaints: []string{"local=true:NoSchedule"}, KubeletConfig: KubeletConfig{ - ImageGCHighThreshold: 85, - ImageGCLowThreshold: 80, + ImageGCHighThreshold: ptr(85), + ImageGCLowThreshold: ptr(80), }, } client := &ensureMachineClient{machine: &Machine{Goal: GoalState{ KubernetesVersion: "1.35.1", SettingsVersion: "etag-42", - MaxPods: 110, + MaxPods: ptr(110), NodeLabels: map[string]string{"source": "remote"}, NodeTaints: []string{"remote=true:NoSchedule"}, KubeletConfig: KubeletConfig{ - ImageGCHighThreshold: 70, - ImageGCLowThreshold: 60, + ImageGCHighThreshold: ptr(70), + ImageGCLowThreshold: ptr(60), }, }}} task := EnsureMachine(client, &goal, true, slog.New(slog.NewTextHandler(io.Discard, nil))) @@ -137,60 +139,31 @@ func TestEnsureMachineAdoptsExistingSettingsVersionWithoutReplacingLocalGoal(t * if goal.SettingsVersion != "etag-42" { t.Fatalf("SettingsVersion = %q, want etag-42", goal.SettingsVersion) } - if goal.MaxPods != 30 || goal.NodeLabels["source"] != "local" || goal.NodeTaints[0] != "local=true:NoSchedule" { - t.Fatalf("local goal was replaced by remote settings: %#v", goal) + if goal.MaxPods == nil || *goal.MaxPods != 110 || goal.NodeLabels["source"] != "remote" || goal.NodeTaints[0] != "remote=true:NoSchedule" { + t.Fatalf("remote goal was not adopted: %#v", goal) } - if goal.KubeletConfig.ImageGCHighThreshold != 85 || goal.KubeletConfig.ImageGCLowThreshold != 80 { - t.Fatalf("local kubelet config was replaced by remote settings: %#v", goal.KubeletConfig) + if goal.KubeletConfig.ImageGCHighThreshold == nil || *goal.KubeletConfig.ImageGCHighThreshold != 70 || + goal.KubeletConfig.ImageGCLowThreshold == nil || *goal.KubeletConfig.ImageGCLowThreshold != 60 { + t.Fatalf("remote kubelet config was not adopted: %#v", goal.KubeletConfig) } } -func TestEnsureMachineUpdatesMismatchedVersion(t *testing.T) { +func TestEnsureMachineAdoptsExistingMismatchedVersion(t *testing.T) { t.Parallel() - goal := GoalState{KubernetesVersion: "1.35.1"} - client := &ensureMachineClient{ - machine: &Machine{Goal: GoalState{KubernetesVersion: "1.34.0", SettingsVersion: "etag-old"}}, - createResult: &Machine{Goal: GoalState{ - KubernetesVersion: "1.35.1", - SettingsVersion: "etag-new", - }}, - } + goal := testGoal("1.35.1", "") + remoteGoal := testGoal("1.34.0", "etag-remote") + client := &ensureMachineClient{machine: &Machine{Goal: remoteGoal}} task := EnsureMachine(client, &goal, true, slog.New(slog.NewTextHandler(io.Discard, nil))) if err := task.Do(context.Background()); err != nil { t.Fatalf("Do() error = %v", err) } - if client.createCalls != 1 { - t.Fatalf("Create() calls = %d, want 1", client.createCalls) - } - if client.createdGoal.KubernetesVersion != "1.35.1" { - t.Fatalf("Create() goal = %#v", client.createdGoal) - } - if goal.SettingsVersion != "etag-new" { - t.Fatalf("SettingsVersion = %q, want etag-new", goal.SettingsVersion) - } -} - -func TestEnsureMachineRejectsUnchangedRemoteVersionAfterUpdate(t *testing.T) { - t.Parallel() - - goal := GoalState{KubernetesVersion: "1.35.1"} - client := &ensureMachineClient{ - machine: &Machine{Goal: GoalState{KubernetesVersion: "1.34.0", SettingsVersion: "etag-old"}}, - createResult: &Machine{Goal: GoalState{ - KubernetesVersion: "1.34.0", - SettingsVersion: "etag-old", - }}, - } - task := EnsureMachine(client, &goal, true, slog.New(slog.NewTextHandler(io.Discard, nil))) - - err := task.Do(context.Background()) - if err == nil || !strings.Contains(err.Error(), `AKS machine Kubernetes version "1.34.0" does not match local bootstrap version "1.35.1"`) { - t.Fatalf("Do() error = %v, want version mismatch", err) + if client.createCalls != 0 { + t.Fatalf("Create() calls = %d, want 0", client.createCalls) } - if goal.SettingsVersion != "" { - t.Fatalf("SettingsVersion = %q, want empty before a valid Machine response", goal.SettingsVersion) + if goal.KubernetesVersion != "1.34.0" || goal.SettingsVersion != "etag-remote" { + t.Fatalf("goal = %#v, want remote version and settings version", goal) } } @@ -203,10 +176,10 @@ func TestEnsureMachineRejectsInvalidExistingMachine(t *testing.T) { wantErr string }{ "best effort preserves local goal after missing settings version": { - machine: &Machine{Goal: GoalState{KubernetesVersion: "1.35.1"}}, + machine: &Machine{Goal: testGoal("1.35.1", "")}, }, "required rejects missing settings version": { - machine: &Machine{Goal: GoalState{KubernetesVersion: "1.35.1"}}, + machine: &Machine{Goal: testGoal("1.35.1", "")}, require: true, wantErr: "goal settings version is empty", }, @@ -221,7 +194,8 @@ func TestEnsureMachineRejectsInvalidExistingMachine(t *testing.T) { t.Run(name, func(t *testing.T) { t.Parallel() - goal := GoalState{KubernetesVersion: "1.35.1", NodeLabels: map[string]string{"source": "local"}} + goal := testGoal("1.35.1", "") + goal.NodeLabels = map[string]string{"source": "local"} client := &ensureMachineClient{machine: tt.machine, getResultSet: true} task := EnsureMachine(client, &goal, tt.require, slog.New(slog.NewTextHandler(io.Discard, nil))) diff --git a/pkg/aksmachine/test_helpers_test.go b/pkg/aksmachine/test_helpers_test.go new file mode 100644 index 00000000..2a11c168 --- /dev/null +++ b/pkg/aksmachine/test_helpers_test.go @@ -0,0 +1,13 @@ +package aksmachine + +func testGoal(kubernetesVersion, settingsVersion string) GoalState { + return GoalState{ + KubernetesVersion: kubernetesVersion, + SettingsVersion: settingsVersion, + MaxPods: ptr(110), + KubeletConfig: KubeletConfig{ + ImageGCHighThreshold: ptr(85), + ImageGCLowThreshold: ptr(80), + }, + } +} diff --git a/pkg/aksmachine/types.go b/pkg/aksmachine/types.go index 35b1f4b8..b31e8358 100644 --- a/pkg/aksmachine/types.go +++ b/pkg/aksmachine/types.go @@ -16,55 +16,121 @@ import ( type GoalState struct { KubernetesVersion string `json:"kubernetesVersion,omitempty"` SettingsVersion string `json:"settingsVersion,omitempty"` - MaxPods int `json:"maxPods,omitempty"` + MaxPods *int `json:"maxPods,omitempty"` NodeLabels map[string]string `json:"nodeLabels,omitempty"` NodeTaints []string `json:"nodeTaints,omitempty"` KubeletConfig KubeletConfig `json:"kubeletConfig"` } type KubeletConfig struct { - ImageGCHighThreshold int `json:"imageGCHighThreshold,omitempty"` - ImageGCLowThreshold int `json:"imageGCLowThreshold,omitempty"` + ImageGCHighThreshold *int `json:"imageGCHighThreshold,omitempty"` + ImageGCLowThreshold *int `json:"imageGCLowThreshold,omitempty"` } -func (g GoalState) validate() error { +// Validate verifies that a goal is complete and can be rendered on a node. +// SettingsVersion is validated by Machine because a local bootstrap goal does +// not have an ETag until it is persisted. +func (g GoalState) Validate() error { if g.KubernetesVersion == "" { return fmt.Errorf("kubernetes version is empty") } - if g.MaxPods < 0 { + if g.MaxPods == nil || *g.MaxPods == 0 { + return fmt.Errorf("max pods is empty") + } + if *g.MaxPods < 0 { return fmt.Errorf("max pods must be non-negative") } - if g.MaxPods > math.MaxInt32 { + if *g.MaxPods > math.MaxInt32 { return fmt.Errorf("max pods must be less than or equal to %d", math.MaxInt32) } - if g.KubeletConfig.ImageGCHighThreshold < 0 { + if g.KubeletConfig.ImageGCHighThreshold == nil || *g.KubeletConfig.ImageGCHighThreshold == 0 { + return fmt.Errorf("image GC high threshold is empty") + } + if *g.KubeletConfig.ImageGCHighThreshold < 0 { return fmt.Errorf("image GC high threshold must be non-negative") } - if g.KubeletConfig.ImageGCLowThreshold < 0 { + if g.KubeletConfig.ImageGCLowThreshold == nil { + return fmt.Errorf("image GC low threshold is empty") + } + if *g.KubeletConfig.ImageGCLowThreshold < 0 { return fmt.Errorf("image GC low threshold must be non-negative") } + if *g.KubeletConfig.ImageGCHighThreshold > 100 { + return fmt.Errorf("image GC high threshold must be less than or equal to 100") + } + if *g.KubeletConfig.ImageGCLowThreshold > 100 { + return fmt.Errorf("image GC low threshold must be less than or equal to 100") + } + if *g.KubeletConfig.ImageGCLowThreshold >= *g.KubeletConfig.ImageGCHighThreshold { + return fmt.Errorf("image GC low threshold must be less than image GC high threshold") + } return nil } // GoalStateFromConfig builds and validates the initial AKS machine goal state // from local agent configuration. func GoalStateFromConfig(cfg *config.Config) (GoalState, error) { + maxPods := cfg.Node.MaxPods + imageGCHighThreshold := cfg.Node.Kubelet.ImageGCHighThreshold + imageGCLowThreshold := cfg.Node.Kubelet.ImageGCLowThreshold goal := GoalState{ KubernetesVersion: cfg.Components.Kubernetes, - MaxPods: cfg.Node.MaxPods, + MaxPods: &maxPods, NodeLabels: maps.Clone(cfg.Node.Labels), NodeTaints: slices.Clone(cfg.Node.Taints), KubeletConfig: KubeletConfig{ - ImageGCHighThreshold: cfg.Node.Kubelet.ImageGCHighThreshold, - ImageGCLowThreshold: cfg.Node.Kubelet.ImageGCLowThreshold, + ImageGCHighThreshold: &imageGCHighThreshold, + ImageGCLowThreshold: &imageGCLowThreshold, }, } - if err := goal.validate(); err != nil { + if err := goal.Validate(); err != nil { return GoalState{}, err } return goal, nil } +// DeepCopy returns a goal whose mutable fields are independent of the source. +func (g GoalState) DeepCopy() *GoalState { + cloned := g + cloned.NodeLabels = maps.Clone(g.NodeLabels) + cloned.NodeTaints = slices.Clone(g.NodeTaints) + if g.MaxPods != nil { + value := *g.MaxPods + cloned.MaxPods = &value + } + if g.KubeletConfig.ImageGCHighThreshold != nil { + value := *g.KubeletConfig.ImageGCHighThreshold + cloned.KubeletConfig.ImageGCHighThreshold = &value + } + if g.KubeletConfig.ImageGCLowThreshold != nil { + value := *g.KubeletConfig.ImageGCLowThreshold + cloned.KubeletConfig.ImageGCLowThreshold = &value + } + return &cloned +} + +// EffectiveGoal overlays a Machine goal on a complete local goal. AKS owns the +// desired values; the local goal only fills scalar fields omitted by the API. +func EffectiveGoal(machine, local GoalState) (GoalState, error) { + effective := machine.DeepCopy() + if effective.MaxPods == nil && local.MaxPods != nil { + value := *local.MaxPods + effective.MaxPods = &value + } + if effective.KubeletConfig.ImageGCHighThreshold == nil && local.KubeletConfig.ImageGCHighThreshold != nil { + value := *local.KubeletConfig.ImageGCHighThreshold + effective.KubeletConfig.ImageGCHighThreshold = &value + } + if effective.KubeletConfig.ImageGCLowThreshold == nil && local.KubeletConfig.ImageGCLowThreshold != nil { + value := *local.KubeletConfig.ImageGCLowThreshold + effective.KubeletConfig.ImageGCLowThreshold = &value + } + if err := effective.Validate(); err != nil { + return GoalState{}, fmt.Errorf("validate effective goal: %w", err) + } + return *effective, nil +} + type ProvisioningState string const ( @@ -90,14 +156,14 @@ type Machine struct { Status Status `json:"status"` } -// Validate verifies that a Machine returned by AKS contains a goal suitable -// for bootstrap or reconciliation. +// Validate verifies the required fields in a Machine returned by AKS. Optional +// scalar settings are validated after local defaults are applied. func (m *Machine) Validate() error { if m == nil { return fmt.Errorf("machine is nil") } - if err := m.Goal.validate(); err != nil { - return fmt.Errorf("goal: %w", err) + if m.Goal.KubernetesVersion == "" { + return fmt.Errorf("goal: kubernetes version is empty") } if m.Goal.SettingsVersion == "" { return fmt.Errorf("goal settings version is empty") diff --git a/pkg/aksmachine/types_test.go b/pkg/aksmachine/types_test.go index b6ab345f..00067f9e 100644 --- a/pkg/aksmachine/types_test.go +++ b/pkg/aksmachine/types_test.go @@ -44,8 +44,8 @@ func TestGoalStateFromConfig(t *testing.T) { if goal.SettingsVersion != "" { t.Fatalf("SettingsVersion = %q, want empty before Machine persistence", goal.SettingsVersion) } - if goal.MaxPods != 42 { - t.Fatalf("MaxPods = %d, want 42", goal.MaxPods) + if goal.MaxPods == nil || *goal.MaxPods != 42 { + t.Fatalf("MaxPods = %v, want 42", goal.MaxPods) } if len(goal.NodeLabels) != 2 { t.Fatalf("NodeLabels length = %d, want 2", len(goal.NodeLabels)) @@ -65,11 +65,11 @@ func TestGoalStateFromConfig(t *testing.T) { if goal.NodeTaints[1] != "edge=true:NoExecute" { t.Fatalf("NodeTaints[1] = %v, want edge=true:NoExecute", goal.NodeTaints[1]) } - if goal.KubeletConfig.ImageGCHighThreshold != 85 { - t.Fatalf("ImageGCHighThreshold = %d, want 85", goal.KubeletConfig.ImageGCHighThreshold) + if goal.KubeletConfig.ImageGCHighThreshold == nil || *goal.KubeletConfig.ImageGCHighThreshold != 85 { + t.Fatalf("ImageGCHighThreshold = %v, want 85", goal.KubeletConfig.ImageGCHighThreshold) } - if goal.KubeletConfig.ImageGCLowThreshold != 80 { - t.Fatalf("ImageGCLowThreshold = %d, want 80", goal.KubeletConfig.ImageGCLowThreshold) + if goal.KubeletConfig.ImageGCLowThreshold == nil || *goal.KubeletConfig.ImageGCLowThreshold != 80 { + t.Fatalf("ImageGCLowThreshold = %v, want 80", goal.KubeletConfig.ImageGCLowThreshold) } } @@ -98,10 +98,13 @@ func TestMachineValidate(t *testing.T) { wantErr: "kubernetes version is empty", }, "missing settings version": { - machine: &Machine{Goal: GoalState{KubernetesVersion: "1.35.1"}}, + machine: &Machine{Goal: testGoal("1.35.1", "")}, wantErr: "goal settings version is empty", }, "complete machine": { + machine: &Machine{Goal: testGoal("1.35.1", "42")}, + }, + "omitted scalar defaults": { machine: &Machine{Goal: GoalState{KubernetesVersion: "1.35.1", SettingsVersion: "42"}}, }, } @@ -123,3 +126,66 @@ func TestMachineValidate(t *testing.T) { }) } } + +func TestEffectiveGoal(t *testing.T) { + t.Parallel() + + local := testGoal("1.34.0", "") + local.MaxPods = ptr(30) + local.NodeLabels = map[string]string{"source": "local"} + local.NodeTaints = []string{"local=true:NoSchedule"} + local.KubeletConfig.ImageGCHighThreshold = ptr(90) + local.KubeletConfig.ImageGCLowThreshold = ptr(75) + machine := GoalState{ + KubernetesVersion: "1.35.0", + SettingsVersion: "42", + NodeLabels: map[string]string{}, + NodeTaints: []string{}, + } + + effective, err := EffectiveGoal(machine, local) + if err != nil { + t.Fatalf("EffectiveGoal() error = %v", err) + } + if effective.KubernetesVersion != "1.35.0" || effective.SettingsVersion != "42" || effective.MaxPods == nil || *effective.MaxPods != 30 { + t.Fatalf("effective versions/maxPods = %#v", effective) + } + if len(effective.NodeLabels) != 0 || len(effective.NodeTaints) != 0 { + t.Fatalf("effective collections = %#v, want authoritative empty collections", effective) + } + if effective.KubeletConfig.ImageGCHighThreshold == nil || *effective.KubeletConfig.ImageGCHighThreshold != 90 || + effective.KubeletConfig.ImageGCLowThreshold == nil || *effective.KubeletConfig.ImageGCLowThreshold != 75 { + t.Fatalf("effective kubelet config = %#v", effective.KubeletConfig) + } + + effective.NodeLabels["source"] = "changed" + if _, ok := machine.NodeLabels["source"]; ok { + t.Fatal("EffectiveGoal returned Machine-owned label map") + } + *effective.MaxPods = 50 + *effective.KubeletConfig.ImageGCHighThreshold = 80 + if *local.MaxPods != 30 || *local.KubeletConfig.ImageGCHighThreshold != 90 { + t.Fatal("EffectiveGoal returned local-owned scalar pointers") + } +} + +func TestEffectiveGoalPreservesExplicitZero(t *testing.T) { + t.Parallel() + + local := testGoal("1.34.0", "") + machine := GoalState{ + KubernetesVersion: "1.35.0", + SettingsVersion: "42", + KubeletConfig: KubeletConfig{ + ImageGCLowThreshold: ptr(0), + }, + } + + effective, err := EffectiveGoal(machine, local) + if err != nil { + t.Fatalf("EffectiveGoal() error = %v", err) + } + if effective.KubeletConfig.ImageGCLowThreshold == nil || *effective.KubeletConfig.ImageGCLowThreshold != 0 { + t.Fatalf("ImageGCLowThreshold = %v, want explicit zero", effective.KubeletConfig.ImageGCLowThreshold) + } +} diff --git a/pkg/cmd/start/start.go b/pkg/cmd/start/start.go index 23e112c5..deb220e1 100644 --- a/pkg/cmd/start/start.go +++ b/pkg/cmd/start/start.go @@ -76,7 +76,7 @@ func runStart(ctx context.Context, cfg *config.Config, logger *slog.Logger) erro return err } - _, gs, containerImageArchives, err := config.ResolveMachineGoalState(ctx, logger, cfg, machineName) + _, gs, containerImageArchives, err := daemon.ResolveMachineGoalState(ctx, logger, cfg, machineName, &goal) if err != nil { return fmt.Errorf("bootstrap failed to resolve goal state: %w", err) } diff --git a/pkg/daemon/goal_state.go b/pkg/daemon/goal_state.go new file mode 100644 index 00000000..ae49c3bc --- /dev/null +++ b/pkg/daemon/goal_state.go @@ -0,0 +1,94 @@ +package daemon + +import ( + "context" + "fmt" + "log/slog" + "maps" + "slices" + + agentconfig "github.com/Azure/unbounded/pkg/agent/config" + "github.com/Azure/unbounded/pkg/agent/goalstates" + + "github.com/Azure/AKSFlexNode/pkg/aksmachine" + "github.com/Azure/AKSFlexNode/pkg/config" +) + +// ResolveMachineGoalState overlays AKS Machine-owned settings on the local +// host configuration before resolving the nspawn goal. +func ResolveMachineGoalState( + ctx context.Context, + log *slog.Logger, + cfg *config.Config, + machineName string, + goal *aksmachine.GoalState, +) (*agentconfig.AgentConfig, *goalstates.MachineGoalState, *goalstates.ContainerImageArchiveStaging, error) { + effectiveGoal, err := effectiveMachineGoal(cfg, goal) + if err != nil { + return nil, nil, nil, err + } + return resolveEffectiveMachineGoalState(ctx, log, cfg, machineName, *effectiveGoal) +} + +func resolveEffectiveMachineGoalState( + ctx context.Context, + log *slog.Logger, + cfg *config.Config, + machineName string, + goal aksmachine.GoalState, +) (*agentconfig.AgentConfig, *goalstates.MachineGoalState, *goalstates.ContainerImageArchiveStaging, error) { + if err := goal.Validate(); err != nil { + return nil, nil, nil, fmt.Errorf("validate machine goal: %w", err) + } + resolvedConfig := cfg.DeepCopy() + if resolvedConfig == nil { + return nil, nil, nil, fmt.Errorf("copy config for machine goal") + } + resolvedConfig.Components.Kubernetes = goal.KubernetesVersion + resolvedConfig.Node.MaxPods = *goal.MaxPods + resolvedConfig.Node.Labels = maps.Clone(goal.NodeLabels) + resolvedConfig.Node.Taints = slices.Clone(goal.NodeTaints) + resolvedConfig.Node.Kubelet.ImageGCHighThreshold = *goal.KubeletConfig.ImageGCHighThreshold + resolvedConfig.Node.Kubelet.ImageGCLowThreshold = *goal.KubeletConfig.ImageGCLowThreshold + return config.ResolveMachineGoalState(ctx, log, resolvedConfig, machineName) +} + +func effectiveMachineGoal(cfg *config.Config, goal *aksmachine.GoalState) (*aksmachine.GoalState, error) { + localGoal, err := aksmachine.GoalStateFromConfig(cfg) + if err != nil { + return nil, fmt.Errorf("build local machine goal: %w", err) + } + if goal == nil { + return &localGoal, nil + } + effectiveGoal, err := aksmachine.EffectiveGoal(*goal, localGoal) + if err != nil { + return nil, err + } + return &effectiveGoal, nil +} + +func goalForRestart(cfg *config.Config, state *State) (*aksmachine.GoalState, error) { + if state != nil && state.AppliedGoal != nil { + goal := state.AppliedGoal.DeepCopy() + if err := goal.Validate(); err != nil { + return nil, fmt.Errorf("validate persisted restart goal: %w", err) + } + return goal, nil + } + + goal, err := aksmachine.GoalStateFromConfig(cfg) + if err != nil { + return nil, fmt.Errorf("build restart goal from config: %w", err) + } + if state != nil { + goal.SettingsVersion = state.AppliedSettingsVersion + if state.AppliedKubernetesVersion != "" { + goal.KubernetesVersion = state.AppliedKubernetesVersion + } + } + if err := goal.Validate(); err != nil { + return nil, fmt.Errorf("validate legacy restart goal: %w", err) + } + return &goal, nil +} diff --git a/pkg/daemon/goal_state_test.go b/pkg/daemon/goal_state_test.go new file mode 100644 index 00000000..16645a3e --- /dev/null +++ b/pkg/daemon/goal_state_test.go @@ -0,0 +1,103 @@ +package daemon + +import ( + "log/slog" + "maps" + "testing" + + "github.com/Azure/AKSFlexNode/pkg/config" +) + +func TestResolveMachineGoalStateUsesCompleteMachineGoal(t *testing.T) { + t.Parallel() + + cfg := &config.Config{ + Azure: config.AzureConfig{TargetAgentPoolName: "flexnode-edge"}, + Components: config.ComponentsConfig{Kubernetes: "1.34.0"}, + Node: config.NodeConfig{ + MaxPods: 30, + Labels: map[string]string{"source": "config"}, + Taints: []string{"config=true:NoSchedule"}, + Kubelet: config.KubeletConfig{ImageGCHighThreshold: 90, ImageGCLowThreshold: 75}, + }, + } + goal := testMachineGoal("1.35.1", "42") + goal.MaxPods = intPointer(50) + goal.NodeLabels = map[string]string{"source": "machine"} + goal.NodeTaints = []string{"machine=true:NoExecute"} + goal.KubeletConfig.ImageGCHighThreshold = intPointer(70) + goal.KubeletConfig.ImageGCLowThreshold = intPointer(60) + + agentCfg, _, _, err := ResolveMachineGoalState(t.Context(), slog.Default(), cfg, "kube1", &goal) + if err != nil { + t.Fatalf("ResolveMachineGoalState: %v", err) + } + if agentCfg.Cluster.Version != "1.35.1" { + t.Fatalf("Cluster.Version = %q, want 1.35.1", agentCfg.Cluster.Version) + } + if got := agentCfg.Kubelet.Configuration["maxPods"]; got != 50 { + t.Fatalf("maxPods = %v, want 50", got) + } + if got := agentCfg.Kubelet.Configuration["imageGCHighThresholdPercent"]; got != 70 { + t.Fatalf("imageGCHighThresholdPercent = %v, want 70", got) + } + if got := agentCfg.Kubelet.Configuration["imageGCLowThresholdPercent"]; got != 60 { + t.Fatalf("imageGCLowThresholdPercent = %v, want 60", got) + } + if agentCfg.Kubelet.Labels["source"] != "machine" { + t.Fatalf("Kubelet.Labels = %#v, want Machine labels", agentCfg.Kubelet.Labels) + } + if len(agentCfg.Kubelet.RegisterWithTaints) != 1 || agentCfg.Kubelet.RegisterWithTaints[0] != "machine=true:NoExecute" { + t.Fatalf("RegisterWithTaints = %#v", agentCfg.Kubelet.RegisterWithTaints) + } + if cfg.Components.Kubernetes != "1.34.0" || cfg.Node.MaxPods != 30 || cfg.Node.Labels["source"] != "config" { + t.Fatalf("base config was mutated: %#v", cfg) + } +} + +func TestGoalForRestartLegacyStatePreservesConfigSettings(t *testing.T) { + t.Parallel() + + cfg := &config.Config{ + Components: config.ComponentsConfig{Kubernetes: "1.34.0"}, + Node: config.NodeConfig{ + MaxPods: 30, + Labels: map[string]string{"source": "config"}, + Taints: []string{"config=true:NoSchedule"}, + Kubelet: config.KubeletConfig{ImageGCHighThreshold: 90, ImageGCLowThreshold: 75}, + }, + } + state := &State{AppliedSettingsVersion: "42", AppliedKubernetesVersion: "1.35.1"} + + goal, err := goalForRestart(cfg, state) + if err != nil { + t.Fatalf("goalForRestart: %v", err) + } + if goal.KubernetesVersion != "1.35.1" || goal.SettingsVersion != "42" || goal.MaxPods == nil || *goal.MaxPods != 30 { + t.Fatalf("goal versions/maxPods = %#v", goal) + } + if !maps.Equal(goal.NodeLabels, cfg.Node.Labels) || len(goal.NodeTaints) != 1 || goal.NodeTaints[0] != cfg.Node.Taints[0] { + t.Fatalf("legacy restart goal lost config settings: %#v", goal) + } + if goal.KubeletConfig.ImageGCHighThreshold == nil || *goal.KubeletConfig.ImageGCHighThreshold != 90 || + goal.KubeletConfig.ImageGCLowThreshold == nil || *goal.KubeletConfig.ImageGCLowThreshold != 75 { + t.Fatalf("legacy restart kubelet config = %#v", goal.KubeletConfig) + } +} + +func TestGoalForRestartClonesCompleteGoal(t *testing.T) { + t.Parallel() + + applied := testMachineGoal("1.35.1", "42") + applied.NodeLabels = map[string]string{"source": "machine"} + state := &State{AppliedGoal: &applied} + + goal, err := goalForRestart(&config.Config{}, state) + if err != nil { + t.Fatalf("goalForRestart: %v", err) + } + goal.NodeLabels["source"] = "changed" + if state.AppliedGoal.NodeLabels["source"] != "machine" { + t.Fatal("goalForRestart returned state-owned label map") + } +} diff --git a/pkg/daemon/nodeoperator.go b/pkg/daemon/nodeoperator.go index 0a082cc7..96483a76 100644 --- a/pkg/daemon/nodeoperator.go +++ b/pkg/daemon/nodeoperator.go @@ -23,6 +23,7 @@ type activeMachine struct { type nodeOperator interface { LoadState(ctx context.Context) (*State, error) ApplyGoalState(ctx context.Context, log *slog.Logger, goal aksmachine.GoalState) (*State, error) + AcknowledgeGoalState(ctx context.Context, goal aksmachine.GoalState) (*State, error) RestartNode(ctx context.Context, log *slog.Logger) error // ResetNode removes nspawn node runtime and persisted daemon state but must // not stop this daemon process. The controller publishes lifecycle completion @@ -38,11 +39,11 @@ func (o *nspawnNodeOperator) RestartNode(ctx context.Context, log *slog.Logger) return err } - cfg := o.cfg.DeepCopy() - if active.State.AppliedKubernetesVersion != "" { - cfg.Components.Kubernetes = active.State.AppliedKubernetesVersion + goal, err := goalForRestart(o.cfg, active.State) + if err != nil { + return err } - _, gs, containerImageArchives, err := config.ResolveMachineGoalState(ctx, log, cfg, active.Name) + _, gs, containerImageArchives, err := resolveEffectiveMachineGoalState(ctx, log, o.cfg, active.Name, *goal) if err != nil { return fmt.Errorf("resolve goal state for node restart: %w", err) } @@ -52,7 +53,7 @@ func (o *nspawnNodeOperator) RestartNode(ctx context.Context, log *slog.Logger) nodestop.StopNode(log, active.Name), nodestart.StartNode(log, gs.NodeStart), nodestart.WaitForKubelet(log, active.Name), - npd.Start(log, cfg, gs.NodeStart), + npd.Start(log, o.cfg, gs.NodeStart), ).Do(ctx) } @@ -86,7 +87,11 @@ func (o *nspawnNodeOperator) ApplyGoalState(ctx context.Context, log *slog.Logge if err != nil { return nil, err } - cfg, err := o.configForGoalState(ctx, log, goal) + cfg, err := o.configForRepave(ctx, log) + if err != nil { + return nil, err + } + effectiveGoal, err := effectiveMachineGoal(cfg, &goal) if err != nil { return nil, err } @@ -96,14 +101,14 @@ func (o *nspawnNodeOperator) ApplyGoalState(ctx context.Context, log *slog.Logge "oldMachine", oldMachine, "newMachine", newMachine, "settingsVersion", goal.SettingsVersion, - "kubernetesVersion", cfg.Components.Kubernetes, + "kubernetesVersion", effectiveGoal.KubernetesVersion, ) - _, gs, containerImageArchives, err := config.ResolveMachineGoalState(ctx, log, cfg, newMachine) + _, gs, containerImageArchives, err := resolveEffectiveMachineGoalState(ctx, log, cfg, newMachine, *effectiveGoal) if err != nil { return nil, fmt.Errorf("resolve goal state for repave: %w", err) } - newState := nextAppliedState(active.State, goal, &activeMachine{Name: newMachine}) + newState := nextAppliedState(active.State, *effectiveGoal, &activeMachine{Name: newMachine}) tasks := phases.Serial(log, nodestop.StopNode(log, oldMachine), @@ -117,7 +122,7 @@ func (o *nspawnNodeOperator) ApplyGoalState(ctx context.Context, log *slog.Logge return newState, nil } -func (o *nspawnNodeOperator) configForGoalState(ctx context.Context, log *slog.Logger, goal aksmachine.GoalState) (*config.Config, error) { +func (o *nspawnNodeOperator) configForRepave(ctx context.Context, log *slog.Logger) (*config.Config, error) { // Keep short-lived bootstrap credentials scoped to this repave. Persisting // them would make a later repave depend on this token's lifetime again. cfg := o.cfg.DeepCopy() @@ -144,18 +149,21 @@ func (o *nspawnNodeOperator) configForGoalState(ctx context.Context, log *slog.L cfg.Node.Kubelet.CACertData = data.CACertData } } - // MaxPods is immutable in AKS, so the startup configuration remains its - // authoritative source rather than reapplying the value from each goal. - if goal.KubernetesVersion != "" && cfg.Components.Kubernetes != goal.KubernetesVersion { - log.Info("updated Kubernetes version for repave", - "oldVersion", cfg.Components.Kubernetes, - "newVersion", goal.KubernetesVersion, - ) - cfg.Components.Kubernetes = goal.KubernetesVersion - } return cfg, nil } +func (o *nspawnNodeOperator) AcknowledgeGoalState(ctx context.Context, goal aksmachine.GoalState) (*State, error) { + active, err := o.findActiveMachine(ctx) + if err != nil { + return nil, err + } + newState := nextAppliedState(active.State, goal, &activeMachine{Name: active.Name}) + if err := o.state.Save(ctx, newState); err != nil { + return nil, fmt.Errorf("save acknowledged machine goal state: %w", err) + } + return newState, nil +} + func (o *nspawnNodeOperator) ResetNode(ctx context.Context, log *slog.Logger) error { return phases.ExecuteTask(ctx, log, ResetNode(log)) } @@ -166,17 +174,19 @@ func (o *nspawnNodeOperator) StopDaemon(ctx context.Context, log *slog.Logger) e func nextAppliedState(current *State, goal aksmachine.GoalState, active *activeMachine) *State { next := &State{ - AppliedSettingsVersion: goal.SettingsVersion, - AppliedKubernetesVersion: goal.KubernetesVersion, - PreviousSettingsVersion: "", - PreviousKubernetesVersion: "", + AppliedGoal: goal.DeepCopy(), } if current != nil { - next.PreviousSettingsVersion = current.AppliedSettingsVersion - next.PreviousKubernetesVersion = current.AppliedKubernetesVersion + if current.AppliedGoal != nil { + next.PreviousAppliedGoal = current.AppliedGoal.DeepCopy() + } else if current.AppliedKubernetesVersion != "" { + next.PreviousSettingsVersion = current.AppliedSettingsVersion + next.PreviousKubernetesVersion = current.AppliedKubernetesVersion + } } if active != nil { next.ActiveMachine = active.Name } + next.populateLegacyFields() return next } diff --git a/pkg/daemon/nodeoperator_test.go b/pkg/daemon/nodeoperator_test.go index c8f3f8a8..6d395efe 100644 --- a/pkg/daemon/nodeoperator_test.go +++ b/pkg/daemon/nodeoperator_test.go @@ -9,7 +9,6 @@ import ( "strings" "testing" - "github.com/Azure/AKSFlexNode/pkg/aksmachine" "github.com/Azure/AKSFlexNode/pkg/bootstrapdata" "github.com/Azure/AKSFlexNode/pkg/config" "github.com/Azure/unbounded/pkg/agent/goalstates" @@ -24,11 +23,11 @@ func TestFindActiveMachine(t *testing.T) { wantErr bool }{ "kube1": { - state: &State{ActiveMachine: goalstates.NSpawnMachineKube1}, + state: &State{AppliedKubernetesVersion: "1.34.0", ActiveMachine: goalstates.NSpawnMachineKube1}, want: goalstates.NSpawnMachineKube1, }, "kube2": { - state: &State{ActiveMachine: goalstates.NSpawnMachineKube2}, + state: &State{AppliedKubernetesVersion: "1.34.0", ActiveMachine: goalstates.NSpawnMachineKube2}, want: goalstates.NSpawnMachineKube2, }, "missing state": { @@ -65,7 +64,7 @@ func TestFindActiveMachine(t *testing.T) { } } -func TestConfigForGoalStateRefreshesBootstrapData(t *testing.T) { +func TestConfigForRepaveRefreshesBootstrapData(t *testing.T) { t.Parallel() cfg := &config.Config{ @@ -88,9 +87,9 @@ func TestConfigForGoalStateRefreshesBootstrapData(t *testing.T) { var logs bytes.Buffer log := slog.New(slog.NewTextHandler(&logs, nil)) - got, err := operator.configForGoalState(t.Context(), log, aksmachine.GoalState{KubernetesVersion: "1.36.2"}) + got, err := operator.configForRepave(t.Context(), log) if err != nil { - t.Fatalf("configForGoalState() error = %v", err) + t.Fatalf("configForRepave() error = %v", err) } if refresher.calls != 1 { t.Fatalf("refresh calls = %d, want 1", refresher.calls) @@ -104,8 +103,8 @@ func TestConfigForGoalStateRefreshesBootstrapData(t *testing.T) { if got.Node.Kubelet.CACertData != "bmV3" { t.Fatalf("kubelet CA data = %q", got.Node.Kubelet.CACertData) } - if got.Components.Kubernetes != "1.36.2" { - t.Fatalf("Kubernetes version = %q", got.Components.Kubernetes) + if got.Components.Kubernetes != "1.35.0" { + t.Fatalf("base Kubernetes version = %q", got.Components.Kubernetes) } if cfg.Azure.BootstrapToken.Token != "oldtok.0123456789abcdef" { t.Fatal("original config bootstrap token was mutated") @@ -114,9 +113,6 @@ func TestConfigForGoalStateRefreshesBootstrapData(t *testing.T) { "refreshed AKS bootstrap data for repave", "updated bootstrap token for repave", "updated kubelet CA data for repave", - "updated Kubernetes version for repave", - "oldVersion=1.35.0", - "newVersion=1.36.2", } { if !strings.Contains(logs.String(), message) { t.Errorf("logs did not contain %q: %s", message, logs.String()) @@ -129,7 +125,7 @@ func TestConfigForGoalStateRefreshesBootstrapData(t *testing.T) { } } -func TestConfigForGoalStateSkipsBootstrapDataRefreshWithoutBothAuthTypes(t *testing.T) { +func TestConfigForRepaveSkipsBootstrapDataRefreshWithoutBothAuthTypes(t *testing.T) { t.Parallel() tests := map[string]*config.Config{ @@ -145,8 +141,8 @@ func TestConfigForGoalStateSkipsBootstrapDataRefreshWithoutBothAuthTypes(t *test t.Parallel() refresher := bootstrapDataRefresherForConfig(cfg) operator := &nspawnNodeOperator{cfg: cfg, bootstrapDataRefresher: refresher} - if _, err := operator.configForGoalState(t.Context(), discardLogger(), aksmachine.GoalState{}); err != nil { - t.Fatalf("configForGoalState() error = %v", err) + if _, err := operator.configForRepave(t.Context(), discardLogger()); err != nil { + t.Fatalf("configForRepave() error = %v", err) } if _, ok := refresher.(noopBootstrapDataRefresher); !ok { t.Fatalf("refresher = %T, want noopBootstrapDataRefresher", refresher) @@ -168,7 +164,7 @@ func TestBootstrapDataRefresherForDualAuthConfig(t *testing.T) { } } -func TestConfigForGoalStateBootstrapDataRefreshFailure(t *testing.T) { +func TestConfigForRepaveBootstrapDataRefreshFailure(t *testing.T) { t.Parallel() cfg := &config.Config{Azure: config.AzureConfig{ @@ -179,9 +175,9 @@ func TestConfigForGoalStateBootstrapDataRefreshFailure(t *testing.T) { cfg: cfg, bootstrapDataRefresher: &fakeBootstrapDataRefresher{err: errors.New("ARM unavailable")}, } - _, err := operator.configForGoalState(t.Context(), discardLogger(), aksmachine.GoalState{}) + _, err := operator.configForRepave(t.Context(), discardLogger()) if err == nil || !errors.Is(err, operator.bootstrapDataRefresher.(*fakeBootstrapDataRefresher).err) { - t.Fatalf("configForGoalState() error = %v", err) + t.Fatalf("configForRepave() error = %v", err) } if cfg.Azure.BootstrapToken.Token != "oldtok.0123456789abcdef" { t.Fatal("original config bootstrap token was mutated") @@ -272,15 +268,102 @@ func discardLogger() *slog.Logger { return slog.New(slog.NewTextHandler(io.Discard, nil)) } +func TestNextAppliedStateRotatesCompleteGoals(t *testing.T) { + t.Parallel() + + currentGoal := testMachineGoal("1.34.0", "41") + currentGoal.NodeLabels = map[string]string{"source": "old"} + current := &State{AppliedGoal: ¤tGoal, ActiveMachine: goalstates.NSpawnMachineKube1} + nextGoal := testMachineGoal("1.35.0", "42") + nextGoal.NodeLabels = map[string]string{"source": "new"} + + got := nextAppliedState(current, nextGoal, &activeMachine{Name: goalstates.NSpawnMachineKube2}) + if got.AppliedGoal == nil || got.AppliedGoal.SettingsVersion != "42" || got.AppliedGoal.NodeLabels["source"] != "new" { + t.Fatalf("AppliedGoal = %#v", got.AppliedGoal) + } + if got.PreviousAppliedGoal == nil || got.PreviousAppliedGoal.SettingsVersion != "41" || got.PreviousAppliedGoal.NodeLabels["source"] != "old" { + t.Fatalf("PreviousAppliedGoal = %#v", got.PreviousAppliedGoal) + } + if got.AppliedSettingsVersion != "42" || got.PreviousSettingsVersion != "41" || got.ActiveMachine != goalstates.NSpawnMachineKube2 { + t.Fatalf("state = %#v", got) + } + + nextGoal.NodeLabels["source"] = "mutated" + currentGoal.NodeLabels["source"] = "mutated" + if got.AppliedGoal.NodeLabels["source"] != "new" || got.PreviousAppliedGoal.NodeLabels["source"] != "old" { + t.Fatal("nextAppliedState retained caller-owned maps") + } +} + +func TestAcknowledgeGoalState(t *testing.T) { + t.Parallel() + + appliedGoal := testMachineGoal("1.34.0", "41") + appliedGoal.NodeLabels = map[string]string{"source": "old"} + appliedGoal.NodeTaints = []string{"source=old:NoSchedule"} + store := &testStateStore{state: &State{AppliedGoal: &appliedGoal, ActiveMachine: goalstates.NSpawnMachineKube1}} + operator := &nspawnNodeOperator{state: store} + desiredGoal := testMachineGoal("1.34.0", "42") + desiredGoal.NodeLabels = map[string]string{"source": "new"} + desiredGoal.NodeTaints = []string{"source=new:NoExecute"} + + got, err := operator.AcknowledgeGoalState(t.Context(), desiredGoal) + if err != nil { + t.Fatalf("AcknowledgeGoalState: %v", err) + } + if store.state != got { + t.Fatal("acknowledged state was not persisted") + } + if got.ActiveMachine != goalstates.NSpawnMachineKube1 || got.AppliedGoal == nil || got.AppliedGoal.SettingsVersion != "42" { + t.Fatalf("state = %#v", got) + } + if got.PreviousAppliedGoal == nil || got.PreviousAppliedGoal.SettingsVersion != "41" { + t.Fatalf("PreviousAppliedGoal = %#v, want settings version 41", got.PreviousAppliedGoal) + } + if got.AppliedSettingsVersion != "42" || got.PreviousSettingsVersion != "41" { + t.Fatalf("legacy projections = %#v", got) + } + + desiredGoal.NodeLabels["source"] = "mutated" + desiredGoal.NodeTaints[0] = "mutated=true:NoSchedule" + appliedGoal.NodeLabels["source"] = "mutated" + if got.AppliedGoal.NodeLabels["source"] != "new" || got.AppliedGoal.NodeTaints[0] != "source=new:NoExecute" || got.PreviousAppliedGoal.NodeLabels["source"] != "old" { + t.Fatal("AcknowledgeGoalState retained caller-owned collections") + } +} + +func TestAcknowledgeGoalStateSaveFailure(t *testing.T) { + t.Parallel() + + saveErr := errors.New("save failed") + appliedGoal := testMachineGoal("1.34.0", "41") + oldState := &State{AppliedGoal: &appliedGoal, ActiveMachine: goalstates.NSpawnMachineKube1} + store := &testStateStore{state: oldState, saveErr: saveErr} + operator := &nspawnNodeOperator{state: store} + + _, err := operator.AcknowledgeGoalState(t.Context(), testMachineGoal("1.34.0", "42")) + if err == nil || !errors.Is(err, saveErr) { + t.Fatalf("AcknowledgeGoalState error = %v, want save failure", err) + } + if store.state != oldState { + t.Fatal("failed acknowledgement replaced persisted state") + } +} + type testStateStore struct { - state *State + state *State + saveErr error } func (s *testStateStore) Load(context.Context) (*State, error) { return s.state, nil } -func (s *testStateStore) Save(context.Context, *State) error { +func (s *testStateStore) Save(_ context.Context, state *State) error { + if s.saveErr != nil { + return s.saveErr + } + s.state = state return nil } diff --git a/pkg/daemon/reconcile_test.go b/pkg/daemon/reconcile_test.go index 43090acc..a47c7b9f 100644 --- a/pkg/daemon/reconcile_test.go +++ b/pkg/daemon/reconcile_test.go @@ -1,9 +1,11 @@ package daemon import ( + "maps" "testing" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/Azure/AKSFlexNode/pkg/aksmachine" ) @@ -11,13 +13,28 @@ import ( func TestDecide(t *testing.T) { t.Parallel() - goal := aksmachine.GoalState{KubernetesVersion: "1.34.0", SettingsVersion: "42"} + goal := testMachineGoal("1.34.0", "42") machine := machineSnapshot{machine: &aksmachine.Machine{Goal: goal}} - applied := &State{AppliedSettingsVersion: "42", AppliedKubernetesVersion: "1.34.0"} - stale := &State{AppliedSettingsVersion: "41", AppliedKubernetesVersion: "1.33.0"} + applied := &State{AppliedGoal: goal.DeepCopy()} + staleGoal := testMachineGoal("1.33.0", "41") + stale := &State{AppliedGoal: &staleGoal} node := nodeSnapshot{node: &corev1.Node{}} missingNode := nodeSnapshot{} deleteNode := nodeSnapshot{node: &corev1.Node{Spec: corev1.NodeSpec{Taints: []corev1.Taint{deletionTaint()}}}} + inPlaceAppliedGoal := testMachineGoal("1.34.0", "41") + inPlaceAppliedGoal.NodeLabels = map[string]string{"workload": "old", "removed": "true"} + inPlaceAppliedGoal.NodeTaints = []string{"dedicated=old:NoSchedule", "removed=true:NoExecute"} + inPlaceGoal := testMachineGoal("1.34.0", "42") + inPlaceGoal.NodeLabels = map[string]string{"workload": "new"} + inPlaceGoal.NodeTaints = []string{"dedicated=new:NoSchedule"} + inPlaceMachine := machineSnapshot{machine: &aksmachine.Machine{Goal: inPlaceGoal}} + inPlaceState := &State{AppliedGoal: &inPlaceAppliedGoal} + inPlaceNode := nodeSnapshot{node: &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"workload": "new"}}, + Spec: corev1.NodeSpec{Taints: []corev1.Taint{ + {Key: "dedicated", Value: "new", Effect: corev1.TaintEffectNoSchedule}, + }}, + }} tests := map[string]struct { machine machineSnapshot @@ -73,6 +90,12 @@ func TestDecide(t *testing.T) { state: nil, want: decisionWaitForNodeSignal, }, + "present node acknowledges RP-applied labels and taints": { + machine: inPlaceMachine, + node: inPlaceNode, + state: inPlaceState, + want: decisionAcknowledgeGoalState, + }, } for name, tt := range tests { @@ -87,6 +110,166 @@ func TestDecide(t *testing.T) { } } +func TestGoalForInPlaceAcknowledgement(t *testing.T) { + t.Parallel() + + newFixture := func() (aksmachine.GoalState, *State, *corev1.Node) { + appliedGoal := testMachineGoal("1.34.0", "41") + appliedGoal.NodeLabels = map[string]string{"workload": "old", "removed": "true"} + appliedGoal.NodeTaints = []string{"dedicated=old:NoSchedule", "removed=true:NoExecute"} + desiredGoal := testMachineGoal("1.34.0", "42") + desiredGoal.NodeLabels = map[string]string{"workload": "new", "empty": ""} + desiredGoal.NodeTaints = []string{"dedicated=new:NoSchedule", "empty:NoSchedule"} + now := metav1.Now() + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{ + "workload": "new", + "empty": "", + "kubernetes.io/hostname": "node1", + "kubernetes.azure.com/managed": "false", + "kubernetes.azure.com/agentpool": "flexpool", + "kubernetes.azure.com/mode": "user", + "kubernetes.azure.com/nodepool-type": "FlexNodes", + }}, + Spec: corev1.NodeSpec{Taints: []corev1.Taint{ + {Key: "unrelated.example/taint", Effect: corev1.TaintEffectNoExecute}, + {Key: "empty", Effect: corev1.TaintEffectNoSchedule, TimeAdded: &now}, + {Key: "dedicated", Value: "new", Effect: corev1.TaintEffectNoSchedule}, + }}, + } + return desiredGoal, &State{AppliedGoal: &appliedGoal}, node + } + + tests := map[string]struct { + mutate func(*aksmachine.GoalState, *State, *corev1.Node) + want bool + wantMaxPods int + }{ + "reflected label and taint delta": {want: true, wantMaxPods: 110}, + "omitted scalar defaults are preserved": { + mutate: func(goal *aksmachine.GoalState, _ *State, _ *corev1.Node) { + goal.MaxPods = nil + goal.KubeletConfig = aksmachine.KubeletConfig{} + }, + want: true, + wantMaxPods: 110, + }, + "nil labels and taints clear applied values": { + mutate: func(goal *aksmachine.GoalState, _ *State, node *corev1.Node) { + goal.NodeLabels = nil + goal.NodeTaints = nil + delete(node.Labels, "workload") + delete(node.Labels, "empty") + node.Spec.Taints = node.Spec.Taints[:1] + }, + want: true, + wantMaxPods: 110, + }, + "empty labels and taints clear applied values": { + mutate: func(goal *aksmachine.GoalState, _ *State, node *corev1.Node) { + goal.NodeLabels = map[string]string{} + goal.NodeTaints = []string{} + delete(node.Labels, "workload") + delete(node.Labels, "empty") + node.Spec.Taints = node.Spec.Taints[:1] + }, + want: true, + wantMaxPods: 110, + }, + "Kubernetes version changed": { + mutate: func(goal *aksmachine.GoalState, _ *State, _ *corev1.Node) { goal.KubernetesVersion = "1.35.0" }, + }, + "max pods changed": { + mutate: func(goal *aksmachine.GoalState, _ *State, _ *corev1.Node) { goal.MaxPods = intPointer(50) }, + }, + "image GC high threshold changed": { + mutate: func(goal *aksmachine.GoalState, _ *State, _ *corev1.Node) { + goal.KubeletConfig.ImageGCHighThreshold = intPointer(90) + }, + }, + "image GC low threshold changed": { + mutate: func(goal *aksmachine.GoalState, _ *State, _ *corev1.Node) { + goal.KubeletConfig.ImageGCLowThreshold = intPointer(75) + }, + }, + "applied goal unavailable": { + mutate: func(_ *aksmachine.GoalState, state *State, _ *corev1.Node) { state.AppliedGoal = nil }, + }, + "desired label missing": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { delete(node.Labels, "workload") }, + }, + "desired label has wrong value": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { node.Labels["workload"] = "old" }, + }, + "desired empty label missing": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { delete(node.Labels, "empty") }, + }, + "removed label remains with another value": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { node.Labels["removed"] = "changed" }, + }, + "desired taint missing": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { + node.Spec.Taints = node.Spec.Taints[:2] + }, + }, + "desired taint has wrong value": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { + node.Spec.Taints[2].Value = "old" + }, + }, + "updated taint key retains another effect": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { + node.Spec.Taints = append(node.Spec.Taints, corev1.Taint{Key: "dedicated", Value: "stale", Effect: corev1.TaintEffectNoExecute}) + }, + }, + "removed taint identity remains": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { + node.Spec.Taints = append(node.Spec.Taints, corev1.Taint{Key: "removed", Value: "changed", Effect: corev1.TaintEffectNoExecute}) + }, + }, + "removed taint key remains with another effect": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { + node.Spec.Taints = append(node.Spec.Taints, corev1.Taint{Key: "removed", Value: "changed", Effect: corev1.TaintEffectNoSchedule}) + }, + }, + "malformed desired taint": { + mutate: func(goal *aksmachine.GoalState, _ *State, _ *corev1.Node) { + goal.NodeTaints = append(goal.NodeTaints, "malformed") + }, + }, + "duplicate desired taint identity": { + mutate: func(goal *aksmachine.GoalState, _ *State, _ *corev1.Node) { + goal.NodeTaints = append(goal.NodeTaints, "dedicated=duplicate:NoSchedule") + }, + }, + "node is deleting": { + mutate: func(_ *aksmachine.GoalState, _ *State, node *corev1.Node) { + now := metav1.Now() + node.DeletionTimestamp = &now + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + goal, state, node := newFixture() + if tt.mutate != nil { + tt.mutate(&goal, state, node) + } + got, ok := goalForInPlaceAcknowledgement(goal, state, node) + if ok != tt.want { + t.Fatalf("goalForInPlaceAcknowledgement() ok = %v, want %v", ok, tt.want) + } + if ok { + if got.MaxPods == nil || *got.MaxPods != tt.wantMaxPods || !maps.Equal(got.NodeLabels, goal.NodeLabels) { + t.Fatalf("acknowledged goal = %#v", got) + } + } + }) + } +} + func TestHasDeletionSignal(t *testing.T) { t.Parallel() diff --git a/pkg/daemon/repave_reconciler.go b/pkg/daemon/repave_reconciler.go index 0b96f167..280eae2e 100644 --- a/pkg/daemon/repave_reconciler.go +++ b/pkg/daemon/repave_reconciler.go @@ -7,11 +7,13 @@ import ( "fmt" "log/slog" "math/big" + "reflect" "strings" "time" corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/util/validation" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/event" @@ -42,6 +44,7 @@ type decisionKind string const ( decisionNoop decisionKind = "Noop" decisionApplyGoalState decisionKind = "ApplyGoalState" + decisionAcknowledgeGoalState decisionKind = "AcknowledgeGoalState" decisionResetDelete decisionKind = "ResetDelete" decisionWaitForMachineDelete decisionKind = "WaitForMachineDelete" decisionWaitForNodeSignal decisionKind = "WaitForNodeSignal" @@ -182,6 +185,8 @@ func (r *repaveReconciler) reconcileOnce(ctx context.Context) error { return r.patchStatus(ctx, aksmachine.ProvisioningStateSucceeded, decision.Goal.SettingsVersion, decision.Reason) case decisionApplyGoalState: return r.applyGoalState(ctx, state, decision.Goal) + case decisionAcknowledgeGoalState: + return r.acknowledgeGoalState(ctx, state, decision.Goal) case decisionResetDelete: return r.resetDelete(ctx) default: @@ -223,7 +228,16 @@ func (r *repaveReconciler) applyGoalState(ctx context.Context, state *State, goa _ = r.patchStatus(ctx, aksmachine.ProvisioningStateFailed, stateObservedVersion(state), err.Error()) return err } - return r.patchStatus(ctx, aksmachine.ProvisioningStateSucceeded, newState.AppliedSettingsVersion, "machine goal state applied") + return r.patchStatus(ctx, aksmachine.ProvisioningStateSucceeded, stateObservedVersion(newState), "machine goal state applied") +} + +func (r *repaveReconciler) acknowledgeGoalState(ctx context.Context, state *State, goal aksmachine.GoalState) error { + newState, err := r.operator.AcknowledgeGoalState(ctx, goal) + if err != nil { + _ = r.patchStatus(ctx, aksmachine.ProvisioningStateFailed, stateObservedVersion(state), err.Error()) + return err + } + return r.patchStatus(ctx, aksmachine.ProvisioningStateSucceeded, stateObservedVersion(newState), "in-place machine goal state observed") } func (r *repaveReconciler) resetDelete(ctx context.Context) error { @@ -273,15 +287,149 @@ func decide(machine machineSnapshot, node nodeSnapshot, state *State) decision { if goalApplied(goal, state) { return decision{Kind: decisionReportSucceeded, Goal: goal, Reason: "goal state is applied"} } + if acknowledgedGoal, ok := goalForInPlaceAcknowledgement(goal, state, node.node); ok { + return decision{Kind: decisionAcknowledgeGoalState, Goal: acknowledgedGoal, Reason: "goal update is already reflected on the node"} + } return decision{Kind: decisionWaitForNodeSignal, Goal: goal, Reason: "goal state differs but node deletion trigger is absent"} } func goalApplied(goal aksmachine.GoalState, state *State) bool { - if state == nil { + return goal.SettingsVersion != "" && stateObservedVersion(state) == goal.SettingsVersion +} + +// goalForInPlaceAcknowledgement recognizes label and taint updates already +// reconciled onto the Node by AKS RP. Non-observable goal changes fail closed +// and continue through the existing node-deletion repave flow. +func goalForInPlaceAcknowledgement(goal aksmachine.GoalState, state *State, node *corev1.Node) (aksmachine.GoalState, bool) { + if goal.SettingsVersion == "" || state == nil || state.AppliedGoal == nil || node == nil || node.DeletionTimestamp != nil { + return aksmachine.GoalState{}, false + } + + appliedGoal := *state.AppliedGoal + if err := appliedGoal.Validate(); err != nil { + return aksmachine.GoalState{}, false + } + + // ARM can omit scalar defaults that were resolved before the applied goal + // was persisted. Complete the new goal with those values before comparing it. + acknowledgedGoal, err := aksmachine.EffectiveGoal(goal, appliedGoal) + if err != nil { + return aksmachine.GoalState{}, false + } + if !goalMatchesOutsideNodeState(acknowledgedGoal, appliedGoal) { + return aksmachine.GoalState{}, false + } + if !nodeLabelsMatchGoal(node.Labels, appliedGoal.NodeLabels, acknowledgedGoal.NodeLabels) { + return aksmachine.GoalState{}, false + } + if !nodeTaintsMatchGoal(node.Spec.Taints, appliedGoal.NodeTaints, acknowledgedGoal.NodeTaints) { + return aksmachine.GoalState{}, false + } + return acknowledgedGoal, true +} + +func goalMatchesOutsideNodeState(goal, applied aksmachine.GoalState) bool { + // Normalize the version and Node-observable fields verified below. Comparing + // the remaining complete value makes future GoalState fields fail closed. + applied.SettingsVersion = goal.SettingsVersion + applied.NodeLabels = goal.NodeLabels + applied.NodeTaints = goal.NodeTaints + return reflect.DeepEqual(applied, goal) +} + +func nodeLabelsMatchGoal(nodeLabels, appliedLabels, desiredLabels map[string]string) bool { + for key, desiredValue := range desiredLabels { + if currentValue, present := nodeLabels[key]; !present || currentValue != desiredValue { + return false + } + } + for key := range appliedLabels { + if _, stillDesired := desiredLabels[key]; stillDesired { + continue + } + if _, stillPresent := nodeLabels[key]; stillPresent { + return false + } + } + return true +} + +type taintIdentity struct { + key string + effect corev1.TaintEffect +} + +func nodeTaintsMatchGoal(nodeTaints []corev1.Taint, appliedTaints, desiredTaints []string) bool { + appliedByIdentity, ok := parseGoalTaints(appliedTaints) + if !ok { + return false + } + desiredByIdentity, ok := parseGoalTaints(desiredTaints) + if !ok { return false } - return goal.SettingsVersion != "" && state.AppliedSettingsVersion == goal.SettingsVersion + + reflectedDesired := make(map[taintIdentity]struct{}, len(desiredByIdentity)) + reconciledKeys := make(map[string]struct{}, len(appliedByIdentity)) + for identity, applied := range appliedByIdentity { + desired, stillDesired := desiredByIdentity[identity] + if !stillDesired || desired.Value != applied.Value { + reconciledKeys[identity.key] = struct{}{} + } + } + for _, current := range nodeTaints { + identity := taintIdentity{key: current.Key, effect: current.Effect} + if desired, wanted := desiredByIdentity[identity]; wanted { + if _, duplicate := reflectedDesired[identity]; duplicate || current.Value != desired.Value { + return false + } + reflectedDesired[identity] = struct{}{} + continue + } + if _, reconciled := reconciledKeys[current.Key]; reconciled { + return false + } + } + return len(reflectedDesired) == len(desiredByIdentity) +} + +func parseGoalTaints(taints []string) (map[taintIdentity]corev1.Taint, bool) { + indexed := make(map[taintIdentity]corev1.Taint, len(taints)) + for _, value := range taints { + taint, ok := parseGoalTaint(value) + if !ok { + return nil, false + } + identity := taintIdentity{key: taint.Key, effect: taint.Effect} + if _, duplicate := indexed[identity]; duplicate { + return nil, false + } + indexed[identity] = taint + } + return indexed, true +} + +func parseGoalTaint(value string) (corev1.Taint, bool) { + keyValue, effectValue, found := strings.Cut(value, ":") + if !found || strings.Contains(effectValue, ":") { + return corev1.Taint{}, false + } + effect := corev1.TaintEffect(effectValue) + switch effect { + case corev1.TaintEffectNoSchedule, corev1.TaintEffectPreferNoSchedule, corev1.TaintEffectNoExecute: + default: + return corev1.Taint{}, false + } + + key, taintValue, hasValue := strings.Cut(keyValue, "=") + if strings.Contains(taintValue, "=") || len(validation.IsQualifiedName(key)) != 0 { + return corev1.Taint{}, false + } + if hasValue && len(validation.IsValidLabelValue(taintValue)) != 0 { + return corev1.Taint{}, false + } + return corev1.Taint{Key: key, Value: taintValue, Effect: effect}, true } func hasDeletionSignal(taints []corev1.Taint) bool { @@ -297,6 +445,9 @@ func stateObservedVersion(state *State) string { if state == nil { return "" } + if state.AppliedGoal != nil { + return state.AppliedGoal.SettingsVersion + } return state.AppliedSettingsVersion } diff --git a/pkg/daemon/repave_reconciler_test.go b/pkg/daemon/repave_reconciler_test.go index f8323568..c0db6a2b 100644 --- a/pkg/daemon/repave_reconciler_test.go +++ b/pkg/daemon/repave_reconciler_test.go @@ -18,8 +18,17 @@ import ( func TestRepaveReconcilerApplyGoalState(t *testing.T) { t.Parallel() - machines := &fakeMachineClient{machine: &aksmachine.Machine{Goal: aksmachine.GoalState{KubernetesVersion: "1.34.0", SettingsVersion: "42"}}} - operator := &fakeNodeOperator{state: &State{AppliedSettingsVersion: "41", AppliedKubernetesVersion: "1.33.0", ActiveMachine: "kube1"}, newState: &State{AppliedSettingsVersion: "42", AppliedKubernetesVersion: "1.34.0", PreviousSettingsVersion: "41", PreviousKubernetesVersion: "1.33.0", ActiveMachine: "kube2"}} + goal := testMachineGoal("1.34.0", "42") + previousGoal := testMachineGoal("1.33.0", "41") + machines := &fakeMachineClient{machine: &aksmachine.Machine{Goal: goal}} + operator := &fakeNodeOperator{ + state: &State{AppliedGoal: &previousGoal, ActiveMachine: "kube1"}, + newState: &State{ + AppliedGoal: &goal, + PreviousAppliedGoal: &previousGoal, + ActiveMachine: "kube2", + }, + } repaves := newTestRepaveReconciler(t, machines, fakeClient(), operator) if err := repaves.reconcileOnce(context.Background()); err != nil { @@ -28,12 +37,132 @@ func TestRepaveReconcilerApplyGoalState(t *testing.T) { if !operator.applied { t.Fatal("ApplyGoalState was not called") } - if operator.state.AppliedSettingsVersion != "42" || operator.state.PreviousSettingsVersion != "41" || operator.state.ActiveMachine != "kube2" { + if stateObservedVersion(operator.state) != "42" || operator.state.PreviousAppliedGoal == nil || operator.state.PreviousAppliedGoal.SettingsVersion != "41" || operator.state.ActiveMachine != "kube2" { + t.Fatalf("state = %#v", operator.state) + } + if got := machines.status.ProvisioningState; got != aksmachine.ProvisioningStateSucceeded { + t.Fatalf("status = %s", got) + } +} + +func TestRepaveReconcilerAcknowledgesInPlaceGoalState(t *testing.T) { + t.Parallel() + + appliedGoal := testMachineGoal("1.34.0", "41") + appliedGoal.NodeLabels = map[string]string{"workload": "old"} + appliedGoal.NodeTaints = []string{"dedicated=old:NoSchedule"} + desiredGoal := testMachineGoal("1.34.0", "42") + desiredGoal.NodeLabels = map[string]string{"workload": "new"} + desiredGoal.NodeTaints = []string{"dedicated=new:NoSchedule"} + machines := &fakeMachineClient{machine: &aksmachine.Machine{Goal: desiredGoal}} + operator := &fakeNodeOperator{state: &State{AppliedGoal: &appliedGoal, ActiveMachine: "kube1"}} + node := &corev1.Node{ + ObjectMeta: metav1.ObjectMeta{Name: "node1", Labels: map[string]string{"workload": "new"}}, + Spec: corev1.NodeSpec{Taints: []corev1.Taint{ + {Key: "dedicated", Value: "new", Effect: corev1.TaintEffectNoSchedule}, + }}, + } + repaves := newTestRepaveReconciler(t, machines, fakeClient(node), operator) + + if err := repaves.reconcileOnce(t.Context()); err != nil { + t.Fatalf("Reconcile: %v", err) + } + if !operator.acknowledged { + t.Fatal("AcknowledgeGoalState was not called") + } + if operator.applied || operator.restarted || operator.reset || operator.stopped { + t.Fatalf("host mutation called: %#v", operator) + } + if stateObservedVersion(operator.state) != "42" || operator.state.ActiveMachine != "kube1" { t.Fatalf("state = %#v", operator.state) } + if operator.state.PreviousAppliedGoal == nil || operator.state.PreviousAppliedGoal.SettingsVersion != "41" { + t.Fatalf("PreviousAppliedGoal = %#v, want settings version 41", operator.state.PreviousAppliedGoal) + } + if got := machines.status.ObservedSettingsVersion; got != "42" { + t.Fatalf("observed settings version = %q, want 42", got) + } if got := machines.status.ProvisioningState; got != aksmachine.ProvisioningStateSucceeded { t.Fatalf("status = %s", got) } + + operator.acknowledged = false + if err := repaves.reconcileOnce(t.Context()); err != nil { + t.Fatalf("second Reconcile: %v", err) + } + if operator.acknowledged { + t.Fatal("AcknowledgeGoalState was called again for an applied ETag") + } + if operator.state.PreviousAppliedGoal == nil || operator.state.PreviousAppliedGoal.SettingsVersion != "41" { + t.Fatalf("second reconcile rotated PreviousAppliedGoal: %#v", operator.state.PreviousAppliedGoal) + } +} + +func TestRepaveReconcilerRetriesStatusAfterAcknowledgement(t *testing.T) { + t.Parallel() + + appliedGoal := testMachineGoal("1.34.0", "41") + appliedGoal.NodeLabels = map[string]string{"workload": "old"} + desiredGoal := testMachineGoal("1.34.0", "42") + desiredGoal.NodeLabels = map[string]string{"workload": "new"} + statusErr := errors.New("patch status") + machines := &fakeMachineClient{ + machine: &aksmachine.Machine{Goal: desiredGoal}, + patchErr: statusErr, + } + operator := &fakeNodeOperator{state: &State{AppliedGoal: &appliedGoal, ActiveMachine: "kube1"}} + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1", Labels: map[string]string{"workload": "new"}}} + repaves := newTestRepaveReconciler(t, machines, fakeClient(node), operator) + + if err := repaves.reconcileOnce(t.Context()); !errors.Is(err, statusErr) { + t.Fatalf("first Reconcile error = %v, want status failure", err) + } + if !operator.acknowledged || stateObservedVersion(operator.state) != "42" { + t.Fatalf("acknowledged state = %#v", operator.state) + } + + operator.acknowledged = false + machines.patchErr = nil + if err := repaves.reconcileOnce(t.Context()); err != nil { + t.Fatalf("second Reconcile: %v", err) + } + if operator.acknowledged { + t.Fatal("second reconcile acknowledged the same ETag again") + } + if operator.state.PreviousAppliedGoal == nil || operator.state.PreviousAppliedGoal.SettingsVersion != "41" { + t.Fatalf("second reconcile rotated PreviousAppliedGoal: %#v", operator.state.PreviousAppliedGoal) + } + if got := machines.status.ObservedSettingsVersion; got != "42" { + t.Fatalf("observed settings version = %q, want 42", got) + } +} + +func TestRepaveReconcilerAcknowledgementFailurePatchesFailed(t *testing.T) { + t.Parallel() + + appliedGoal := testMachineGoal("1.34.0", "41") + appliedGoal.NodeLabels = map[string]string{"workload": "old"} + desiredGoal := testMachineGoal("1.34.0", "42") + desiredGoal.NodeLabels = map[string]string{"workload": "new"} + machines := &fakeMachineClient{machine: &aksmachine.Machine{Goal: desiredGoal}} + ackErr := errors.New("save acknowledgement") + operator := &fakeNodeOperator{ + state: &State{AppliedGoal: &appliedGoal, ActiveMachine: "kube1"}, + ackErr: ackErr, + } + node := &corev1.Node{ObjectMeta: metav1.ObjectMeta{Name: "node1", Labels: map[string]string{"workload": "new"}}} + repaves := newTestRepaveReconciler(t, machines, fakeClient(node), operator) + + err := repaves.reconcileOnce(t.Context()) + if !errors.Is(err, ackErr) { + t.Fatalf("Reconcile error = %v, want acknowledgement failure", err) + } + if got := machines.status.ProvisioningState; got != aksmachine.ProvisioningStateFailed { + t.Fatalf("status = %s, want Failed", got) + } + if got := machines.status.ObservedSettingsVersion; got != "41" { + t.Fatalf("observed settings version = %q, want 41", got) + } } func TestRepaveReconcilerResetDelete(t *testing.T) { @@ -112,6 +241,7 @@ func fakeClient(objects ...client.Object) client.Client { type fakeMachineClient struct { machine *aksmachine.Machine status aksmachine.Status + patchErr error notFound bool } @@ -128,20 +258,22 @@ func (f *fakeMachineClient) Get(context.Context) (*aksmachine.Machine, error) { func (f *fakeMachineClient) PatchStatus(_ context.Context, status aksmachine.Status) error { f.status = status - return nil + return f.patchErr } type fakeNodeOperator struct { - state *State - newState *State - err error - restartErr error - resetErr error - stopErr error - applied bool - restarted bool - reset bool - stopped bool + state *State + newState *State + err error + ackErr error + restartErr error + resetErr error + stopErr error + applied bool + acknowledged bool + restarted bool + reset bool + stopped bool } func (f *fakeNodeOperator) LoadState(context.Context) (*State, error) { @@ -157,6 +289,19 @@ func (f *fakeNodeOperator) ApplyGoalState(context.Context, *slog.Logger, aksmach return f.state, nil } +func (f *fakeNodeOperator) AcknowledgeGoalState(_ context.Context, goal aksmachine.GoalState) (*State, error) { + f.acknowledged = true + if f.ackErr != nil { + return nil, f.ackErr + } + activeMachineName := "" + if f.state != nil { + activeMachineName = f.state.ActiveMachine + } + f.state = nextAppliedState(f.state, goal, &activeMachine{Name: activeMachineName}) + return f.state, nil +} + func (f *fakeNodeOperator) RestartNode(context.Context, *slog.Logger) error { f.restarted = true return f.restartErr diff --git a/pkg/daemon/state.go b/pkg/daemon/state.go index 3c16a4fc..31ac76dd 100644 --- a/pkg/daemon/state.go +++ b/pkg/daemon/state.go @@ -23,14 +23,40 @@ const ( stateFileName = "daemon-state.json" ) -// State records the last safely applied AKS machine goal and the previous -// known-good goal needed for rollback-oriented reconciliation. +// State records the current and previous safely applied AKS Machine goals and +// the active nspawn machine. type State struct { + AppliedGoal *aksmachine.GoalState `json:"appliedGoal,omitempty"` + PreviousAppliedGoal *aksmachine.GoalState `json:"previousAppliedGoal,omitempty"` + + // Deprecated: these projections keep state readable by older agent binaries. + // AppliedGoal and PreviousAppliedGoal are authoritative when present. AppliedSettingsVersion string `json:"appliedSettingsVersion,omitempty"` AppliedKubernetesVersion string `json:"appliedKubernetesVersion,omitempty"` PreviousSettingsVersion string `json:"previousSettingsVersion,omitempty"` PreviousKubernetesVersion string `json:"previousKubernetesVersion,omitempty"` - ActiveMachine string `json:"activeMachine,omitempty"` + + ActiveMachine string `json:"activeMachine,omitempty"` +} + +func (s *State) validate() error { + if s == nil { + return fmt.Errorf("daemon state is nil") + } + if s.AppliedGoal == nil && s.AppliedKubernetesVersion == "" { + return fmt.Errorf("daemon state applied goal is missing") + } + if s.AppliedGoal != nil { + if err := s.AppliedGoal.Validate(); err != nil { + return fmt.Errorf("daemon state applied goal: %w", err) + } + } + if s.PreviousAppliedGoal != nil { + if err := s.PreviousAppliedGoal.Validate(); err != nil { + return fmt.Errorf("daemon state previous applied goal: %w", err) + } + } + return nil } type saveStateTask struct { @@ -58,10 +84,19 @@ func (t *saveStateTask) Do(ctx context.Context) error { } func SeededState(goal aksmachine.GoalState) *State { - return &State{ - AppliedSettingsVersion: goal.SettingsVersion, - AppliedKubernetesVersion: goal.KubernetesVersion, - ActiveMachine: goalstates.NSpawnMachineKube1, + state := &State{AppliedGoal: goal.DeepCopy(), ActiveMachine: goalstates.NSpawnMachineKube1} + state.populateLegacyFields() + return state +} + +func (s *State) populateLegacyFields() { + if s.AppliedGoal != nil { + s.AppliedSettingsVersion = s.AppliedGoal.SettingsVersion + s.AppliedKubernetesVersion = s.AppliedGoal.KubernetesVersion + } + if s.PreviousAppliedGoal != nil { + s.PreviousSettingsVersion = s.PreviousAppliedGoal.SettingsVersion + s.PreviousKubernetesVersion = s.PreviousAppliedGoal.KubernetesVersion } } @@ -77,6 +112,9 @@ func activeMachineFromStore(ctx context.Context, store stateStore) (*activeMachi if state == nil { return nil, fmt.Errorf("daemon state is missing active machine") } + if err := state.validate(); err != nil { + return nil, err + } if !validActiveMachine(state.ActiveMachine) { return nil, fmt.Errorf("daemon state active machine %q is invalid", state.ActiveMachine) } @@ -125,14 +163,20 @@ func (s *fileStateStore) Load(context.Context) (*State, error) { if err := json.Unmarshal(data, &state); err != nil { return nil, fmt.Errorf("decode daemon state %s: %w", s.path, err) } + if err := state.validate(); err != nil { + return nil, fmt.Errorf("validate daemon state %s: %w", s.path, err) + } + state.populateLegacyFields() return &state, nil } func (s *fileStateStore) Save(_ context.Context, state *State) error { - if state == nil { - return fmt.Errorf("daemon state is nil") + if err := state.validate(); err != nil { + return err } - data, err := json.MarshalIndent(state, "", " ") + stateForPersistence := *state + stateForPersistence.populateLegacyFields() + data, err := json.MarshalIndent(&stateForPersistence, "", " ") if err != nil { return fmt.Errorf("marshal daemon state: %w", err) } diff --git a/pkg/daemon/state_test.go b/pkg/daemon/state_test.go index 620b221a..a859d0f5 100644 --- a/pkg/daemon/state_test.go +++ b/pkg/daemon/state_test.go @@ -2,12 +2,11 @@ package daemon import ( "context" + "encoding/json" "os" "path/filepath" "strings" "testing" - - "github.com/Azure/AKSFlexNode/pkg/aksmachine" ) func TestFileStateStoreSaveLoad(t *testing.T) { @@ -18,12 +17,12 @@ func TestFileStateStoreSaveLoad(t *testing.T) { t.Fatalf("newFileStateStore: %v", err) } want := &State{ - AppliedSettingsVersion: "42", - AppliedKubernetesVersion: "1.34.0", - PreviousSettingsVersion: "41", - PreviousKubernetesVersion: "1.33.0", - ActiveMachine: "kube2", + AppliedGoal: testMachineGoal("1.34.0", "42").DeepCopy(), + PreviousAppliedGoal: testMachineGoal("1.33.0", "41").DeepCopy(), + ActiveMachine: "kube2", } + want.AppliedGoal.NodeLabels = map[string]string{"workload": "flex"} + want.AppliedGoal.NodeTaints = []string{"dedicated=flex:NoSchedule"} if err := store.Save(context.Background(), want); err != nil { t.Fatalf("Save: %v", err) @@ -32,9 +31,22 @@ func TestFileStateStoreSaveLoad(t *testing.T) { if err != nil { t.Fatalf("Load: %v", err) } - if got.AppliedSettingsVersion != want.AppliedSettingsVersion || got.ActiveMachine != want.ActiveMachine { + if got.ActiveMachine != want.ActiveMachine || got.AppliedGoal == nil || got.AppliedGoal.NodeLabels["workload"] != "flex" || + got.PreviousAppliedGoal == nil || got.PreviousAppliedGoal.SettingsVersion != "41" || + got.AppliedSettingsVersion != "42" || got.AppliedKubernetesVersion != "1.34.0" { t.Fatalf("state = %#v, want %#v", got, want) } + persistedData, err := os.ReadFile(store.path) + if err != nil { + t.Fatalf("ReadFile: %v", err) + } + var persisted State + if err := json.Unmarshal(persistedData, &persisted); err != nil { + t.Fatalf("Unmarshal persisted state: %v", err) + } + if persisted.AppliedSettingsVersion != "42" || persisted.PreviousSettingsVersion != "41" { + t.Fatalf("legacy state projections = %#v", persisted) + } } func TestFileStateStoreLoadMissing(t *testing.T) { @@ -53,6 +65,80 @@ func TestFileStateStoreLoadMissing(t *testing.T) { } } +func TestFileStateStoreLoadCompatibility(t *testing.T) { + t.Parallel() + + tests := map[string]struct { + data string + check func(*testing.T, *State) + }{ + "legacy state remains partial": { + data: `{ + "appliedSettingsVersion":"42", + "appliedKubernetesVersion":"1.34.0", + "previousSettingsVersion":"41", + "previousKubernetesVersion":"1.33.0", + "activeMachine":"kube1" + }`, + check: func(t *testing.T, state *State) { + t.Helper() + if state.AppliedGoal != nil || state.PreviousAppliedGoal != nil { + t.Fatalf("legacy goals were fabricated: %#v", state) + } + if state.AppliedSettingsVersion != "42" || state.AppliedKubernetesVersion != "1.34.0" { + t.Fatalf("legacy projections = %#v", state) + } + }, + }, + "complete goals override stale projections": { + data: `{ + "appliedGoal":{"kubernetesVersion":"1.34.0","settingsVersion":"42","maxPods":110,"kubeletConfig":{"imageGCHighThreshold":85,"imageGCLowThreshold":80}}, + "previousAppliedGoal":{"kubernetesVersion":"1.33.0","settingsVersion":"41","maxPods":110,"kubeletConfig":{"imageGCHighThreshold":85,"imageGCLowThreshold":80}}, + "appliedSettingsVersion":"stale", + "appliedKubernetesVersion":"1.99.0", + "previousSettingsVersion":"stale", + "previousKubernetesVersion":"1.98.0", + "activeMachine":"kube2" + }`, + check: func(t *testing.T, state *State) { + t.Helper() + if state.AppliedGoal == nil || state.AppliedGoal.SettingsVersion != "42" || state.PreviousAppliedGoal == nil { + t.Fatalf("complete goals = %#v", state) + } + if state.AppliedSettingsVersion != "42" || state.AppliedKubernetesVersion != "1.34.0" || + state.PreviousSettingsVersion != "41" || state.PreviousKubernetesVersion != "1.33.0" { + t.Fatalf("legacy projections were not corrected: %#v", state) + } + }, + }, + } + + for name, tt := range tests { + t.Run(name, func(t *testing.T) { + t.Parallel() + + path := filepath.Join(t.TempDir(), "state.json") + store, err := newFileStateStore(path) + if err != nil { + t.Fatalf("newFileStateStore: %v", err) + } + data := []byte(tt.data) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("WriteFile: %v", err) + } + if err := os.WriteFile(path+".sha256", []byte(checksum(data)+"\n"), 0o600); err != nil { + t.Fatalf("WriteFile checksum: %v", err) + } + + state, err := store.Load(t.Context()) + if err != nil { + t.Fatalf("Load: %v", err) + } + tt.check(t, state) + }) + } +} + func TestFileStateStoreChecksumMismatch(t *testing.T) { t.Parallel() @@ -61,7 +147,7 @@ func TestFileStateStoreChecksumMismatch(t *testing.T) { if err != nil { t.Fatalf("newFileStateStore: %v", err) } - if err := store.Save(context.Background(), &State{AppliedSettingsVersion: "42"}); err != nil { + if err := store.Save(context.Background(), &State{AppliedSettingsVersion: "42", AppliedKubernetesVersion: "1.34.0"}); err != nil { t.Fatalf("Save: %v", err) } if err := os.WriteFile(path, []byte(`{"appliedSettingsVersion":"43"}`), 0o600); err != nil { @@ -102,7 +188,7 @@ func TestFileStateStoreDelete(t *testing.T) { if err != nil { t.Fatalf("newFileStateStore: %v", err) } - if err := store.Save(context.Background(), &State{AppliedSettingsVersion: "42"}); err != nil { + if err := store.Save(context.Background(), &State{AppliedSettingsVersion: "42", AppliedKubernetesVersion: "1.34.0"}); err != nil { t.Fatalf("Save: %v", err) } if err := store.Delete(context.Background()); err != nil { @@ -119,7 +205,9 @@ func TestFileStateStoreDelete(t *testing.T) { func TestSeededState(t *testing.T) { t.Parallel() - state := SeededState(aksmachine.GoalState{KubernetesVersion: "1.34.0", SettingsVersion: "42"}) + goal := testMachineGoal("1.34.0", "42") + goal.NodeLabels = map[string]string{"workload": "flex"} + state := SeededState(goal) if state.AppliedSettingsVersion != "42" { t.Fatalf("AppliedSettingsVersion = %q, want 42", state.AppliedSettingsVersion) } @@ -132,6 +220,13 @@ func TestSeededState(t *testing.T) { if state.PreviousSettingsVersion != "" || state.PreviousKubernetesVersion != "" { t.Fatalf("previous state = %#v, want empty", state) } + if state.AppliedGoal == nil || state.AppliedGoal.NodeLabels["workload"] != "flex" { + t.Fatalf("AppliedGoal = %#v", state.AppliedGoal) + } + goal.NodeLabels["workload"] = "changed" + if state.AppliedGoal.NodeLabels["workload"] != "flex" { + t.Fatal("SeededState retained caller-owned label map") + } } func TestSaveStateValidation(t *testing.T) { @@ -161,11 +256,11 @@ func TestActiveMachineFromStore(t *testing.T) { wantErr bool }{ "kube1": { - state: &State{ActiveMachine: "kube1"}, + state: &State{AppliedKubernetesVersion: "1.34.0", ActiveMachine: "kube1"}, want: "kube1", }, "kube2": { - state: &State{ActiveMachine: "kube2"}, + state: &State{AppliedKubernetesVersion: "1.34.0", ActiveMachine: "kube2"}, want: "kube2", }, "missing state": { diff --git a/pkg/daemon/test_helpers_test.go b/pkg/daemon/test_helpers_test.go new file mode 100644 index 00000000..783b724f --- /dev/null +++ b/pkg/daemon/test_helpers_test.go @@ -0,0 +1,19 @@ +package daemon + +import "github.com/Azure/AKSFlexNode/pkg/aksmachine" + +func testMachineGoal(kubernetesVersion, settingsVersion string) aksmachine.GoalState { + return aksmachine.GoalState{ + KubernetesVersion: kubernetesVersion, + SettingsVersion: settingsVersion, + MaxPods: intPointer(110), + KubeletConfig: aksmachine.KubeletConfig{ + ImageGCHighThreshold: intPointer(85), + ImageGCLowThreshold: intPointer(80), + }, + } +} + +func intPointer(value int) *int { + return &value +}