fix(app): ensure placement errors surface per node [EE-7065] (#11820)

Co-authored-by: testa113 <testa113>
6 months ago · a80aa2b45c
parent 9dd9ffdb3b
commit a80aa2b45c
7 changed files with 105 additions and 78 deletions
--- a/.storybook/preview.tsx
+++ b/.storybook/preview.tsx
@ -8,7 +8,6 @@ import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
 initMSW(
  {
    onUnhandledRequest: ({ method, url }) => {
-      console.log(method, url);
      if (url.startsWith('/api')) {
        console.error(`Unhandled ${method} request to ${url}.

--- a/app/react/edge/edge-stacks/CreateView/CreateForm.validation.ts
+++ b/app/react/edge/edge-stacks/CreateView/CreateForm.validation.ts
@ -17,7 +17,7 @@ import { useCurrentUser } from '@/react/hooks/useUser';
 import { relativePathValidation } from '@/react/portainer/gitops/RelativePathFieldset/validation';
 import { CustomTemplate } from '@/react/portainer/templates/custom-templates/types';
 import { TemplateViewModel } from '@/react/portainer/templates/app-templates/view-model';
-import { GitFormModel } from '@/react/portainer/gitops/types';
+import { DeployMethod, GitFormModel } from '@/react/portainer/gitops/types';

 import { envVarValidation } from '@@/form-components/EnvironmentVariablesFieldset';
 import { file } from '@@/form-components/yup-file-validation';
@ -76,10 +76,17 @@ export function useValidation({
          }),
          git: mixed().when('method', {
            is: 'repository',
-            then: buildGitValidationSchema(
-              gitCredentialsQuery.data || [],
-              !!customTemplate
-            ),
+            then: () => {
+              const deploymentMethod: DeployMethod =
+                values.deploymentType === DeploymentType.Compose
+                  ? 'compose'
+                  : 'manifest';
+              return buildGitValidationSchema(
+                gitCredentialsQuery.data || [],
+                !!customTemplate,
+                deploymentMethod
+              );
+            },
          }) as SchemaOf<GitFormModel>,
          relativePath: relativePathValidation(),
          useManifestNamespaces: boolean().default(false),
--- a/app/react/edge/edge-stacks/CreateView/KubeManifestForm.tsx
+++ b/app/react/edge/edge-stacks/CreateView/KubeManifestForm.tsx
@ -95,6 +95,7 @@ export function KubeManifestForm({

      {method === git.value && (
        <GitForm
+          deployMethod="manifest"
          errors={errors?.git}
          value={values.git}
          onChange={(gitValues) =>
--- a/app/react/kubernetes/applications/DetailsView/PlacementsDatatable/PlacementsDatatableSubRow.tsx
+++ b/app/react/kubernetes/applications/DetailsView/PlacementsDatatable/PlacementsDatatableSubRow.tsx
@ -170,6 +170,7 @@ function UnmatchedAffinitiesInfo({
            'datatable-highlighted': isHighlighted,
            'datatable-unhighlighted': !isHighlighted,
          })}
+          key={aff.map((term) => term.key).join('')}
        >
          <td />
          <td colSpan={cellCount - 1}>
--- a/app/react/kubernetes/applications/DetailsView/PlacementsDatatable/columns/status.tsx
+++ b/app/react/kubernetes/applications/DetailsView/PlacementsDatatable/columns/status.tsx
@ -11,11 +11,13 @@ export const status = columnHelper.accessor('acceptsApplication', {
  cell: ({ getValue }) => {
    const acceptsApplication = getValue();
    return (
-      <Icon
-        icon={acceptsApplication ? Check : X}
-        mode={acceptsApplication ? 'success' : 'danger'}
-        size="sm"
-      />
+      <div className="flex items-center h-full">
+        <Icon
+          icon={acceptsApplication ? Check : X}
+          mode={acceptsApplication ? 'success' : 'danger'}
+          size="sm"
+        />
+      </div>
    );
  },
  meta: {
--- a/app/react/kubernetes/applications/DetailsView/PlacementsDatatable/usePlacementTableData.tsx
+++ b/app/react/kubernetes/applications/DetailsView/PlacementsDatatable/usePlacementTableData.tsx
@ -2,9 +2,9 @@ import { useCurrentStateAndParams } from '@uirouter/react';
 import { useMemo } from 'react';
 import { Pod, Taint, Node } from 'kubernetes-types/core/v1';
 import _ from 'lodash';
-import * as JsonPatch from 'fast-json-patch';

 import { useNodesQuery } from '@/react/kubernetes/cluster/HomeView/nodes.service';
+import { KubernetesPodNodeAffinityNodeSelectorRequirementOperators } from '@/kubernetes/pod/models';

 import {
  BasicTableSettings,
@ -15,7 +15,7 @@ import {
 import { useTableState } from '@@/datatables/useTableState';

 import { useApplication, useApplicationPods } from '../../application.queries';
-import { NodePlacementRowData } from '../types';
+import { Affinity, Label, NodePlacementRowData } from '../types';

 interface TableSettings extends BasicTableSettings, RefreshableTableSettings {}

@ -162,6 +162,68 @@ function computeTolerations(nodes: Node[], pod: Pod): NodePlacementRowData[] {
  return nodePlacements;
 }

+function getUnmatchedNodeSelectorLabels(node: Node, pod: Pod): Label[] {
+  const nodeLabels = node.metadata?.labels || {};
+  const podNodeSelectorLabels = pod.spec?.nodeSelector || {};
+
+  return Object.entries(podNodeSelectorLabels)
+    .filter(([key, value]) => nodeLabels[key] !== value)
+    .map(([key, value]) => ({
+      key,
+      value,
+    }));
+}
+
+// Function to get unmatched required node affinities
+function getUnmatchedRequiredNodeAffinities(node: Node, pod: Pod): Affinity[] {
+  const basicNodeAffinity =
+    pod.spec?.affinity?.nodeAffinity
+      ?.requiredDuringSchedulingIgnoredDuringExecution;
+
+  const unmatchedRequiredNodeAffinities: Affinity[] =
+    basicNodeAffinity?.nodeSelectorTerms.map(
+      (selectorTerm) =>
+        selectorTerm.matchExpressions?.flatMap((matchExpression) => {
+          const exists = !!node.metadata?.labels?.[matchExpression.key];
+          const isIn =
+            exists &&
+            _.includes(
+              matchExpression.values,
+              node.metadata?.labels?.[matchExpression.key]
+            );
+
+          // Check if the match expression is satisfied
+          if (
+            (matchExpression.operator === 'Exists' && exists) ||
+            (matchExpression.operator === 'DoesNotExist' && !exists) ||
+            (matchExpression.operator === 'In' && isIn) ||
+            (matchExpression.operator === 'NotIn' && !isIn) ||
+            (matchExpression.operator === 'Gt' &&
+              exists &&
+              parseInt(node.metadata?.labels?.[matchExpression.key] || '', 10) >
+                parseInt(matchExpression.values?.[0] || '', 10)) ||
+            (matchExpression.operator === 'Lt' &&
+              exists &&
+              parseInt(node.metadata?.labels?.[matchExpression.key] || '', 10) <
+                parseInt(matchExpression.values?.[0] || '', 10))
+          ) {
+            return [];
+          }
+
+          // Return the unmatched affinity
+          return [
+            {
+              key: matchExpression.key,
+              operator:
+                matchExpression.operator as KubernetesPodNodeAffinityNodeSelectorRequirementOperators,
+              values: matchExpression.values?.join(', ') || '',
+            },
+          ];
+        }) || []
+    ) || [];
+  return unmatchedRequiredNodeAffinities;
+}
+
 // Node requirement depending on the operator value
 // https://kubernetes.io/docs/concepts/scheduling-eviction/assign-pod-node/#node-affinity
 function computeAffinities(
@ -173,76 +235,32 @@ function computeAffinities(
    (node, nodeIndex) => {
      let { acceptsApplication } = nodePlacements[nodeIndex];

-      if (pod.spec?.nodeSelector) {
-        const patch = JsonPatch.compare(
-          node.metadata?.labels || {},
-          pod.spec.nodeSelector
-        );
-        _.remove(patch, { op: 'remove' });
-        const unmatchedNodeSelectorLabels = patch.map((operation) => ({
-          key: _.trimStart(operation.path, '/'),
-          value: operation.op,
-        }));
-        if (unmatchedNodeSelectorLabels.length) {
-          acceptsApplication = false;
-        }
-      }
+      // check node selectors for unmatched labels
+      const unmatchedNodeSelectorLabels = getUnmatchedNodeSelectorLabels(
+        node,
+        pod
+      );

-      const basicNodeAffinity =
-        pod.spec?.affinity?.nodeAffinity
-          ?.requiredDuringSchedulingIgnoredDuringExecution;
-      if (basicNodeAffinity) {
-        const unmatchedTerms = basicNodeAffinity.nodeSelectorTerms.map(
-          (selectorTerm) => {
-            const unmatchedExpressions = selectorTerm.matchExpressions?.flatMap(
-              (matchExpression) => {
-                const exists = {}.hasOwnProperty.call(
-                  node.metadata?.labels,
-                  matchExpression.key
-                );
-                const isIn =
-                  exists &&
-                  _.includes(
-                    matchExpression.values,
-                    node.metadata?.labels?.[matchExpression.key]
-                  );
-                if (
-                  (matchExpression.operator === 'Exists' && exists) ||
-                  (matchExpression.operator === 'DoesNotExist' && !exists) ||
-                  (matchExpression.operator === 'In' && isIn) ||
-                  (matchExpression.operator === 'NotIn' && !isIn) ||
-                  (matchExpression.operator === 'Gt' &&
-                    exists &&
-                    parseInt(
-                      node.metadata?.labels?.[matchExpression.key] || '',
-                      10
-                    ) > parseInt(matchExpression.values?.[0] || '', 10)) ||
-                  (matchExpression.operator === 'Lt' &&
-                    exists &&
-                    parseInt(
-                      node.metadata?.labels?.[matchExpression.key] || '',
-                      10
-                    ) < parseInt(matchExpression.values?.[0] || '', 10))
-                ) {
-                  return [];
-                }
-                return [true];
-              }
-            );
+      // check node affinities that are required during scheduling
+      const unmatchedRequiredNodeAffinities =
+        getUnmatchedRequiredNodeAffinities(node, pod);

-            return unmatchedExpressions;
-          }
-        );
-
-        _.remove(unmatchedTerms, (i) => !i);
-        if (unmatchedTerms.length) {
-          acceptsApplication = false;
-        }
+      // If there are any unmatched affinities or node labels, the node does not accept the application
+      if (
+        unmatchedRequiredNodeAffinities.length ||
+        unmatchedNodeSelectorLabels.length
+      ) {
+        acceptsApplication = false;
      }
-      return {
+
+      const nodePlacementRowData: NodePlacementRowData = {
        ...nodePlacements[nodeIndex],
        acceptsApplication,
+        unmatchedNodeSelectorLabels,
+        unmatchedNodeAffinities: unmatchedRequiredNodeAffinities,
      };
+
+      return nodePlacementRowData;
    }
  );
  return nodePlacementsFromAffinities;
--- a/app/react/portainer/templates/custom-templates/EditView/InnerForm.tsx
+++ b/app/react/portainer/templates/custom-templates/EditView/InnerForm.tsx
@ -51,7 +51,6 @@ export function InnerForm({
    isSubmitting,
    dirty,
  } = useFormikContext<FormValues>();
-  console.log({ isEditorReadonly, isSubmitting, isLoading });
  usePreventExit(
    initialValues.FileContent,
    values.FileContent,