fix: [APL-354] - disable shared CPU whenever APL is enabled #11216

dennisvankekem · 2024-11-06T15:27:21Z

Description 📝

Shared CPU's are currently unstable with APL, therefore we want to disable the entire table whenever APL is enabled.

Changes 🔄

List any change relevant to the reviewer.

added 'shared' to LinodeTypeClass
added isAPLEnabled to isPlanPanelDisabled
created new notice for APL

Target release date 🗓️

November the 11th

Preview 📷

How to test 🧪

Make sure APL feature flag is enabled and you are enrolled into the beta
go to create Kubernetes cluster page
Enable APL
Observe that the entire shared CPU table is disabled

github-actions · 2024-11-06T16:01:51Z

Coverage Report: ✅
Base Coverage: 87.45%
Current Coverage: 87.45%

dennisvankekem · 2024-11-06T17:17:48Z

Will fix test tomorrow

packages/manager/src/features/components/PlansPanel/APLNotice.tsx

abailly-akamai · 2024-11-07T14:42:52Z

packages/api-v4/src/linodes/types.ts

@@ -332,6 +332,7 @@ export interface LinodeType extends BaseType {
 export type LinodeTypeClass =
  | 'nanode'
  | 'standard'
+  | 'shared'


I am confused by this. This isn't a proper value returned by the API
https://techdocs.akamai.com/linode-api/reference/get-linode-types

Orr API types are packaged separately and needs to strictly adhere to API types.

Is it not a proper value, or has the documentation never been updated for a new value? Because it does as a matter of fact seem to work.

Otherwise I'm curious how we can call for the shared type class

standard = shared
I have just created a cluster with shared node pool. Below the payload:
{"control_plane":{"acl":{"enabled":true,"revision-id":""},"high_availability":false},"k8s_version":"1.31","label":"jeho-sh","node_pools":[{"type":"g6-standard-1","count":3}],"region":"us-ord","apl_enabled":false}

standard = shared, but this is not the case for checking whenever the shared tab is active, because standard is not being used as an ID for that tab, but rather the type is being extended with 'shared' only in the plan segment

abailly-akamai · 2024-11-07T14:45:03Z

packages/manager/src/utilities/planNotices.ts

+    isAPLEnabled?: boolean
+  ) =>
+    (hasSelectedRegion && !isSelectedRegionEligibleForPlan(planType)) ||
+    (planType === 'shared' && Boolean(isAPLEnabled));


I don't like adding a feature specific check in a generic component.

can we modified

isPlanPanelDisabled: ( planType?: LinodeTypeClass, isAPLEnabled?: boolean ) => boolean;

to being able to pass a custom function returning a boolean?

isPlanPanelDisabled: ( planType?: LinodeTypeClass, disableFn?: (arg) => boolean ) => boolean;

something like that

also, see my initial comment about types, based on that I would not expect this to work?

seems fair enough

Regarding this, looking deeper in the code. It might be redundant to do so actually.

we call the function in KubernetesPlansPanel. If I were to lift the APL part out it would probably also live in KubernetesPlansPanel so we might as well change this line:

wholePanelIsDisabled={isPlanPanelDisabled(plan, isAPLEnabled)}

to something like this:

wholePanelIsDisabled={isPlanPanelDisabled(plan) || isDisabledByAPL()}

where isDisabledByAPL()

would be something like this:

const isDisabledByAPL = () => (planType === 'shared' && Boolean(isAPLEnabled));

I too would like to see a disableFn instead of having the logic in a generic component, this does not really look future proof. If you think otherwise please explain.

@ElderMatt the logic has since been moved out of the isPlanPanelDisabled function. I've implemented my proposal for the time being with a separate isDisabledByAPL in KubernetesPlansPanel .

If we allow for a disableFn parameter then it defeats the purpose of isPlanPanelDisabled because we'll basically send the result of said function with disableFn. Because in order to use disableFn we first have to decide if something disables the plan, which is the entire purpose of the generic function. Therefore, I went with my proposed solution.

dennisvankekem · 2024-11-11T11:26:05Z

Regarding 'shared' type, it seems that it's indeed not an official type, however it is used as type in the plans panel, so I've tried my best to work around it

dennisvankekem · 2024-11-11T12:22:16Z

adding @ElderMatt as internal reviewer

dennisvankekem · 2024-11-11T12:31:36Z

error from storybook Could not load .//src/components/Notice/Notice (imported by src/features/components/PlansPanel/APLNotice.tsx): ENOENT: no such file or directory, open './/src/components/Notice/Notice' doesn't make sense to me, it loads Notice the exact same as the other notices like MetalNotice

linode-gh-bot · 2024-11-11T13:35:22Z

Cloud Manager UI test results

🔺 6 failing tests on test run #5 ↗︎

❌ Failing	✅ Passing	↪️ Skipped	🕐 Duration
`6 Failing`	`439 Passing`	`2 Skipped`	`111m 39s`

Details

Failing Tests
	Spec	Test
❌	`plan-selection.spec.ts`	displays linode plans panel based on availability » displays the proper plans based on the region and types
❌	`plan-selection.spec.ts`	displays kubernetes plans panel based on availability » displays the proper plans based on the region and types
❌	`resize-database.spec.ts`	Resizes a g6-standard-6 MySQL v8.x 3-node cluster » Can resize active database clusters
❌	`resize-database.spec.ts`	Resizes a g6-standard-6 MySQL v8.x 3-node cluster » Can resize active database clusters from g6-standard-6 type and switch plan type
❌	`resize-database.spec.ts`	Resizes a g6-dedicated-16 MySQL v5.x 3-node cluster » Can resize active database clusters
❌	`resize-database.spec.ts`	Resizes a g6-dedicated-16 MySQL v5.x 3-node cluster » Can resize active database clusters from g6-dedicated-16 type and switch plan type

Troubleshooting

Use this command to re-run the failing tests:

yarn cy:run -s "cypress/e2e/core/linodes/plan-selection.spec.ts,cypress/e2e/core/linodes/plan-selection.spec.ts,cypress/e2e/core/databases/resize-database.spec.ts,cypress/e2e/core/databases/resize-database.spec.ts,cypress/e2e/core/databases/resize-database.spec.ts,cypress/e2e/core/databases/resize-database.spec.ts"

packages/manager/src/utilities/planNotices.ts

ElderMatt · 2024-11-11T16:27:49Z

packages/manager/src/utilities/planNotices.ts

+    isAPLEnabled?: boolean
+  ) =>
+    (hasSelectedRegion && !isSelectedRegionEligibleForPlan(planType)) ||
+    (planType === 'shared' && Boolean(isAPLEnabled));


I too would like to see a disableFn instead of having the logic in a generic component, this does not really look future proof. If you think otherwise please explain.

abailly-akamai · 2024-11-12T14:54:36Z

packages/manager/src/features/components/PlansPanel/APLNotice.tsx

+  dataTestId?: string;
+}
+
+const programInfo = `Shared CPU instances are currently not available for Application Platform for LKE`;


Not a big deal, bit no interpolation needed so we can use reg quotes or move this to a constant file.

abailly-akamai · 2024-11-12T15:00:57Z

packages/manager/src/features/components/PlansPanel/PlanInformation.tsx

@@ -82,6 +87,7 @@ export const PlanInformation = (props: PlanInformationProps) => {
          hasDisabledClass={getDisabledClass('metal')}
        />
      ) : null}
+      {planType === 'shared' ? <APLNotice dataTestId="apl-notice" /> : null}


this is problematic:

it shows in the Linode create flow (/linodes/create) which seems irrelevant to APL

it shows for ALL users (shouldn't it only show for users enrolled int he beta?)

This should only show in the LKE plan selection and the isAPLEnabled? logic should be contained there

We also have a lot of test failures on this PR.

@dennisvankekem FYI a lot of components are being moved to a new @linode/ui package for modularization purposes. Besides the e2e failures, this is probably the issues you're seeing in the CI

disable shared CPU whenever APL is enabled

313ceb4

dennisvankekem requested a review from a team as a code owner November 6, 2024 15:27

dennisvankekem requested review from hana-akamai and harsh-akamai and removed request for a team November 6, 2024 15:27

abailly-akamai self-requested a review November 6, 2024 15:38

abailly-akamai assigned dennisvankekem Nov 6, 2024

abailly-akamai added the LKE Related to Linode Kubernetes Engine offerings label Nov 6, 2024

dennisvankekem added 2 commits November 7, 2024 13:27

fix: removed undefined type for isAPLEnabled in isPlanPanelDisabled

537abcd

fix: increased minimal memory for APL from 8000 to 16000

e41de7f

abailly-akamai reviewed Nov 7, 2024

View reviewed changes

fix: shared type workaround

b7ed77f

feat: added APL Notice to planInformation test

5d6a1b3

ElderMatt suggested changes Nov 11, 2024

View reviewed changes

abailly-akamai reviewed Nov 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: [APL-354] - disable shared CPU whenever APL is enabled #11216

fix: [APL-354] - disable shared CPU whenever APL is enabled #11216

dennisvankekem commented Nov 6, 2024

github-actions bot commented Nov 6, 2024 •

edited

Loading

dennisvankekem commented Nov 6, 2024

abailly-akamai Nov 7, 2024

dennisvankekem Nov 8, 2024

dennisvankekem Nov 8, 2024

j-zimnowoda Nov 11, 2024

dennisvankekem Nov 11, 2024 •

edited

Loading

abailly-akamai Nov 7, 2024

dennisvankekem Nov 8, 2024

dennisvankekem Nov 11, 2024

ElderMatt Nov 11, 2024 •

edited

Loading

dennisvankekem Nov 11, 2024

dennisvankekem commented Nov 11, 2024

dennisvankekem commented Nov 11, 2024

dennisvankekem commented Nov 11, 2024

linode-gh-bot commented Nov 11, 2024

ElderMatt Nov 11, 2024 •

edited

Loading

abailly-akamai Nov 12, 2024

abailly-akamai Nov 12, 2024

abailly-akamai Nov 12, 2024

fix: [APL-354] - disable shared CPU whenever APL is enabled #11216

Are you sure you want to change the base?

fix: [APL-354] - disable shared CPU whenever APL is enabled #11216

Conversation

dennisvankekem commented Nov 6, 2024

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

github-actions bot commented Nov 6, 2024 • edited Loading

dennisvankekem commented Nov 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisvankekem Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ElderMatt Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dennisvankekem commented Nov 11, 2024

dennisvankekem commented Nov 11, 2024

dennisvankekem commented Nov 11, 2024

linode-gh-bot commented Nov 11, 2024

Cloud Manager UI test results

🔺 6 failing tests on test run #5 ↗︎

Details

Troubleshooting

ElderMatt Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 6, 2024 •

edited

Loading

dennisvankekem Nov 11, 2024 •

edited

Loading

ElderMatt Nov 11, 2024 •

edited

Loading

ElderMatt Nov 11, 2024 •

edited

Loading